-
Notifications
You must be signed in to change notification settings - Fork 5k
Reflection.Emit should respect RVAs of 0 #116766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reflection.Emit should respect RVAs of 0 #116766
Conversation
The underlying Reflection.Emit infrastructure was treating an RVA as "normal" and indexing into the dynamic IL data stream. This caused confusion in the rest of the system since an RVA of 0 means there is no IL body (that is, no IL Header). Making this consistent ensures that scenario relying on an RVA of 0 now operate naturally with those emitted by .NET compilers that produce IL. This change also removes some of the checks that were put in place for Reflection.Emit and relies on the EE at runtime to handle those cases.
Tagging subscribers to this area: @dotnet/area-system-reflection-emit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates Reflection.Emit to treat an RVA of 0 as “no IL body,” removes custom empty-body validation, relies on the execution engine for those checks, and adjusts the native emitter/loader to never return or load RVA 0. Key changes include:
- Removing pre-flight IL-body validation and empty-method throws in both managed and runtime code
- Adjusting CCeeGen to pad the IL section to avoid RVA 0 and asserting non-zero offsets
- Updating the loader to skip
GetMethodBuffer
when the RVA is 0, and refining native signatures
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineConstructor.cs | Dropped obsolete test expecting an exception on uncreated constructors |
src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveTypeBuilderAPIsTests.cs | Removed test for missing IL throw in CreateType for PInvoke/InternalCall |
src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveILGeneratorTests.cs | Added tests for empty and minimal IL bodies in saved assemblies |
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs | Removed IL validation checks for empty method bodies |
src/coreclr/vm/comdynamic.cpp | Initialized methodRVA , asserted non-zero per ECMA-335 |
src/coreclr/vm/ceeload.cpp | Changed GetIL to accept an RVA and skip zero |
src/coreclr/md/ceefilegen/cceegen.cpp | Padded IL section to avoid RVA 0 and asserted non-zero offsets |
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs | Simplified IL-body attachment logic, removed custom empty-body checks |
Comments suppressed due to low confidence (2)
src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveTypeBuilderAPIsTests.cs:570
- [nitpick] Add a test case after setting PInvoke/InternalCall flags to call
concreteTypeWithNativeAndPinvokeMethod.CreateType()
and assert no exception is thrown, since empty-body validation has been removed.
implFlagsSetMethod.SetImplementationFlags(MethodImplAttributes.InternalCall);
src/coreclr/vm/ceeload.cpp:2116
- The signature of Module::GetIL was changed to take an RVA instead of a DWORD. Ensure the corresponding declaration in the header (e.g. ceeload.h) is updated to match to avoid build or linkage errors.
TADDR Module::GetIL(RVA target)
...raries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveILGeneratorTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs
Show resolved
Hide resolved
Remove unused member fields.
...es/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveTypeBuilderAPIsTests.cs
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeMethodBuilder.cs
Show resolved
Hide resolved
/ba-g WASM timeouts Unrelated failures on mono (ios/tvos) |
Fixes #116649
The underlying Reflection.Emit infrastructure was treating an RVA of 0 as "normal" and indexing into the dynamic IL data stream. This caused confusion in the rest of the system since an RVA of 0 means there is no IL body (that is, no IL Header). Making Reflection.Emit consistent ensures that scenarios relying on an RVA of 0 now operate identical with those emitted by .NET compilers.
This change also removes some of the checks that were put in place for Reflection.Emit and relies on the EE at runtime to handle those cases.