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

P/Invokes should use C types not C++. #94942

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Nov 18, 2023

A recent C++ toolchain update uncovered an invalid P/Invoke signature using C++ types as opposed to C types. This caused the test to fail due to invalid assumptions. The test was updated to be valid.

Fixes #94931

@AaronRobinsonMSFT
Copy link
Member Author

/cc @jkotas @elinor-fung

@jkotas
Copy link
Member

jkotas commented Nov 18, 2023

We do not seem to have the difference between managed and unmanaged x86/x64 calling conventions for small return types noted in https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/botr/clr-abi.md . It would be nice to fix that.

For unmanaged calling conventions, the state of unused bits in the value returned in RAX or XMM0 is undefined. (https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170#return-values)

For managed calling conventions, the primitive values smaller than 32-bits must be zero-extended (or sign-extended?) to 32-bits. @dotnet/jit-contrib What is the exact rule that the JIT implements?

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Nov 18, 2023

For managed methods, the callee widens small return values. For native methods, the caller widens small return values.

I think we always zero extend. Looks like we sign extend in some cases.

if (!bIntrinsicImported)
{
//-------------------------------------------------------------------------
//
/* If the call is of a small type and the callee is managed, the callee will normalize the result
before returning.
However, we need to normalize small type values returned by unmanaged
functions (pinvoke). The pinvoke stub does the normalization, but we need to do it here
if we use the shorter inlined pinvoke stub. */
if (checkForSmallType && varTypeIsIntegral(callRetTyp) && genTypeSize(callRetTyp) < genTypeSize(TYP_INT))
{
call = gtNewCastNode(genActualType(callRetTyp), call, false, callRetTyp);
}
}
impPushOnStack(call, tiRetVal);
}

@AaronRobinsonMSFT
Copy link
Member Author

@AndyAyersMS Is there a follow up issue to file here?

@AndyAyersMS
Copy link
Member

You mean for codegen? I don't see one, unless we're not actually doing what is outlined above.

@AaronRobinsonMSFT
Copy link
Member Author

This is step one of the fix. The current update works and passes on macOS. Pushing to CI to make sure Windows and Linux remain in the same state.

@AaronRobinsonMSFT
Copy link
Member Author

@jkotas Please take another look at this. I'd like to get the CI unblocked. Feel free to merge if you are okay with the changes.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you

@jkotas jkotas merged commit f0ba96a into dotnet:main Nov 20, 2023
101 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_94931 branch November 20, 2023 15:07
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants