From: Sergey Andreenko Date: Wed, 23 Dec 2020 06:43:04 +0000 (-0800) Subject: Fix handling of Arm32 struct with 8-byte alignment and 12-byte rounded size. (#46320) X-Git-Tag: submit/tizen/20210909.063632~4023 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=a70005f4d89e03622b4dd3bea058f102e60702af;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Fix handling of Arm32 struct with 8-byte alignment and 12-byte rounded size. (#46320) * Add a repro. * Fix arm32 struct with 8-byte alignment and 12-byte rounded size. * format fixes * review --- diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6539b75..2376314 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1614,7 +1614,6 @@ public: assert(GetByteSize() > TARGET_POINTER_SIZE * numRegs); const unsigned stackByteSize = GetByteSize() - TARGET_POINTER_SIZE * numRegs; - assert(IsSplit() || ((stackByteSize % m_byteAlignment) == 0)); return stackByteSize; } @@ -1798,19 +1797,38 @@ public: return m_byteOffset; } - void SetByteSize(unsigned byteSize, unsigned byteAlignment) + void SetByteSize(unsigned byteSize, bool isStruct, bool isFloatHfa) { - assert(byteAlignment != 0); - const unsigned alignedByteSize = roundUp(byteSize, byteAlignment); + +#ifdef OSX_ARM64_ABI + unsigned roundedByteSize; + // Only struct types need extension or rounding to pointer size, but HFA does not. + if (isStruct && !isFloatHfa) + { + roundedByteSize = roundUp(byteSize, TARGET_POINTER_SIZE); + } + else + { + roundedByteSize = byteSize; + } +#else // OSX_ARM64_ABI + unsigned roundedByteSize = roundUp(byteSize, TARGET_POINTER_SIZE); +#endif // OSX_ARM64_ABI + +#if !defined(TARGET_ARM) + // Arm32 could have a struct with 8 byte alignment + // which rounded size % 8 is not 0. + assert(m_byteAlignment != 0); + assert(roundedByteSize % m_byteAlignment == 0); +#endif // TARGET_ARM #if defined(DEBUG_ARG_SLOTS) if (!isStruct) { - assert(alignedByteSize == getSlotCount() * TARGET_POINTER_SIZE); + assert(roundedByteSize == getSlotCount() * TARGET_POINTER_SIZE); } #endif - m_byteSize = alignedByteSize; - m_byteAlignment = byteAlignment; + m_byteSize = roundedByteSize; } unsigned GetByteSize() const @@ -1818,6 +1836,11 @@ public: return m_byteSize; } + void SetByteAlignment(unsigned byteAlignment) + { + m_byteAlignment = byteAlignment; + } + unsigned GetByteAlignment() const { return m_byteAlignment; @@ -1947,6 +1970,7 @@ public: unsigned byteSize, unsigned byteAlignment, bool isStruct, + bool isFloatHfa, bool isVararg = false); #ifdef UNIX_AMD64_ABI @@ -1958,6 +1982,7 @@ public: unsigned byteSize, unsigned byteAlignment, const bool isStruct, + const bool isFloatHfa, const bool isVararg, const regNumber otherRegNum, const unsigned structIntRegs, @@ -1972,6 +1997,7 @@ public: unsigned byteSize, unsigned byteAlignment, bool isStruct, + bool isFloatHfa, bool isVararg = false); void RemorphReset(); @@ -2500,7 +2526,9 @@ public: unsigned ehFuncletCount(); // Return the count of funclets in the function unsigned bbThrowIndex(BasicBlock* blk); // Get the index to use as the cache key for sharing throw blocks -#else // !FEATURE_EH_FUNCLETS + +#else // !FEATURE_EH_FUNCLETS + bool ehAnyFunclets() { return false; @@ -2514,7 +2542,7 @@ public: { return blk->bbTryIndex; } // Get the index to use as the cache key for sharing throw blocks -#endif // !FEATURE_EH_FUNCLETS +#endif // !FEATURE_EH_FUNCLETS // Returns a flowList representing the "EH predecessors" of "blk". These are the normal predecessors of // "blk", plus one special case: if "blk" is the first block of a handler, considers the predecessor(s) of the first diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 9dc357b..b72d32e 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -991,6 +991,7 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned argNum, unsigned byteSize, unsigned byteAlignment, bool isStruct, + bool isFloatHfa, bool isVararg /*=false*/) { fgArgTabEntry* curArgTabEntry = new (compiler, CMK_fgArgInfo) fgArgTabEntry; @@ -1026,7 +1027,8 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned argNum, curArgTabEntry->isNonStandard = false; curArgTabEntry->isStruct = isStruct; curArgTabEntry->SetIsVararg(isVararg); - curArgTabEntry->SetByteSize(byteSize, byteAlignment); + curArgTabEntry->SetByteAlignment(byteAlignment); + curArgTabEntry->SetByteSize(byteSize, isStruct, isFloatHfa); curArgTabEntry->SetByteOffset(0); hasRegArgs = true; @@ -1043,6 +1045,7 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned unsigned byteSize, unsigned byteAlignment, const bool isStruct, + const bool isFloatHfa, const bool isVararg, const regNumber otherRegNum, const unsigned structIntRegs, @@ -1050,7 +1053,7 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned const SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR* const structDescPtr) { fgArgTabEntry* curArgTabEntry = - AddRegArg(argNum, node, use, regNum, numRegs, byteSize, byteAlignment, isStruct, isVararg); + AddRegArg(argNum, node, use, regNum, numRegs, byteSize, byteAlignment, isStruct, isFloatHfa, isVararg); assert(curArgTabEntry != nullptr); curArgTabEntry->isStruct = isStruct; // is this a struct arg @@ -1080,6 +1083,7 @@ fgArgTabEntry* fgArgInfo::AddStkArg(unsigned argNum, unsigned byteSize, unsigned byteAlignment, bool isStruct, + bool isFloatHfa, bool isVararg /*=false*/) { fgArgTabEntry* curArgTabEntry = new (compiler, CMK_fgArgInfo) fgArgTabEntry; @@ -1121,14 +1125,14 @@ fgArgTabEntry* fgArgInfo::AddStkArg(unsigned argNum, curArgTabEntry->isStruct = isStruct; curArgTabEntry->SetIsVararg(isVararg); - curArgTabEntry->SetByteSize(byteSize, byteAlignment); + curArgTabEntry->SetByteAlignment(byteAlignment); + curArgTabEntry->SetByteSize(byteSize, isStruct, isFloatHfa); curArgTabEntry->SetByteOffset(nextStackByteOffset); hasStackArgs = true; AddArg(curArgTabEntry); DEBUG_ARG_SLOTS_ONLY(nextSlotNum += numSlots;) nextStackByteOffset += curArgTabEntry->GetByteSize(); - assert(nextStackByteOffset % curArgTabEntry->GetByteAlignment() == 0); return curArgTabEntry; } @@ -2788,11 +2792,13 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) const unsigned byteSize = TARGET_POINTER_SIZE; const unsigned byteAlignment = TARGET_POINTER_SIZE; const bool isStruct = false; + const bool isFloatHfa = false; // This is a register argument - put it in the table. call->fgArgInfo->AddRegArg(argIndex, argx, call->gtCallThisArg, regNum, numRegs, byteSize, byteAlignment, - isStruct, callIsVararg UNIX_AMD64_ABI_ONLY_ARG(REG_STK) UNIX_AMD64_ABI_ONLY_ARG(0) - UNIX_AMD64_ABI_ONLY_ARG(0) UNIX_AMD64_ABI_ONLY_ARG(nullptr)); + isStruct, isFloatHfa, + callIsVararg UNIX_AMD64_ABI_ONLY_ARG(REG_STK) UNIX_AMD64_ABI_ONLY_ARG(0) + UNIX_AMD64_ABI_ONLY_ARG(0) UNIX_AMD64_ABI_ONLY_ARG(nullptr)); intArgRegNum++; #ifdef WINDOWS_AMD64_ABI @@ -2995,6 +3001,8 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) } #endif // FEATURE_HFA + const bool isFloatHfa = (hfaType == TYP_FLOAT); + #ifdef TARGET_ARM passUsingFloatRegs = !callIsVararg && (isHfaArg || varTypeUsesFloatReg(argx)) && !opts.compUseSoftFP; bool passUsingIntRegs = passUsingFloatRegs ? false : (intArgRegNum < MAX_REG_ARG); @@ -3219,7 +3227,7 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) // Arm64 Apple has a special ABI for passing small size arguments on stack, // bytes are aligned to 1-byte, shorts to 2-byte, int/float to 4-byte, etc. // It means passing 8 1-byte arguments on stack can take as small as 8 bytes. - unsigned argAlignBytes = eeGetArgAlignment(argType, (hfaType == TYP_FLOAT)); + unsigned argAlignBytes = eeGetArgAlignment(argType, isFloatHfa); #endif // @@ -3465,12 +3473,12 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) #endif // This is a register argument - put it in the table - newArgEntry = call->fgArgInfo->AddRegArg(argIndex, argx, args, nextRegNum, size, byteSize, argAlignBytes, - isStructArg, callIsVararg UNIX_AMD64_ABI_ONLY_ARG(nextOtherRegNum) - UNIX_AMD64_ABI_ONLY_ARG(structIntRegs) - UNIX_AMD64_ABI_ONLY_ARG(structFloatRegs) - UNIX_AMD64_ABI_ONLY_ARG(&structDesc)); - + newArgEntry = + call->fgArgInfo->AddRegArg(argIndex, argx, args, nextRegNum, size, byteSize, argAlignBytes, isStructArg, + isFloatHfa, callIsVararg UNIX_AMD64_ABI_ONLY_ARG(nextOtherRegNum) + UNIX_AMD64_ABI_ONLY_ARG(structIntRegs) + UNIX_AMD64_ABI_ONLY_ARG(structFloatRegs) + UNIX_AMD64_ABI_ONLY_ARG(&structDesc)); newArgEntry->SetIsBackFilled(isBackFilled); newArgEntry->isNonStandard = isNonStandard; @@ -3531,7 +3539,7 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) { // This is a stack argument - put it in the table newArgEntry = call->fgArgInfo->AddStkArg(argIndex, argx, args, size, byteSize, argAlignBytes, isStructArg, - callIsVararg); + isFloatHfa, callIsVararg); #ifdef UNIX_AMD64_ABI // TODO-Amd64-Unix-CQ: This is temporary (see also in fgMorphArgs). if (structDesc.passedInRegisters) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.cs b/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.cs new file mode 100644 index 0000000..983d8ac --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.cs @@ -0,0 +1,196 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// The test was reproducing an issue on Arm32 when we required an 8-byte alignment +// for a struct which size was rounded to 4-byte. + +using System; +using System.Runtime.InteropServices; +using System.Runtime.CompilerServices; + +namespace Runtime_46239 +{ + + [StructLayout(LayoutKind.Sequential, Pack = 1)] + internal struct S1 // Marshal.SizeOf 12 bytes, EE getClassSize 12 (here and below for arm32). + { + public ulong tmp1; + public Object q; + } + + [StructLayout(LayoutKind.Sequential, Pack = 1)] + internal struct S2 // Marshal.SizeOf 12 bytes, EE getClassSize 12 + { + public ulong tmp1; + public int tmp2; + } + + [StructLayout(LayoutKind.Explicit)] + internal struct S3 // Marshal.SizeOf 16 bytes, EE getClassSize 12 + { + [FieldOffset(0)] + public ulong tmp1; + [FieldOffset(8)] + public Object tmp2; + } + + [StructLayout(LayoutKind.Explicit)] + internal struct S4 // Marshal.SizeOf 16 bytes, EE getClassSize 16 + { + [FieldOffset(0)] + public ulong tmp1; + [FieldOffset(8)] + public int tmp2; + } + + internal struct S5 // Marshal.SizeOf 16 bytes, EE getClassSize 16 + { + public ulong tmp1; + public Object tmp2; + } + + internal struct S6 // Marshal.SizeOf 16 bytes, EE getClassSize 16 + { + public ulong tmp1; + public int tmp2; + } + + class Program + { + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test1(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int num, T a, T b) + { + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test2(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int num, T a, T b, T c) + { + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test3(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, byte b1, T a, T b, T c) + { + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test4(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, T a, T b, T c) + { + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test5(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int num, T a, T b, int i) + { + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test6(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, T a, T b, int i) + { + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test5(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, byte b1, T a, T b, byte b2) + { + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test6(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, T a, T b, byte b1) + { + return 100; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int test() where T : struct + { + + // Marshal.SizeOf throws "System.ArgumentException: Type 'Runtime_46239.S1' cannot + // be marshaled as an unmanaged structure; no meaningful size or offset can be computed." + // on non-Windows platforms. + // + // int size = Marshal.SizeOf(typeof(T)); + // Console.WriteLine("size of " + typeof(T).Name + " is: " + size); + + + if (test1(1, 2, 3, 4, 5, 6, 7, 8, 1, new T(), new T()) != 100) + { + Console.WriteLine("test1() failed."); + return 101; + } + if (test2(1, 2, 3, 4, 5, 6, 7, 8, 1, new T(), new T(), new T()) != 100) + { + Console.WriteLine("test2() failed."); + return 101; + } + if (test3(1, 2, 3, 4, 5, 6, 7, 8, 1, new T(), new T(), new T()) != 100) + { + Console.WriteLine("test3() failed."); + return 101; + } + if (test4(1, 2, 3, 4, 5, 6, 7, 8, new T(), new T(), new T()) != 100) + { + Console.WriteLine("test4() failed."); + return 101; + } + if (test5(1, 2, 3, 4, 5, 6, 7, 8, 1, new T(), new T(), 2) != 100) + { + Console.WriteLine("test5() failed."); + return 101; + } + if (test6(1, 2, 3, 4, 5, 6, 7, 8, new T(), new T(), 1) != 100) + { + Console.WriteLine("test6() failed."); + return 101; + } + return 100; + + } + + static int Main(string[] args) + { + + if (test() != 100) + { + Console.WriteLine("test() failed."); + return 101; + } + + if (test() != 100) + { + Console.WriteLine("test() failed."); + return 101; + } + + if (test() != 100) + { + Console.WriteLine("test() failed."); + return 101; + } + + if (test() != 100) + { + Console.WriteLine("test() failed."); + return 101; + } + + if (test() != 100) + { + Console.WriteLine("test() failed."); + return 101; + } + + if (test() != 100) + { + Console.WriteLine("test() failed."); + return 101; + } + + return 100; + } + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.csproj new file mode 100644 index 0000000..adc3b72 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.csproj @@ -0,0 +1,13 @@ + + + Exe + 0 + + + None + True + + + + +