-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Restore erroneously removed encoding of the argument count in a generic method instantiation #98731
Conversation
…ic method instantiation
@jeffschwMSFT who else should we add to the review? In terms of testing on CI, are multicore jit tests kicked off manually? |
The only test coverage I found so far is https://github.com/dotnet/runtime/tree/71344cea37d84bb403d3d303b2add9e6135c539c/src/tests/baseservices/TieredCompilation and it doesn't appear to verify whether the profiled methods were actually jitted. It looks like it's meant for manual verification, presumably on a runtime build with logging turned on. |
Could you please share a link to the PR where this happened? |
This happened in #68717 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks
Backport candidate? |
Yes, I think so. |
/backport to release/8.0-staging |
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8008709752 |
When IBC was removed, we accidentally removed the argument count from the encoding of generic method instantiations (it was previously being conditionally omitted when IBC was active, so it should have stayed.)
This can cause crashes in multicore jit scenarios and generally makes it not work for generic methods.
Adding this back will ensure that profiles recorded in the future will be valid, though we can't fix existing ones.
Still looking into where & how I could add regression coverage for this, but I'm not sure it's necessary.