Uh oh!
There was an error while loading. Please reload this page.
Implementation of SOSDacApi GetMethodDescName for cDAC - #106169
Conversation
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!
- Remove TypeHandleArray and MethodTableArray in favor of contract specific logic
…ime into cdac-methodtable-name
Co-Authored-By: David Wrighton <davidwr@microsoft.com>
mock the additional data and globals
- Add test suite for the DacStreams contract
Tagging subscribers to this area: @tommcdon |
lambdageek
left a comment
There was a problem hiding this comment.
First set of comments. Didn't look at Ecma or the formatter yet
Uh oh!
There was an error while loading. Please reload this page.
| { | ||
| get | ||
| { | ||
| int tokenRemainderBitCount = _target.ReadGlobal<byte>(Constants.Globals.MethodDescTokenRemainderBitCount); |
There was a problem hiding this comment.
Can we either store the token remainder bit count in the MethodDesc, or just pre-compose the token when creating the MethodDesc instance? I've been avoiding storing Target and contracts in data structures, so they're just dumb value holders, not fancy algorithms.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
lambdageek
left a comment
There was a problem hiding this comment.
LGTM. modulo nits. And it would be really really nice if RuntimeTypeSystem_1.MethodDesc didn't hold on to Target
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Net; |
1. Rename mcNDirect to mcPInvoke 2. Rename MethodDescClassification to MethodClassification 3. Remove mc prefix from all MethodClassification enum values 4. Move handling of various forms of MethodDescs to using an AsBlah method, and handle the flags in those methods, to make the actual contract methods somewhat simpler. 5. Compute the token eagerly on MethodDesc creation to avoid having to hold a pointer to the Target
| // Return true if a MethodDesc represents a dynamically generated method, either an IL Stub dynamically | ||
| // generated by the runtime, or a MethodDesc that describes a method represented by the System.Reflection.Emit.DynamicMethod class | ||
| // A dynamic method is also a StoredSigMethodDesc | ||
| public virtual bool IsDynamicMethod(MethodDescHandle methodDesc, out ReadOnlySpan<byte> methodName); |
There was a problem hiding this comment.
Would it make sense to just make methodName a string instead of ReadOnlySpan<byte>, so the consumer doesn't have to know the encoding?
There was a problem hiding this comment.
Not a bad idea. In general, I don't want to expose arrays to contract users, as its far to easy to mutate them, but strings are nice in that they are our only truly immutable type in the BCL, and work nicely for this sort of thing.
| // Return true for a MethodDesc that describes a method represented by the System.Reflection.Emit.DynamicMethod class | ||
| // A LCG method is also a StoredSigMethodDesc | ||
| public virtual bool IsLCGMethod(MethodDescHandle methodDesc); |
There was a problem hiding this comment.
I know we have IsLCGMethod on the native MethodDesc, but is LCG the term we want to keep using going forwards? Would IsReflectionEmitMethod make more sense?
At least personally, LCG is not a term I use - I just think of it as reflection emit.
There was a problem hiding this comment.
DynamicMethod is the public name of the LCG feature.
ReflectionEmit is more general. Some methods emitted using Reflection.Emit are dynamic methods, some are not.
There was a problem hiding this comment.
So then would this make sense:
IsDynamicMethod->IsDynamicallyGeneratedMethodIsLCGMethod->IsDynamicMethod
There was a problem hiding this comment.
... whereas I think of Reflection.Emit as a thing that only applies to things that generate the full metadata and types which is also not completely accurate... The actual term in the BCL is that an LCGMethod maps to a method represented by the System.Reflection.Emit.DynamicMethod class, but calling it a DynamicMethod... might be extra special confusing, since the runtime has multiple meanings of what a DynamicMethodDesc represents, only 1 of which is a System.Reflection.Emit.DynamicMethod. (And of course LCG stands for Lightweight CodeGen, which is an internal codename that probably doesn't still need to be kept around.) I'd like to hear what @lambdageek, and @AaronRobinsonMSFT think on this, as I know I'm not objective here as I've worked on this stuff for FAR too long to see things reasonably. Possibly the right approach would be to call it DynamicMethod and sometime in .NET 10, rename the DynamicMethodDesc to something like RuntimeILMethodDesc.
There was a problem hiding this comment.
I like the rename that elinor mentioned above at #106169 (comment).
rename the
DynamicMethodDescto something likeRuntimeILMethodDesc.
I also agree with your suggestion above. I could also see GeneratedMethodDesc or ILEmitMethodDesc as options too.
There was a problem hiding this comment.
I like IsLCGMethod -> IsDynamicMethod and DynamicMethodDesc ->ILEmitMethodDesc.
Not sure about IsDynamicMethod -> IsDynamicallyGeneratedMethod. Would just IsGeneratedILMethod be too broad?
There was a problem hiding this comment.
DynamicMethodDesc ->ILEmitMethodDesc.
How about DynamicMethodDesc ->NoMetadataMethodDesc and IsDynamicMethod -> IsNoMetadata (existing property)?
Part of the codebase uses the NoMetadata name already:
runtime/src/coreclr/vm/method.hpp
Lines 626 to 630 in fa33edd
MethodDesc::IsDynamicMethod and MethodDesc::IsNoMetadata have the same implementation.The key property of the DynamicMethodDesc is that it has no ECMA-335 metadata, it has no metadata token, etc.
Dynamic IL generation is not the key property of DynamicMethodDesc. The regular ECMA-335 MethodDescs can have dynamically generated (IL) code too. For example, UnsafeAccessors have dynamically generated IL but they are regular MethodDescs.
There was a problem hiding this comment.
I like @jkotas's idea here. Then the IsDynamicMethod api at the cdac level makes total sense, the existing DynamicMethodDesc translates to NoMetadataMethodDesc and gets a better name that more matches its utility, and we get rid of having both MethodDesc::IsNoMetadata and MethodDesc::IsDynamicMethod which are today exactly the same thing. While I'm at it, I'll rename mcDynamic to mcNoMetadata
There was a problem hiding this comment.
I'm trending toward the idea of putting this rename in a separate PR. Opinions?
There was a problem hiding this comment.
Offline discussion is to put together a separate PR for that. I'll just add a commit to update the cdac contract side naming to IsDynamicMethod, and the renaming of the underlying structures to be less confusing will be a separate PR that will probably miss .NET 9.
Add a number of new
MethodDesccontract definitionsIsGenericMethodDefinitionGetGenericMethodInstantiationGetMethodTokenIsArrayMethodIsDynamicMethodIsStoredSigMethodDescIsNoMetadataMethodIsILStubUpdate cDAC compat asserts in cDAC to always be enabled by using a tls variable in
mscordaccoreImplement
GetMethodDescNameonISOSDacInterfacein thecdacreaderStub out an implementation of
GetPathin theLoadercontract used in a fallback after a fallback. This will need further work, but is included to make sure the code path isn't lost.Fix the
EcmaMetadataReaderto be able to find blobs in the metadataAdd ability to read target data from a buffer held on the cdac side using the
Targetclass. This was needed to handle signature containing aCorElementType.Internal.And finally actually implement the name generation algorithm via a line for line port from the CoreCLR codebase.
Contributes to #99302