-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[x86/Linux] Fix WIN64EXCEPTIONS build error #8629
Conversation
\CC @seanshpark |
I think it would be useful to batch all commits for the WIN64EXCEPTIONS into a single PR. It is hard to tell whether the small incremental changes like this one are good. |
@jkotas I'll submit all the related commits to this PR. |
9095fe5
to
8202b32
Compare
@@ -837,6 +837,13 @@ RtlVirtualUnwind_Unsafe( | |||
|
|||
#ifdef _TARGET_X86_ | |||
#ifndef FEATURE_PAL | |||
// | |||
// x86 ABI does not define RUNTIME_FUNCTION. Define our own to allow unification between x86 and other platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parjong I am not sure I understand how could the Windows build get broken. Things that you change for the WIN64EXCEPTIONS should be done in a way that doesn't influence windows x86 stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I am trying not to affect windows x86 stuff via submitting small commits frequently (I have no mean to check windows build issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that windows build uses PORTABILITY_ASSERT that raises compile error (instead of runtime error).
80636a4
to
cc26828
Compare
@@ -11,7 +11,7 @@ class Thread; | |||
#endif // DEBUG_REGDISPLAY | |||
|
|||
|
|||
#if defined(_TARGET_X86_) | |||
#if defined(_TARGET_X86_) && !defined(WIN64EXCEPTIONS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the REGDISPLAY commented out for WIN64EXCEPTIONS? It is a structure that is generally needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll just need to conditionally modify it to contain fields like in the amd64 or ARM version that are used for the WIN64EXCEPTIONS, like pCurrentContextPointers, ctxPtrsOne, ctxPtrsTwo, pCallerContextPointers or pCurrentContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By mistake, I thought that GetRegdisplayReturnValue
is available only for WIN64EXCEPTIONS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli Is it always required when WIN64EXCPTION
is defined? If there is no constraints on the offsets of each fields in REGDISPLAY
, then it would be better to extract these fields as a separate base class. Could you let me know your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the REGDISPLAY structure is always needed no matter whether WIN64EXCEPTION is defined or not. The only difference is in the few fields. I don't think adding a base class is worth the hassle. We can possibly clean it up this way after everything works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that code cleanup need to be done separately. I created #8643 as a staring point of this cleanup issue.
@@ -35,6 +35,24 @@ struct REGDISPLAY { | |||
PCODE ControlPC; | |||
TADDR PCTAddr; | |||
|
|||
#ifdef WIN64EXCEPTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also ifdef out the pContextForUnwind, it is not used with WIN64EXCEPTIONS. So maybe you can move this block next to the pContextForUnwind so that you can have single #ifdef / #else / #endif there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds good! I revised PR as you suggested.
e43b3af
to
2751c62
Compare
@@ -370,7 +389,9 @@ inline void FillRegDisplay(const PREGDISPLAY pRD, PT_CONTEXT pctx, PT_CONTEXT pC | |||
|
|||
#ifdef _TARGET_X86_ | |||
pRD->pContext = pctx; | |||
#ifndef WIN64EXCEPTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be different. We need to use the common code that's below for _WIN64 for WIN64EXCEPTIONS on Linux x86 too. So please keep this part of the code that is for TARGET_X86 untouched, just change the condition from TARGET_x86 to #ifndef WIN64EXCEPTIONS and the #elif defined(_WIN64) below to #else. Then in the #ifdef TARGET_AMD64 etc add branch for TARGET_X86 and put the registers copying in there in a way similar to what we do for the other targets.
There is also an #ifdef for _TARGET_ARM below, which would need to be merged with the _WIN64 part. They are almost the same, so it would be good anyways. Just be careful, the ARM path has two additional tiny details, explicit copying of Lr and Pc.
@@ -7556,7 +7556,7 @@ void InitSavedExceptionInfo() | |||
void FaultingExceptionFrame::Init(CONTEXT *pContext) | |||
{ | |||
WRAPPER_NO_CONTRACT; | |||
#if defined(_TARGET_X86_) | |||
#if defined(_TARGET_X86_) && !defined(WIN64EXCEPTIONS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that in many places where you use this #if
condition, it would make sense to use #ifndef (WIN64EXCEPTIONS)
with #ifdef _TARGET_X86_ ... #else PORTABILITY_ASSERT #endif
in it.
@@ -90,15 +91,24 @@ extern VOID ResetSEHRecord(PEXCEPTION_REGISTRATION_RECORD record); | |||
|
|||
#endif | |||
|
|||
|
|||
PEXCEPTION_REGISTRATION_RECORD GetCurrentSEHRecord(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetCurrentSEHRecord etc. is a part of the change that was made to compile with the WIN32EXCEPTIONS and that should be reverted now (it was made in #8613)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right
@@ -6,7 +6,7 @@ | |||
|
|||
extern "C" | |||
{ | |||
void ThrowControlForThread() | |||
void ThrowControlForThread(ONWIN64EXCEPTIONS(FaultingExceptionFrame *pfef)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ONWIN64EXCEPTIONS is not needed here, as we now have the WIN64EXCEPTIONS always on for Linux x86.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to revise this once we are able to run "Hello, World" with WIN64EXCEPTIONS.
@@ -1035,7 +1033,7 @@ StackWalkAction Thread::StackWalkFrames(PSTACKWALKFRAMESCALLBACK pCallback, | |||
FillRegDisplay(&rd, &ctx); | |||
} | |||
|
|||
#ifdef STACKWALKER_MAY_POP_FRAMES | |||
#if defined(STACKWALKER_MAY_POP_FRAMES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit, can you remove this cosmetic change, please?
@@ -697,13 +697,15 @@ EXTERN_C void __stdcall OnHijackFPTripThread(); // hijacked JIT code is returni | |||
|
|||
void CommonTripThread(); | |||
|
|||
#ifdef WIN64EXCEPTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is used at one place only, I don't think it is worth introducing the macro.
@@ -725,7 +723,7 @@ PCODE Thread::VirtualUnwindNonLeafCallFrame(T_CONTEXT* pContext, KNONVOLATILE_CO | |||
#if defined(_WIN64) | |||
UINT64 EstablisherFrame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli Is this _WIN64
is for general 64-bit architecture? Then, it would be better to use _BIT64 instead of _WIN64 (and _BIT32 for the below as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the _WIN64 is equivalent to BIT64. It would be nice to clean that up in the codebase at some point. There are about 600 usages in 180 files of the _WIN64.
But for now, it would not hurt if you could change it to BIT64 in place you are changing (please note it is BIT64, not _BIT64).
f2af7ac
to
65bdb9d
Compare
@dotnet-bot test this please |
@janvorli I revised PR per feedback, but CI checks are suddenly disappeared.. |
@dotnet-bot help |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories.
The following jobs are launched by default for each PR against dotnet/coreclr:master.
The following optional jobs are available in PRs against dotnet/coreclr:master.
Have a nice day! |
@dotnet-bot test this please |
@dotnet-bot test ci please |
|
||
PEXCEPTION_REGISTRATION_RECORD GetCurrentSEHRecord(); | ||
PEXCEPTION_REGISTRATION_RECORD GetFirstCOMPlusSEHRecord(Thread*); | ||
#ifdef WIN64EXCEPTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please merge the two ifdef blocks since they have the same condition?
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
@janvorli I revised PR per feedback. Please take a look. |
@@ -1355,11 +1355,11 @@ struct MSLAYOUT DebuggerIPCE_JITFuncData | |||
LSPTR_DJI nativeCodeJITInfoToken; | |||
VMPTR_MethodDesc vmNativeCodeMethodDescToken; | |||
|
|||
#if defined(DBG_TARGET_WIN64) || defined(DBG_TARGET_ARM) | |||
#ifdef WIN64EXCEPTIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change also the same condition at src\debug\di\rsthread.cpp:5852
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janvorli Revised.
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
@dotnet-bot test Linux ARM Emulator Cross Release Build please |
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
1 similar comment
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
@dotnet-bot test Linux ARM Emulator Cross Release Build please |
@mmitche both ARM CI legs are failing with |
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
@dotnet-bot test Linux ARM Emulator Cross Release Build please |
* Move GetUnwindInfo and GetNumberOfUnwindInfos into the real code header This commit fixes #8342. * Use WIN64EXCEPTIONS instead of _TARGET_X86_ * Revise FaultingExceptionFrame This commit revises FaultingExceptionFrame to support WIN64EXCEPTIONS in x86/Linux port. * Add RUNTIME_FUNCTION__EndAddress as NYI * Revise regdisp.h * Revise eetwain.h * Comment out exinfo.cpp if WIN64EXCEPTIONS is defined * Revises excep.cpp * Fix mistmatch in ThrowControlForThread defintion * Revises cgenx86.cpp * Disable SEH-based exception handlers when WIN64EXCEPTIONS is defined * Revise stackwalk.cpp * Revise jitinterface.cpp * Revise readytorun.h * Revise dbgipcevents.h * Revise zapcode.cpp * Revise clrnt.h * Fix Windows build error * Mark FaultingExceptionFrame::UpdateRegDisplay as NYI * Revise per feedback * Revert #if defined(..) as #ifdef * Fix style changes * Fix style changes * Remove #undef _TARGET_X86_ * 2nd attempt to fix Windows build error * Revise per feedback * Revert the chagnes in clrdefinitions.cmake and add BIT32 in CMakeLists.txt * Use !BIT64 instead of BIT32 * Include exceptionhandling.cpp and gcinfodecoder.cpp in build This commit includes exceptionhandling.cpp and gcinfodecoder.cpp in build, and fixes related compile errors. * Fix COMPlus_EndCatch undefined reference * Fix build error * Fix GcInfoDecoder-related undefined references * Fix AdjustContextForVirtualStub undefined reference * Fix GetCallerSP undefined reference * Fix ResetThreadAbortState undefined reference * Attempt to fix Windows build error * Fix CLRNoCatchHandler undefined reference * Another attemp to fix Windows build error * Fix GetXXXFromRedirectedStubStackFrame undefined references * Fix Windows Build Error * Add RtlpGetFunctionEndAddress and RtlVirtualUnwind as NYI * Fix undefined references on JIT helpers * Enable Dummy Application Run with WIN64EXCEPTIONS * Revert "Move GetUnwindInfo and GetNumberOfUnwindInfos into the real code header" This reverts commit c2bad85. * Use indirect code header when WIN64EXCEPTIONS is enabled * Port 'SyncRegDisplayToCurrentContext' and 'FillRegDisplay' * Revise style 'RUNTIME_FUNCTION__SetUnwindInfoAddress' * Extract out HandlerData from #ifdef region * Add UNIXTODO * Add UNIXTODO * Port 'GetRegdisplayReturnValue' * Fix incorrect comment * Remove messages that mentions WIN32EXCEPTIONS * Revise AdjustContextForWriteBarrier * Port 'FaultingExceptionFrame::UpdateRegDisplay' * Extract out 'AdjustContextForVirtualStub' and 'CLRNoCatchHandler' from #ifdef region * Merge two #ifdef regions * Set WIN64EXCEPTIONS as a default for x86/Linux * Remove unnecessary #ifdef from ThrowControlForThread * Remove unnecessary stubs * Add Dependency Check between Compile Flags * Revise per feedback
* Move GetUnwindInfo and GetNumberOfUnwindInfos into the real code header This commit fixes dotnet/coreclr#8342. * Use WIN64EXCEPTIONS instead of _TARGET_X86_ * Revise FaultingExceptionFrame This commit revises FaultingExceptionFrame to support WIN64EXCEPTIONS in x86/Linux port. * Add RUNTIME_FUNCTION__EndAddress as NYI * Revise regdisp.h * Revise eetwain.h * Comment out exinfo.cpp if WIN64EXCEPTIONS is defined * Revises excep.cpp * Fix mistmatch in ThrowControlForThread defintion * Revises cgenx86.cpp * Disable SEH-based exception handlers when WIN64EXCEPTIONS is defined * Revise stackwalk.cpp * Revise jitinterface.cpp * Revise readytorun.h * Revise dbgipcevents.h * Revise zapcode.cpp * Revise clrnt.h * Fix Windows build error * Mark FaultingExceptionFrame::UpdateRegDisplay as NYI * Revise per feedback * Revert #if defined(..) as #ifdef * Fix style changes * Fix style changes * Remove #undef _TARGET_X86_ * 2nd attempt to fix Windows build error * Revise per feedback * Revert the chagnes in clrdefinitions.cmake and add BIT32 in CMakeLists.txt * Use !BIT64 instead of BIT32 * Include exceptionhandling.cpp and gcinfodecoder.cpp in build This commit includes exceptionhandling.cpp and gcinfodecoder.cpp in build, and fixes related compile errors. * Fix COMPlus_EndCatch undefined reference * Fix build error * Fix GcInfoDecoder-related undefined references * Fix AdjustContextForVirtualStub undefined reference * Fix GetCallerSP undefined reference * Fix ResetThreadAbortState undefined reference * Attempt to fix Windows build error * Fix CLRNoCatchHandler undefined reference * Another attemp to fix Windows build error * Fix GetXXXFromRedirectedStubStackFrame undefined references * Fix Windows Build Error * Add RtlpGetFunctionEndAddress and RtlVirtualUnwind as NYI * Fix undefined references on JIT helpers * Enable Dummy Application Run with WIN64EXCEPTIONS * Revert "Move GetUnwindInfo and GetNumberOfUnwindInfos into the real code header" This reverts commit dotnet/coreclr@c2bad85. * Use indirect code header when WIN64EXCEPTIONS is enabled * Port 'SyncRegDisplayToCurrentContext' and 'FillRegDisplay' * Revise style 'RUNTIME_FUNCTION__SetUnwindInfoAddress' * Extract out HandlerData from #ifdef region * Add UNIXTODO * Add UNIXTODO * Port 'GetRegdisplayReturnValue' * Fix incorrect comment * Remove messages that mentions WIN32EXCEPTIONS * Revise AdjustContextForWriteBarrier * Port 'FaultingExceptionFrame::UpdateRegDisplay' * Extract out 'AdjustContextForVirtualStub' and 'CLRNoCatchHandler' from #ifdef region * Merge two #ifdef regions * Set WIN64EXCEPTIONS as a default for x86/Linux * Remove unnecessary #ifdef from ThrowControlForThread * Remove unnecessary stubs * Add Dependency Check between Compile Flags * Revise per feedback Commit migrated from dotnet/coreclr@2fc4478
This is the first step to resolve #8631.