[ARM64/Unix] #9500

sdmaclea · 2017-02-10T22:39:31Z

Patches to support correct ARM64 functional behaviour on Ubuntu 16.04

@janvorli

Add native unwind info

Restore non-volatile machine state in CallEHFunclet

Use empty stack slot to save argument

When FEATURE_PAL is enableds USE_REDIRECT_FOR_GCSTRESS is not supported

janvorli · 2017-02-13T13:32:41Z

src/vm/arm64/calldescrworkerarm64.S

@@ -46,27 +46,27 @@ LOCAL_LABEL(donestack):

    // If FP arguments are supplied in registers (x8 != NULL) then initialize all of them from the pointer


Please fix the comment to refer to r9 instead of r8.

janvorli · 2017-02-13T13:37:53Z

src/pal/src/arch/arm64/context2.S

@@ -8,6 +8,7 @@
 //

 #include "unixasmmacros.inc"
+#include "asmconstants.h"

 #define CONTEXT_ARM64   0x00400000L


Could you please move these constants to the asmconstants.h too to match the amd64 version of this file?

janvorli · 2017-02-13T13:39:39Z

src/pal/src/arch/arm64/exceptionhelper.S

-    EMIT_BREAKPOINT
+    // Save the FP & LR to the stack so that the unwind can work at the instruction after
+    // loading the FP from the context, but before loading the SP from the context.
+    stp fp, lr, [sp, -16]!


A nit - can you please keep the parameter indentation uniform in the function?

janvorli · 2017-02-13T13:40:59Z

src/pal/src/arch/arm64/exceptionhelper.S

+    // loading the FP from the context, but before loading the SP from the context.
+    stp fp, lr, [sp, -16]!
+
+    ldr	x19,	[x0, #(CONTEXT_X19)]


Is there a reason for not using ldp here?

janvorli · 2017-02-13T13:45:42Z

src/pal/inc/unixasmmacrosarm64.inc

 C_FUNC(\Name\()_End):
+        .global C_FUNC(\Name\()_End)


Was this change necessary? We have the .global before the label on amd64, so if it was just a cosmetic change, I would prefer reverting it to keep the look consistent with amd64.

At the time I was trying to remove cosmetic difference between amd64 and arm64. I'll take a look.

I just confirmed that this currently matches amd64.

I am sorry, you're right. I don't know at what I was looking before.

janvorli · 2017-02-13T13:48:00Z

src/vm/arm64/asmhelpers.S

@@ -264,7 +264,7 @@ WRITE_BARRIER_END JIT_CheckedWriteBarrier
 //
 WRITE_BARRIER_ENTRY JIT_WriteBarrier
    dmb  ST
-    str  x15, [x14]
+    str      x15, [x14]


A nit - this change breaks the indentation, I guess it was just accidental.

janvorli · 2017-02-13T13:49:18Z

src/vm/arm64/asmhelpers.S

-    PROLOG_SAVE_REG_PAIR   x23, x24, #48
-    PROLOG_SAVE_REG_PAIR   x25, x26, #64
-    PROLOG_SAVE_REG_PAIR   x27, x28, #80
+    NESTED_ENTRY OnHijackTripThread, _TEXT, NoHandler


The NESTED_ENTRY should not be indented to keep it in line with the other functions in this file. And the same for the NESTED_END.

janvorli · 2017-02-13T13:54:18Z

src/vm/exceptionhandling.cpp

-    {
-        pAbortContext->Fp = pContextRecord->Fp;
-    }
+    UPDATEREG(Fp);


I think this will likely break Windows ARM64 that according to the comment don't have Fp in current context pointers pointing to a location of the Fp.
CC: @rahku

I can add a #ifdef FEATURE_PAL around it until it is fixed on Windows.

This appeared to be a required fix for Unix.

I guess I am actually mistaken. I have read the comment that was there before as "the unwinder doesn't set the context pointer for Fp", which would mean that it is not being set in the unwinder inside of windows that we use. But reading it again, it seems that it actually means that it just requires changes to the MachState that you have made, so it should probably be ok.
@rahku, @gkhanna79 - am I right?

I believe you are correct. @rahku should comment why MachState didnt support FP earlier, though, before we enable the change for both platforms.

Yes this looks good. It was a mistake to not add FP in MachState earlier which was fixed but this code was not fixed.

janvorli · 2017-02-13T14:13:21Z

src/vm/arm64/gmscpu.h

+    // On PAL, we don't always have the context pointers available due to
+    // a limitation of an unwinding library. In such case, preserve
+    // the unwound values.
+    CalleeSavedRegisters m_Unwound;


This is not needed for ARM64. The reason we have it for AMD64 is OSX which uses a different libunwind (the one that's part of the LLVM) that doesn't support unw_get_save_loc. See https://github.com/dotnet/coreclr/pull/527/files.

janvorli · 2017-02-13T14:33:34Z

src/vm/stackwalk.cpp

@@ -1778,7 +1778,7 @@ StackWalkAction StackFrameIterator::Filter(void)
                                pCurrTracker = pCurrTracker->GetPreviousExceptionTracker();
                            }

-                            if (pCurrTracker != NULL)
+                            if ((pCurrTracker != NULL) && !pCurrTracker->IsEHClauseFilter())


Do you have a repro for this issue? I would like to see what's going on here and see why we don't hit this problem on amd64.
It looks suspicious based on the comment at line 1743 that I've added when I was implementing the Unix exception handling at the beginning of the opensourcing of .net core.

I think I was debugging this test

JIT/Methodical/eh/basics/throwinfilter_d/throwinfilter_d.exe

The issue happened in the nested call to IL_Throw.

If this is not the correct fix, then it should be changed to an assert to detect this unexpected condition.

I will look into it first.

I have tried to run the test on amd64 with GC stress mode C and this "if" was never entered. Could you please verify that the issue really happened in the JIT/Methodical/eh/basics/throwinfilter_d/throwinfilter_d.exe test?

Sorry, I saw it with this config

env | grep COMPlus COMPlus_GCStress=2

Here is the gdb log from my AMD64 run

(gdb) b IL_Throw Function "IL_Throw" not defined. Make breakpoint pending on future shared library load? (y or [n]) y Breakpoint 1 (IL_Throw) pending. (gdb) r Starting program: /home/sdmaclea/tests-2017-01-11/Tests/coreoverlay/corerun /home/sdmaclea/test/JIT/Methodical/eh/basics/throwinfilt er_d/throwinfilter_d.exe [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". [New Thread 0x7ffff45d2700 (LWP 23557)] [New Thread 0x7ffff3dd1700 (LWP 23558)] [New Thread 0x7ffff35d0700 (LWP 23559)] [New Thread 0x7ffff2dcf700 (LWP 23560)] [New Thread 0x7ffff25ce700 (LWP 23561)] [New Thread 0x7ffff1da2700 (LWP 23562)] In try, throwing the first obj Thread 1 "corerun" hit Breakpoint 1, IL_Throw (obj=0x7fff5c01ada0) at /home/sdmaclea/git/coreclr/src/vm/jithelpers.cpp:5374 (gdb) b stackwalk.cpp:1768 Breakpoint 2 at 0x7ffff5e0aa4d: file /home/sdmaclea/git/coreclr/src/vm/stackwalk.cpp, line 1768. (gdb) c Continuing. Thread 1 "corerun" hit Breakpoint 2, StackFrameIterator::Filter (this=0x7fffffff9680) at /home/sdmaclea/git/coreclr/src/vm/stackwalk .cpp:1768 (gdb) display pCurrTracker 1: pCurrTracker = (ExceptionTracker *) 0x0 (gdb) cond 2 pCurrTracker (gdb) c Continuing. In filter Thread 1 "corerun" hit Breakpoint 1, IL_Throw (obj=0x7fff5c01ada0) at /home/sdmaclea/git/coreclr/src/vm/jithelpers.cpp:5374 (gdb) c Continuing. Thread 1 "corerun" hit Breakpoint 2, StackFrameIterator::Filter (this=0x7fffffff6ae0) at /home/sdmaclea/git/coreclr/src/vm/stackwalk .cpp:1768 1: pCurrTracker = (ExceptionTracker *) 0x67ae00 (gdb) print pCurrTracker->IsEHClauseFilter() $1 = true

@sdmaclea I can repro the GC hole on amd64 too with GCStress 2. Let me get my head around the exact way it happens. It seems it would make sense to remove this commit from this PR and create a separate PR for it so that we can merge in the remainder of the commits without unnecessary delay.

sdmaclea · 2017-02-14T18:22:18Z

I believe all clear comments are handled with my latest push

sdmaclea · 2017-02-15T18:39:44Z

@janvorli How do I remove this commit from the pull request? Do you want me to append a revert of this commit?

janvorli · 2017-02-15T18:43:34Z

@sdmaclea you can use interactive rebase to remove the commit and then force push your changes to origin. But adding a commit that reverts the change would be ok too.

Also fix indentation

sdmaclea · 2017-02-15T18:51:28Z

OK that should be the identical code without the GCStress=2 commit.

Do you want me to upload the other patch as a separate PR?

janvorli

LGTM, thank you!

sdmaclea

@janvorli

This is all whitespace

git diff -w HEAD~1..HEAD

janvorli · 2017-02-17T17:04:01Z

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

janvorli · 2017-02-17T17:04:54Z

@dotnet-bot test Linux ARM Emulator Cross Debug Build please

sdmaclea · 2017-02-17T18:52:36Z

@janvorli Thanks

janvorli · 2017-02-17T19:01:10Z

And thank you a lot for all the great work you did on that!

jashook · 2017-02-17T23:59:34Z

@dotnet/arm64-contrib It looks like this commit broke the arm64 Windows_NT build.

Is anyone looking into this?

janvorli · 2017-02-18T00:41:13Z

src/vm/arm64/asmconstants.h

@@ -89,7 +89,11 @@ ASMCONSTANTS_C_ASSERT(MachState__isValid == offsetof(MachState, _isValid))
 #define LazyMachState_captureX19_X29 MachState__captureX19_X29
 ASMCONSTANTS_C_ASSERT(LazyMachState_captureX19_X29 == offsetof(LazyMachState, captureX19_X29))

+#ifdef FEATURE_PAL
+#define LazyMachState_captureSp     (MachState__isValid+8+12*8) // padding for alignment & m_Unwound


@sdmaclea I have missed this before - since the m_Unwound was removed, this should be different, right?

@sdmaclea oops, sorry for the false alarm. I was looking at each commit separately and right after I have written the comment, I have realized that this was removed by another commit.

sdmaclea · 2017-02-18T00:19:15Z

src/unwinder/arm64/unwinder_arm64.cpp

    PKNONVOLATILE_CONTEXT_POINTERS ContextPointers;
 } ARM64_UNWIND_PARAMS, *PARM64_UNWIND_PARAMS;

 #define UNWIND_PARAMS_SET_TRAP_FRAME(Params, Address, Size)

-#define UPDATE_CONTEXT_POINTERS(Params, RegisterNumber, Address)
-#define UPDATE_FP_CONTEXT_POINTERS(Params, RegisterNumber, Address)
+#define UPDATE_CONTEXT_POINTERS(Params, RegisterNumber, Address)                    \


@jashook

@jashook @janvorli
There are only a few files touched which I expect to be shared by Windows. Based on the build failures, my guess is it is this change to src/unwinder/arm64/unwinder_arm64.cpp

These lines probably need to be marked with #ifdef FEATURE_PAL

I was surprised to see these failures, but looks like there was no premerge run against Windows arm64. It was run against arm and x86 only.

I'll create a PR if you want and you can see if it fixes WinArm64

sdmaclea · 2017-02-18T00:45:01Z

src/vm/arm64/asmconstants.h

@@ -89,11 +89,7 @@ ASMCONSTANTS_C_ASSERT(MachState__isValid == offsetof(MachState, _isValid))
 #define LazyMachState_captureX19_X29 MachState__captureX19_X29
 ASMCONSTANTS_C_ASSERT(LazyMachState_captureX19_X29 == offsetof(LazyMachState, captureX19_X29))

-#ifdef FEATURE_PAL
-#define LazyMachState_captureSp     (MachState__isValid+8+12*8) // padding for alignment & m_Unwound
-#else
 #define LazyMachState_captureSp     (MachState__isValid+8) // padding for alignment


@janvorli I thought it was removed here. Did I miss something?

janvorli · 2017-02-18T00:47:28Z

@sdmaclea, @jashook I am sorry, I wanted to run the Win ARM64 test run and then unfortunately forgotten about it.

jashook · 2017-02-18T00:48:56Z

src/unwinder/arm64/unwinder_arm64.cpp

+#define UPDATE_CONTEXT_POINTERS(Params, RegisterNumber, Address)                    \
+do {                                                                                \
+    if (ARGUMENT_PRESENT(Params)) {                                                 \
+        PKNONVOLATILE_CONTEXT_POINTERS ContextPointers = (Params)->ContextPointers; \


I assume the fix to the build break is to use the type PT_KNONVOLATILE_CONTEXT_POINTERS instead of PKNONVOLATILE_CONTEXT_POINTERS. @janvorli this type is correctly shared between both the windows SDK and the PAL correct?

The original code was a nop. #9663 restores that behavior.

For Unix, the calling code was change to provide the necessary support of copying the pointers. It seems bad to implement for Windows without any testing.

I see, well, for what it is worth, I have successfully built with PT_KNONVOLATILE_CONTEXT_POINTERS.

I don't think the PT_KNONVOLATILE_CONTEXT_POINTERS vs PKNONVOLATILE_CONTEXT_POINTERS is a problem here. The _ARM64_UNWIND_PARAMS in this file was using PKNONVOLATILE_CONTEXT_POINTERS before this change.
Where can I take a look at the build error?

https://ci.dot.net/job/dotnet_coreclr/job/master/job/arm64_cross_debug_windows_nt/lastFailedBuild/

janvorli · 2017-02-18T01:03:19Z

@jashook actually, you are right. And all the PKNONVOLATILE_CONTEXT_POINTERS should be changed to PT_KNONVOLATILE_CONTEXT_POINTERS, since it is currently wrong. This change just uncovered the problem, we were not accessing any members of the struct before and so it was not a problem that the struct actually is a AMD64 version (the build error happens during building cross tools that run on amd64, but target arm64).

sdmaclea · 2017-02-18T01:11:22Z

Since I have been natively compiling, I didn't see the issue. Sound like I should also test a cross compiled build to look for other errors...

I am happy to prepare the patch .... I'll even cross build on amd64...

janvorli · 2017-02-18T01:39:10Z

@sdmaclea that would be great. Although I am not sure if crossbuild for ARM64 works on AMD64 Linux yet. I used to have some problems with the rootfs creation in the past.

Fixes #9526

[Arm64/Windows] Backport #9500 changes

[Arm64/Windows] Backport dotnet#9500 changes

* [Arm64/Unix] Update arm64 *.S files to match *.asm * [Arm64/Unix] Fix CONTEXTToNativeContext() * [Arm64/Unix] ThrowExceptionFromContextInternal * [Arm64/Unix] Preserve x8 argument register * [ARM64/Unix] Add CFI directives Add native unwind info * [Arm64/Unix] Fix RtlRestoreContext * [Arm64/Unix] Restore FP from CurrentContextPointers * [Arm64/Unix] fix pointer math * [Arm64/Unix] Fix CallDescrWorkerInternal personality * [Arm64/Unix] More Fp fixups * [Arm64/Unix] CallEHFunclet machine state Restore non-volatile machine state in CallEHFunclet * [Arm64/Unix] CallDescrWorkerInternal Use empty stack slot to save argument * [Arm64/Unix] RtlVirtualUnwind update pointers * [Arm64] LazyMachState fixes * [Arm64/Unix] disable USE_REDIRECT_FOR_GCSTRESS When FEATURE_PAL is enableds USE_REDIRECT_FOR_GCSTRESS is not supported * [Arm64] ClearRegDisplayArgumentAndScratchRegisters() * [Arm64] Remove unnecesary copy in TransitionFrame * [Arm64/Unix] Fix comment per review * [Arm64/Unix] move constants per review * [Arm64/Unix] Use ldp per review Also fix indentation * [Arm64/Unix] Fix indentation per review * [Arm64/Unix] Remove m_Unwound per review comments * [Arm64/Unix] Use PREPARE_EXTERNAL_VAR to access globals * [Arm64/Unix] Fix more whitespace per earlier review comments

xiangzhai · 2020-03-06T04:00:59Z

:mips-interest

* [Arm64/Unix] Update arm64 *.S files to match *.asm * [Arm64/Unix] Fix CONTEXTToNativeContext() * [Arm64/Unix] ThrowExceptionFromContextInternal * [Arm64/Unix] Preserve x8 argument register * [ARM64/Unix] Add CFI directives Add native unwind info * [Arm64/Unix] Fix RtlRestoreContext * [Arm64/Unix] Restore FP from CurrentContextPointers * [Arm64/Unix] fix pointer math * [Arm64/Unix] Fix CallDescrWorkerInternal personality * [Arm64/Unix] More Fp fixups * [Arm64/Unix] CallEHFunclet machine state Restore non-volatile machine state in CallEHFunclet * [Arm64/Unix] CallDescrWorkerInternal Use empty stack slot to save argument * [Arm64/Unix] RtlVirtualUnwind update pointers * [Arm64] LazyMachState fixes * [Arm64/Unix] disable USE_REDIRECT_FOR_GCSTRESS When FEATURE_PAL is enableds USE_REDIRECT_FOR_GCSTRESS is not supported * [Arm64] ClearRegDisplayArgumentAndScratchRegisters() * [Arm64] Remove unnecesary copy in TransitionFrame * [Arm64/Unix] Fix comment per review * [Arm64/Unix] move constants per review * [Arm64/Unix] Use ldp per review Also fix indentation * [Arm64/Unix] Fix indentation per review * [Arm64/Unix] Remove m_Unwound per review comments * [Arm64/Unix] Use PREPARE_EXTERNAL_VAR to access globals * [Arm64/Unix] Fix more whitespace per earlier review comments Commit migrated from dotnet/coreclr@9baa44a

sdmaclea added 17 commits February 10, 2017 22:27

[Arm64/Unix] Update arm64 *.S files to match *.asm

0e778d7

[Arm64/Unix] Fix CONTEXTToNativeContext()

6caae84

[Arm64/Unix] ThrowExceptionFromContextInternal

2e7f2d1

[Arm64/Unix] Preserve x8 argument register

86cfff3

[ARM64/Unix] Add CFI directives

2fad44a

Add native unwind info

[Arm64/Unix] Fix RtlRestoreContext

011e693

[Arm64/Unix] Restore FP from CurrentContextPointers

efb5663

[Arm64/Unix] fix pointer math

745ae83

[Arm64/Unix] Fix CallDescrWorkerInternal personality

da3093a

[Arm64/Unix] More Fp fixups

d05be2c

[Arm64/Unix] CallEHFunclet machine state

6e4d219

Restore non-volatile machine state in CallEHFunclet

[Arm64/Unix] CallDescrWorkerInternal

775fd5a

Use empty stack slot to save argument

[Arm64/Unix] RtlVirtualUnwind update pointers

da460d6

[Arm64] LazyMachState fixes

dfca815

[Arm64/Unix] disable USE_REDIRECT_FOR_GCSTRESS

dccb0d3

When FEATURE_PAL is enableds USE_REDIRECT_FOR_GCSTRESS is not supported

[Arm64] ClearRegDisplayArgumentAndScratchRegisters()

1bf23c0

[Arm64] Remove unnecesary copy in TransitionFrame

72a3896

dnfclas added the cla-already-signed label Feb 10, 2017

janvorli reviewed Feb 13, 2017

View reviewed changes

sdmaclea force-pushed the PR-ARM64-UNIX branch from d5ef870 to 27d180a Compare February 15, 2017 18:46

sdmaclea added 5 commits February 15, 2017 18:48

[Arm64/Unix] Fix comment per review

3e9676c

[Arm64/Unix] move constants per review

b8be0a5

[Arm64/Unix] Use ldp per review

fb41fca

Also fix indentation

[Arm64/Unix] Fix indentation per review

1f5e9a7

[Arm64/Unix] Remove m_Unwound per review comments

ce64cbb

sdmaclea force-pushed the PR-ARM64-UNIX branch from 27d180a to ce64cbb Compare February 15, 2017 18:49

janvorli approved these changes Feb 17, 2017

View reviewed changes

sdmaclea commented Feb 17, 2017

View reviewed changes

janvorli merged commit 9baa44a into dotnet:master Feb 17, 2017

sdmaclea deleted the PR-ARM64-UNIX branch February 17, 2017 18:53

janvorli reviewed Feb 18, 2017

View reviewed changes

sdmaclea commented Feb 18, 2017

View reviewed changes

sdmaclea added a commit to sdmaclea/coreclr that referenced this pull request Feb 18, 2017

[Arm64/Win] Fix for break caused by dotnet#9500

0aca9fc

jashook reviewed Feb 18, 2017

View reviewed changes

sdmaclea mentioned this pull request Feb 18, 2017

[WIP][Arm64/Win] Fix for break caused by #9500 #9663

Closed

This was referenced Feb 18, 2017

[ARM64] Use PT_KNONVOLATILE... for cross #9665

Merged

Add new regression. #9672

Merged

sdmaclea added a commit to sdmaclea/coreclr that referenced this pull request Mar 15, 2017

[Arm64/Windows] Backport dotnet#9500 changes

a230fe2

Fixes #9526

rahku pushed a commit that referenced this pull request Apr 4, 2017

Merge pull request #10190 from sdmaclea/PR-ARM64-BACKPORT-9500

7f296fe

[Arm64/Windows] Backport #9500 changes

hadibrais pushed a commit to hadibrais/coreclr that referenced this pull request Apr 7, 2017

Merge pull request dotnet#10190 from sdmaclea/PR-ARM64-BACKPORT-9500

2b1e7e0

[Arm64/Windows] Backport dotnet#9500 changes

karelz modified the milestone: 2.0.0 Aug 28, 2017

sdmaclea mentioned this pull request Jan 31, 2020

[Arm64/Windows] Backport #9500 changes as needed dotnet/runtime#7408

Closed

xiangzhai mentioned this pull request Mar 7, 2020

[MIPS64] apply_reg_state: ip and cfa unchanged; stopping here dotnet/runtime#33335

Closed

		@@ -46,27 +46,27 @@ LOCAL_LABEL(donestack):

		// If FP arguments are supplied in registers (x8 != NULL) then initialize all of them from the pointer

[ARM64/Unix] #9500

[ARM64/Unix] #9500

Uh oh!

Conversation

sdmaclea commented Feb 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea commented Feb 14, 2017

Uh oh!

sdmaclea commented Feb 15, 2017

Uh oh!

janvorli commented Feb 15, 2017

Uh oh!

sdmaclea commented Feb 15, 2017

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

sdmaclea left a comment

Choose a reason for hiding this comment

Uh oh!

janvorli commented Feb 17, 2017

Uh oh!

janvorli commented Feb 17, 2017

Uh oh!

sdmaclea commented Feb 17, 2017

Uh oh!