PAL_VirtualUnwind - is there need to add special case for NetBSD?

[krytarowski]

File: src/pal/src/exception/seh-unwind.cpp

BOOL PAL_VirtualUnwind(CONTEXT *context, KNONVOLATILE_CONTEXT_POINTERS *contextPointers)
{
 int st;
 unw_context_t unwContext;
 unw_cursor_t cursor;

#if defined(__APPLE__) || defined(__FreeBSD__) || defined(_ARM64_) || defined(_ARM_)
 DWORD64 curPc;
#endif

 if ((context->ContextFlags & CONTEXT_EXCEPTION_ACTIVE) != 0)
 {
 // The current frame is a source of hardware exception. Due to the fact that
 // we use the low level unwinder to unwind just one frame a time, the
 // unwinder doesn't have the signal_frame flag set. So it doesn't
 // know that it should not decrement the PC before looking up the unwind info.
 // So we compensate it by incrementing the PC before passing it to the unwinder.
 // Without it, the unwinder would not find unwind info if the hardware exception
 // happened in the first instruction of a function.
 CONTEXTSetPC(context, CONTEXTGetPC(context) + 1);
 }

#if UNWIND_CONTEXT_IS_UCONTEXT_T
 WinContextToUnwindContext(context, &unwContext);
#else
 st = unw_getcontext(&unwContext);
 if (st < 0)
 {
 return FALSE;
 }
#endif
 st = unw_init_local(&cursor, &unwContext);
 if (st < 0)
 {
 return FALSE;
 }

#if !UNWIND_CONTEXT_IS_UCONTEXT_T
 // Set the unwind context to the specified windows context
 WinContextToUnwindCursor(context, &cursor);
#endif

#if defined(__APPLE__) || defined(__FreeBSD__) || defined(_ARM64_) || defined(_ARM_)
 // OSX and FreeBSD appear to do two different things when unwinding
 // 1: If it reaches where it cannot unwind anymore, say a 
 // managed frame. It wil return 0, but also update the $pc
 // 2: If it unwinds all the way to _start it will return
 // 0 from the step, but $pc will stay the same.
 // The behaviour of libunwind from nongnu.org is to null the PC
 // So we bank the original PC here, so we can compare it after
 // the step
 curPc = CONTEXTGetPC(context);
#endif

 st = unw_step(&cursor);
 if (st < 0)
 {
 return FALSE;
 }

 // Check if the frame we have unwound to is a frame that caused
 // synchronous signal, like a hardware exception and record it
 // in the context flags.
 if (unw_is_signal_frame(&cursor) > 0)
 {
 context->ContextFlags |= CONTEXT_EXCEPTION_ACTIVE;
 }
 else
 {
 context->ContextFlags &= ~CONTEXT_EXCEPTION_ACTIVE;
 }

 // Update the passed in windows context to reflect the unwind
 //
 UnwindContextToWinContext(&cursor, context);
#if defined(__APPLE__) || defined(__FreeBSD__) || defined(_ARM64_) || defined(_ARM_)
 if (st == 0 && CONTEXTGetPC(context) == curPc)
 {
 CONTEXTSetPC(context, 0);
 }
#endif

 if (contextPointers != NULL)
 {
 GetContextPointers(&cursor, &unwContext, contextPointers);
 }
 return TRUE;
}

How to check if I need the extra code path enabled also for NetBSD?

[krytarowski]

@janvorli ^

[janvorli]

To check if you need that, just create a simple .NET app that throws an unhandled exception - as simple as throwing Exception from Main without any try / catch around it. If the app hangs, you need the workaround. If it prints the unhandled exception message and then dumps core, you don't.

[krytarowski]

I gave this a try:

diff --git a/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp b/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp
index 8aa7a0f..316dd17 100644
--- a/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp
+++ b/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp
@@ -28,31 +28,31 @@ int DllTest1()
 {
 Trace("Starting pal_sxs test1 DllTest1\n");

- PAL_TRY(VOID*, unused, NULL)
- {
+// PAL_TRY(VOID*, unused, NULL)
+// {
 volatile int* p = (volatile int *)0x11; // Invalid pointer

 bTry = TRUE; // Indicate we hit the PAL_TRY block
 *p = 1; // Causes an access violation exception

 Fail("ERROR: code was executed after the access violation.\n");
- }
- PAL_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
- {
- if (!bTry)
- {
- Fail("ERROR: PAL_EXCEPT was hit without PAL_TRY being hit.\n");
- }
-
- // Validate that the faulting address is correct; the contents of "p" (0x11).
- if (ex.ExceptionRecord.ExceptionInformation[1] != 0x11)
- {
- Fail("ERROR: PAL_EXCEPT ExceptionInformation[1] != 0x11\n");
- }
-
- bExcept = TRUE; // Indicate we hit the PAL_EXCEPT block 
- }
- PAL_ENDTRY;
+// }
+// PAL_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
+// {
+// if (!bTry)
+// {
+// Fail("ERROR: PAL_EXCEPT was hit without PAL_TRY being hit.\n");
+// }
+//
+// // Validate that the faulting address is correct; the contents of "p" (0x11).
+// if (ex.ExceptionRecord.ExceptionInformation[1] != 0x11)
+// {
+// Fail("ERROR: PAL_EXCEPT ExceptionInformation[1] != 0x11\n");
+// }
+//
+// bExcept = TRUE; // Indicate we hit the PAL_EXCEPT block 
+// }
+// PAL_ENDTRY;

 if (!bTry)
 {
chieftec$ git diff src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp|tee 
diff --git a/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp b/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp
index 8aa7a0f..316dd17 100644
--- a/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp
+++ b/src/pal/tests/palsuite/exception_handling/pal_sxs/test1/dlltest1.cpp
@@ -28,31 +28,31 @@ int DllTest1()
 {
 Trace("Starting pal_sxs test1 DllTest1\n");

- PAL_TRY(VOID*, unused, NULL)
- {
+// PAL_TRY(VOID*, unused, NULL)
+// {
 volatile int* p = (volatile int *)0x11; // Invalid pointer

 bTry = TRUE; // Indicate we hit the PAL_TRY block
 *p = 1; // Causes an access violation exception

 Fail("ERROR: code was executed after the access violation.\n");
- }
- PAL_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
- {
- if (!bTry)
- {
- Fail("ERROR: PAL_EXCEPT was hit without PAL_TRY being hit.\n");
- }
-
- // Validate that the faulting address is correct; the contents of "p" (0x11).
- if (ex.ExceptionRecord.ExceptionInformation[1] != 0x11)
- {
- Fail("ERROR: PAL_EXCEPT ExceptionInformation[1] != 0x11\n");
- }
-
- bExcept = TRUE; // Indicate we hit the PAL_EXCEPT block 
- }
- PAL_ENDTRY;
+// }
+// PAL_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
+// {
+// if (!bTry)
+// {
+// Fail("ERROR: PAL_EXCEPT was hit without PAL_TRY being hit.\n");
+// }
+//
+// // Validate that the faulting address is correct; the contents of "p" (0x11).
+// if (ex.ExceptionRecord.ExceptionInformation[1] != 0x11)
+// {
+// Fail("ERROR: PAL_EXCEPT ExceptionInformation[1] != 0x11\n");
+// }
+//
+// bExcept = TRUE; // Indicate we hit the PAL_EXCEPT block 
+// }
+// PAL_ENDTRY;

 if (!bTry)
 {

And the result is as follows:

$ LD_LIBRARY_PATH=. ./paltest_pal_sxs_test1 
PAL_SXS test1 SIGSEGV handler 0x0
Starting pal_sxs test1 DllTest2
DLLTest2 PASSED
Starting pal_sxs test1 DllTest1
pal_sxs test1: signal handler called
ERROR: executed signal handler NOT from try/catch

Is it everything to check? Is it fine?

[janvorli]

No, I meant testing it with a managed app. Or, alternatively, you can try to get a current context somewhere with a call stack containing only native frames and then calling the PAL_VirtualUnwind on that context in a loop until you get zero in the IP. If it keeps looping forever, you need that fix. So something like this:

CONTEXT context;
RtlCaptureContext(&context);
do
{
 PAL_VirtualUnwind(&context, NULL);
}
while (GetIP(context) != 0);

[krytarowski]

Thank you, I will test it.

[myungjoo]

If you are confident with the usage of libunwind and C/C++ programming, you may test with a sample C app that has functions calls from main() and see what libunwind (unw_step() ) returns with the topmost stack starting from the function calling unw_step(). If the libunwind cursor has already reached the topmost and calling unw_step() on the cursor again returns 0 and the PC value of the cursor unchanged, you need this.

That "special" condition is required for the case where PAL_VirtualUnwind() does not properly tell you if it has reached the top (or the bottom depending on the convention) or not; e.g., PC in context is kept unchanged if it has reached the end.

Ultimately, such a test could be done in CMakeList.txt and automatically configure the macro ifdef properly. But, I'm satisfied with the current one for now. :)

[krytarowski]

It's worth to add such check for PAL tests.

According to my tests I think I need this patch too.

When running CoreFX tests, I have got more tests passed:

$ cat /tmp/log-with-patch-applied.txt | gawk 'match($0, /Total: ([0-9]+), Errors: ([0-9]+), Failed: ([0-9]+), Skipped: ([0-9]+), Time: ([^s]+)s/, a) { t["Total"]+=a[1]; t["Errors"]+=a[2]; t["Failed"]+=a[3]; t["Skipped"]+=a[4]; t["Time"]+=a[5]} END { print "Total: " t["Total"]+0 " Errors: " t["Errors"]+0 " Failed: " t["Failed"]+0 " Skipped: " t["Skipped"]+0 " Time: " t["Time"]+0 }' 
Total: 19787 Errors: 0 Failed: 2 Skipped: 0 Time: 5494
$ cat /tmp/log-without-patch.txt | gawk 'match($0, /Total: ([0-9]+), Errors: ([0-9]+), Failed: ([0-9]+), Skipped: ([0-9]+), Time: ([^s]+)s/, a) { t["Total"]+=a[1]; t["Errors"]+=a[2]; t["Failed"]+=a[3]; t["Skipped"]+=a[4]; t["Time"]+=a[5]} END { print "Total: " t["Total"]+0 " Errors: " t["Errors"]+0 " Failed: " t["Failed"]+0 " Skipped: " t["Skipped"]+0 " Time: " t["Time"]+0 }'
Total: 16655 Errors: 0 Failed: 2 Skipped: 0 Time: 4603

[krytarowski]

With increased timeout for a test from 15min to 45min:

Total: 89989 Errors: 0 Failed: 2 Skipped: 0 Time: 9797