[clr-interp] Fix argument alignment of 16 byte or greater aligned structures in the interpreter

src/coreclr/interpreter/compiler.cpp

@@ -1823,6 +1823,13 @@ int32_t InterpCompiler::GetInterpTypeStackSize(CORINFO_CLASS_HANDLE clsHnd, Inte
         // All vars are stored at 8 byte aligned offsets
         if (align < INTERP_STACK_SLOT_SIZE)
             align = INTERP_STACK_SLOT_SIZE;
+
+        // We do not align beyond the stack alignment 
+        // (This is relevant for structs with very high alignment requirements, 
+        // where we align within struct layout, but the structs are not actually
+        // aligned on the stack)
+        if (align > INTERP_STACK_ALIGNMENT)
+            align = INTERP_STACK_ALIGNMENT;
     }
     else
     {
@@ -3445,7 +3452,7 @@ void InterpCompiler::EmitPushUnboxAny(const CORINFO_GENERICHANDLE_RESULT& arg1,
     PushStackType(resultStackType, clsHndStack);
     int resultVar = m_pStackPointer[-1].var;
 
-    PushStackType(StackTypeI, NULL);
+    PushStackType(StackTypeByRef, NULL);
     int32_t intermediateVar = m_pStackPointer[-1].var;
     m_pStackPointer--;
 

src/coreclr/interpreter/compileropt.cpp

@@ -12,6 +12,24 @@ int32_t InterpCompiler::AllocVarOffset(int var, int32_t *pPos)
     offset = *pPos;
     size = m_pVars[var].size;
 
+    size_t align = INTERP_STACK_SLOT_SIZE;
+
+    if (size > INTERP_STACK_SLOT_SIZE)
+    {
+        assert(m_pVars[var].interpType == InterpTypeVT);
+        align = m_compHnd->getClassAlignmentRequirement(m_pVars[var].clsHnd);
+        if (align < INTERP_STACK_SLOT_SIZE)
+            align = INTERP_STACK_SLOT_SIZE;
+        if (align > INTERP_STACK_ALIGNMENT)
+            align = INTERP_STACK_ALIGNMENT;
+    }
+    else
+    {
+        assert(m_pVars[var].interpType != InterpTypeVT || m_compHnd->getClassAlignmentRequirement(m_pVars[var].clsHnd) <= INTERP_STACK_SLOT_SIZE);
+    }
+
+    offset = (int32_t)ALIGN_UP_TO(offset, align);
+
     m_pVars[var].offset = offset;
 
     *pPos = ALIGN_UP_TO(offset + size, INTERP_STACK_SLOT_SIZE);

src/coreclr/vm/amd64/AsmHelpers.asm

@@ -879,6 +879,12 @@ LEAF_ENTRY Store_XMM3, _TEXT
         jmp qword ptr [r11]
 LEAF_END Store_XMM3, _TEXT
 
+LEAF_ENTRY InjectInterpStackAlign, _TEXT
+        add r10, 8
+        add r11, 8
+        jmp qword ptr [r11]
+LEAF_END InjectInterpStackAlign, _TEXT
+
 ; Copy arguments from the interpreter stack to the processor stack.
 ; The CPU stack slots are aligned to pointer size.
 LEAF_ENTRY Load_Stack, _TEXT

src/coreclr/vm/amd64/asmhelpers.S

@@ -1148,6 +1148,12 @@ LEAF_END Store_XMM7, _TEXT
 // Routines for passing value type arguments by reference in general purpose registers RDI, RSI, RDX, RCX, R8, R9
 // from the interpreter to native code
 
+LEAF_ENTRY InjectInterpStackAlign, _TEXT
+        add r10, 8
+        add r11, 8
+        jmp qword ptr [r11]
+LEAF_END InjectInterpStackAlign, _TEXT
+
 // Copy arguments from the the interpreter stack to the CPU stack.
 // The CPU stack slots are aligned to pointer size.
 LEAF_ENTRY Load_Stack, _TEXT

src/coreclr/vm/arm64/asmhelpers.S

@@ -1312,6 +1312,12 @@ ALTERNATE_ENTRY Store_D7
     EPILOG_BRANCH_REG x11
 LEAF_END Store_D1_D2_D3_D4_D5_D6_D7
 
+LEAF_ENTRY InjectInterpStackAlign
+    add x9, x9, #8
+    ldr x11, [x10], #8
+    EPILOG_BRANCH_REG x11
+LEAF_END InjectInterpStackAlign
+
 // Copy arguments from the interpreter stack to the processor stack
 // The CPU stack slots are aligned to pointer size.
 LEAF_ENTRY Load_Stack

src/coreclr/vm/arm64/asmhelpers.asm

@@ -1627,6 +1627,12 @@ RefCopyDone$argReg
         EPILOG_BRANCH_REG x11
     LEAF_END Store_D1_D2_D3_D4_D5_D6_D7
 
+    LEAF_ENTRY InjectInterpStackAlign
+        add x9, x9, #8
+        ldr x11, [x10], #8
+        EPILOG_BRANCH_REG x11
+    LEAF_END InjectInterpStackAlign
+
     ; Routines for passing value type arguments by reference in general purpose registers X0..X7
     ; from the interpreter to native code
     ; Copy arguments from the interpreter stack to the processor stack

src/coreclr/vm/callstubgenerator.cpp

@@ -6,6 +6,7 @@
 #include "callstubgenerator.h"
 #include "ecall.h"
 
+extern "C" void InjectInterpStackAlign();
 extern "C" void Load_Stack();
 extern "C" void Store_Stack();
 
@@ -1350,7 +1351,7 @@ CallStubHeader *CallStubGenerator::GenerateCallStubForSig(MetaSig &sig)
 {
     STANDARD_VM_CONTRACT;
 
-    // Allocate space for the routines. The size of the array is conservatively set to twice the number of arguments
+    // Allocate space for the routines. The size of the array is conservatively set to three times the number of arguments
     // plus one slot for the target pointer and reallocated to the real size at the end.
     size_t tempStorageSize = ComputeTempStorageSize(sig);
     PCODE *pRoutines = (PCODE*)alloca(tempStorageSize);
@@ -1404,10 +1405,32 @@ CallStubHeader *CallStubGenerator::GenerateCallStubForSig(MetaSig &sig)
     }
 };
 
-void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
+void CallStubGenerator::TerminateCurrentRoutineIfNotOfNewType(RoutineType type, PCODE *pRoutines)
 {
+    if ((m_r1 != NoRange) && (type != RoutineType::GPReg))
+    {
+        pRoutines[m_routineIndex++] = GetGPRegRangeRoutine(m_r1, m_r2);
+        m_r1 = NoRange;
+    }
+    else if (m_x1 != NoRange && type != RoutineType::FPReg)
+    {
+        pRoutines[m_routineIndex++] = GetFPRegRangeRoutine(m_x1, m_x2);
+        m_x1 = NoRange;
+    }
+    else if (m_s1 != NoRange && type != RoutineType::Stack)
+    {
+        pRoutines[m_routineIndex++] = GetStackRoutine();
+        pRoutines[m_routineIndex++] = ((int64_t)(m_s2 - m_s1 + 1) << 32) | m_s1;
+        m_s1 = NoRange;
+    }
 
+    return;
+}
+
+void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
+{
     ArgIterator argIt(&sig);
+    int32_t interpreterStackOffset = 0;
 
     m_r1 = NoRange; // indicates that there is no active range of general purpose registers
     m_r2 = 0;
@@ -1431,6 +1454,7 @@ void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
         // we need to "inject" it here.
         // CLR ABI specifies that unlike the native Windows x64 calling convention, it is passed in the first argument register.
         m_r1 = 0;
+        interpreterStackOffset += INTERP_STACK_SLOT_SIZE;
     }
 
     if (argIt.HasParamType())
@@ -1442,6 +1466,7 @@ void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
         ArgLocDesc paramArgLocDesc;
         argIt.GetParamTypeLoc(&paramArgLocDesc);
         ProcessArgument(NULL, paramArgLocDesc, pRoutines);
+        interpreterStackOffset += INTERP_STACK_SLOT_SIZE;
     }
 
     int ofs;
@@ -1453,6 +1478,40 @@ void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
         ArgLocDesc argLocDesc;
         argIt.GetArgLoc(ofs, &argLocDesc);
 
+        // Each argument takes at least one slot on the interpreter stack
+        int interpStackSlotSize = INTERP_STACK_SLOT_SIZE;
+
+        // Each entry on the interpreter stack is always aligned to at least 8 bytes, but some arguments are 16 byte aligned
+        TypeHandle thArgTypeHandle;
+        if ((argIt.GetArgType(&thArgTypeHandle) == ELEMENT_TYPE_VALUETYPE) && thArgTypeHandle.GetSize() > 8)
+        {
+            unsigned align = CEEInfo::getClassAlignmentRequirementStatic(thArgTypeHandle);
+            if (align < INTERP_STACK_SLOT_SIZE)
+            {
+                align = INTERP_STACK_SLOT_SIZE;
+            }
+            else if (align > INTERP_STACK_ALIGNMENT)
+            {
+                align = INTERP_STACK_ALIGNMENT;
+            }
+            assert(align == 8 || align == 16); // At the moment, we can only have an 8 or 16 byte alignment requirement here
+            if (interpreterStackOffset != ALIGN_UP(interpreterStackOffset, align))
+            {
+                TerminateCurrentRoutineIfNotOfNewType(RoutineType::None, pRoutines);
+
+                interpreterStackOffset += INTERP_STACK_SLOT_SIZE;
+                pRoutines[m_routineIndex++] = (PCODE)InjectInterpStackAlign;
+#if LOG_COMPUTE_CALL_STUB
+                printf("Inject stack align argument\n");
+#endif
+            }
+
+            assert(interpreterStackOffset == ALIGN_UP(interpreterStackOffset, align));
+
+            interpStackSlotSize = ALIGN_UP(thArgTypeHandle.GetSize(), align);
+        }
+        interpreterStackOffset += interpStackSlotSize;
+
 #ifdef UNIX_AMD64_ABI
         if (argIt.GetArgLocDescForStructInRegs() != NULL)
         {
@@ -1517,19 +1576,7 @@ void CallStubGenerator::ComputeCallStub(MetaSig &sig, PCODE *pRoutines)
 
     // All arguments were processed, but there is likely a pending ranges to store.
     // Process such a range if any.
-    if (m_r1 != NoRange)
-    {
-        pRoutines[m_routineIndex++] = GetGPRegRangeRoutine(m_r1, m_r2);
-    }
-    else if (m_x1 != NoRange)
-    {
-        pRoutines[m_routineIndex++] = GetFPRegRangeRoutine(m_x1, m_x2);
-    }
-    else if (m_s1 != NoRange)
-    {
-        pRoutines[m_routineIndex++] = GetStackRoutine();
-        pRoutines[m_routineIndex++] = ((int64_t)(m_s2 - m_s1 + 1) << 32) | m_s1;
-    }
+    TerminateCurrentRoutineIfNotOfNewType(RoutineType::None, pRoutines);
 
     ReturnType returnType = GetReturnType(&argIt);
 
@@ -1550,30 +1597,15 @@ void CallStubGenerator::ProcessArgument(ArgIterator *pArgIt, ArgLocDesc& argLocD
 {
     LIMITED_METHOD_CONTRACT;
 
-    // Check if we have a range of registers or stack arguments that we need to store because the current argument
-    // terminates it.
-    if ((argLocDesc.m_cGenReg == 0) && (m_r1 != NoRange))
-    {
-        // No GP register is used to pass the current argument, but we already have a range of GP registers,
-        // store the routine for the range
-        pRoutines[m_routineIndex++] = GetGPRegRangeRoutine(m_r1, m_r2);
-        m_r1 = NoRange;
-    }
-    else if (((argLocDesc.m_cFloatReg == 0)) && (m_x1 != NoRange))
-    {
-        // No floating point register is used to pass the current argument, but we already have a range of FP registers,
-        // store the routine for the range
-        pRoutines[m_routineIndex++] = GetFPRegRangeRoutine(m_x1, m_x2);
-        m_x1 = NoRange;
-    }
-    else if ((argLocDesc.m_byteStackSize == 0) && (m_s1 != NoRange))
-    {
-        // No stack argument is used to pass the current argument, but we already have a range of stack arguments,
-        // store the routine for the range
-        pRoutines[m_routineIndex++] = GetStackRoutine();
-        pRoutines[m_routineIndex++] = ((int64_t)(m_s2 - m_s1 + 1) << 32) | m_s1;
-        m_s1 = NoRange;
-    }
+    RoutineType argType = RoutineType::None;
+    if (argLocDesc.m_cGenReg != 0)
+        argType = RoutineType::GPReg;
+    else if (argLocDesc.m_cFloatReg != 0)
+        argType = RoutineType::FPReg;
+    else if (argLocDesc.m_byteStackSize != 0)
+        argType = RoutineType::Stack;
+
+    TerminateCurrentRoutineIfNotOfNewType(argType, pRoutines);
 
     if (argLocDesc.m_cGenReg != 0)
     {

src/coreclr/vm/callstubgenerator.h

@@ -145,10 +145,20 @@ class CallStubGenerator
         int numArgs = sig.NumFixedArgs() + (sig.HasThis() ? 1 : 0);
 
         // The size of the temporary storage is the size of the CallStubHeader plus the size of the routines array.
-        // The size of the routines array is twice the number of arguments plus one slot for the target method pointer.
-        return sizeof(CallStubHeader) + ((numArgs + 1) * 2 + 1) * sizeof(PCODE);
+        // The size of the routines array is three times the number of arguments plus one slot for the target method pointer.
+        return sizeof(CallStubHeader) + ((numArgs + 1) * 3 + 1) * sizeof(PCODE);
     }
     void ComputeCallStub(MetaSig &sig, PCODE *pRoutines);
+
+    enum class RoutineType
+    {
+        None,
+        GPReg,
+        FPReg,
+        Stack
+    };
+
+    void TerminateCurrentRoutineIfNotOfNewType(RoutineType type, PCODE *pRoutines);
 };
 
 void InitCallStubGenerator();

src/tests/Interop/PInvoke/Int128/Int128Test.cs

@@ -115,7 +115,6 @@ public static void TestInt128FieldLayout()
         StructWithInt128 rhs = new StructWithInt128(new Int128(13, 14));
 
         Int128Native.AddStructWithInt128_ByRef(ref lhs, ref rhs);
-        // Interpreter-FIXME: Incorrect result. Tracked by https://github.com/dotnet/runtime/issues/118618
         Assert.Equal(new StructWithInt128(new Int128(24, 26)), lhs);
 
         Int128 value2;