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

Respect IsDynamicCodeSupported in more places in Linq.Expressions #88539

Merged
merged 4 commits into from
Jul 11, 2023

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jul 7, 2023

  • CanEmitObjectArrayDelegate
  • CanCreateArbitraryDelegates

These properties are all set to false when running on NativeAOT, so have them respect RuntimeFeature.IsDynamicCodeSupported when not running on NativeAOT.

However, CanEmitObjectArrayDelegate needs a work around because DynamicDelegateAugments.CreateObjectArrayDelegate does not exist in CoreClr's System.Private.CoreLib.

Allow System.Linq.Expressions to create an object[] delegate using Ref.Emit even though RuntimeFeature.IsDynamicCodeSupported is set to false (ex. using a feature switch). To enable this, add an internal method in CoreLib that temporarily allows the current thread to skip the RuntimeFeature check and allows DynamicMethod instances to be created. When System.Linq.Expressions needs to generate one of these delegates, it calls the internal method through Reflection and continues to use Ref.Emit to generate the delegate.

Fix #81803

* CanEmitObjectArrayDelegate
* CanCreateArbitraryDelegates

These properties are all set to `false` when running on NativeAOT, so have them respect RuntimeFeature.IsDynamicCodeSupported.

Fix dotnet#81803
…sting in CoreClr's System.Private.CoreLib.

Allow System.Linq.Expressions to create an object[] delegate using Ref.Emit even though RuntimeFeature.IsDynamicCodeSupported is set to false (ex. using a feature switch). To enable this, add an internal method in CoreLib that temporarily allows the current thread to skip the RuntimeFeature check and allows DynamicMethod instances to be created. When System.Linq.Expressions needs to generate one of these delegates, it calls the internal method through Reflection and continues to use Ref.Emit to generate the delegate.
@ghost
Copy link

ghost commented Jul 7, 2023

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details
  • CanEmitObjectArrayDelegate
  • CanCreateArbitraryDelegates

These properties are all set to false when running on NativeAOT, so have them respect RuntimeFeature.IsDynamicCodeSupported when not running on NativeAOT.

However, CanEmitObjectArrayDelegate needs a work around because DynamicDelegateAugments.CreateObjectArrayDelegate does not exist in CoreClr's System.Private.CoreLib.

Allow System.Linq.Expressions to create an object[] delegate using Ref.Emit even though RuntimeFeature.IsDynamicCodeSupported is set to false (ex. using a feature switch). To enable this, add an internal method in CoreLib that temporarily allows the current thread to skip the RuntimeFeature check and allows DynamicMethod instances to be created. When System.Linq.Expressions needs to generate one of these delegates, it calls the internal method through Reflection and continues to use Ref.Emit to generate the delegate.

Fix #81803

Author: eerhardt
Assignees: -
Labels:

area-System.Linq.Expressions

Milestone: -

@eerhardt
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines -14 to 16
// This can be flipped to true using feature switches at publishing time
// This can be flipped to false using feature switches at publishing time
internal static bool CanEmitObjectArrayDelegate => true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to change this into

internal static bool CanEmitObjectArrayDelegate => RuntimeFeature.CanEmitObjectArrayDelegate;

If it remains a constant value it will still cause issues for iOS-like platforms as described in: #87924

My last comment: #87924 (comment) breaks down the benefits of having these conditional variables as feature switches.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be a good enhancement to make in a future PR. It isn't necessary to fix the issue this PR is fixing - #81803.

@eerhardt
Copy link
Member Author

Frozen collection test failures in NativeAOT builds are #88628.

@eerhardt
Copy link
Member Author

Failure is #88582.

@eerhardt eerhardt merged commit d2fae90 into dotnet:main Jul 11, 2023
163 of 166 checks passed
@eerhardt eerhardt deleted the Fix81803 branch July 11, 2023 16:55
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
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.

PublishAot affects non-AOT behavior
4 participants