From 9e1799cb4032674f3a57d52dd7de617eb2d48b0b Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Sat, 6 Apr 2024 22:02:05 +0200 Subject: [PATCH 1/3] Guard ABI changes necessary for funclet unwinding with UsesFunclets()/FEATURE_EH_FUNCLETS instead of UNIX_X86_ABI --- src/coreclr/gcdump/i386/gcdumpx86.cpp | 4 ++-- src/coreclr/jit/emit.cpp | 6 +++--- src/coreclr/jit/gcencode.cpp | 4 +--- src/coreclr/jit/morph.cpp | 16 +++++++--------- src/coreclr/vm/gc_unwind_x86.inl | 4 ++-- 5 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/coreclr/gcdump/i386/gcdumpx86.cpp b/src/coreclr/gcdump/i386/gcdumpx86.cpp index c5d359aac517e4..c5eb632ee95ec1 100644 --- a/src/coreclr/gcdump/i386/gcdumpx86.cpp +++ b/src/coreclr/gcdump/i386/gcdumpx86.cpp @@ -456,10 +456,10 @@ size_t GCDump::DumpGCTable(PTR_CBYTE table, /* non-ptr arg push */ curOffs += (val & 0x07); -#ifndef UNIX_X86_ABI +#ifndef FEATURE_EH_FUNCLETS // For x86/Linux, non-ptr arg pushes can be reported even for EBP frames _ASSERTE(!header.ebpFrame); -#endif // UNIX_X86_ABI +#endif // FEATURE_EH_FUNCLETS argCnt++; DumpEncoding(bp, table-bp); bp = table; diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 5259b936646fbf..b8d586111e5ed6 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -6680,9 +6680,9 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, emitFullyInt = fullyInt; emitFullGCinfo = fullPtrMap; - -#ifndef UNIX_X86_ABI - emitFullArgInfo = !emitHasFramePtr; +#if TARGET_X86 + // On x86 with funclets we emit full ptr map even for EBP frames + emitFullArgInfo = comp->UsesFunclets() ? !emitHasFramePtr : fullPtrMap; #else emitFullArgInfo = fullPtrMap; #endif diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index b7972f216d53ed..e21fe864984ef7 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -2584,9 +2584,7 @@ size_t GCInfo::gcMakeRegPtrTable(BYTE* dest, int mask, const InfoHdr& header, un assert((codeDelta & 0x7) == codeDelta); *dest++ = 0xB0 | (BYTE)codeDelta; -#ifndef UNIX_X86_ABI - assert(!compiler->isFramePointerUsed()); -#endif + assert(compiler->UsesFunclets() || !compiler->isFramePointerUsed()); /* Remember the new 'last' offset */ diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 5f53d088f8a2ed..b58a0edb94abfb 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14232,6 +14232,13 @@ void Compiler::fgSetOptions() if (info.compXcptnsCount > 0) { codeGen->setFramePointerRequiredEH(true); + + if (UsesFunclets()) + { + assert(!codeGen->isGCTypeFixed()); + // Enforce fully interruptible codegen for funclet unwinding + SetInterruptible(true); + } } #else // !TARGET_X86 @@ -14243,15 +14250,6 @@ void Compiler::fgSetOptions() #endif // TARGET_X86 -#ifdef UNIX_X86_ABI - if (info.compXcptnsCount > 0) - { - assert(!codeGen->isGCTypeFixed()); - // Enforce fully interruptible codegen for funclet unwinding - SetInterruptible(true); - } -#endif // UNIX_X86_ABI - if (compMethodRequiresPInvokeFrame()) { codeGen->setFramePointerRequired(true); // Setup of Pinvoke frame currently requires an EBP style frame diff --git a/src/coreclr/vm/gc_unwind_x86.inl b/src/coreclr/vm/gc_unwind_x86.inl index 5b308911bc0b16..fe8c23a2a36a96 100644 --- a/src/coreclr/vm/gc_unwind_x86.inl +++ b/src/coreclr/vm/gc_unwind_x86.inl @@ -1519,10 +1519,10 @@ unsigned scanArgRegTableI(PTR_CBYTE table, bool hasPartialArgInfo; -#ifndef UNIX_X86_ABI +#ifndef FEATURE_EH_FUNCLETS hasPartialArgInfo = info->ebpFrame; #else - // For x86/Linux, interruptible code always has full arg info + // For x86/Funclets, interruptible code always has full arg info // // This should be aligned with emitFullArgInfo setting at // emitter::emitEndCodeGen (in JIT) From a37d0d7f7be515d7408d0ccd22fff8c85f785ba3 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Sat, 6 Apr 2024 22:58:05 +0200 Subject: [PATCH 2/3] Fix the emitFullArgInfo condition. --- src/coreclr/jit/emit.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index b8d586111e5ed6..f8b320c07d44cd 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -6682,9 +6682,9 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, emitFullGCinfo = fullPtrMap; #if TARGET_X86 // On x86 with funclets we emit full ptr map even for EBP frames - emitFullArgInfo = comp->UsesFunclets() ? !emitHasFramePtr : fullPtrMap; + emitFullArgInfo = comp->UsesFunclets() ? fullPtrMap : !emitHasFramePtr; #else - emitFullArgInfo = fullPtrMap; + emitFullArgInfo = !emitHasFramePtr; #endif #if EMITTER_STATS From 3932a338a078267f43030a475ca6389c319f891c Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Sun, 7 Apr 2024 10:22:20 +0200 Subject: [PATCH 3/3] Update comments, add test case --- src/coreclr/gcdump/i386/gcdumpx86.cpp | 2 +- src/coreclr/vm/gc_unwind_x86.inl | 2 +- .../SmokeTests/Exceptions/Exceptions.cs | 25 +++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/coreclr/gcdump/i386/gcdumpx86.cpp b/src/coreclr/gcdump/i386/gcdumpx86.cpp index c5eb632ee95ec1..ba267bd3470c7e 100644 --- a/src/coreclr/gcdump/i386/gcdumpx86.cpp +++ b/src/coreclr/gcdump/i386/gcdumpx86.cpp @@ -457,7 +457,7 @@ size_t GCDump::DumpGCTable(PTR_CBYTE table, curOffs += (val & 0x07); #ifndef FEATURE_EH_FUNCLETS - // For x86/Linux, non-ptr arg pushes can be reported even for EBP frames + // For funclets, non-ptr arg pushes can be reported even for EBP frames _ASSERTE(!header.ebpFrame); #endif // FEATURE_EH_FUNCLETS argCnt++; diff --git a/src/coreclr/vm/gc_unwind_x86.inl b/src/coreclr/vm/gc_unwind_x86.inl index fe8c23a2a36a96..017f8f8f803a9e 100644 --- a/src/coreclr/vm/gc_unwind_x86.inl +++ b/src/coreclr/vm/gc_unwind_x86.inl @@ -1522,7 +1522,7 @@ unsigned scanArgRegTableI(PTR_CBYTE table, #ifndef FEATURE_EH_FUNCLETS hasPartialArgInfo = info->ebpFrame; #else - // For x86/Funclets, interruptible code always has full arg info + // For funclets, interruptible code always has full arg info // // This should be aligned with emitFullArgInfo setting at // emitter::emitEndCodeGen (in JIT) diff --git a/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs b/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs index 55028fdee9adb9..3fac6aa67daf8f 100644 --- a/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs +++ b/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; using System.Text; @@ -160,6 +161,8 @@ public static int Main() TestFirstChanceExceptionEvent(); + TestUnwindInFunclet(); + throw new Exception("UnhandledException"); return Fail; @@ -300,5 +303,27 @@ static bool FilterWithGC() CreateSomeGarbage(); return true; } + + static void TestUnwindInFunclet() + { + try + { + throw new Exception(); + } + catch (Exception e) + { + // x86 pushes the call arguments on the stack and moves the stack pointer. + // We use a non-inlined call with enough parameters to force this to happen, + // so we can verify that the unwinder can walk through this funclet. The + // unwinding is triggered by the GC.Collect call. + MultiparameterCallWithGC(0, 1, 2, 3, 4); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void MultiparameterCallWithGC(nint a, nint b, nint c, nint d, nint f) + { + GC.Collect(); + } }