Implement cDAC TraverseLoaderHeap via Loader contract#125129
Implement cDAC TraverseLoaderHeap via Loader contract#125129
Conversation
|
Tagging subscribers to this area: @agocke |
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
… typo fix Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…ructs for LoaderHeap types Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…loaderheap.h Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs
Show resolved
Hide resolved
…ype; embed kind check in GetFirstLoaderHeapBlock Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
…to loaderheap.h Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
|
We should also rebase/merge on top of main and fix any conflicts or problems that arise. |
|
@jkotas adding the virtual destructor did not cause the correct offsetof() to be picked up by the cDAC in, for example, LoaderAllocator. DavidWr has indicated that it is not reliable / platform dependent whether this will be optimized out. |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ISOSDacInterface.cs
Outdated
Show resolved
Hide resolved
This reverts commit 0b34106.
Ok. I think we are still exposed to the fragility of offsetof and multiple inheritance as copilot pointed it in #125129 (comment) . If it is not causing observable issues, we can leave it like that. |
docs/design/datacontracts/Loader.md
Outdated
|
|
||
| // Returns the first block of the loader heap linked list, or TargetPointer.Null if the heap has no blocks. | ||
| // Throws NotImplementedException for unknown kind values. | ||
| TargetPointer GetFirstLoaderHeapBlock(TargetPointer loaderHeap, LoaderHeapKind kind); |
There was a problem hiding this comment.
From what I can see, this LoaderHeapKind appears to have no impact on the final result. Both kinds derive from UnlockedLoaderHeapBaseTraversable and the m_pFirstBlock pointer that we need is also a member of that type. Let me know if I am missing something.
I'm thinking we should drop this kind argument from the API. If we did I'm guessing a number of other things would change too:
- change the cDac contract to depend on a single UnlockedLoaderHeapBaseTraversable::m_pFirst field instead of per-derived type definitions of the same field.
- get rid of the kind enum
- Reduce the two IData types down to a single one for the base class
- Change SOSDacInterface13.TraverseLoaderHeap impl to ignore the kind argument from the caller
There was a problem hiding this comment.
From what I can see, this LoaderHeapKind appears to have no impact on the final result
Unfortunately, it does have an impact as discussed above. Loader heap implementation uses C++ multiple inheritance that introduces multiple different this pointers for the same object.
I agree that it would be nice to do what you are proposing. We would have to get the multiple inheritance under control first.
There was a problem hiding this comment.
Ah I walked into the midst unaware!
What if we removed the multiple inheritance with a pattern like this:
class LoaderHeapBackout final : public ILoaderHeapBackout
{
LoaderHeap* m_owner;
void RealBackoutMem(...) override { m_owner->RealBackoutMem(...); }
};
class UnlockedLoaderHeapBase : public UnlockedLoaderHeapBaseTraversable
{
LoaderHeapBackout m_backout; // pass out the address of this field when an
// ILoaderHeapBackout is needed.
};IMO the conceptual simplification to data contract would be worth doing a small runtime refactor.
There was a problem hiding this comment.
Adding a virtual destructor onto the base class solves the issue.
…125867) A more complete example of what I was talking about in #125129 (comment). We can either merge this PR or close it and incorporate similar changes in 125129 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a5d88bc to
36f229e
Compare
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs
Outdated
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs
Outdated
Show resolved
Hide resolved
|
wait on codeflow from microsoft/clrmd#1410 for clean CI |
Implements
ISOSDacInterface13::TraverseLoaderHeap(with and withoutkind) in the managed cDAC stack via theLoadercontract. Adds separate cDAC types forLoaderHeapandExplicitControlLoaderHeap, with aLoaderHeapKindenum embedded inGetFirstLoaderHeapBlock.Description
C++ VM
loaderheap.h: forward-declarescdac_data<T>, addsfriend struct cdac_data<UnlockedLoaderHeapBaseTraversable>, and provides acdac_data<UnlockedLoaderHeapBaseTraversable>specialization exposingFirstBlock— matching the pattern used inarraylist.handloaderallocator.hpp.datadescriptor.inc: two separate cDAC types (LoaderHeap,ExplicitControlLoaderHeap) each with aFirstBlockfield viacdac_data<UnlockedLoaderHeapBaseTraversable>::FirstBlock;LoaderHeapBlockfields via directoffsetof(public struct members).Managed contracts
LoaderHeapKindenum (Normal = 0,ExplicitControl = 1) inILoader.cs;GetFirstLoaderHeapBlocktakes aLoaderHeapKindand dispatches toData.LoaderHeaporData.ExplicitControlLoaderHeapaccordingly — unknown kinds throwNotImplementedException.Data/ExplicitControlLoaderHeap.cs(IData<T>) readsFirstBlockfrom theExplicitControlLoaderHeapcDAC type.DataType.ExplicitControlLoaderHeapadded.Legacy DAC (
SOSDacImpl.cs)TraverseLoaderHeap(no-kind) andTraverseLoaderHeapWithKindimplemented independently via the sameILoadercontract methods — no cross-dependency.kindmaps toLoaderHeapKind; out-of-range values returnE_NOTIMPL.Tests
28 unit tests covering empty heap, single block, and multi-block scenarios across both heap kinds and 4 pointer-size variants.
Docs
Loader.mdupdated withLoaderHeapKindenum, updatedGetFirstLoaderHeapBlocksignature,ExplicitControlLoaderHeapdata descriptor row, and revised algorithm pseudocode for both dispatch paths.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.