[mono][interp] Disable inlining of methods that call methods using StackCrawlMark #116134
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The stack mark is used only to prevent inlining of the callers of this method (since it sets a flag on the method in question). The stack mark itself is not currently used on mono.
BrzVlad
requested review from
kotlarmilos,
steveisok,
vitek-karas and
thaystg
as code owners
github-actions
bot
added
the
needs-area-label
BrzVlad
removed
the
needs-area-label
There was a problem hiding this comment.
Pull Request Overview
This pull request disables inlining for methods that call methods marked with special flags (such as those using StackCrawlMark) to prevent incorrect results due to inlining. Key changes include:
- Adding a flag check (METHOD_ATTRIBUTE_REQSECOBJ) in the interpreter transform to prevent inlining.
- Updating the signature of ves_icall_System_Reflection_Assembly_GetCallingAssembly and its registration in icall-def.h.
- Replacing the public extern GetCallingAssembly with a managed implementation that passes a StackCrawlMark parameter in Assembly.Mono.cs.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mono/mono/mini/interp/transform.c | Added a flag check to disable inlining for methods requiring a security object. |
| src/mono/mono/metadata/icall.c | Updated signature of GetCallingAssembly to accept a StackCrawlMark parameter. |
| src/mono/mono/metadata/icall-def.h | Adjusted the HANDLES macro to reflect the new parameter count and type. |
| src/mono/System.Private.CoreLib/src/System/Reflection/Assembly.Mono.cs | Replaced the extern GetCallingAssembly with a managed wrapper that sets a default StackCrawlMark. |
Comments suppressed due to low confidence (1)
src/mono/System.Private.CoreLib/src/System/Reflection/Assembly.Mono.cs:46
- The removal of the DynamicSecurityMethod attribute from GetCallingAssembly might affect required security semantics for stack walks. Verify if this change is intentional and restore the attribute if needed.
public static Assembly GetCallingAssembly()
| @@ -3854,6 +3854,8 @@ interp_transform_call (TransformData *td, MonoMethod *method, MonoMethod *target | |||
| } else if (td->method->wrapper_type == MONO_WRAPPER_RUNTIME_INVOKE) { | |||
| // This scenario causes https://github.com/dotnet/runtime/issues/83792 | |||
| return FALSE; | |||
| } else if (target_method->flags & METHOD_ATTRIBUTE_REQSECOBJ) { | |||
There was a problem hiding this comment.
[nitpick] Consider adding a brief comment explaining why the METHOD_ATTRIBUTE_REQSECOBJ flag check is necessary in this context to prevent unintended inlining.
Copilot uses AI. Check for mistakes.
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos |
kotlarmilos
approved these changes
Jun 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With JIT we don't inline methods doing calls. This is no longer the case on interpreter, since .NET8, after #83548. The side effect of that change was that, because we weren't checking for the presence of the StackCrawlMark attribute, we could be inlining the callers of special methods like
Type.GetType,MethodBase.GetCurrentMethod,Assembly.GetCallingAssemblyetc. This would make them return incorrect results. Given inlining happens only once the method is tiered up, this regression wasn't detected.