-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[release/9.0-staging] Fix obtaining type handles of IDynamicInterfaceCastableImplementation #109909
[release/9.0-staging] Fix obtaining type handles of IDynamicInterfaceCastableImplementation #109909
Conversation
Fixes #109496. We try to optimize away type handles (MethodTables) of types that are only needed due to metadata. Trying to grab type handle of such type throws. This fixes it by unconditionally generating type handles of IDynamicInterfaceCastableImplementation.
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
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. we will take for consideration in 9.0.x
@MichalStrehovsky please take a look at the PR failures and merge when ready |
The logs expired weeks ago. The timing out leg doesn't run any of the affected code and cannot possibly be related. Going to add a bypass, but don't have rights to merge. |
/ba-g timeout in unrelated leg |
Never mind, looks like I do. For some reason I thought these branches are protected. |
Backport of #109875 to release/9.0-staging
/cc @MichalStrehovsky
Customer Impact
Hit by a 1st party in #109496. Users using CsWinRT might run into an issue where the compiler optimized away a part of a type that is necessary for execution.
Regression
Not a regression, this was broken since 7.0, but nobody was building WinUI apps with native AOT because CsWinRT didn't support it.
Testing
This requires a bit of bad luck to hit. The compiler will only optimize this away if the only thing that references the type in the entire app is a custom attribute. We now have a targeted test for this situation.
Risk
Low risk, this just suppresses an optimization.
IMPORTANT: If this backport is for a servicing release, please verify that:
The PR target branch is
release/X.0-staging
, notrelease/X.0
.If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.