Establish stack frame inside CallEHFunclet

[parjong]

This commit revises the implementation of CallEHFunclet to establish
stack frame appropriately.

This commit allows libunwind to work correctly for the frame created by
CallEHFunclet.

[parjong]

This commit tries to fix #4579

[rahku]

Is the issue because alloc_stack is missing unwind codes?

[parjong]

unw_step (which is used inside PAL_VirtualUnwind) failed to recognize the stack allocation, and thus all the registers are spoiled during stack unwind (e.g LR is updated with the value of R11).

[rahku]

instead of this fix. I would experiment with the unwind annotations for alloc_stack & free_stack https://github.com/dotnet/coreclr/blob/master/src/pal/inc/unixasmmacrosarm.inc#L60 . Maybe free_stack does not need .pad annotation

[janvorli]

It is strange that the .pad in the alloc_stack doesn't work. I wonder - should the .pad actually precede the sub sp, sp, \Size in the macro? Could you please try if that change alone fixes the problem? Looking at https://sourceware.org/binutils/docs/as/ARM-Unwinding-Tutorial.html, that tutorial uses first the .pad and then the sub sp, sp, #nn while our macro does the opposite.

[parjong]

@janvorli I reverted this change and changed code as you mentioned, but it looks like that unwind still fails.

[parjong]

@rahku @janvorli When I removed .pad instruction from free_stack, I got the following error:

In try
In try 2, 1st throw
In 1st catch, 2nd throw
terminate called after throwing an instance of 'PAL_SEHException'

[parjong]

CC @myungjoo @leemgs

[myungjoo]

.pad should have pointed r7 saved in the stack (need to add 12) or .setfp along with setting r7, pointing at r7 saved in stack is required.

[myungjoo]

You may need to manually formulate r7 and .setfp as I did in https://github.com/dotnet/coreclr/blob/master/src/pal/inc/unixasmmacrosarm.inc

because r7 is not at the top of the stack.

Please test gdb stack tracing at a callee of the corresponding asm function if it traces the caller of the function.

Note that in the case of ThePreStub along with Roslyn issue, it seems that you need to configure .save .pad .setfp and fp register (r7) all together consistnetly. Unlike the common FP usage, R7 is supposed to point at the saved (caller's value) r7 in the stack. I will need to confirm this by looking at gnueabi doc or libunwind-arm code; however this is sort of empirical result with libunwind-arm and gdb.

@parjong I am returning the day after tomorrow. I will be able to look at this issue then.

[myungjoo]

Can we simply push r7, lr instead of r4-r11, lr here?

[parjong]

I tried that first, but I encountered the following assertion failure:

Assert failure(PID 4686 [0x0000124e], Thread: 4686 [0x124e]): dac_cast<DWORD>(address) & THUMB_CODE
    File: /home/parjong/projects/toolchains/coreclr/src/debug/ee/controller.h Line: 727
    Image: /mnt/dotnet-latest/Linux.arm.Debug/corerun

**** MessageBox invoked, title 'corerun - Assert Failure (PID 4686, Thread 4686/0x124e)' ****
  dac_cast<DWORD>(address) & THUMB_CODE

[parjong]

I'll check whether it is possible only with {r4, r7, lr}.

[myungjoo]

This seems to be caused by bugs from other assembly files.
I've found FP handling issues in many places and will send patches soon there.

With such functions corrected along with the alternative patch on this file:

diff --git a/src/vm/arm/ehhelpers.S b/src/vm/arm/ehhelpers.S
index dfe1b11..c32a06b 100644
--- a/src/vm/arm/ehhelpers.S
+++ b/src/vm/arm/ehhelpers.S
@@ -47,6 +47,8 @@ OFFSET_OF_FRAME=(4 + SIZEOF__GSCookie)
         //
         // Runtime check for 8-byte alignment. 
         PROLOG_STACK_SAVE r7
+        // We lose stack unwindability here by configuring fp(r7) incorrectely
+        // here.
         and r0, r7, #4
         sub sp, sp, r0

@@ -99,6 +101,7 @@ OFFSET_OF_FRAME=(4 + SIZEOF__GSCookie)
         NESTED_ENTRY CallEHFunclet, _TEXT, NoHandler

         PROLOG_PUSH  "{r4-r11, lr}"
+        PROLOG_STACK_SAVE_OFFSET  r7, #12
         alloc_stack  4

         // On entry:
@@ -124,7 +127,8 @@ OFFSET_OF_FRAME=(4 + SIZEOF__GSCookie)
         // frame pointer for accessing the locals in the parent method.
         NESTED_ENTRY CallEHFilterFunclet, _TEXT, NoHandler

-        PROLOG_PUSH  "{lr}"
+        PROLOG_PUSH  "{r7, lr}"
+        PROLOG_STACK_SAVE r7
         alloc_stack  4

         // On entry:
@@ -140,6 +144,6 @@ OFFSET_OF_FRAME=(4 + SIZEOF__GSCookie)
         blx r2

         free_stack   4
-        EPILOG_POP   "{pc}"
+        EPILOG_POP   "{r7, pc}"

         NESTED_END CallEHFilterFunclet, _TEXT

It works. For example:

bash-3.2# ./corerun seh_tests.exe
Testcase successful
bash-3.2# ./corerun csc.exe /lib:. /r:mscorlib.dll hello.cs
Microsoft (R) Visual C# Compiler version 42.42.42.42
Copyright (C) Microsoft Corporation. All rights reserved.

bash-3.2# ./corerun hello.exe
Hello World
bash-3.2# 

p.s. PROLOG_STACK_SAVE_OFFSET requires updates from #4641 .

[prajwal-aithal]

@dotnet-bot test Linux ARM emulator

[myungjoo]

@parjong It looks like that our codes are inter-dependent to each other. Let's refactor our codes tomorrow.

[parjong]

@myungjoo I revised PR per feedback. Currently, I have to push '{r4 - r11, lr}' due to the aforementioned assertion failure (should be revised after #4641).

[dnfclas]

@parjong, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[parjong]

@dotnet-bot test Ubuntu x64 Checked Build and Test please

[parjong]

@dotnet-bot test OSX x64 Checked Build and Test please

[parjong]

@jkotas @janvorli Please take a look.

[janvorli]

I would prefer you using the PROLOG_STACK_SAVE_OFFSET macro that @myungjoo created in #4641. We prefer not using the unwinder instructions explicitly in the assembler helpers.

[myungjoo]

Please note that #4641 has your commit included and revised.

[parjong]

I'll close this PR since PR #4641 that includes all the necessary patches is merged.