Port from tfs: onload exception debugger crash fix (#4868)

The ExceptionHijackPersonalityRoutine was retrieving a context from a
particular stack frame, but the calling conventions for the function
executing in that frame allow it to overwrite the context. This was
causing the debugger to crash for exceptions thrown from the onload
method in winforms on x64.

The fix is to instead retrieve the context from the previous stack
frame, whose layout is set up explicitly in ExceptionHijack.

Author: sbomer

src/debug/ee/amd64/dbghelpers.S

@@ -45,6 +45,13 @@ NESTED_ENTRY ExceptionHijack, _TEXT, UnhandledExceptionHandlerUnix
         // So we allocate 4 stack slots as the outgoing argument home and just copy the 
         // arguments set up by DacDbi into these stack slots.  We will take a perf hit,
         // but this is not a perf critical code path anyway.
+
+        // There is an additional dependency on this alloc_stack: the
+        // ExceptionHijackPersonalityRoutine assumes that it can find
+        // the first argument to HijackWorker in the stack frame of
+        // ExceptionHijack, at an offset of exactly 0x20 bytes from
+        // ExceptionHijackWorker's stack frame. Therefore it is
+        // important that we move the stack pointer by the same amount.
         alloc_stack 0x20
         END_PROLOGUE
 
@@ -53,6 +60,11 @@ NESTED_ENTRY ExceptionHijack, _TEXT, UnhandledExceptionHandlerUnix
         // stack for us.  However, the Orcas compilers don't like a 0-sized frame, so 
         // we need to allocate something here and then just copy the stack arguments to 
         // their new argument homes.
+
+        // In x86, ExceptionHijackWorker is marked STDCALL, so it finds
+        // its arguments on the stack. In x64, it gets its arguments in
+        // registers (set up for us by DacDbiInterfaceImpl::Hijack),
+        // and this stack space may be reused.
         mov     rax, [rsp + 20h]
         mov     [rsp], rax
         mov     rax, [rsp + 28h]

src/debug/ee/amd64/dbghelpers.asm

@@ -48,6 +48,13 @@ NESTED_ENTRY ExceptionHijack, _TEXT, ExceptionHijackPersonalityRoutine
         ; So we allocate 4 stack slots as the outgoing argument home and just copy the 
         ; arguments set up by DacDbi into these stack slots.  We will take a perf hit,
         ; but this is not a perf critical code path anyway.
+
+        ; There is an additional dependency on this alloc_stack: the
+        ; ExceptionHijackPersonalityRoutine assumes that it can find
+        ; the first argument to HijackWorker in the stack frame of
+        ; ExceptionHijack, at an offset of exactly 0x20 bytes from
+        ; ExceptionHijackWorker's stack frame. Therefore it is
+        ; important that we move the stack pointer by the same amount.
         alloc_stack 20h
         END_PROLOGUE
 
@@ -56,6 +63,11 @@ NESTED_ENTRY ExceptionHijack, _TEXT, ExceptionHijackPersonalityRoutine
         ; stack for us.  However, the Orcas compilers don't like a 0-sized frame, so 
         ; we need to allocate something here and then just copy the stack arguments to 
         ; their new argument homes.
+
+        ; In x86, ExceptionHijackWorker is marked STDCALL, so it finds
+        ; its arguments on the stack. In x64, it gets its arguments in
+        ; registers (set up for us by DacDbiInterfaceImpl::Hijack),
+        ; and this stack space may be reused.
         mov     rax, [rsp + 20h]
         mov     [rsp], rax
         mov     rax, [rsp + 28h]

src/debug/ee/debugger.cpp

@@ -13502,7 +13502,17 @@ ExceptionHijackPersonalityRoutine(IN     PEXCEPTION_RECORD   pExceptionRecord
     CONTEXT * pHijackContext = NULL;
 
     // Get the 1st parameter (the Context) from hijack worker.
-    pHijackContext = *reinterpret_cast<CONTEXT **>(pDispatcherContext->EstablisherFrame);
+    // EstablisherFrame points to the stack slot 8 bytes above the
+    // return address to the ExceptionHijack. This would contain the
+    // parameters passed to ExceptionHijackWorker, which is marked
+    // STDCALL, but the x64 calling convention lets the
+    // ExceptionHijackWorker use that stack space, resulting in the
+    // context being overwritten. Instead, we get the context from the
+    // previous stack frame, which contains the arguments to
+    // ExceptionHijack, placed there by the debugger in
+    // DacDbiInterfaceImpl::Hijack. This works because ExceptionHijack
+    // allocates exactly 4 stack slots.
+    pHijackContext = *reinterpret_cast<CONTEXT **>(pDispatcherContext->EstablisherFrame + 0x20);
 
     // This copies pHijackContext into pDispatcherContext, which the OS can then
     // use to walk the stack.