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 totalILArgs for explicithis #85347

Merged
merged 2 commits into from
Apr 26, 2023
Merged

Conversation

MichalPetryka
Copy link
Contributor

Fixes totalILArgs counting this twice with explicithis.

Split off from #85197.

@MichalPetryka MichalPetryka marked this pull request as ready for review April 25, 2023 19:21
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 25, 2023
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

What about:

private bool hasThis() { return ((callConv & CorInfoCallConv.CORINFO_CALLCONV_HASTHIS) != 0); }
private bool hasExplicitThis() { return ((callConv & CorInfoCallConv.CORINFO_CALLCONV_EXPLICITTHIS) != 0); }
private uint totalILArgs() { return (uint)(numArgs + (hasThis() ? 1 : 0)); }

I suspect there are a few more places that will need changes, would suggest you review the places where totalILArgs is used and make sure they're ok. It may be hard to surface these during testing as explicit this signatures are restricted to cases we don't optimize (yet).

@AndyAyersMS
Copy link
Member

@markples perhaps you can review this one?

@markples
Copy link
Member

It looks like hasthis is also a thing to audit, though that would uncover separate bugs and doesn't need to be part of this PR. Unfortunately, there are a lot of them. In general, it seems to be documented that explicit-call is only for signatures, not methods themselves (assuming that this isn't one of those things were the spec isn't followed), so places that check hascall and ignore explicitcall might be fine anyway.

Copy link
Member

@markples markples left a comment

Choose a reason for hiding this comment

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

My hasthis comment doesn't seem like it should block anything here. Thanks!

@AndyAyersMS AndyAyersMS merged commit ff0f603 into dotnet:main Apr 26, 2023
@AndyAyersMS
Copy link
Member

@MichalPetryka thanks -- and thanks for splitting this off from your other PR.

@ghost ghost locked as resolved and limited conversation to collaborators May 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants