JIT: Sign/zero-extend small primitive arguments for pinvokes on arm32 and Apple arm64

docs/design/coreclr/botr/clr-abi.md

@@ -23,6 +23,7 @@ ARM64: See [Overview of ARM64 ABI conventions](https://learn.microsoft.com/cpp/b
 ## Non-Windows ABI documentation
 
 Arm corporation ABI documentation (for ARM32 and ARM64) is [here](https://developer.arm.com/architectures/system-architectures/software-standards/abi) and [here](https://github.com/ARM-software/abi-aa).
+Apple's ARM64 calling convention differences can be found [here](https://developer.apple.com/documentation/xcode/writing-arm64-code-for-apple-platforms).
 
 The Linux System V x86_64 ABI is documented in [System V Application Binary Interface / AMD64 Architecture Processor Supplement](https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-1.0.pdf), with document source material [here](https://gitlab.com/x86-psABIs/x86-64-ABI).
 
@@ -117,6 +118,10 @@ ARM64-only: When a method returns a structure that is larger than 16 bytes the c
 
 Primitive value types smaller than 32-bits are widened to 32-bits: signed small types are sign extended and unsigned small types are zero extended. This can be different from the standard calling conventions that may leave the state of unused bits in the return register undefined.
 
+## Small primitive arguments
+
+Small primitive arguments have undefined upper bits. This can be different from the standard calling conventions that may require normalization (e.g. on ARM32 and Apple ARM64).
+
 # PInvokes
 
 The convention is that any method with an InlinedCallFrame (either an IL stub or a normal method with an inlined PInvoke) saves/restores all non-volatile integer registers in its prolog/epilog respectively. This is done so that the InlinedCallFrame can just contain a return address, a stack pointer and a frame pointer. Then using just those three it can start a full stack walk using the normal RtlVirtualUnwind.

src/coreclr/jit/importercalls.cpp

@@ -6184,6 +6184,18 @@ void Compiler::impPopCallArgs(CORINFO_SIG_INFO* sig, GenTreeCall* call)
             argNode = impImplicitR4orR8Cast(argNode, jitSigType);
             // insert any widening or narrowing casts for backwards compatibility
             argNode = impImplicitIorI4Cast(argNode, jitSigType);
+
+            if ((compAppleArm64Abi() || TargetArchitecture::IsArm32) && call->IsUnmanaged() &&
+                varTypeIsSmall(jitSigType))
+            {
+                // Apple arm64 and arm32 ABIs require arguments to be zero/sign
+                // extended up to 32 bit. The managed ABI does not require
+                // this.
+                if (fgCastNeeded(argNode, jitSigType))
+                {
+                    argNode = gtNewCastNode(TYP_INT, argNode, false, jitSigType);
+                }
+            }
         }
 
         NewCallArg arg;

src/coreclr/nativeaot/Runtime/CommonMacros.h

@@ -293,14 +293,11 @@ typedef uint8_t CODE_LOCATION;
 
 typedef bool CLR_BOOL;
 
-#if defined(TARGET_X86) || defined(TARGET_AMD64)
-// The return value is artificially widened on x86 and amd64
 typedef int32_t FC_BOOL_RET;
-#else
-typedef bool FC_BOOL_RET;
-#endif
+typedef int32_t FC_BOOL_ARG;
 
 #define FC_RETURN_BOOL(x)   do { return !!(x); } while(0)
+#define FC_ACCESS_BOOL(x) ((uint8_t)x != 0)
 
 #ifndef DACCESS_COMPILE
 #define IN_DAC(x)

src/coreclr/nativeaot/Runtime/GCHelpers.cpp

@@ -170,9 +170,9 @@ FCIMPL0(int32_t, RhGetMaxGcGeneration)
 }
 FCIMPLEND
 
-FCIMPL2(int32_t, RhGetGcCollectionCount, int32_t generation, CLR_BOOL getSpecialGCCount)
+FCIMPL2(int32_t, RhGetGcCollectionCount, int32_t generation, FC_BOOL_ARG getSpecialGCCount)
 {
-    return GCHeapUtilities::GetGCHeap()->CollectionCount(generation, getSpecialGCCount);
+    return GCHeapUtilities::GetGCHeap()->CollectionCount(generation, FC_ACCESS_BOOL(getSpecialGCCount));
 }
 FCIMPLEND
 

src/coreclr/nativeaot/Runtime/StackFrameIterator.cpp

@@ -2139,9 +2139,9 @@ bool StackFrameIterator::Next(uint32_t* puExCollideClauseIdx, bool* pfUnwoundRev
     return isValid;
 }
 
-FCIMPL4(FC_BOOL_RET, RhpSfiInit, StackFrameIterator* pThis, PAL_LIMITED_CONTEXT* pStackwalkCtx, CLR_BOOL instructionFault, CLR_BOOL* pfIsExceptionIntercepted)
+FCIMPL4(FC_BOOL_RET, RhpSfiInit, StackFrameIterator* pThis, PAL_LIMITED_CONTEXT* pStackwalkCtx, FC_BOOL_ARG instructionFault, CLR_BOOL* pfIsExceptionIntercepted)
 {
-    bool isValid = pThis->Init(pStackwalkCtx, instructionFault);
+    bool isValid = pThis->Init(pStackwalkCtx, FC_ACCESS_BOOL(instructionFault));
 
     if (pfIsExceptionIntercepted)
     {

src/coreclr/nativeaot/Runtime/threadstore.cpp

@@ -406,9 +406,9 @@ EXTERN_C void* QCALLTYPE RhpGetCurrentThread()
     return ThreadStore::GetCurrentThread();
 }
 
-FCIMPL3(void, RhpInitiateThreadAbort, void* thread, Object * threadAbortException, CLR_BOOL doRudeAbort)
+FCIMPL3(void, RhpInitiateThreadAbort, void* thread, Object * threadAbortException, FC_BOOL_ARG doRudeAbort)
 {
-    GetThreadStore()->InitiateThreadAbort((Thread*)thread, threadAbortException, doRudeAbort);
+    GetThreadStore()->InitiateThreadAbort((Thread*)thread, threadAbortException, FC_ACCESS_BOOL(doRudeAbort));
 }
 FCIMPLEND
 

src/coreclr/vm/binder.cpp

@@ -775,22 +775,22 @@ static void FCallCheckSignature(MethodDesc* pMD, PCODE pImpl)
             expectedType = pMD->IsCtor() ? NULL : "void";
             break;
         case ELEMENT_TYPE_BOOLEAN:
-            expectedType = (argIndex == -2) ? "FC_BOOL_RET" : "CLR_BOOL";
+            expectedType = (argIndex == -2) ? "FC_BOOL_RET" : "FC_BOOL_ARG";
             break;
         case ELEMENT_TYPE_CHAR:
-            expectedType = (argIndex == -2) ? "FC_CHAR_RET" : "CLR_CHAR";
+            expectedType = (argIndex == -2) ? "FC_CHAR_RET" : "FC_CHAR_ARG";
             break;
         case ELEMENT_TYPE_I1:
-            expectedType = (argIndex == -2) ? "FC_INT8_RET" : "INT8";
+            expectedType = (argIndex == -2) ? "FC_INT8_RET" : "FC_INT8_ARG";
             break;
         case ELEMENT_TYPE_U1:
-            expectedType = (argIndex == -2) ? "FC_UINT8_RET" : "UINT8";
+            expectedType = (argIndex == -2) ? "FC_UINT8_RET" : "FC_UINT8_ARG";
             break;
         case ELEMENT_TYPE_I2:
-            expectedType = (argIndex == -2) ? "FC_INT16_RET" : "INT16";
+            expectedType = (argIndex == -2) ? "FC_INT16_RET" : "FC_INT16_ARG";
             break;
         case ELEMENT_TYPE_U2:
-            expectedType = (argIndex == -2) ? "FC_UINT16_RET" : "UINT16";
+            expectedType = (argIndex == -2) ? "FC_UINT16_RET" : "FC_UINT16_ARG";
             break;
         //case ELEMENT_TYPE_I4:
         //     expectedType = "INT32";

src/coreclr/vm/comwaithandle.cpp

@@ -16,7 +16,7 @@
 #include "excep.h"
 #include "comwaithandle.h"
 
-FCIMPL3(INT32, WaitHandleNative::CorWaitOneNative, HANDLE handle, INT32 timeout, CLR_BOOL useTrivialWaits)
+FCIMPL3(INT32, WaitHandleNative::CorWaitOneNative, HANDLE handle, INT32 timeout, FC_BOOL_ARG useTrivialWaits)
 {
     FCALL_CONTRACT;
 
@@ -28,7 +28,7 @@ FCIMPL3(INT32, WaitHandleNative::CorWaitOneNative, HANDLE handle, INT32 timeout,
 
     Thread* pThread = GET_THREAD();
 
-    WaitMode waitMode = (WaitMode)((!useTrivialWaits ? WaitMode_Alertable : WaitMode_None) | WaitMode_IgnoreSyncCtx);
+    WaitMode waitMode = (WaitMode)((!FC_ACCESS_BOOL(useTrivialWaits) ? WaitMode_Alertable : WaitMode_None) | WaitMode_IgnoreSyncCtx);
     retVal = pThread->DoAppropriateWait(1, &handle, TRUE, timeout, waitMode);
 
     HELPER_METHOD_FRAME_END();
@@ -55,7 +55,7 @@ extern "C" INT32 QCALLTYPE WaitHandle_CorWaitOnePrioritizedNative(HANDLE handle,
 }
 #endif
 
-FCIMPL4(INT32, WaitHandleNative::CorWaitMultipleNative, HANDLE *handleArray, INT32 numHandles, CLR_BOOL waitForAll, INT32 timeout)
+FCIMPL4(INT32, WaitHandleNative::CorWaitMultipleNative, HANDLE *handleArray, INT32 numHandles, FC_BOOL_ARG waitForAll, INT32 timeout)
 {
     FCALL_CONTRACT;
 
@@ -67,13 +67,13 @@ FCIMPL4(INT32, WaitHandleNative::CorWaitMultipleNative, HANDLE *handleArray, INT
 #ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT
     // There are some issues with wait-all from an STA thread
     // - https://github.com/dotnet/runtime/issues/10243#issuecomment-385117537
-    if (waitForAll && numHandles > 1 && pThread->GetApartment() == Thread::AS_InSTA)
+    if (FC_ACCESS_BOOL(waitForAll) && numHandles > 1 && pThread->GetApartment() == Thread::AS_InSTA)
     {
         COMPlusThrow(kNotSupportedException, W("NotSupported_WaitAllSTAThread"));
     }
 #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT
 
-    ret = pThread->DoAppropriateWait(numHandles, handleArray, waitForAll, timeout, (WaitMode)(WaitMode_Alertable | WaitMode_IgnoreSyncCtx));
+    ret = pThread->DoAppropriateWait(numHandles, handleArray, FC_ACCESS_BOOL(waitForAll), timeout, (WaitMode)(WaitMode_Alertable | WaitMode_IgnoreSyncCtx));
 
     HELPER_METHOD_FRAME_END();
     return ret;

src/coreclr/vm/comwaithandle.h

@@ -18,8 +18,8 @@
 class WaitHandleNative
 {
 public:
-    static FCDECL3(INT32, CorWaitOneNative, HANDLE handle, INT32 timeout, CLR_BOOL useTrivialWaits);
-    static FCDECL4(INT32, CorWaitMultipleNative, HANDLE *handleArray, INT32 numHandles, CLR_BOOL waitForAll, INT32 timeout);
+    static FCDECL3(INT32, CorWaitOneNative, HANDLE handle, INT32 timeout, FC_BOOL_ARG useTrivialWaits);
+    static FCDECL4(INT32, CorWaitMultipleNative, HANDLE *handleArray, INT32 numHandles, FC_BOOL_ARG waitForAll, INT32 timeout);
     static FCDECL3(INT32, CorSignalAndWaitOneNative, HANDLE waitHandleSignalUNSAFE, HANDLE waitHandleWaitUNSAFE, INT32 timeout);
 };
 #ifdef TARGET_UNIX

src/coreclr/vm/fcall.h

@@ -1335,7 +1335,10 @@ typedef UINT32 FC_UINT8_RET;
 typedef INT32 FC_INT16_RET;
 typedef UINT32 FC_UINT16_RET;
 
+// Small primitive args are not widened.
+typedef INT32 FC_BOOL_ARG;
 
+#define FC_ACCESS_BOOL(x) ((BYTE)x != 0)
 
 // The fcall entrypoints has to be at unique addresses. Use this helper macro to make
 // the code of the fcalls unique if you get assert in ecall.cpp that mentions it.

src/coreclr/vm/jitinterface.cpp

@@ -106,27 +106,27 @@ int64_t AtomicLoad64WithoutTearing(int64_t volatile *valueRef)
 #endif // TARGET_64BIT
 }
 
-FCIMPL1(INT64, GetCompiledILBytes, CLR_BOOL currentThread)
+FCIMPL1(INT64, GetCompiledILBytes, FC_BOOL_ARG currentThread)
 {
     FCALL_CONTRACT;
 
-    return currentThread ? t_cbILJittedForThread : AtomicLoad64WithoutTearing(&g_cbILJitted);
+    return FC_ACCESS_BOOL(currentThread) ? t_cbILJittedForThread : AtomicLoad64WithoutTearing(&g_cbILJitted);
 }
 FCIMPLEND
 
-FCIMPL1(INT64, GetCompiledMethodCount, CLR_BOOL currentThread)
+FCIMPL1(INT64, GetCompiledMethodCount, FC_BOOL_ARG currentThread)
 {
     FCALL_CONTRACT;
 
-    return currentThread ? t_cMethodsJittedForThread : AtomicLoad64WithoutTearing(&g_cMethodsJitted);
+    return FC_ACCESS_BOOL(currentThread) ? t_cMethodsJittedForThread : AtomicLoad64WithoutTearing(&g_cMethodsJitted);
 }
 FCIMPLEND
 
-FCIMPL1(INT64, GetCompilationTimeInTicks, CLR_BOOL currentThread)
+FCIMPL1(INT64, GetCompilationTimeInTicks, FC_BOOL_ARG currentThread)
 {
     FCALL_CONTRACT;
 
-    return currentThread ? t_c100nsTicksInJitForThread : AtomicLoad64WithoutTearing(&g_c100nsTicksInJit);
+    return FC_ACCESS_BOOL(currentThread) ? t_c100nsTicksInJitForThread : AtomicLoad64WithoutTearing(&g_c100nsTicksInJit);
 }
 FCIMPLEND
 

src/coreclr/vm/jitinterface.h

@@ -1136,8 +1136,8 @@ extern thread_local int64_t t_cbILJittedForThread;
 extern thread_local int64_t t_cMethodsJittedForThread;
 extern thread_local int64_t t_c100nsTicksInJitForThread;
 
-FCDECL1(INT64, GetCompiledILBytes, CLR_BOOL currentThread);
-FCDECL1(INT64, GetCompiledMethodCount, CLR_BOOL currentThread);
-FCDECL1(INT64, GetCompilationTimeInTicks, CLR_BOOL currentThread);
+FCDECL1(INT64, GetCompiledILBytes, FC_BOOL_ARG currentThread);
+FCDECL1(INT64, GetCompiledMethodCount, FC_BOOL_ARG currentThread);
+FCDECL1(INT64, GetCompilationTimeInTicks, FC_BOOL_ARG currentThread);
 
 #endif // JITINTERFACE_H

src/coreclr/vm/qcall.h

@@ -27,7 +27,7 @@
 //
 //
 // The preferred type of QCall arguments is primitive types that efficiently handled by the P/Invoke marshaler (INT32, LPCWSTR, BOOL).
-// (Notice that BOOL is the correct boolean flavor for QCall arguments. CLR_BOOL is the correct boolean flavor for FCall arguments.)
+// (Notice that BOOL is the correct boolean flavor for QCall arguments. FC_BOOL_ARG is the correct boolean flavor for FCall arguments.)
 //
 // The pointers to common unmanaged EE structures should be wrapped into helper handle types. This is to make the managed implementation
 // type safe and avoid falling into unsafe C# everywhere. See the AssemblyHandle below for a good example.

src/coreclr/vm/reflectioninvocation.cpp

@@ -410,7 +410,7 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod,
     Object *target,
     PVOID* args, // An array of byrefs
     SignatureNative* pSigUNSAFE,
-    CLR_BOOL fConstructor)
+    FC_BOOL_ARG fConstructor)
 {
     FCALL_CONTRACT;
 
@@ -443,7 +443,7 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod,
 
     BOOL fCtorOfVariableSizedObject = FALSE;
 
-    if (fConstructor)
+    if (FC_ACCESS_BOOL(fConstructor))
     {
         // If we are invoking a constructor on an array then we must
         // handle this specially.
@@ -550,7 +550,7 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod,
     if (!pMeth->IsStatic() && !fCtorOfVariableSizedObject) {
         PVOID pThisPtr;
 
-        if (fConstructor)
+        if (FC_ACCESS_BOOL(fConstructor))
         {
             // Copy "this" pointer: only unbox if type is value type and method is not unboxing stub
             if (ownerType.IsValueType() && !pMeth->IsUnboxingStub()) {
@@ -672,7 +672,7 @@ FCIMPL4(Object*, RuntimeMethodHandle::InvokeMethod,
     CallDescrWorkerWithHandler(&callDescrData);
 
     // It is still illegal to do a GC here.  The return type might have/contain GC pointers.
-    if (fConstructor)
+    if (FC_ACCESS_BOOL(fConstructor))
     {
         // We have a special case for Strings...The object is returned...
         if (fCtorOfVariableSizedObject) {

src/coreclr/vm/runtimehandles.cpp

@@ -1815,7 +1815,7 @@ FCIMPLEND
 FCIMPL3(Object *, SignatureNative::GetCustomModifiersAtOffset,
     SignatureNative* pSignatureUNSAFE,
     INT32 offset,
-    CLR_BOOL fRequired)
+    FC_BOOL_ARG fRequired)
 {
     FCALL_CONTRACT;
 
@@ -1840,7 +1840,7 @@ FCIMPL3(Object *, SignatureNative::GetCustomModifiersAtOffset,
         INT32 cMods = 0;
         CorElementType cmodType;
 
-        CorElementType cmodTypeExpected = fRequired ? ELEMENT_TYPE_CMOD_REQD : ELEMENT_TYPE_CMOD_OPT;
+        CorElementType cmodTypeExpected = FC_ACCESS_BOOL(fRequired) ? ELEMENT_TYPE_CMOD_REQD : ELEMENT_TYPE_CMOD_OPT;
 
         // Discover the number of required and optional custom modifiers.
         while(TRUE)

src/coreclr/vm/runtimehandles.h

@@ -199,7 +199,7 @@ extern "C" void QCALLTYPE RuntimeTypeHandle_RegisterCollectibleTypeDependency(QC
 class RuntimeMethodHandle {
 
 public:
-    static FCDECL4(Object*, InvokeMethod, Object *target, PVOID* args, SignatureNative* pSig, CLR_BOOL fConstructor);
+    static FCDECL4(Object*, InvokeMethod, Object *target, PVOID* args, SignatureNative* pSig, FC_BOOL_ARG fConstructor);
 
     static FCDECL2(Object*, ReboxToNullable, Object *pBoxedValUNSAFE, ReflectClassBaseObject *pDestUNSAFE);
     static FCDECL1(Object*, ReboxFromNullable, Object *pBoxedValUNSAFE);
@@ -368,7 +368,7 @@ class SignatureNative : public Object
 
     static FCDECL2(FC_INT8_RET, GetCallingConventionFromFunctionPointerAtOffset, SignatureNative* pSig, INT32 offset);
 
-    static FCDECL3(Object *, GetCustomModifiersAtOffset, SignatureNative* pSig, INT32 offset, CLR_BOOL fRequired);
+    static FCDECL3(Object *, GetCustomModifiersAtOffset, SignatureNative* pSig, INT32 offset, FC_BOOL_ARG fRequired);
 
     BOOL HasThis() { LIMITED_METHOD_CONTRACT; return (m_managedCallingConvention & CALLCONV_HasThis); }
     INT32 NumFixedArgs() { WRAPPER_NO_CONTRACT; return m_PtrArrayarguments->GetNumComponents(); }

src/tests/JIT/Regression/JitBlue/Runtime_101046/CMakeLists.txt

@@ -0,0 +1,7 @@
+include_directories(${INC_PLATFORM_DIR})
+
+# This test always needs to be optimized to hit the problem.
+set(CMAKE_BUILD_TYPE Release)
+
+add_library(Runtime101046Native SHARED Runtime_101046.cpp)
+target_link_libraries(Runtime101046Native PRIVATE platformdefines)

src/tests/JIT/Regression/JitBlue/Runtime_101046/Runtime_101046.cpp

@@ -0,0 +1,10 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#include <platformdefines.h>
+#include <stdint.h>
+
+extern "C" DLL_EXPORT int32_t ReturnExtendedShort(int16_t s)
+{
+    return s;
+}

src/tests/JIT/Regression/JitBlue/Runtime_101046/Runtime_101046.cs

@@ -0,0 +1,31 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// Reference source for the .il file
+
+using System;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+using Xunit;
+
+public unsafe class Runtime_101046
+{
+    [Fact]
+    public static int TestEntryPoint()
+    {
+        ushort value = unchecked((ushort)-123);
+        return Test(ref value) == -123 ? 100 : 101;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static int Test(ref ushort p)
+    {
+        return Runtime101046Native.ReturnExtendedShort((short)p); // this (short) cast is removed in the .il version
+    }
+}
+
+internal static unsafe class Runtime101046Native
+{
+    [DllImport(nameof(Runtime101046Native))]
+    public static extern int ReturnExtendedShort(short s);
+}

src/tests/JIT/Regression/JitBlue/Runtime_101046/Runtime_101046.csproj

@@ -0,0 +1,12 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <!-- Needed for CMakeProjectReference -->
+    <RequiresProcessIsolation>true</RequiresProcessIsolation>
+    <Optimize>True</Optimize>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <CMakeProjectReference Include="CMakeLists.txt" />
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>