cDac work necessary to get the MethodTableName #104759

davidwrighton · 2024-07-11T21:27:43Z

Add new details to the RuntimeTypeSystem and Loader contracts as needed to load metadata and examine and identify all of the kinds of type that the CoreCLR type system can represent

Add a type name generator based on a line by line copy from the CoreCLR codebase

Add an Ecma335 metadata parser which is pure safe managed code, and is capable of loading metadata that is not structured as a single array. This implementation is heavily based on the dnmd implementation.

Provide impelementations and documentation for all of the new contracts except for the RW metadata one. (Its rarely needed, so we can add it later)

Enhance the target infrastructure to better handle various forms of arrays, and contracts which do math based on target pointer sizes.

Contributes to #99302

Add implementations for changes to RuntimeTypeSystem contract Add SOSDacInterface call into cDac for GetMethodTableName

- Move pointer to Module class - Replace use of SBuffer abstraction with a simple counted byte memory block Add metadata details to Loader contract Add Metadata helper api for use by contracts within the cDAC (and possibly clients of cDAC too)

1. It works now!

davidwrighton · 2024-07-11T21:35:50Z

There are a few remaining TODOs ... notably, I haven't implemented sorted lookups, and the ecma api is currently only low level token and constant access, and the fallback path for MethodTable to Name mapping but these could be fixed in a follow up PR, or if this sits for long enough, I'll fix it here.

jkoritzinsky · 2024-07-11T21:40:31Z

src/native/managed/cdacreader/src/Helpers/Metadata.cs

+    public EcmaMetadataSchema Schema { get; init; }
+
+    private ReadOnlyMemory<byte>[] _tables;
+    public ReadOnlySpan<ReadOnlyMemory<byte>> Tables => _tables;


Could we use ReadOnlySequence<byte> here? Basically, instead of having each element of the span represent the memory of a table, we could represent the underlying memory of the image as a ReadOnlySequence<byte> that stitches together the various components of the image.

You could use DNMD's implementation of the "save to memory" function to provide the remaining byte arrays to stitch together the separate tables and heaps into a valid ECMA-335 image represented as a ReadOnlySequence<byte>.

If we were to do this, we could (performance-permitting), refactor System.Reflection.Metadata to support this use case and remove the actual metadata reader implementation from here (and only have the "stitch into a sequence that represents metadata" code).

Could we use ReadOnlySequence here? Basically, instead of having each element of the span represent the memory of a table, we could represent the underlying memory of the image as a ReadOnlySequence that stitches together the various components of the image.

My gut reaction here would be introducing this abstraction is a bit of magic I'm not sure is a win. Do we have an example I can see that shows a benefit?

I think that's not a bad idea, or at least, it doesn't really look like one. I don't really want to pursue it yet though.

This implementation should allow us to fill out the remaining metadata needs of the cDAC without all that much difficulty (other than the IMetadataImport export requirement, which System.Reflection.Metadata doesn't help with at all, in fact, I'm not sure the IMetadataImport api surface can be implemented on top of System.Reflection.Metadata).

I want to explore the possibility in .NET 10 of changing how we produce triage dump metadata when it becomes CDAC driven. Currently we produce an parallel data stream that has precomputed strings, but an alternative approach that might be more scalable to our real needs could be to have triage dump metadata be fully real metadata but only the exact bytes that the reader needs (not entire tables). Having a custom metadata reader makes adding that sort of hook a fair bit easier.

I'm fine with deferring it to a future experiment.

I think it may be worth revisiting when we explore switching the runtimes to use DNMD as the metadata library, though the "only specific bytes that are needed" model is also quite compelling/interesting to me.

Maybe we can make an issue to track possible improvements to metadata handling in dumps with the various ideas after this is merged so we don't lose them?

System.Reflection.Metadata is the managed low-level metadata reader library. I do not think we want to be growing a different one. We should refactor System.Reflection.Metadata as needed.

@jkotas, I don't see that as a practical near term effort. To make it able to handle the cases that come up here, would require replacing all of the memory handling of that library with something different, which I can't see happening as part of this effort until we've stabilized the capabilities of what this logic will require the metadata system to be able to process. In general, I view what I've built as a short term solution, that will either last until we replace it with dnmd, or we do a major refactor of S.R.M to make it able to operate on things that aren't a pointer to a contiguous memory blob.

AaronRobinsonMSFT

Are we adding tests for this?

AaronRobinsonMSFT · 2024-07-11T22:32:39Z

docs/design/datacontracts/RuntimeTypeSystem.md

+struct TypeHandle
+{
+}
+


Suggested change

AaronRobinsonMSFT · 2024-07-11T22:39:44Z

docs/design/datacontracts/RuntimeTypeSystem.md

+}
+
+
+internal enum CorElementType


Let's reference something here for this and make it all hexadecimal.

Instead of spelling this out here, please define the the type and then comment where the values come from.

internal enum CorElementType { // Values defined in ECMA-335 - II.23.1.16 Element types used in signatures }

AaronRobinsonMSFT · 2024-07-11T22:41:34Z

docs/design/datacontracts/RuntimeTypeSystem.md

@@ -69,9 +134,15 @@ internal partial struct RuntimeTypeSystem_1
    internal enum WFLAGS_HIGH : uint
    {
        Category_Mask = 0x000F0000,
+        Category_ElementType_Mask = 0x000E0000,
+        Category_IfArrayThenSzArray = 0x00020000,
        Category_Array = 0x00080000,


This should be in order.

docs/design/datacontracts/RuntimeTypeSystem.md

AaronRobinsonMSFT · 2024-07-11T22:43:19Z

docs/design/datacontracts/RuntimeTypeSystem.md

    }
 }
 ```

+Internally the contract depends on a `TypeDescHandle` structure that represents the address of something that implements the TypeDesc data descriptor.
+
+Internally the contract depends on the `TypeHandle` structure being capable of holding a TypeDescHandle or a MethodTableHandle.


This is an implementation detail of CoreCLR. I don't think this what we want to express in the contract directly.

That's why this is documented as part of version 1 of the contract. By definition the specific versions are about the particular implementation details. You'll note that the version free portion of the contract documentation does not say this at all.

Also, notably, I think we should think about changing our cdacreader dll into being constructed from 2 different dlls. 1 which is the contract and targets and such., and the second which is the legacy api surface which is what we're actually exposing from cdacreader.dll.

AaronRobinsonMSFT · 2024-07-11T22:52:57Z

src/coreclr/vm/ceeload.cpp

@@ -3923,27 +3923,28 @@ void ReflectionModule::CaptureModuleMetaDataToMemory()
    IfFailThrow(hr);

    // Operate on local data, and then persist it into the module once we know it's valid.
-    NewHolder<SBuffer> pBuffer(new SBuffer());
+    int sizeInInts = (numBytes / sizeof(int32_t)) + 2;


Suggested change

int sizeInInts = (numBytes / sizeof(int32_t)) + 2;

int sizeInInts = (numBytes / sizeof(uint32_t)) + 2;

What is the "+ 2" for?

AaronRobinsonMSFT · 2024-07-11T22:55:42Z

src/coreclr/vm/ceeload.h

+    // The first uint32_t is the number of bytes in the saved metadata
+    // Starting at the address of the second uint32_t value is the saved metadata itself
+protected:
+    TADDR m_pDynamicMetadata;


Can we type this as:

struct DynamicMetadata { uint32_t Length; uint32_t Data[0]; };

The above will be documented on the native side and the native side can then document the layout?

AaronRobinsonMSFT · 2024-07-11T23:01:44Z

src/native/managed/cdacreader/src/Contracts/RuntimeTypeSystem_1.cs

+        if (address == 0)
+            return default;
+
+        if (((ulong)address & 2) == (ulong)2)


Suggested change

if (((ulong)address & 2) == (ulong)2)

if (((ulong)address & 2) != 0)

AaronRobinsonMSFT · 2024-07-11T23:03:50Z

src/native/managed/cdacreader/src/Data/EEClass.cs

    }

    public TargetPointer MethodTable { get; init; }
    public ushort NumMethods { get; init; }
    public uint CorTypeAttr { get; init; }
+    public byte InternalCorElementType { get; init; }


Please doc what makes this "Internal", that is generally an important distinction.

AaronRobinsonMSFT · 2024-07-11T23:06:21Z

src/native/managed/cdacreader/src/Helpers/Metadata.cs

+    GenericParam = 0x2a,
+    MethodSpec = 0x2b,
+    GenericParamConstraint = 0x2c,
+    Count = 0x2d


Suggested change

Count = 0x2d

Count

lambdageek

I didn't look at the EcmaMetadataReader or the TypeNameBuilder yet. Initial comments to answer questions and raise some concerns about MethodTableArray and TypeHandleArray.

docs/design/datacontracts/RuntimeTypeSystem.md

lambdageek · 2024-07-12T17:27:57Z

docs/design/datacontracts/RuntimeTypeSystem.md

+| Data Descriptor Name |
+| --- |
+| `EEClass` |
+| `ArraayClass` |
+| `TypeDesc` |
+| `ParamTypeDesc` |
+| `TypeVarTypeDesc` |
+| `FnPtrTypeDesc` |
+| `GenericsDictInfo` |
+


👍

I'd like to update all the contracts to have tables like this

I've been just doing a basic list - the table does look nicer.

runtime/docs/design/datacontracts/Exception.md

Lines 26 to 28 in 856a0a6

Data descriptors used:

- `ExceptionInfo`

- `Exception`

We probably want to put globals in a table too.

In my second pass at this today, I went and did a bit where I put all the types, and all the fields, and comments for the meaning of the fields. It looks really nice, and you guys will see it next week.

The other thing we should probably do is put all the magic constants, like enum values in a similar table. Then we can add some build time validation that a various constants have the expected values for the particular contract version we're working with (This could probably even be _DEBUG only validation if its easier to do.). I don't think we need a data descriptor for those values... just enough validation that we know to update the contract version when magic numbers change. I'm not going to do that in this change, but it seems like a good idea for us to implement soonish.

lambdageek · 2024-07-12T17:31:16Z

docs/design/datacontracts/RuntimeTypeSystem.md

+    {
+        // validate that address points to something that looks like a TypeHandle.
+
+        if (address & 2 == 2)


I'd like us to use an enum for this kind of pointer bit-stealing. See EEClassOrCanonMTBits

docs/design/datacontracts/RuntimeTypeSystem.md

lambdageek · 2024-07-12T19:32:34Z

src/native/managed/cdacreader/src/Contracts/RuntimeTypeSystem_1.cs

+        int numberOfGenericArgs = _target.ProcessedData.GetOrAdd<GenericsDictInfo>(genericsDictInfo).NumTypeArgs;
+        MethodTableArray instantiation = _target.ProcessedData.GetOrAdd<(TargetPointer, int), MethodTableArray>
+            ((dictionaryPointer, numberOfGenericArgs));
+
+        return instantiation.Types.AsSpan();


why does the key for the data include the number of generic args? can the same dictionaryPointer be viewed with a different number of generic args?

Because just the pointer to the start of the data doesn't quite encode enough to identify the number of generic args. I'm going to restructure this so that instead of this being a general purpose TypeHandleArray... that it becomes a TypeInstantiation type, that type is defined in the contract implementation, and the pointer that makes identifies it is the MethodTable pointer. That should make this simpler, remove the extra key concept, remove the concern about Data contracts knowing about contracts, and make it so that repeated calls to GetInstantiation api are just the cost of doing data lookup, which should help us put our new design on a path to actually be faster than the old model.

lambdageek · 2024-07-12T19:34:38Z

src/native/managed/cdacreader/src/Data/MethodTableArray.cs

+
+namespace Microsoft.Diagnostics.DataContractReader.Data;
+
+internal sealed class MethodTableArray : IData<MethodTableArray, (TargetPointer ptr, int size)>


this doesn't feel like it belongs in Data. I've typically viewed Data as straightforward data-descriptor extractor. Something we could potentially source-generate in the future. This MehtodTableArray feels more like it belongs to the RuntimeTypeSystem contract proper

I think we need a general structure for post-processed data produced by the contracts. @lambdageek you have already done this with the _methodTables array in the RuntimeTypeSystem contract, and I think that is a better place to put the processing, but I don't think its a particulaly good way to structure managing the memory of the data.

As I was writing up the DacStreams contract for the pointer to name mapping, I think I found a better approach.

Use the ProcessedData/IData interfaces to hold the data. I dislike the RuntimeTypeSystem approach of having some state in the actual contract implementation. I want ALL state for the contracts to be held in ProcessedData, or on other structures associated with the Target (such as the Metadata).

Put the data structure into the contract implementation.

This both lets us have a single part of the system to do all memory invalidation, lets us get the caching behavior to avoid reprocessing the world all of the time, and places all of the logic that understands runtime data structures into the contract.

src/native/managed/cdacreader/src/Data/MethodTableArray.cs

src/native/managed/cdacreader/src/Data/TypeHandleArray.cs

- Remove TypeHandleArray and MethodTableArray in favor of contract specific logic

…ime into cdac-methodtable-name

docs/design/datacontracts/DacStreams.md

docs/design/datacontracts/RuntimeTypeSystem.md

src/coreclr/debug/runtimeinfo/contracts.jsonc

lambdageek · 2024-07-16T16:25:03Z

src/coreclr/vm/ceeload.h

+{
+    uint32_t Size;
+    BYTE Data[0];
+    template<typename T> friend struct ::cdac_offsets;


nit: technically this is unnecessary (except, perhaps, as documentation), all members of a struct are public, so datadescriptor.h can just use offsetof directly

src/native/managed/cdacreader/src/Target.cs

lambdageek · 2024-07-16T18:02:11Z

I expected to see updates to the tests due to the MethodTable=>TypeHandle change.

(VS Code's test extension seems to detect and run the testsuite correctly (at least if you build Debug libraries) if you want to run them locally if you open the test project)

Co-Authored-By: David Wrighton <davidwr@microsoft.com>

lambdageek · 2024-07-16T20:20:17Z

@davidwrighton here's a commit that has the minimal set of changes to make the existing MethodTableTests tests pass again (MethodTableHandle => TypeHandle and also add mock fields for MethodTable and EEClass)

lambdageek@8e2dbca

- Add test suite for the DacStreams contract

src/native/managed/cdacreader/src/Helpers/EcmaMetadataReader.cs

davidwrighton added 5 commits July 9, 2024 10:11

Progress towards GetMethodTableName

255da99

Move metadata reader into helpers

0671cd4

Add implementations for changes to RuntimeTypeSystem contract Add SOSDacInterface call into cDac for GetMethodTableName

Fix bugs found when trying to actually use this logic

d359ed9

1. It works now!

Update contract documentation

fc24cd4

davidwrighton requested review from elinor-fung and lambdageek July 11, 2024 21:27

dotnet-issue-labeler bot added the area-TypeSystem-coreclr label Jul 11, 2024

dotnet-policy-service bot assigned davidwrighton Jul 11, 2024

Merge branch 'main' into cdac-methodtable-name

d93e98b

davidwrighton mentioned this pull request Jul 11, 2024

[cdac] Implement !PrintException #99302

Open

25 tasks

jkoritzinsky reviewed Jul 11, 2024

View reviewed changes

AaronRobinsonMSFT reviewed Jul 11, 2024

View reviewed changes

Feedback

fead409

lambdageek reviewed Jul 12, 2024

View reviewed changes

davidwrighton added 5 commits July 15, 2024 10:50

Add support for the EEName DacStream

d2b36fc

- Refactor to expose TypeHandle exclusively from the contracts

8fdcc24

- Remove TypeHandleArray and MethodTableArray in favor of contract specific logic

Address concern around data structure for DynamicMetadata

30da32c

Merge branch 'cdac-methodtable-name' of github.com:davidwrighton/runt…

87f7003

…ime into cdac-methodtable-name

Fix build break

143cb86

Fix musl build issue

7284f83

lambdageek reviewed Jul 16, 2024

View reviewed changes

lambdageek added a commit to lambdageek/runtime that referenced this pull request Jul 16, 2024

Cherrypick RuntimeTypeSystem changes from dotnet#104759

2d5ec4f

Co-Authored-By: David Wrighton <davidwr@microsoft.com>

update tests

54ab23e

lambdageek mentioned this pull request Jul 16, 2024

[cdac] Begin adding MethodDesc APIs to the RuntimeTypeSystem contract #104811

Merged

Remove unnecessary usings

2a41206

build-analysis bot mentioned this pull request Jul 16, 2024

[Test Failure] RemoteExecutionException: Half-way through waiting for remote process #104366

Closed

davidwrighton added 2 commits July 16, 2024 15:42

- Transform magic numbers from DacStreams implementation into constants

dbe3cf4

- Add test suite for the DacStreams contract

Feedback

3a35f41

This was referenced Jul 17, 2024

Unload_TwoCollectibleWithOneAssemblyAndOneInstanceReferencingAnother test failed in CI #104153

Open

[Android] No attached devices found dotnet/dnceng#2057

Open

lambdageek approved these changes Jul 17, 2024

View reviewed changes

src/native/managed/cdacreader/src/Helpers/EcmaMetadataReader.cs Show resolved Hide resolved

davidwrighton merged commit 2263059 into dotnet:main Jul 17, 2024
151 checks passed

github-actions bot locked and limited conversation to collaborators Aug 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cDac work necessary to get the MethodTableName #104759

cDac work necessary to get the MethodTableName #104759

davidwrighton commented Jul 11, 2024 •

edited

Loading

davidwrighton commented Jul 11, 2024 •

edited

Loading

jkoritzinsky Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

davidwrighton Jul 11, 2024

jkoritzinsky Jul 12, 2024

jkotas Jul 12, 2024

davidwrighton Jul 15, 2024

AaronRobinsonMSFT left a comment

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

davidwrighton Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

AaronRobinsonMSFT Jul 11, 2024

lambdageek left a comment

lambdageek Jul 12, 2024

elinor-fung Jul 12, 2024 •

edited

Loading

davidwrighton Jul 12, 2024

davidwrighton Jul 15, 2024

lambdageek Jul 12, 2024

lambdageek Jul 12, 2024

davidwrighton Jul 15, 2024

lambdageek Jul 12, 2024

davidwrighton Jul 12, 2024

lambdageek Jul 16, 2024

lambdageek commented Jul 16, 2024

lambdageek commented Jul 16, 2024 •

edited

Loading

	int sizeInInts = (numBytes / sizeof(int32_t)) + 2;
	int sizeInInts = (numBytes / sizeof(uint32_t)) + 2;

	if (((ulong)address & 2) == (ulong)2)
	if (((ulong)address & 2) != 0)


		namespace Microsoft.Diagnostics.DataContractReader.Data;

		internal sealed class MethodTableArray : IData<MethodTableArray, (TargetPointer ptr, int size)>

cDac work necessary to get the MethodTableName #104759

cDac work necessary to get the MethodTableName #104759

Conversation

davidwrighton commented Jul 11, 2024 • edited Loading

davidwrighton commented Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinor-fung Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek commented Jul 16, 2024

lambdageek commented Jul 16, 2024 • edited Loading

davidwrighton commented Jul 11, 2024 •

edited

Loading

davidwrighton commented Jul 11, 2024 •

edited

Loading

elinor-fung Jul 12, 2024 •

edited

Loading

lambdageek commented Jul 16, 2024 •

edited

Loading