Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[x86/Linux] Add Dummy Exception Handler #8613

Merged
merged 1 commit into from
Dec 14, 2016
Merged

[x86/Linux] Add Dummy Exception Handler #8613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/vm/exceptmacros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ VOID DECLSPEC_NORETURN RaiseTheExceptionInternalOnly(OBJECTREF throwable, BOOL r
void UnwindAndContinueRethrowHelperInsideCatch(Frame* pEntryFrame, Exception* pException);
VOID DECLSPEC_NORETURN UnwindAndContinueRethrowHelperAfterCatch(Frame* pEntryFrame, Exception* pException);

#if defined(FEATURE_PAL) && defined(WIN64EXCEPTIONS)
#ifdef FEATURE_PAL
VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHardwareException);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By mistake, I was working on a bit old branch. I reverted this macro to make it easy to move on WIN64EXCEPTIONS.


#define INSTALL_MANAGED_EXCEPTION_DISPATCHER \
Expand Down Expand Up @@ -334,14 +334,14 @@ VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHar
UNREACHABLE(); \
}

#else // FEATURE_PAL && WIN64EXCEPTIONS
#else // FEATURE_PAL

#define INSTALL_MANAGED_EXCEPTION_DISPATCHER
#define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER
#define INSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP
#define UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP

#endif // FEATURE_PAL && WIN64EXCEPTIONS
#endif // FEATURE_PAL

#define INSTALL_UNWIND_AND_CONTINUE_HANDLER_NO_PROBE \
{ \
Expand Down
31 changes: 28 additions & 3 deletions src/vm/i386/excepcpu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,37 @@ class Thread;
// Actually, the handler getting set is properly registered
#endif

#ifdef FEATURE_PAL

extern VOID SetSEHRecord(PEXCEPTION_REGISTRATION_RECORD record);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder - what if we just defined implemented __writefsdword and __readfsdword for FEATURE_PAL and put the implementation to the unixstubs.cpp? Then the change in this file could be minimalized to just adding declaration of the two functions if FEATURE_PAL is defined.
The implementation in unixstubs.cpp would be trivial - just to add CurrentSEHRecord variable (btw, it needs to be thread local) and let the __writefsdword set it and __readfsdword read it.

Copy link
Author

@parjong parjong Dec 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I once tried that, but failed. Clang recognizes __readfsdword as a builtin function.

The implementation of SetSEHRecord and ResetSEHRecord is almost same as you suggested for __writefdsword and __readfsdword. I just put them in excepx86.cpp as GetCurrentSEHRecord need to be revised, too

  • VerifyValidTransitionFromManagedCode invokes GetCurrentSEHRecord, and then segmentation fault happens at __readfsdword.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I haven't realized that these functions are defined. Let's leave it as it is then.

extern VOID ResetSEHRecord(PEXCEPTION_REGISTRATION_RECORD record);

#define INSTALL_SEH_RECORD(record) \
SetSEHRecord(record); \

#define UNINSTALL_SEH_RECORD(record) \
ResetSEHRecord(record);

#else // FEATURE_PAL

#define INSTALL_SEH_RECORD(record) \
{ \
(record)->Next = (PEXCEPTION_REGISTRATION_RECORD)__readfsdword(0); \
__writefsdword(0, (DWORD) (record)); \
}

#define UNINSTALL_SEH_RECORD(record) \
{ \
__writefsdword(0, (DWORD) ((record)->Next)); \
}

#endif // FEATURE_PAL

#define INSTALL_EXCEPTION_HANDLING_RECORD(record) \
{ \
PEXCEPTION_REGISTRATION_RECORD __record = (record); \
_ASSERTE(__record < GetCurrentSEHRecord()); \
__record->Next = (PEXCEPTION_REGISTRATION_RECORD)__readfsdword(0); \
__writefsdword(0, (DWORD)__record); \
INSTALL_SEH_RECORD(record); \
}

//
Expand All @@ -44,7 +69,7 @@ class Thread;
{ \
PEXCEPTION_REGISTRATION_RECORD __record = (record); \
_ASSERTE(__record == GetCurrentSEHRecord()); \
__writefsdword(0, (DWORD)__record->Next); \
UNINSTALL_SEH_RECORD(record); \
}

// stackOverwriteBarrier is used to detect overwriting of stack which will mess up handler registration
Expand Down
29 changes: 28 additions & 1 deletion src/vm/i386/excepx86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1971,11 +1971,17 @@ PTR_CONTEXT GetCONTEXTFromRedirectedStubStackFrame(CONTEXT * pContext)
}

#if !defined(DACCESS_COMPILE)
#ifdef FEATURE_PAL
static PEXCEPTION_REGISTRATION_RECORD CurrentSEHRecord = EXCEPTION_CHAIN_END;
#endif

PEXCEPTION_REGISTRATION_RECORD GetCurrentSEHRecord()
{
WRAPPER_NO_CONTRACT;

#ifdef FEATURE_PAL
LPVOID fs0 = CurrentSEHRecord;
#else // FEATURE_PAL
LPVOID fs0 = (LPVOID)__readfsdword(0);

#if 0 // This walk is too expensive considering we hit it every time we a CONTRACT(NOTHROW)
Expand Down Expand Up @@ -2006,11 +2012,26 @@ PEXCEPTION_REGISTRATION_RECORD GetCurrentSEHRecord()
pEHR = pEHR->Next;
}
#endif
#endif
#endif // 0
#endif // FEATURE_PAL

return (EXCEPTION_REGISTRATION_RECORD*) fs0;
}

#ifdef FEATURE_PAL
VOID SetSEHRecord(PEXCEPTION_REGISTRATION_RECORD record)
{
WRAPPER_NO_CONTRACT;
record->Next = CurrentSEHRecord;
CurrentSEHRecord = record;
}

VOID ResetSEHRecord(PEXCEPTION_REGISTRATION_RECORD record)
{
CurrentSEHRecord = record->Next;
}
#endif // FEATURE_PAL

PEXCEPTION_REGISTRATION_RECORD GetFirstCOMPlusSEHRecord(Thread *pThread) {
WRAPPER_NO_CONTRACT;
#ifndef FEATURE_PAL
Expand Down Expand Up @@ -3748,4 +3769,10 @@ AdjustContextForVirtualStub(
return TRUE;
}

#ifdef FEATURE_PAL
VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHardwareException)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this function? It is only used for WIN64EXCEPTIONS.

Copy link
Author

@parjong parjong Dec 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current definition of INSTALL_MANAGED_EXCEPTION_DISPATCHER and UNINSTALL_MANAGED_EXCEPTION_DISPATCHER uses DispatchManagedException even when WIN64EXCEPTIONS is not used:

299 #ifdef FEATURE_PAL
300 VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHardwareException);
301
302 #define INSTALL_MANAGED_EXCEPTION_DISPATCHER        \
303         PAL_SEHException exCopy;                    \
304         bool hasCaughtException = false;            \
305         try {
306
307 #define UNINSTALL_MANAGED_EXCEPTION_DISPATCHER      \
308         }                                           \
309         catch (PAL_SEHException& ex)                \
310         {                                           \
311             exCopy = std::move(ex);                 \
312             hasCaughtException = true;              \
313         }                                           \
314         if (hasCaughtException)                     \
315         {                                           \
316             DispatchManagedException(exCopy, false);\
317         }
318

For me, the use of PAL_SEHException seems to be related with FEATURE_PAL rather than WIN64EXCEPTIONS, and thus I try to add its own DispatchManagedException instead of changing the above macros.

Please let me know which one would be better: revise the macro or add a dummy dispatcher.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I guess the dummy dispatcher will be easier to remove once we move to WIN64EXCEPTIONS in the future. But I would recommend not to implement is as throw, but rather put in a DebugBreak();. The throw cannot work in most of the cases since it would hit managed frames where the C++ exception handling would get lost. So it would be better to break into the debugger or crash right on the spot if there is no debugger attached.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DebugBreak introduces function declared 'noreturn' should not return [-Werror,-Winvalid-noreturn] error. Is it reasonable to use UNREACHABLE() instead of DebugBreak() here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please use that.

{
UNREACHABLE();
}
#endif
#endif // !DACCESS_COMPILE