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 fat call transform for x86 #1451

Merged
merged 1 commit into from
Jan 9, 2020

Conversation

MichalStrehovsky
Copy link
Member

AddArgumentToTail with a null first argument will AV.

I think this also works with methods that take a return buffer (methods that return large structs), but might require a pair of eyes more experienced in this codebase.

`AddArgumentToTail` with a null first argument will AV.

I *think* this also works with methods that take a return buffer (methods that return large structs), but might require a pair of eyes more experienced in this codebase.
@MichalStrehovsky MichalStrehovsky added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 8, 2020
@sandreenko
Copy link
Contributor

I think this also works with methods that take a return buffer (methods that return large structs), but might require a pair of eyes more experienced in this codebase.

Could you please explain that to me? What is special about methods with a return buffer in the context of this change?

@MichalStrehovsky
Copy link
Member Author

I think this also works with methods that take a return buffer (methods that return large structs), but might require a pair of eyes more experienced in this codebase.

Could you please explain that to me? What is special about methods with a return buffer in the context of this change?

The #if branch for the #else that I'm touching here is handling fatCall->HasRetBufArg(). I was wondering if I need to handle it in the #else too, but things seem to work without that.

@sandreenko
Copy link
Contributor

The #if branch for the #else that I'm touching here is handling fatCall->HasRetBufArg(). I was wondering if I need to handle it in the #else too, but things seem to work without that.

Got it, yes, I have checked that and it is correct, gtPrependNewCallArg can handle args = nullptr.

Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@MichalStrehovsky MichalStrehovsky merged commit c2c25c8 into dotnet:master Jan 9, 2020
@MichalStrehovsky MichalStrehovsky deleted the fatcall-x86 branch January 9, 2020 07:11
@@ -439,7 +439,14 @@ class IndirectCallTransformer
fatCall->gtCallArgs = compiler->gtPrependNewCallArg(hiddenArgument, fatCall->gtCallArgs);
}
#else
AddArgumentToTail(fatCall->gtCallArgs, hiddenArgument);
if (fatCall->gtCallArgs == nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this check actually be in AddArgumentToTail?

Copy link
Member Author

Choose a reason for hiding this comment

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

We would have to change the signature of the method and either pass fatCall->gtCallArgs by reference or have it return a Use* that we can assign. Doable, but I already merged this and I really dislike dealing with the JIT code formatter (which I'm sure would moan about something).

Copy link
Member

Choose a reason for hiding this comment

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

@MichalStrehovsky if you use jitutils then jit code formatting is pretty simple

  • clone jitutils repo
  • run bootstrap.cmd from repo root
  • cd to your runtime repo
  • run jit-format --fix --untidy --runtime src\coreclr

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndyAyersMS thanks! I'm bookmarking your comment

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants