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

Unblock SDK merge by fixing Assembly.GetCallingAssembly() #69225

Merged
merged 1 commit into from
May 12, 2022

Conversation

steveharter
Copy link
Member

SDK merge is blocked due to test failures; see dotnet/sdk#25317 (comment).

The issue is that the test is calling Assembly.GetCallingAssembly() which returns the "Anonymously Hosted DynamicMethods Assembly" instead of the expected assembly. This broke due to the recent Invoke+Emit work where a DynamicMethod is created to invoke that method and the native code that attempts to skip the reflection stack frames was not correct.

Todo:

  • This issue can be reproduced in a stand-alone console app by simply using reflection to invoke a method that calls GetCallingAssembly(). However, adding the equivalent to an Xunit test I was unable to reproduce the issue. The console app included several more stack frames, including the "InvokeStub_" frame and the MethodInvoker or ConstructorInvoker frame however the xUnit test did not include those frames. We need to determine why this is the case and add an appropriate test.
  • The fix here simply checks for a dynamic method starting with "InvokeStub_"; ideally it also checks to see if that dynamic method is the one generated for invoke vs. some other code that also generates a dynamic method that starts with "InvokeStub_".

@ghost
Copy link

ghost commented May 12, 2022

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

Issue Details

SDK merge is blocked due to test failures; see dotnet/sdk#25317 (comment).

The issue is that the test is calling Assembly.GetCallingAssembly() which returns the "Anonymously Hosted DynamicMethods Assembly" instead of the expected assembly. This broke due to the recent Invoke+Emit work where a DynamicMethod is created to invoke that method and the native code that attempts to skip the reflection stack frames was not correct.

Todo:

  • This issue can be reproduced in a stand-alone console app by simply using reflection to invoke a method that calls GetCallingAssembly(). However, adding the equivalent to an Xunit test I was unable to reproduce the issue. The console app included several more stack frames, including the "InvokeStub_" frame and the MethodInvoker or ConstructorInvoker frame however the xUnit test did not include those frames. We need to determine why this is the case and add an appropriate test.
  • The fix here simply checks for a dynamic method starting with "InvokeStub_"; ideally it also checks to see if that dynamic method is the one generated for invoke vs. some other code that also generates a dynamic method that starts with "InvokeStub_".
Author: steveharter
Assignees: steveharter
Labels:

area-System.Reflection, area-VM-coreclr

Milestone: -

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Agree with the TODOs

@AaronRobinsonMSFT
Copy link
Member

@steveharter Do we want an issue to track this or is this next on your plate?

@steveharter
Copy link
Member Author

Test failures due to #69232

@steveharter
Copy link
Member Author

@steveharter Do we want an issue to track this or is this next on your plate?

Added #69251

jkotas added a commit to jkotas/runtime that referenced this pull request May 15, 2022
jkotas added a commit that referenced this pull request May 15, 2022
)

* Revert "Fix Assembly.GetCallingAssembly() (#69225)"

This reverts commit 8420dae.

* Revert "Add IL Emit support for MethodInfo.Invoke() and friends (#67917)"

This reverts commit 5195418.
@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 2022
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.

3 participants