This repository was archived by the owner on Jan 23, 2023. It is now read-only.
[x86/Linux] Fix WIN64EXCEPTIONS build error#8629
Merged
janvorli merged 62 commits intodotnet:masterfrom Jan 11, 2017
Merged
Conversation
Author
|
\CC @seanshpark |
Member
|
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. |
Author
|
@jkotas I'll submit all the related commits to this PR. |
This was referenced Dec 14, 2016
9095fe5 to
8202b32
Compare
janvorli
reviewed
Dec 14, 2016
| #ifdef _TARGET_X86_ | ||
| #ifndef FEATURE_PAL | ||
| // | ||
| // x86 ABI does not define RUNTIME_FUNCTION. Define our own to allow unification between x86 and other platforms. |
Member
There was a problem hiding this comment.
@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.
Author
There was a problem hiding this comment.
Sure, I am trying not to affect windows x86 stuff via submitting small commits frequently (I have no mean to check windows build issue).
Author
There was a problem hiding this comment.
It turns out that windows build uses PORTABILITY_ASSERT that raises compile error (instead of runtime error).
80636a4 to
cc26828
Compare
janvorli
reviewed
Dec 15, 2016
src/inc/regdisp.h
Outdated
|
|
||
|
|
||
| #if defined(_TARGET_X86_) | ||
| #if defined(_TARGET_X86_) && !defined(WIN64EXCEPTIONS) |
Member
There was a problem hiding this comment.
Why is the REGDISPLAY commented out for WIN64EXCEPTIONS? It is a structure that is generally needed.
Member
There was a problem hiding this comment.
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
Author
There was a problem hiding this comment.
By mistake, I thought that GetRegdisplayReturnValue is available only for WIN64EXCEPTIONS.
Author
There was a problem hiding this comment.
@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?
Member
There was a problem hiding this comment.
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.
Author
There was a problem hiding this comment.
I agree that code cleanup need to be done separately. I created #8643 as a staring point of this cleanup issue.
janvorli
reviewed
Dec 15, 2016
src/inc/regdisp.h
Outdated
| PCODE ControlPC; | ||
| TADDR PCTAddr; | ||
|
|
||
| #ifdef WIN64EXCEPTIONS |
Member
There was a problem hiding this comment.
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.
Author
There was a problem hiding this comment.
It sounds good! I revised PR as you suggested.
e43b3af to
2751c62
Compare
Author
janvorli
suggested changes
Dec 15, 2016
src/inc/regdisp.h
Outdated
|
|
||
| #ifdef _TARGET_X86_ | ||
| pRD->pContext = pctx; | ||
| #ifndef WIN64EXCEPTIONS |
Member
There was a problem hiding this comment.
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.
src/vm/excep.cpp
Outdated
| { | ||
| WRAPPER_NO_CONTRACT; | ||
| #if defined(_TARGET_X86_) | ||
| #if defined(_TARGET_X86_) && !defined(WIN64EXCEPTIONS) |
Member
There was a problem hiding this comment.
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.
| #endif | ||
|
|
||
|
|
||
| PEXCEPTION_REGISTRATION_RECORD GetCurrentSEHRecord(); |
Member
There was a problem hiding this comment.
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)
Author
src/vm/i386/unixstubs.cpp
Outdated
| extern "C" | ||
| { | ||
| void ThrowControlForThread() | ||
| void ThrowControlForThread(ONWIN64EXCEPTIONS(FaultingExceptionFrame *pfef)) |
Member
There was a problem hiding this comment.
The ONWIN64EXCEPTIONS is not needed here, as we now have the WIN64EXCEPTIONS always on for Linux x86.
Author
There was a problem hiding this comment.
It would be better to revise this once we are able to run "Hello, World" with WIN64EXCEPTIONS.
src/vm/stackwalk.cpp
Outdated
| } | ||
|
|
||
| #ifdef STACKWALKER_MAY_POP_FRAMES | ||
| #if defined(STACKWALKER_MAY_POP_FRAMES) |
Member
There was a problem hiding this comment.
A nit, can you remove this cosmetic change, please?
src/vm/threads.h
Outdated
|
|
||
| void CommonTripThread(); | ||
|
|
||
| #ifdef WIN64EXCEPTIONS |
Member
There was a problem hiding this comment.
Since this is used at one place only, I don't think it is worth introducing the macro.
parjong
commented
Dec 15, 2016
| @@ -725,7 +723,7 @@ PCODE Thread::VirtualUnwindNonLeafCallFrame(T_CONTEXT* pContext, KNONVOLATILE_CO | |||
| #if defined(_WIN64) | |||
| UINT64 EstablisherFrame; | |||
Author
There was a problem hiding this comment.
@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).
Member
There was a problem hiding this comment.
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
Author
|
@dotnet-bot test this please |
Author
|
@janvorli I revised PR per feedback, but CI checks are suddenly disappeared.. |
Author
|
@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! |
Author
|
@dotnet-bot test this please |
Author
|
@dotnet-bot test ci please |
janvorli
reviewed
Jan 6, 2017
src/vm/i386/excepcpu.h
Outdated
|
|
||
| PEXCEPTION_REGISTRATION_RECORD GetCurrentSEHRecord(); | ||
| PEXCEPTION_REGISTRATION_RECORD GetFirstCOMPlusSEHRecord(Thread*); | ||
| #ifdef WIN64EXCEPTIONS |
Member
There was a problem hiding this comment.
Can you please merge the two ifdef blocks since they have the same condition?
Author
|
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
Author
|
@janvorli I revised PR per feedback. Please take a look. |
Author
janvorli
suggested changes
Jan 10, 2017
| VMPTR_MethodDesc vmNativeCodeMethodDescToken; | ||
|
|
||
| #if defined(DBG_TARGET_WIN64) || defined(DBG_TARGET_ARM) | ||
| #ifdef WIN64EXCEPTIONS |
Member
There was a problem hiding this comment.
Please change also the same condition at src\debug\di\rsthread.cpp:5852
janvorli
approved these changes
Jan 10, 2017
Author
|
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
Author
|
@dotnet-bot test Linux ARM Emulator Cross Release Build please |
Author
|
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
1 similar comment
Author
|
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
Author
|
@dotnet-bot test Linux ARM Emulator Cross Release Build please |
Member
|
@mmitche both ARM CI legs are failing with |
Member
Member
|
@dotnet-bot test Linux ARM Emulator Cross Debug Build please |
Member
|
@dotnet-bot test Linux ARM Emulator Cross Release Build please |
manofstick
pushed a commit
to manofstick/coreclr
that referenced
this pull request
Jan 16, 2017
* 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
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
* 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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the first step to resolve #8631.