-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Implement remaining unimplemented APIs for Builder types #96805
Conversation
…s, fix bugs found
…ctor field/method/type token logic because of global members
Tagging subscribers to this area: @dotnet/area-system-reflection-emit Issue Details
Fixes #96436
|
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/FieldBuilderImpl.cs
Outdated
Show resolved
Hide resolved
...braries/System.Reflection.Emit/src/System/Reflection/Emit/GenericTypeParameterBuilderImpl.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/MethodBuilderImpl.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/MethodBuilderImpl.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/PseudoCustomAttributesData.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs
Show resolved
Hide resolved
...es/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedTextSection.cs
Outdated
Show resolved
Hide resolved
...es/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveModuleBuilderTests.cs
Outdated
Show resolved
Hide resolved
…t/TypeBuilderImpl.cs Co-authored-by: Aaron Robinson <arobins@microsoft.com>
src/libraries/System.Reflection.Emit/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/MethodBuilderImpl.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
Show resolved
Hide resolved
...ries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveILGeneratorTests.cs
Outdated
Show resolved
Hide resolved
...ries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveILGeneratorTests.cs
Show resolved
Hide resolved
public void EmitCalliBlittable() | ||
{ | ||
RemoteExecutor.Invoke(() => |
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.
Why was the use of remote executor changed?
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.
Now using collectable ALC instead of default, the discussion is here #96805 (comment)
runtime/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveTools.cs
Lines 12 to 22 in e6d9333
class TestAssemblyLoadContext : AssemblyLoadContext | |
{ | |
public TestAssemblyLoadContext() : base(isCollectible: true) | |
{ | |
} | |
protected override Assembly? Load(AssemblyName name) | |
{ | |
return null; | |
} | |
} |
@@ -515,9 +515,9 @@ public void SaveMultipleGenericTypeParametersToEnsureSortingWorks() | |||
saveMethod.Invoke(assemblyBuilder, new object[] { file.Path }); | |||
|
|||
Module m = AssemblySaveTools.LoadAssemblyFromPath(file.Path).Modules.First(); | |||
Type[] type1Params = m.GetTypes()[2].GetGenericArguments(); | |||
Type[] type1Params = m.GetTypes()[0].GetGenericArguments(); |
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.
Why were the items reversed before?
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.
Before the type tokens created on TypeBuilder.CreateType()
and before saving the types are ordered with the token order, the types here created in inverse order, you can see that above on row 512-514.
Now with a global type that has global methods/fields this ordering/token assignment not working anymore, now all types and its members tokens created before save (therefore in defined order no matter when TypeBuilder.CreateType()
called)
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Steve Harter <steveharter@users.noreply.github.com>
Assert.Throws<InvalidOperationException>(() => module.DefineInitializedData("MyField2", new byte[] { 1, 0, 1 }, FieldAttributes.Public)); | ||
} | ||
|
||
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotMonoRuntime))] |
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.
The Assert at row 257 fails on mono, not sure it is expected ...
Assert.Equal(name + "_0" + "_" + address.ToString(), name + "_" + (address % minAlignmentRequired).ToString() + "_" + address.ToString());
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.
I can see the failure locally after building with mono runtime, though could not find the root cause, so filed an issue.
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.
@buyaa-n I don't understand the test. What is forcing the address to be aligned? is it just based on the length of the blob of data?
I'm looking at sections II.16.3.1 ("Data declaration") and II.16.3.2 ("Accessing data from the PE file") in ECMA-335 and I don't see anything about alignment.
The only mention of alignment I see is:
(Each data item shall have a DataLabel if it is to be referenced afterwards; missing a DataLabel is useful in order to insert alignment padding between data items)
so is DefineInitializedData
responsible for adding the padding? And the test is verifying that the right amount got added?
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.
I don't understand the test. What is forcing the address to be aligned? is it just based on the length of the blob of data?
I'm looking at sections II.16.3.1 ("Data declaration") and II.16.3.2 ("Accessing data from the PE file") in ECMA-335 and I don't see anything about alignment.
Right, when I encountered below assert that is failing when a byte array with a size of 1 or 4 (not multiple of 8) is provided for the field RVA data, I also checked ECMA spec and did not found anything about this alignment requirement
Line 190 in 2f63046
Debug.Assert(MappedFieldDataSize % MappedFieldDataAlignment == 0); |
It looks like a runtime requirement, not ECMA spec, anyway I had to add alignment for each field RVA:
runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
Lines 453 to 454 in f36b7cd
fieldDataBuilder.WriteBytes(field._rvaData!); | |
fieldDataBuilder.Align(ManagedPEBuilder.MappedFieldDataAlignment); |
Then this test and the metadata Assert passes.
so is DefineInitializedData responsible for adding the padding? And the test is verifying that the right amount got added?
By my understanding yes, the failing part of the test is asserting if the fields aligned properly. Before adding the alignment same failure was happening locally, now this fail on mono even there is no mono specific code
...es/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveModuleBuilderTests.cs
Outdated
Show resolved
Hide resolved
All failures are unrelated and known, merging |
* Implement 'DefinPInvokeMethod', save required/optional CustomModifiers, fix bugs found * Add global method, get method impl and tests * Implement DefineInitializedData(...) and UninitializedData(...), refactor field/method/type token logic because of global members * Update src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs Co-authored-by: Aaron Robinson <arobins@microsoft.com> * Load assemblies in unloadable context Co-authored-by: Steve Harter <steveharter@users.noreply.github.com> * Add test for DefineUninitializedData(...) * Set parameter count to 0 for RtFieldInfo * Throw when member token is not populated when Module.Get***MetadataToken methods called * Retrieving standalone signature not supported on mono * Create byte array with the size directly instead of null check --------- Co-authored-by: Aaron Robinson <arobins@microsoft.com> Co-authored-by: Steve Harter <steveharter@users.noreply.github.com>
DefineInitializedDataCore(...)
,DefineUninitializedDataCore(...)
,DefinePInvokeMethodCore(...)
forTypeBuilerImpl
.CreateGlobalFunctionsCore()
,DefineGlobalMethodCore(...)
,DefineInitializedDataCore(...)
,DefineUninitializedDataCore(...)
,DefinePInvokeMethodCore(...)
,GetMethodImpl(...)
,GetMethods()
forModuleBuilerImpl
.requiredCustomModifiers
,optionalCustomModifiers
for fields, method return type and parameters, which mostly involves Signature changesFixes #96436 and #97116
Contributes to #92975