From 73c21b5876646b3fdea751bc84ef21f3fa0098df Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 2 Apr 2025 17:31:00 +0200 Subject: [PATCH 1/9] Change win-x86 configuration to build with FEATURE_EH_FUNCLETS --- src/coreclr/clr.featuredefines.props | 5 +---- src/coreclr/clrdefinitions.cmake | 4 +--- src/coreclr/jit/targetx86.h | 3 --- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/coreclr/clr.featuredefines.props b/src/coreclr/clr.featuredefines.props index 883d0bda0f5574..1e4050c8d56e0a 100644 --- a/src/coreclr/clr.featuredefines.props +++ b/src/coreclr/clr.featuredefines.props @@ -2,6 +2,7 @@ true true + true true @@ -22,10 +23,6 @@ true - - true - - true diff --git a/src/coreclr/clrdefinitions.cmake b/src/coreclr/clrdefinitions.cmake index cb3b645ff0e58d..9dece5e3b89fd1 100644 --- a/src/coreclr/clrdefinitions.cmake +++ b/src/coreclr/clrdefinitions.cmake @@ -206,9 +206,7 @@ if(CLR_CMAKE_TARGET_WIN32) endif(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_I386) endif(CLR_CMAKE_TARGET_WIN32) -if (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32) - add_compile_definitions($<$>>:FEATURE_EH_FUNCLETS>) -endif (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32) +add_compile_definitions($<$>>:FEATURE_EH_FUNCLETS>) if (CLR_CMAKE_TARGET_WIN32 AND (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64)) add_definitions(-DFEATURE_SPECIAL_USER_MODE_APC) diff --git a/src/coreclr/jit/targetx86.h b/src/coreclr/jit/targetx86.h index e630a1ae842120..ac0130f5d274ce 100644 --- a/src/coreclr/jit/targetx86.h +++ b/src/coreclr/jit/targetx86.h @@ -52,9 +52,6 @@ // target #define FEATURE_EH 1 // To aid platform bring-up, eliminate exceptional EH clauses (catch, filter, // filter-handler, fault) and directly execute 'finally' clauses. -#if !defined(UNIX_X86_ABI) - #define FEATURE_EH_WINDOWS_X86 1 // Enable support for SEH regions -#endif #define ETW_EBP_FRAMED 1 // if 1 we cannot use EBP as a scratch register and must create EBP based // frames for most methods #define CSE_CONSTS 1 // Enable if we want to CSE constants From ca2f490ff6f797306833905ae0dbf9673bc30e57 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 14 May 2025 16:29:36 -0700 Subject: [PATCH 2/9] Debugger fixes for the Windows X86 EH Funclets model Notable areas improved - The funclets model uses the vectored exception handler to funnel debug events to the debugger - FramePointer details for the debugger are inscrutable and weird. I've made it work, but enough of this is driven by subtleties in the stackwalker that I would not be surprised if there are additional issues here - RtlpGetFunctionEndAddress needs to use a DAC pointer to read the unwind info. - Funclet prologs for CoreCLR X86 Funclets need to have at least 1 instruction in them so that the Native->IL mapping is correct. Otherwise it gets shadowed by a mapping which says it is a PROLOG instruction. This is probably fixable by allowing an Native -> IL mapping for both the PROLOG as well as the IL offset in the funclet, but a nop here is both easy to generate an unlikely to be a major cost. - Likewise, EnC frame generation is no longer able to rely on the shadow stack pointer logic injected by EH handling, and needed 1 bit of info not in the current emitted stack information. Notably, that there is synchronized codegen in the method. Instead of modifying the gcinfo to include that data, we just put in fake offsets for where the synchronized region is, and use the existence of any data as a flag in the EnC layout. NOTE: I also removed the general purpose logic to read the synchronized range from the data. Also NOTE: Before enabling funclets by default, manually test a set of scenarios which do EnC with various frame layouts. Notably, frame layouts with localloc, frame layouts with locals that trigger GSCookies to be used, frame layouts of static generic methods. --- src/coreclr/debug/ee/debugger.cpp | 3 ++- src/coreclr/debug/ee/frameinfo.cpp | 6 +++--- src/coreclr/inc/clrnt.h | 2 +- src/coreclr/inc/eetwain.h | 7 ++++++- src/coreclr/inc/regdisp.h | 4 ++++ src/coreclr/jit/codegenxarch.cpp | 7 +++++++ src/coreclr/jit/gcencode.cpp | 9 +++++++++ src/coreclr/jit/lclvars.cpp | 2 ++ .../src/System/Runtime/StackFrameIterator.cs | 2 ++ src/coreclr/vm/eetwain.cpp | 14 ++++++++++++-- src/coreclr/vm/excep.cpp | 8 +++++++- src/coreclr/vm/exceptionhandling.cpp | 6 +++++- src/coreclr/vm/gc_unwind_x86.inl | 10 ++++++++++ 13 files changed, 70 insertions(+), 10 deletions(-) diff --git a/src/coreclr/debug/ee/debugger.cpp b/src/coreclr/debug/ee/debugger.cpp index 69c06d3309701c..0798d4507974ef 100644 --- a/src/coreclr/debug/ee/debugger.cpp +++ b/src/coreclr/debug/ee/debugger.cpp @@ -7573,7 +7573,8 @@ HRESULT Debugger::SendException(Thread *pThread, } CONTRACTL_END; - LOG((LF_CORDB, LL_INFO10000, "D::SendException\n")); + LOG((LF_CORDB, LL_INFO10000, "D::SendException pThread=0x%p fFirstChance=%s currentIP=0x%p currentSP= 0x%p fContinuable=%s fAttaching=%s fForceNonInterceptable=%s\n", + pThread, fFirstChance ? "true" : "false", (void*)currentIP, (void*)currentSP, fContinuable ? "true" : "false", fAttaching ? "true" : "false", fForceNonInterceptable ? "true" : "false")); if (CORDBUnrecoverableError(this)) { diff --git a/src/coreclr/debug/ee/frameinfo.cpp b/src/coreclr/debug/ee/frameinfo.cpp index b5f87320a5dca3..bd4866eaa96661 100644 --- a/src/coreclr/debug/ee/frameinfo.cpp +++ b/src/coreclr/debug/ee/frameinfo.cpp @@ -1265,7 +1265,7 @@ FramePointer GetFramePointerForDebugger(DebuggerFrameData* pData, CrawlFrame* pC FramePointer fpResult; -#if defined(FEATURE_EH_FUNCLETS) +#if !defined(TARGET_X86) if (pData->info.frame == NULL) { // This is a managed method frame. @@ -1277,7 +1277,7 @@ FramePointer GetFramePointerForDebugger(DebuggerFrameData* pData, CrawlFrame* pC fpResult = FramePointer::MakeFramePointer((LPVOID)(pData->info.frame)); } -#else // !FEATURE_EH_FUNCLETS +#else // !TARGET_X86 if ((pCF == NULL || !pCF->IsFrameless()) && pData->info.frame != NULL) { // @@ -1299,7 +1299,7 @@ FramePointer GetFramePointerForDebugger(DebuggerFrameData* pData, CrawlFrame* pC fpResult = FramePointer::MakeFramePointer((LPVOID)GetRegdisplayStackMark(&(pData->regDisplay))); } -#endif // !FEATURE_EH_FUNCLETS +#endif // !TARGET_X86 LOG((LF_CORDB, LL_INFO100000, "GFPFD: Frame pointer is 0x%p\n", fpResult.GetSPValue())); diff --git a/src/coreclr/inc/clrnt.h b/src/coreclr/inc/clrnt.h index bcd4427538babe..8040117a28b8f6 100644 --- a/src/coreclr/inc/clrnt.h +++ b/src/coreclr/inc/clrnt.h @@ -211,7 +211,7 @@ RtlpGetFunctionEndAddress ( _In_ TADDR ImageBase ) { - PUNWIND_INFO pUnwindInfo = (PUNWIND_INFO)(ImageBase + FunctionEntry->UnwindData); + DPTR(UNWIND_INFO) pUnwindInfo = dac_cast(ImageBase + FunctionEntry->UnwindData); return FunctionEntry->BeginAddress + pUnwindInfo->FunctionLength; } diff --git a/src/coreclr/inc/eetwain.h b/src/coreclr/inc/eetwain.h index 59fdb2922c99da..78dd453f55b2a2 100644 --- a/src/coreclr/inc/eetwain.h +++ b/src/coreclr/inc/eetwain.h @@ -259,7 +259,7 @@ virtual void * GetGSCookieAddr(PREGDISPLAY pContext, virtual bool IsInPrologOrEpilog(DWORD relPCOffset, GCInfoToken gcInfoToken, size_t* prologSize) = 0; - +#ifndef FEATURE_EH_FUNCLETS /* Returns true if the given IP is in the synchronized region of the method (valid for synchronized methods only) */ @@ -267,6 +267,7 @@ virtual bool IsInSynchronizedRegion( DWORD relOffset, GCInfoToken gcInfoToken, unsigned flags) = 0; +#endif // FEATURE_EH_FUNCLETS #endif // !USE_GC_INFO_DECODER /* @@ -513,6 +514,7 @@ bool IsInPrologOrEpilog( GCInfoToken gcInfoToken, size_t* prologSize); +#ifndef FEATURE_EH_FUNCLETS /* Returns true if the given IP is in the synchronized region of the method (valid for synchronized functions only) */ @@ -521,6 +523,7 @@ bool IsInSynchronizedRegion( DWORD relOffset, GCInfoToken gcInfoToken, unsigned flags); +#endif // FEATURE_EH_FUNCLETS #endif // !USE_GC_INFO_DECODER /* @@ -710,6 +713,7 @@ bool IsInPrologOrEpilog( return false; } +#ifndef FEATURE_EH_FUNCLETS virtual bool IsInSynchronizedRegion( DWORD relOffset, @@ -720,6 +724,7 @@ bool IsInSynchronizedRegion( _ASSERTE(FALSE); return false; } +#endif // FEATURE_EH_FUNCLETS #endif // !USE_GC_INFO_DECODER virtual diff --git a/src/coreclr/inc/regdisp.h b/src/coreclr/inc/regdisp.h index 59944c23c5cefa..7a5cf9d9d0cec4 100644 --- a/src/coreclr/inc/regdisp.h +++ b/src/coreclr/inc/regdisp.h @@ -132,7 +132,11 @@ inline TADDR GetRegdisplayFP(REGDISPLAY *display) { inline LPVOID GetRegdisplayFPAddress(REGDISPLAY *display) { LIMITED_METHOD_CONTRACT; +#ifdef FEATURE_EH_FUNCLETS + return &display->pCurrentContext->Ebp; +#else return (LPVOID)display->GetEbpLocation(); +#endif } inline TADDR GetRegdisplayPCTAddr(REGDISPLAY *display) diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 69390fe6c340a6..e536234c080d22 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -11101,6 +11101,13 @@ void CodeGen::genFuncletProlog(BasicBlock* block) #ifdef UNIX_X86_ABI // Add a padding for 16-byte alignment inst_RV_IV(INS_sub, REG_SPBASE, 12, EA_PTRSIZE); +#else + if (!IsTargetAbi(CORINFO_NATIVEAOT_ABI)) + { + // Funclet prologs need to have at least 1 byte or the IL->Native mapping data will not + // include the first IL instruction in the funclet. + instGen(INS_nop); + } #endif } diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index a8ba3bf4b23ad0..6c8879ce0af1b1 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -1578,6 +1578,15 @@ size_t GCInfo::gcInfoBlockHdrSave( assert(header->epilogCount <= 1); } #endif + if (compiler->UsesFunclets() && compiler->info.compFlags & CORINFO_FLG_SYNCH && compiler->opts.compDbgEnC) + { + // While the sync start offset and end offset are not used by the stackwalker/EH system + // in funclets mode, we do need to know if the code is synchronized if we are generating + // and edit and continue method, so that we can properly manage the stack during a Remap + // operation. Instead of inventing a new encoding, just encode some non-0 offsets into these fields. + header->syncStartOffset = 1; + header->syncEndOffset = 2; + } header->revPInvokeOffset = INVALID_REV_PINVOKE_OFFSET; if (compiler->opts.IsReversePInvoke()) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 79aa47da44b318..87d405a5239666 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -4085,6 +4085,7 @@ unsigned Compiler::lvaGetMaxSpillTempSize() * | security object | * |-----------------------| * | ParamTypeArg | +// If funclet support is disabled * |-----------------------| * | Last-executed-filter | * |-----------------------| @@ -4092,6 +4093,7 @@ unsigned Compiler::lvaGetMaxSpillTempSize() * ~ Shadow SPs ~ * | | * |-----------------------| +// Endif funclet support is disabled * | | * ~ Variables ~ * | | diff --git a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/StackFrameIterator.cs b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/StackFrameIterator.cs index f758b16b5d22aa..e89eb88a3f0b69 100644 --- a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/StackFrameIterator.cs +++ b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/StackFrameIterator.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Runtime.InteropServices; #if !NATIVEAOT using System.Runtime.ExceptionServices; @@ -60,6 +61,7 @@ internal unsafe struct StackFrameIterator #pragma warning restore CA1822 #endif // NATIVEAOT + [StackTraceHidden] internal bool Init(EH.PAL_LIMITED_CONTEXT* pStackwalkCtx, bool instructionFault = false, bool* fIsExceptionIntercepted = null) { return InternalCalls.RhpSfiInit(ref this, pStackwalkCtx, instructionFault, fIsExceptionIntercepted); diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp index 9290e3f15a9d93..bc10c6c942c0c7 100644 --- a/src/coreclr/vm/eetwain.cpp +++ b/src/coreclr/vm/eetwain.cpp @@ -161,7 +161,7 @@ bool VarIsInReg(ICorDebugInfo::VarLoc varLoc) * could easily duplicate what they do, which is why we're calling into them. */ -HRESULT EECodeManager::FixContextForEnC(PCONTEXT pCtx, + HRESULT EECodeManager::FixContextForEnC(PCONTEXT pCtx, EECodeInfo * pOldCodeInfo, const ICorDebugInfo::NativeVarInfo * oldMethodVars, SIZE_T oldMethodVarsCount, @@ -206,6 +206,11 @@ HRESULT EECodeManager::FixContextForEnC(PCONTEXT pCtx, return E_FAIL; // stack should be empty - @TODO : Barring localloc } +#ifdef FEATURE_EH_FUNCLETS + // EnC remap inside handlers is not supported + if (pOldCodeInfo->IsFunclet() || pNewCodeInfo->IsFunclet()) + return CORDBG_E_ENC_IN_FUNCLET; +#else if (oldInfo->handlers) { bool hasInnerFilter; @@ -229,13 +234,16 @@ HRESULT EECodeManager::FixContextForEnC(PCONTEXT pCtx, } } } +#endif // FEATURE_EH_FUNCLETS /* @TODO: Check if we have grown out of space for locals, in the face of localloc */ _ASSERTE(!oldInfo->localloc && !newInfo->localloc); +#ifndef FEATURE_EH_FUNCLETS // @TODO: If nesting level grows above the MAX_EnC_HANDLER_NESTING_LEVEL, // we should return EnC_NESTED_HANLDERS _ASSERTE(oldInfo->handlers && newInfo->handlers); +#endif LOG((LF_ENC, LL_INFO100, "EECM::FixContextForEnC: Checks out\n")); @@ -1542,7 +1550,7 @@ OBJECTREF EECodeManager::GetInstance( PREGDISPLAY pContext, #endif #else // FEATURE_EH_FUNCLETS - if (pCodeInfo->GetMethodDesc()->AcquiresInstMethodTableFromThis()) // Generic Context is "this" + if (pCodeInfo->GetMethodDesc()->AcquiresInstMethodTableFromThis() && (hdrInfoBody->genericsContext)) // Generic Context is "this" { // Untracked table must have at least one entry - this pointer _ASSERTE(hdrInfoBody->untrackedCnt > 0); @@ -1797,6 +1805,7 @@ bool EECodeManager::IsInPrologOrEpilog(DWORD relPCoffset, (info.epilogOffs != hdrInfo::NOT_IN_EPILOG)); } +#ifndef FEATURE_EH_FUNCLETS /***************************************************************************** * * Returns true if the given IP is in the synchronized region of the method (valid for synchronized functions only) @@ -1827,6 +1836,7 @@ bool EECodeManager::IsInSynchronizedRegion(DWORD relOffset, // Everything after the epilog is also in synchronized region. (info.epilogCnt != 0 && info.syncEpilogStart + info.epilogSize <= relOffset); } +#endif // FEATURE_EH_FUNCLETS #endif // !USE_GC_INFO_DECODER /***************************************************************************** diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 4d0470610d84e7..e920dc9c4f796e 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -6999,7 +6999,7 @@ LONG WINAPI CLRVectoredExceptionHandlerShim(PEXCEPTION_POINTERS pExceptionInfo) return EXCEPTION_CONTINUE_SEARCH; } -#if defined(TARGET_X86) +#if defined(TARGET_X86) && !defined(FEATURE_EH_FUNCLETS) if (dwCode == EXCEPTION_BREAKPOINT || dwCode == EXCEPTION_SINGLE_STEP) { // For interop debugging, debugger bashes our managed exception handler. @@ -11324,7 +11324,13 @@ void SoftwareExceptionFrame::UpdateRegDisplay_Impl(const PREGDISPLAY pRD, bool u ENUM_CALLEE_SAVED_REGISTERS(); #undef CALLEE_SAVED_REGISTER +#if defined(DACCESS_COMPILE) && defined(TARGET_X86) +// X86 unwinding always works in terms of context pointers, so they need to be in the correct address space when debugging +// This may work for other architectures as well, but that isn't tested. +#define CALLEE_SAVED_REGISTER(regname) pRD->pCurrentContextPointers->regname = &pRD->pCurrentContext->regname; +#else #define CALLEE_SAVED_REGISTER(regname) pRD->pCurrentContextPointers->regname = m_ContextPointers.regname; +#endif ENUM_CALLEE_SAVED_REGISTERS(); #undef CALLEE_SAVED_REGISTER diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index b939ddc968cc3b..fc6a08f1aeae69 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -2956,7 +2956,11 @@ void MarkInlinedCallFrameAsEHHelperCall(Frame* pFrame) static TADDR GetSpForDiagnosticReporting(REGDISPLAY *pRD) { #ifdef ESTABLISHER_FRAME_ADDRESS_IS_CALLER_SP - return CallerStackFrame::FromRegDisplay(pRD).SP; + TADDR sp = CallerStackFrame::FromRegDisplay(pRD).SP; +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_X86) + sp -= sizeof(TADDR); // For X86 with funclets we want the address 1 pointer into the callee. +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_X86) + return sp; #else return GetSP(pRD->pCurrentContext); #endif diff --git a/src/coreclr/vm/gc_unwind_x86.inl b/src/coreclr/vm/gc_unwind_x86.inl index e1502ec6049e9a..3ba207aad819e9 100644 --- a/src/coreclr/vm/gc_unwind_x86.inl +++ b/src/coreclr/vm/gc_unwind_x86.inl @@ -653,6 +653,15 @@ inline size_t GetSizeOfFrameHeaderForEnC(hdrInfo * info) { WRAPPER_NO_CONTRACT; +#ifdef FEATURE_EH_FUNCLETS + _ASSERTE(info->ebpFrame); + unsigned position = info->savedRegsCountExclFP + + info->localloc + + info->genericsContext + // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG + ((info->syncStartOffset != INVALID_SYNC_OFFSET) ? 1 : 0) // Is this method synchronized + + 1; // for ebpFrame + return position * sizeof(TADDR); +#else // See comment above Compiler::lvaAssignFrameOffsets() in src\jit\il\lclVars.cpp // for frame layout @@ -664,6 +673,7 @@ inline size_t GetSizeOfFrameHeaderForEnC(hdrInfo * info) // to get the total size of the header. return sizeof(TADDR) + GetEndShadowSPSlotsOffset(info, MAX_EnC_HANDLER_NESTING_LEVEL); +#endif // FEATURE_EH_FUNCLETS } #endif // FEATURE_NATIVEAOT From 3708d1a10014b9a3d4971b683ff50d290444024f Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 14 May 2025 16:31:15 -0700 Subject: [PATCH 3/9] Revert "Change win-x86 configuration to build with FEATURE_EH_FUNCLETS" This reverts commit 73c21b5876646b3fdea751bc84ef21f3fa0098df. --- src/coreclr/clr.featuredefines.props | 5 ++++- src/coreclr/clrdefinitions.cmake | 4 +++- src/coreclr/jit/targetx86.h | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/coreclr/clr.featuredefines.props b/src/coreclr/clr.featuredefines.props index 1e4050c8d56e0a..883d0bda0f5574 100644 --- a/src/coreclr/clr.featuredefines.props +++ b/src/coreclr/clr.featuredefines.props @@ -2,7 +2,6 @@ true true - true true @@ -23,6 +22,10 @@ true + + true + + true diff --git a/src/coreclr/clrdefinitions.cmake b/src/coreclr/clrdefinitions.cmake index 9dece5e3b89fd1..cb3b645ff0e58d 100644 --- a/src/coreclr/clrdefinitions.cmake +++ b/src/coreclr/clrdefinitions.cmake @@ -206,7 +206,9 @@ if(CLR_CMAKE_TARGET_WIN32) endif(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_I386) endif(CLR_CMAKE_TARGET_WIN32) -add_compile_definitions($<$>>:FEATURE_EH_FUNCLETS>) +if (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32) + add_compile_definitions($<$>>:FEATURE_EH_FUNCLETS>) +endif (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32) if (CLR_CMAKE_TARGET_WIN32 AND (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64)) add_definitions(-DFEATURE_SPECIAL_USER_MODE_APC) diff --git a/src/coreclr/jit/targetx86.h b/src/coreclr/jit/targetx86.h index ac0130f5d274ce..e630a1ae842120 100644 --- a/src/coreclr/jit/targetx86.h +++ b/src/coreclr/jit/targetx86.h @@ -52,6 +52,9 @@ // target #define FEATURE_EH 1 // To aid platform bring-up, eliminate exceptional EH clauses (catch, filter, // filter-handler, fault) and directly execute 'finally' clauses. +#if !defined(UNIX_X86_ABI) + #define FEATURE_EH_WINDOWS_X86 1 // Enable support for SEH regions +#endif #define ETW_EBP_FRAMED 1 // if 1 we cannot use EBP as a scratch register and must create EBP based // frames for most methods #define CSE_CONSTS 1 // Enable if we want to CSE constants From 1e4ecc8c0c13d63ace499c36b70bb5dfc6940808 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 15 May 2025 11:40:48 -0700 Subject: [PATCH 4/9] Update src/coreclr/jit/codegenxarch.cpp Co-authored-by: Filip Navara --- src/coreclr/jit/codegenxarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index e536234c080d22..25d559d2806b36 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -11102,7 +11102,7 @@ void CodeGen::genFuncletProlog(BasicBlock* block) // Add a padding for 16-byte alignment inst_RV_IV(INS_sub, REG_SPBASE, 12, EA_PTRSIZE); #else - if (!IsTargetAbi(CORINFO_NATIVEAOT_ABI)) + if (!compiler->IsTargetAbi(CORINFO_NATIVEAOT_ABI)) { // Funclet prologs need to have at least 1 byte or the IL->Native mapping data will not // include the first IL instruction in the funclet. From c4710f02cd29b82358c5af8ec35084de50653335 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 15 May 2025 15:06:07 -0700 Subject: [PATCH 5/9] JIT formatting --- src/coreclr/jit/gcencode.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 6c8879ce0af1b1..3da2ce7929b437 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -1585,7 +1585,7 @@ size_t GCInfo::gcInfoBlockHdrSave( // and edit and continue method, so that we can properly manage the stack during a Remap // operation. Instead of inventing a new encoding, just encode some non-0 offsets into these fields. header->syncStartOffset = 1; - header->syncEndOffset = 2; + header->syncEndOffset = 2; } header->revPInvokeOffset = INVALID_REV_PINVOKE_OFFSET; From 7c14b4e929c3e74ed1ff1e5b92281769ccf5af58 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 20 May 2025 16:43:48 -0700 Subject: [PATCH 6/9] Update src/coreclr/jit/gcencode.cpp Co-authored-by: Juan Hoyos <19413848+hoyosjs@users.noreply.github.com> --- src/coreclr/jit/gcencode.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 3da2ce7929b437..3320cdd682ba00 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -1582,7 +1582,7 @@ size_t GCInfo::gcInfoBlockHdrSave( { // While the sync start offset and end offset are not used by the stackwalker/EH system // in funclets mode, we do need to know if the code is synchronized if we are generating - // and edit and continue method, so that we can properly manage the stack during a Remap + // an edit and continue method, so that we can properly manage the stack during a Remap // operation. Instead of inventing a new encoding, just encode some non-0 offsets into these fields. header->syncStartOffset = 1; header->syncEndOffset = 2; From b860621b521a57a3dc40c8cbfb85cef0424f955d Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 21 May 2025 11:07:47 -0700 Subject: [PATCH 7/9] Reapply "Change win-x86 configuration to build with FEATURE_EH_FUNCLETS" This reverts commit 3708d1a10014b9a3d4971b683ff50d290444024f. --- src/coreclr/clr.featuredefines.props | 5 +---- src/coreclr/clrdefinitions.cmake | 4 +--- src/coreclr/jit/targetx86.h | 3 --- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/coreclr/clr.featuredefines.props b/src/coreclr/clr.featuredefines.props index 883d0bda0f5574..1e4050c8d56e0a 100644 --- a/src/coreclr/clr.featuredefines.props +++ b/src/coreclr/clr.featuredefines.props @@ -2,6 +2,7 @@ true true + true true @@ -22,10 +23,6 @@ true - - true - - true diff --git a/src/coreclr/clrdefinitions.cmake b/src/coreclr/clrdefinitions.cmake index cb3b645ff0e58d..9dece5e3b89fd1 100644 --- a/src/coreclr/clrdefinitions.cmake +++ b/src/coreclr/clrdefinitions.cmake @@ -206,9 +206,7 @@ if(CLR_CMAKE_TARGET_WIN32) endif(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_I386) endif(CLR_CMAKE_TARGET_WIN32) -if (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32) - add_compile_definitions($<$>>:FEATURE_EH_FUNCLETS>) -endif (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32) +add_compile_definitions($<$>>:FEATURE_EH_FUNCLETS>) if (CLR_CMAKE_TARGET_WIN32 AND (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64)) add_definitions(-DFEATURE_SPECIAL_USER_MODE_APC) diff --git a/src/coreclr/jit/targetx86.h b/src/coreclr/jit/targetx86.h index e630a1ae842120..ac0130f5d274ce 100644 --- a/src/coreclr/jit/targetx86.h +++ b/src/coreclr/jit/targetx86.h @@ -52,9 +52,6 @@ // target #define FEATURE_EH 1 // To aid platform bring-up, eliminate exceptional EH clauses (catch, filter, // filter-handler, fault) and directly execute 'finally' clauses. -#if !defined(UNIX_X86_ABI) - #define FEATURE_EH_WINDOWS_X86 1 // Enable support for SEH regions -#endif #define ETW_EBP_FRAMED 1 // if 1 we cannot use EBP as a scratch register and must create EBP based // frames for most methods #define CSE_CONSTS 1 // Enable if we want to CSE constants From d5aa0156c4d5a79e950dda8aa9b1281b8c49d8a5 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 21 May 2025 15:11:07 -0700 Subject: [PATCH 8/9] Fix issue around synchronized methods --- src/coreclr/jit/gcencode.cpp | 13 ++++++++++--- src/coreclr/jit/lclvars.cpp | 3 +++ src/coreclr/vm/gc_unwind_x86.inl | 19 ++++++++++++++++++- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 3320cdd682ba00..edf1a9ebd601ef 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -1578,14 +1578,21 @@ size_t GCInfo::gcInfoBlockHdrSave( assert(header->epilogCount <= 1); } #endif - if (compiler->UsesFunclets() && compiler->info.compFlags & CORINFO_FLG_SYNCH && compiler->opts.compDbgEnC) + if (compiler->UsesFunclets() && compiler->info.compFlags & CORINFO_FLG_SYNCH) { // While the sync start offset and end offset are not used by the stackwalker/EH system // in funclets mode, we do need to know if the code is synchronized if we are generating // an edit and continue method, so that we can properly manage the stack during a Remap - // operation. Instead of inventing a new encoding, just encode some non-0 offsets into these fields. + // operation, for determining the ParamTypeArg for collectible generics purposes, and + // for determining the offset of the localloc variable in the stack frame. + // Instead of inventing a new encoding, just encode some non-0 offsets into these fields. + // to indicate that the method is synchronized. + // + // Use 1 for both offsets, since that doesn't actually make sense and implies that the + // sync region is 0 bytes long. The JIT will never emit a sync region of 0 bytes in non- + // funclet mode. header->syncStartOffset = 1; - header->syncEndOffset = 2; + header->syncEndOffset = 1; } header->revPInvokeOffset = INVALID_REV_PINVOKE_OFFSET; diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 87d405a5239666..5c9fae90046118 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -3673,6 +3673,9 @@ PhaseStatus Compiler::lvaMarkLocalVars() // saved EBP <-- EBP points here // other callee-saved registers // InfoHdrSmall.savedRegsCountExclFP specifies this size // optional GS cookie // InfoHdrSmall.security is 1 if this exists + // if FEATURE_EH_FUNCLETS + // issynchronized bool if it is a synchronized method + // endif // FEATURE_EH_FUNCLETS // LocAllocSP slot // -- lower addresses -- // diff --git a/src/coreclr/vm/gc_unwind_x86.inl b/src/coreclr/vm/gc_unwind_x86.inl index 3ba207aad819e9..96600f73c984e8 100644 --- a/src/coreclr/vm/gc_unwind_x86.inl +++ b/src/coreclr/vm/gc_unwind_x86.inl @@ -189,7 +189,12 @@ size_t DecodeGCHdrInfo(GCInfoToken gcInfoToken, header.syncEndOffset = decodeUnsigned(table); _ASSERTE(header.syncStartOffset != INVALID_SYNC_OFFSET && header.syncEndOffset != INVALID_SYNC_OFFSET); +#ifdef FEATURE_EH_FUNCLETS + _ASSERTE(header.syncStartOffset == 1); + _ASSERTE(header.syncEndOffset == 1); +#else _ASSERTE(header.syncStartOffset < header.syncEndOffset); +#endif } if (header.revPInvokeOffset == HAS_REV_PINVOKE_FRAME_OFFSET) @@ -366,6 +371,9 @@ size_t GetLocallocSPOffset(hdrInfo * info) _ASSERTE(info->localloc && info->ebpFrame); unsigned position = info->savedRegsCountExclFP + +#ifdef FEATURE_EH_FUNCLETS + ((info->syncStartOffset != INVALID_SYNC_OFFSET) ? 1 : 0) + // Is this method synchronized +#endif 1; return position * sizeof(TADDR); } @@ -377,12 +385,20 @@ size_t GetParamTypeArgOffset(hdrInfo * info) _ASSERTE((info->genericsContext || info->handlers) && info->ebpFrame); +#ifdef FEATURE_EH_FUNCLETS unsigned position = info->savedRegsCountExclFP + + ((info->syncStartOffset != INVALID_SYNC_OFFSET) ? 1 : 0) + // Is this method synchronized info->localloc + 1; // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG +#else + unsigned position = info->savedRegsCountExclFP + + info->localloc + + 1; // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG +#endif return position * sizeof(TADDR); } +#ifndef FEATURE_EH_FUNCLETS inline size_t GetStartShadowSPSlotsOffset(hdrInfo * info) { LIMITED_METHOD_DAC_CONTRACT; @@ -425,6 +441,7 @@ inline size_t GetEndShadowSPSlotsOffset(hdrInfo * info, unsigned maxHandlerNesti return GetStartShadowSPSlotsOffset(info) + (numberOfShadowSPSlots * sizeof(TADDR)); } +#endif // FEATURE_EH_FUNCLETS /***************************************************************************** * returns the base frame pointer corresponding to the target nesting level. @@ -524,7 +541,6 @@ FrameType GetHandlerFrameInfo(hdrInfo * info, _ASSERTE(condition); \ } - PTR_TADDR pFirstBaseSPslot = GetFirstBaseSPslotPtr(frameEBP, info); TADDR baseSP = GetOutermostBaseFP(frameEBP, info); bool nonLocalHandlers = false; // Are the funclets invoked by EE (instead of managed code itself) bool hasInnerFilter = false; @@ -539,6 +555,7 @@ FrameType GetHandlerFrameInfo(hdrInfo * info, // expected to be in decreasing order. size_t lvl = 0; #ifndef FEATURE_EH_FUNCLETS + PTR_TADDR pFirstBaseSPslot = GetFirstBaseSPslotPtr(frameEBP, info); PTR_TADDR pSlot; for(lvl = 0, pSlot = pFirstBaseSPslot; *pSlot && lvl < unwindLevel; From d32e3d2ff6e147aa29ce62a985a2017cb2fcf2fd Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 21 May 2025 15:11:33 -0700 Subject: [PATCH 9/9] Revert "Reapply "Change win-x86 configuration to build with FEATURE_EH_FUNCLETS"" This reverts commit b860621b521a57a3dc40c8cbfb85cef0424f955d. --- src/coreclr/clr.featuredefines.props | 5 ++++- src/coreclr/clrdefinitions.cmake | 4 +++- src/coreclr/jit/targetx86.h | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/coreclr/clr.featuredefines.props b/src/coreclr/clr.featuredefines.props index 1e4050c8d56e0a..883d0bda0f5574 100644 --- a/src/coreclr/clr.featuredefines.props +++ b/src/coreclr/clr.featuredefines.props @@ -2,7 +2,6 @@ true true - true true @@ -23,6 +22,10 @@ true + + true + + true diff --git a/src/coreclr/clrdefinitions.cmake b/src/coreclr/clrdefinitions.cmake index 9dece5e3b89fd1..cb3b645ff0e58d 100644 --- a/src/coreclr/clrdefinitions.cmake +++ b/src/coreclr/clrdefinitions.cmake @@ -206,7 +206,9 @@ if(CLR_CMAKE_TARGET_WIN32) endif(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_I386) endif(CLR_CMAKE_TARGET_WIN32) -add_compile_definitions($<$>>:FEATURE_EH_FUNCLETS>) +if (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32) + add_compile_definitions($<$>>:FEATURE_EH_FUNCLETS>) +endif (NOT CLR_CMAKE_TARGET_ARCH_I386 OR NOT CLR_CMAKE_TARGET_WIN32) if (CLR_CMAKE_TARGET_WIN32 AND (CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64)) add_definitions(-DFEATURE_SPECIAL_USER_MODE_APC) diff --git a/src/coreclr/jit/targetx86.h b/src/coreclr/jit/targetx86.h index ac0130f5d274ce..e630a1ae842120 100644 --- a/src/coreclr/jit/targetx86.h +++ b/src/coreclr/jit/targetx86.h @@ -52,6 +52,9 @@ // target #define FEATURE_EH 1 // To aid platform bring-up, eliminate exceptional EH clauses (catch, filter, // filter-handler, fault) and directly execute 'finally' clauses. +#if !defined(UNIX_X86_ABI) + #define FEATURE_EH_WINDOWS_X86 1 // Enable support for SEH regions +#endif #define ETW_EBP_FRAMED 1 // if 1 we cannot use EBP as a scratch register and must create EBP based // frames for most methods #define CSE_CONSTS 1 // Enable if we want to CSE constants