Emission and reading of custom modifiers #85504
Conversation
Fixes dotnet#71883. * Emit custom modifiers to the native metadata format * Read custom modifiers at runtime * Bugfix in the type loader
ghost
assigned 
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsFixes #71883.
|
| // We call Reverse for compat with CoreCLR that also reverses these. | ||
| // ILDasm also reverses these but don't be fooled: you can go to | ||
| // View -> MetaInfo -> Show to see the file format order in ILDasm. | ||
| Array.Reverse(result); |
src/coreclr/tools/Common/Internal/Metadata/NativeFormat/Generator/SchemaDef.cs
Show resolved
Hide resolved
| @@ -47,18 +47,21 @@ private Field HandleFieldDefinition(Cts.FieldDesc field) | |||
| private void InitializeFieldDefinition(Cts.FieldDesc entity, Field record) | |||
| { | |||
| record.Name = HandleString(entity.Name); | |||
There was a problem hiding this comment.
I'll add an else branch that throws on non-ecma. This is never reached for non-ecma.
| @@ -61,7 +61,6 @@ private Method HandleMethodDefinition(Cts.MethodDesc method) | |||
| private void InitializeMethodDefinition(Cts.MethodDesc entity, Method record) | |||
| { | |||
| record.Name = HandleString(entity.Name); | |||
| record.Signature = HandleMethodSignature(entity.Signature); | |||
There was a problem hiding this comment.
This is never reached for non-ecma methods. There is an else branch that throws NotImplemented. There's no scenario for non-ECMA metadata generation.
All of this should be eventually rewritten to work on top of S.R.Metadata instead of the type system.
| @@ -98,7 +101,11 @@ private TypeDesc InitializeFieldType() | |||
| BlobReader signatureReader = metadataReader.GetBlobReader(metadataReader.GetFieldDefinition(_handle).Signature); | |||
|
|
|||
| EcmaSignatureParser parser = new EcmaSignatureParser(Module, signatureReader, NotFoundBehavior.Throw); | |||
| var fieldType = parser.ParseFieldSignature(); | |||
| var fieldType = parser.ParseFieldSignature(out EmbeddedSignatureData[] data); | |||
There was a problem hiding this comment.
It looks wasteful to use the expensive parsing method that creates Stack, etc. Can we have bool on the parser that tracks whether we have encountered any modifiers?
There was a problem hiding this comment.
(Or make the expensive parsing method cheaper, e.g. use inline storage for stack of limited depth.)
There was a problem hiding this comment.
The modifiers handling within the type system is utter garbage. The Stack should be rewritten to use a stack-based linked list once C# can express that (I haven't been keeping track with all the unscoped ref stuff - maybe it can already express it). I don't think it makes sense to add extra complexity to the already incomprehensible modifiers handling if we know we'll be able to get rid of this cost.
We already pay this cost when parsing method signatures. I've never seen it in profiles.
| _embeddedSignatureDataList = new List<EmbeddedSignatureData>(); | ||
| return ParsePropertySignatureInternal(); | ||
| } | ||
| finally |
There was a problem hiding this comment.
EcmaSignatureParser is a struct that is not meant to be reused for multiple parsing operatins. This cleanup should not be needed.
(I know that pattern is used in existing methods, but it looks unnecessary.)
There was a problem hiding this comment.
This is part of the "the modifiers stuff in the managed type system is incomprehensible". I just cargo cult my way through it.
I'll delete this part of the copy paste.
There was a problem hiding this comment.
Delete the cargo cult from the two other places in this file as well?
There was a problem hiding this comment.
I'll do this in a separate PR. I don't like deleting unrelated mystery code in a 500-line PR because the mystery can unravel itself.
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The failures in extra-platforms are going to be fixed with #85579. Hopefully I didn't miss anything. This failed every single leg. |
Fixes #71883.