Skip to content

Conversation

@trylek
Copy link
Member

@trylek trylek commented Jun 29, 2022

I have created a regression test to cover the scenario described in the GitHub issue.
In runtime main the test is passing now for me so I don't think there's anything to
investigate right now.

Thanks

Tomas

Fixes: #71319

@MichalStrehovsky
Copy link
Member

Thank you for adding the regression test! Looks like it's asserting the checked legs, so we actually found something interesting!

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

The test looks good. Maybe we could check it in ActiveIssue'd. Looks like the Mono team also has a bug to look at.

@trylek
Copy link
Member Author

trylek commented Jun 29, 2022

Hmm, that is weird, I believe I tested the test locally in debug mode and I didn't see any assertions although this one looks deterministic. I'll take a first look to get more info regarding how we should proceed here. Thanks Michal for reviewing!

@trylek trylek force-pushed the RegressionTestGitHub71319 branch from 1a14b47 to 0bcd1bb Compare July 7, 2022 23:46
@trylek
Copy link
Member Author

trylek commented Jul 7, 2022

I have managed to repro the failure locally - it was a bit tricky as it only reproes when tiered compilation is turned off. According to my understanding the problem is that CanCastTo uses multiple code paths some of which (most notably the Array-related code paths) require cooperative GC mode; I have fixed this by changing ResolveVirtualStaticMethod to use the targeted version CanCastToInterface which doesn't require the GC cooperative mode.

@trylek trylek force-pushed the RegressionTestGitHub71319 branch from 0bcd1bb to 26b9035 Compare July 10, 2022 21:07
@trylek
Copy link
Member Author

trylek commented Jul 11, 2022

I have blocked out the failing Mono version of the test and the PR pipeline is green now. @davidwrighton, could you please review the runtime change to double-check that it's functionally equivalent?

@trylek
Copy link
Member Author

trylek commented Jul 12, 2022

Considering David is on vacation this week I'm inclined to merge this in before today Preview 7 fork as it fixes an important CoreCLR runtime bug; I'll address any feedback David might have in a follow-up change once he's back. Assuming there's no pushback I'll merge this in after 2PM today.

@trylek trylek merged commit 29a6a45 into dotnet:main Jul 12, 2022
@trylek trylek deleted the RegressionTestGitHub71319 branch July 12, 2022 21:03
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 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.

Targets of variant static interface method resolution

2 participants