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 stack walking and reporting of default interface methods #21525

Merged
merged 7 commits into from Dec 18, 2018

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Dec 13, 2018

Default interface methods in their unresolved state don't have a generic context. The generic context is only added once the method is resolved to its implementation.

Fixes #16376. Fixes #16898. Fixes #21044.

Default interface methods in their unresolved state don't have a generic context. The generic context is only added once the method is resolved to its implementation.
@MichalStrehovsky MichalStrehovsky added this to the 3.0 milestone Dec 13, 2018
@jkotas
Copy link
Member

jkotas commented Dec 13, 2018

src/vm/frames.h:2208:5: error: 'VPtrSize' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]

@janvorli
Copy link
Member

janvorli commented Dec 13, 2018

src/vm/frames.h:2208:5: error: 'VPtrSize' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]

This happens when you try to use override in a class where it was not used before. Basically, once you use override for a single virtual method, you need to use it for all. So here, it would be better to stick with just marking the method as virtual and not using the override.

DWORD cbSigSize;
pFunction->GetSig(&pSig, &cbSigSize);

MetaSig msig(pSig, cbSigSize, pFunction->GetModule(), &typeContext);
Copy link
Member

@jkotas jkotas Dec 14, 2018

Choose a reason for hiding this comment

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

Do we need a matching fix for PromoteCallerStackUsingGCRefMap - adjust the GCRef map that gets saved into R2R image at crossgen time?

You should be able to verify it by compiling the affected tests using crossgen before running them.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Dec 14, 2018

Choose a reason for hiding this comment

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

Yeah, it didn't work. Thanks for spotting! I pushed out a fix.

src/vm/frames.h Outdated
//
// See code:CEEInfo::getMethodSigInternal
//
return pMD->GetMethodTable()->IsInterface();
Copy link
Member

@jkotas jkotas Dec 14, 2018

Choose a reason for hiding this comment

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

Are there any cases where we actually get into StubDispatchFrame with hidden generic arg?

Maybe this can just return TRUE unconditionally.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Dec 14, 2018

Choose a reason for hiding this comment

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

Normal virtual method calls on generic types maybe? Or they don't go through here?

Copy link
Member

@jkotas jkotas Dec 14, 2018

Choose a reason for hiding this comment

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

Normal virtual method calls do not have hidden generic arg. The context is in this pointer.

Generic virtual method calls do not go through here.

Copy link
Member

@jkotas jkotas Dec 14, 2018

Choose a reason for hiding this comment

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

I will leave it up to you whether to do this simplification.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Dec 17, 2018

Choose a reason for hiding this comment

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

Okay, simplified it, but I also added an assert to guard that assumption.

jkotas
jkotas approved these changes Dec 14, 2018
@jkotas
Copy link
Member

jkotas commented Dec 17, 2018

src\vm\frames.h(2281): warning C4189: 'pMD': local variable is initialized but not referenced

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Dec 17, 2018

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests
@dotnet-bot test Windows_NT x64 Formatting

@MichalStrehovsky MichalStrehovsky merged commit 352a5cb into dotnet:master Dec 18, 2018
31 checks passed
@MichalStrehovsky MichalStrehovsky deleted the gcStress branch Dec 18, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this issue Feb 18, 2022
…coreclr#21525)

Default interface methods in their unresolved state don't have a generic context. The generic context is only added once the method is resolved to its implementation.

Commit migrated from dotnet/coreclr@352a5cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants