[x86/Linux] Port RtlVirtualUnwind by parjong · Pull Request #8911 · dotnet/coreclr

parjong · 2017-01-12T03:45:07Z

This commit (partially) ports RtlVirtualUnwind as a step to resolve #8887.

parjong · 2017-01-12T05:57:06Z

We should merge this PR after merging #8889 as it depends on #8889. We could not build x86/Linux port if we merge this PR without merging #8889.

janvorli · 2017-01-12T10:07:43Z

 #endif

-#ifdef _WIN64
+#ifdef WIN64EXCEPTIONS


This doesn't seem right. The WIN64EXCEPTIONS are defined for TARGET_ARM too, so this change would create duplicite typedefs. So I wonder how come the CI build succeeded. Maybe the WIN64EXCEPTIONS is not defined in this file.

janvorli · 2017-01-16T11:43:49Z


    pRD->ctxPtrsOne.Lr = &pctx->Lr;
 #elif defined(_TARGET_X86_) // _TARGET_ARM_
+    pRD->pEdi = &(pctx->Edi);


This won't work for the stack walking with WIN64EXCEPTIONS. When WIN64EXCEPTIONS are defined, the stack walking uses pCurrentContextPointers and doesn't know anything about the pEsi, pEbx etc. So while the unwinder would store the context pointers in these, the pCurrentContext would always just point to the context structure instead of the real location where the register value came from.
What I believe you need to do instead is to remove the setting of the pEsi etc here and then in the OOPStackUnwinderX86::VirtualUnwind set the pRD->pEsi etc to the corresponding values from the pCurrentContext before calling the CodeManager::UnwindStackFrame. And right after calling them, copy them from the RegDisplay::pEsi etc to the corresponding members of the pCurrentContext.
Also, you need to do that only for EDI, ESI, EBX and EBP. The CodeManager::UnwindStackFrame doesn't update any others. These are the only callee saved registers.

Also, the CopyRegDisplay needs to be modified so that it uses the code path that's now for non-x86 targets for x86 on Linux too.

parjong · 2017-01-17T07:50:31Z

@dotnet-bot test Windows_NT x64 Debug Build and Test please

parjong · 2017-01-17T08:48:54Z

@dotnet-bot test Windows_NT x86 Checked Build and Test please

janvorli · 2017-01-17T09:55:49Z

+#undef CALLEE_SAVED_REGISTER
+    }
+
+    if ( rd.pEbp == NULL )


A nit - can you please remove extra space after ( and before ) here?

janvorli · 2017-01-17T09:55:54Z

+    EECodeInfo      codeInfo;
+    codeInfo.Init((PCODE) ControlPc);
+
+    if ( !UnwindStackFrame(&rd, &codeInfo, UpdateAllRegs, &codeManState, NULL) )


A nit - can you please remove extra space after ( and before ) here?

janvorli · 2017-01-17T09:56:19Z

+
+    if ( !UnwindStackFrame(&rd, &codeInfo, UpdateAllRegs, &codeManState, NULL) )
+    {
+      return HRESULT_FROM_WIN32(ERROR_READ_FAULT);


A nit - wrong indentation

janvorli · 2017-01-17T10:28:18Z

+    )
+{
+    *EstablisherFrame   = ContextRecord->Esp;
+    *HandlerRoutine     = NULL;


The HandlerRoutine is an optional pointer, based on the annotation (__deref_opt_out_opt). Can you please wrap the assignment in check it is not NULL?

janvorli · 2017-01-17T10:33:38Z

-                                     unsigned        flags,
-                                     CodeManState   *pState,
-                                     StackwalkCacheUnwindInfo  *pUnwindInfo /* out-only, perf improvement */)
+bool UnwindStackFrame(PREGDISPLAY     pContext,


I am not sure I understand why you have moved this function out of the EECodeManager and then just call it from there.

Stack walker directly calls EECodeManager::UnwindStackFrame, but this implementation does not update WIN64EXCEPTIONS-related fields (which results in unwind failure).

janvorli · 2017-01-17T10:34:31Z

+                                     CodeManState   *pState,
+                                     StackwalkCacheUnwindInfo  *pUnwindInfo /* out-only, perf improvement */)
+{
+    return UnwindStackFrame(pContext, pCodeInfo, flags, pState, pUnwindInfo);


I believe this creates an infinite loop, since it would call itself instead of the function.

You're right... I'll fix it.

janvorli · 2017-01-17T10:50:10Z

+        __deref_opt_out_opt PEXCEPTION_ROUTINE *HandlerRoutine);
+
+private:
+    static HRESULT UnwindPrologue(


It looks like the UnwindPrologue and GetEstabliserFrame can be removed since they are not even implemented.

janvorli

These are the last requests to change. We can merge it after that.

janvorli · 2017-01-17T12:18:44Z

+                                     CodeManState   *pState,
+                                     StackwalkCacheUnwindInfo  *pUnwindInfo /* out-only, perf improvement */)
+{
+    return UnwindStackFrameEx(pContext, pCodeInfo, flags, pState, pUnwindInfo);


The "Ex" seems little strange since the standalone function is not an extended version. You could have kept the same name and just classify it by "::" to indicate that the global version should be called here:
return ::UnwindStackFrame(pContext, pCodeInfo, flags, pState, pUnwindInfo);
Would you mind changing it that way?

janvorli · 2017-01-17T12:22:59Z

+    *EstablisherFrame   = ContextRecord->Esp;
+    if (HandlerRoutine != NULL)
+    {
+        *HandlerRoutine     = NULL;


One last nit - can you please normalize the spacing in this function? What I mean is to ensure that there is just one space at each side of the = and also one space between a type and variable in variable declarations.

parjong · 2017-01-17T23:50:44Z

@janvorli Is there any issues with GC? I recently observed some randome failures on some GC testcases.

parjong · 2017-01-17T23:50:56Z

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

janvorli · 2017-01-17T23:57:03Z

@parjong what were the failing tests?

parjong · 2017-01-18T00:06:27Z

@janvorli Oops, I restarted the test without recording failed tests. I'll let you know when I see it again.

parjong · 2017-01-18T03:03:24Z

@janvorli Ubuntu x64 Checked Build and Test failed with the following failure:

FAILED   - GC/Scenarios/DoublinkList/doublinkgen/doublinkgen.sh
               BEGIN EXECUTION
               /mnt/j/workspace/dotnet_coreclr/master/checked_ubuntu_tst_prtest/bin/tests/Windows_NT.x64.Checked/Tests/coreoverlay/corerun doublinkgen.exe
               Test should return with ExitCode 100 ...
               ./doublinkgen.sh: line 138:  7818 Aborted                 (core dumped) $_DebuggerFullPath "$CORE_ROOT/corerun" doublinkgen.exe $CLRTestExecutionArguments
               Expected: 100
               Actual: 134
               END EXECUTION - FAILED

It seems that the current PR should not affect x64/Linux port, but x64/Linux port shows test failure. Could you let me know if you have any clue?

janvorli · 2017-01-18T09:41:45Z

@parjong this test is known to be failing occassionaly. See #6574 .

janvorli

LGTM, thank you!

* [x86/Linux] (Partially) port RtlVirtualUnwind * Rewrite x86 Unwinder using UnwindStackFrame * Extract UnwindStackFrame from EECodeManager * Port 'InlinedCallFrame::UpdateRegDisplay' Commit migrated from dotnet/coreclr@0096973

dnfclas added the cla-already-signed label Jan 12, 2017

parjong changed the title ~~[x86/Linux] port RtlVirtualUnwind~~ [x86/Linux] port RtlVirtualUnwind - WIP Jan 12, 2017

parjong changed the title ~~[x86/Linux] port RtlVirtualUnwind - WIP~~ [x86/Linux] port RtlVirtualUnwind (WIP) Jan 12, 2017

parjong changed the title ~~[x86/Linux] port RtlVirtualUnwind (WIP)~~ [x86/Linux] Port RtlVirtualUnwind (WIP) Jan 12, 2017

janvorli suggested changes Jan 16, 2017

View reviewed changes

parjong added 12 commits January 17, 2017 18:33

[x86/Linux] (Partially) port RtlVirtualUnwind

1dfd219

Fix Windows build error

b4eb9cb

Fix ARM/Windows build error

67b00e5

Rewrite x86 Unwinder using UnwindStackFrame

92419eb

Revert src/inc/daccess.h

08ec2b6

Extract UnwindStackFrame from EECodeManager

6f6a007

Revise regdisp.h

5943f14

Port 'InlinedCallFrame::UpdateRegDisplay'

fe7b46c

Revise x86 unwinder

d06a40b

Fix Incorrect RtlVirtualUnwind Implementation

e5dd50b

Check ContextPointers once

1496247

Do NOT mark untouched Ebp as updated

c1eb1ca

parjong changed the title ~~[x86/Linux] Port RtlVirtualUnwind (WIP)~~ [x86/Linux] Port RtlVirtualUnwind Jan 17, 2017

janvorli reviewed Jan 17, 2017

View reviewed changes

parjong added 2 commits January 17, 2017 19:07

Revert wrong fix and fix correctly

94fd902

Fix Format Error

7c05bd1

janvorli reviewed Jan 17, 2017

View reviewed changes

Wrap HandlerRoutine update with NULL check

7aa77e0

janvorli reviewed Jan 17, 2017

View reviewed changes

parjong added 2 commits January 17, 2017 19:58

Fix infinite self-loop bug

083963d

Remove unused declarations

6b041ae

janvorli reviewed Jan 17, 2017

View reviewed changes

parjong added 2 commits January 18, 2017 07:20

Rename UnwindStackFrameEx as UnwindStackFrame

9268ca4

Normalize spacing in OOPStackUnwinderX86::VirtualUnwind

ebbe8da

parjong mentioned this pull request Jan 17, 2017

[x86/Linux] Enable FEATURE_EH_FUNCLETS #8889

Merged

janvorli approved these changes Jan 18, 2017

View reviewed changes

janvorli merged commit 0096973 into dotnet:master Jan 18, 2017

parjong deleted the fix/port_RtlVirtualUnwind branch January 18, 2017 22:14

karelz modified the milestone: 2.0.0 Aug 28, 2017

parjong mentioned this pull request Jan 31, 2020

[x86/Linux] Catch exceptions thrown from unsafe code dotnet/runtime#7244

Closed

Conversation

parjong commented Jan 12, 2017

Uh oh!

parjong commented Jan 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parjong commented Jan 17, 2017

Uh oh!

parjong commented Jan 17, 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!

parjong Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

janvorli left a comment

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!

parjong commented Jan 17, 2017

Uh oh!

parjong commented Jan 17, 2017

Uh oh!

janvorli commented Jan 17, 2017

Uh oh!

parjong commented Jan 18, 2017

Uh oh!

parjong commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janvorli commented Jan 18, 2017

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

parjong commented Jan 12, 2017 •

edited

Loading

parjong Jan 17, 2017 •

edited

Loading

parjong commented Jan 18, 2017 •

edited

Loading