Skip to content
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

Merged
merged 16 commits into from
Jan 19, 2024

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Jan 10, 2024

  • Implemented DefineInitializedDataCore(...), DefineUninitializedDataCore(...), DefinePInvokeMethodCore(...) for TypeBuilerImpl.
  • Implemented CreateGlobalFunctionsCore(), DefineGlobalMethodCore(...), DefineInitializedDataCore(...), DefineUninitializedDataCore(...), DefinePInvokeMethodCore(...), GetMethodImpl(...), GetMethods() for ModuleBuilerImpl.
  • requiredCustomModifiers, optionalCustomModifiers for fields, method return type and parameters, which mostly involves Signature changes
  • Had to refactor type/method/field token generation logic again because of global method/fields
  • Fix bugs found and reported

Fixes #96436 and #97116
Contributes to #92975

@ghost
Copy link

ghost commented Jan 10, 2024

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Implemented DefineInitializedDataCore(...), DefineUninitializedDataCore(...), DefinePInvokeMethodCore(...) for TypeBuilerImpl.
  • Implemented CreateGlobalFunctionsCore(), DefineGlobalMethodCore(...), DefineInitializedDataCore(...), DefineUninitializedDataCore(...), DefinePInvokeMethodCore(...), GetMethodImpl(...), GetMethods() for ModuleBuilerImpl.
  • requiredCustomModifiers, optionalCustomModifiers for fields, method return type and parameters, which mostly involves Signature changes
  • Had to refactor type/method/field token generation logic again because of global method/fields
  • Fix bugs found and reported

Fixes #96436
Contributes to #92975

Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection.Emit

Milestone: -

…t/TypeBuilderImpl.cs

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@buyaa-n buyaa-n marked this pull request as ready for review January 11, 2024 23:38
@buyaa-n buyaa-n changed the title Implement remaining unimplemented APIs for ***Builder types Implement remaining unimplemented APIs for Builder types Jan 11, 2024
public void EmitCalliBlittable()
{
RemoteExecutor.Invoke(() =>
Copy link
Member

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?

Copy link
Member Author

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)

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();
Copy link
Member

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?

Copy link
Member Author

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)

@buyaa-n
Copy link
Member Author

buyaa-n commented Jan 19, 2024

All failures are unrelated and known, merging

@buyaa-n buyaa-n merged commit 62d33ee into dotnet:main Jan 19, 2024
148 of 152 checks passed
@buyaa-n buyaa-n deleted the more_impl branch January 19, 2024 21:33
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
* 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>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CheckInterfaces is too permissive and not permissive enough
4 participants