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

fix invalid IL in profiler stub helper #45453

Merged
merged 2 commits into from
Dec 3, 2020

Conversation

davmason
Copy link
Member

@davmason davmason commented Dec 2, 2020

Fixes #45411

Profilers can set COR_PRF_MONITOR_CODE_TRANSITIONS to be notified when a PInvoke, COM call, etc causes execution to leave or enter the runtime. It works by injecting some additional IL in the transition stubs. The injected IL was loading integer constants of 0 for the pThis argument of StubHelpers.ProfilerBeginTransitionCallback(IntPtr pSecretParam, IntPtr pThread, object pThis). This worked fine but was technically illegal according to the spec. The first two arguments do want an integer constant for the IntPtr, but the third is a reference and needs to be a reference type.

#43386 added checks that would cause the JIT to throw an InvalidProgramException if illegal implicit casts were detected, causing some profiler tests to fail.

@davmason davmason requested a review from a team December 2, 2020 01:54
@davmason davmason self-assigned this Dec 2, 2020
@ghost
Copy link

ghost commented Dec 2, 2020

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

Issue Details

Fixes #45411

Profilers can set COR_PRF_MONITOR_CODE_TRANSITIONS to be notified when a PInvoke, COM call, etc causes execution to leave or enter the runtime. It works by injecting some additional IL in the transition stubs. The injected IL was loading integer constants of 0 for the pThis argument of StubHelpers.ProfilerBeginTransitionCallback(IntPtr pSecretParam, IntPtr pThread, object pThis). This worked fine but was technically illegal according to the spec. The first two arguments do want an integer constant for the IntPtr, but the third is a reference and needs to be a reference type.

#44506 added checks that would cause the JIT to throw an InvalidProgramException if illegal implicit casts were detected, causing some profiler tests to fail.

Author: davmason
Assignees: davmason
Labels:

area-Diagnostics-coreclr

Milestone: -

@davmason
Copy link
Member Author

davmason commented Dec 2, 2020

@sandreenko while investigating and testing this I found out that the checks you added in #43386 are ignored if the call ends up being inlined. That seems counter intuitive to me, I am not sure if you know about that already so I thought I would point it out.

@sandreenko
Copy link
Contributor

@sandreenko while investigating and testing this I found out that the checks you added in #43386 are ignored if the call ends up being inlined. That seems counter intuitive to me, I am not sure if you know about that already so I thought I would point it out.

I am not sure what you mean, do we try to inline a method that does not pass impCheckImplicitArgumentCoercion` checks? Could you give me an example?

I can imagine a situation where we have 3 calls: call1->call2(arg a)->call3(arg b) and when we check call2(arg a) impCheckImplicitArgumentCoercion return true, when we check call3(arg b) inside call2 it returns true, but when we inline call3 into call2 it returns false somehow but then I expect this assert in impInlineInitVars to fail

assert(impCheckImplicitArgumentCoercion(sigType, inlArgNode->gtType));

@davmason
Copy link
Member Author

davmason commented Dec 2, 2020

@sandreenko nevermind, I reran my tests and I see them failing even with inlined methods. Last night when I was testing I must have been changing multiple things at once,

Copy link
Contributor

@sdmaclea sdmaclea left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't know this code area well.

@davmason davmason merged commit fde9281 into dotnet:master Dec 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 2, 2021
@davmason davmason deleted the profiler_interop branch January 20, 2021 08:58
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.

COR_PRF_MONITOR_CODE_TRANSITIONS causes InvalidProgramException
3 participants