[x86/Linux] Do NOT use pXXX fields #9121

parjong · 2017-01-26T00:14:33Z

This commit removes pXXX fields in REGDISPLAY, and revises all the references on these fields (as discussed in #8998) to refers corresponding fields in pCurrentContext and pCurrentContetPointers.

parjong · 2017-01-26T01:09:05Z

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

parjong · 2017-01-26T01:18:38Z

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

parjong · 2017-01-26T03:37:02Z

@jkotas @janvorli Please take a look.

parjong · 2017-01-26T03:37:35Z

\CC @seanshpark @wateret

janvorli · 2017-01-26T15:46:03Z

@parjong I think this refactoring went too far. It seems that instead of defining all the four methods per each register, it would be enough to define just GetRegisterLocation(rd, reg) macro or inline functions GetXXXLocation() where XXX is Eax, Ebx, Ecx, Edx, ... Then you would replace all usages of pXXX by GetXXXLocation(). It seems easier to read that way with using C++ dereferencing to read and write the values instead of introducing new functions.
Also the RestoreXXX has different semantics for WIN64EXCEPTIONS than for !WIN64EXCEPTIONS, which is really confusing.
The TrashXX functions are completely unnecessary, since they are just setters.
Could you please simplify it this way?

Edit: I can see now that the Trash is actually different from the setter for non-volatile registers.
Also, the Restore name itself is confusing, since it just assigns the context pointer for !WIN64EXCEPTIONS, which doesn't feel like restoring anything.

janvorli · 2017-01-26T23:31:50Z

Also, I've missed before that you haven't removed the pXXX members for WIN64EXCEPTIONS, which I have expected to happen.

parjong · 2017-01-30T22:31:49Z

@janvorli Sorry for late response. I was out of office in last 4 days.

Do you mean that the value of *pCurrentContextPointers->(Reg) may differ from that of pCurrentContext->(Reg)? From your first comment, GetXXXLocation may work when we read the value, but I'm not sure how we could update the value only with this method. Could you let me know some details?

For your second comment, this PR removes pXXX members for WIN64EXCEPTIONS. These members are inside #ifndef WIN64EXCEPTIONS ~ #else region, and thus do not exist for WIN64EXCEPTIONS. (I'll revise this PR to make it easy to read).

janvorli · 2017-01-30T22:49:14Z

@parjong than you for your response. As for my last comment, I am sorry, I can see now that you are right and the pXXX members are no longer there for WIN64EXCEPTIONS. I somehow confused myself.

As for the GetXXXLocation, you are right, we would also need a SetXXXLocation function. But these two should be sufficient and it would be very easy to understand their intent.

parjong · 2017-01-30T23:15:45Z

@janvorli Thank you for comment. Could you let me know some details about pCurrentContextPointers? (I'm trying to revise TrashXXX methods)

UnwindEbpDoubleAlignFrame updates all the registers with some garbage data. To revise that code, I introduced TrashXXX methods because I though that these updates should not be reflected in pCurrentContextPointers.

Is it enough to use SetXXX for this code?

janvorli · 2017-01-30T23:20:42Z

@parjong I think that basically everything that was done through pXXX before should now be done through pCurrentContextPointers->XXX. I see this change as a pure replacement of pXXX with the other ones.

parjong · 2017-01-31T03:41:09Z

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

parjong · 2017-01-31T07:52:02Z

@janvorli PR is revised per feedback. Please take a look.

janvorli · 2017-01-31T09:55:44Z

src/inc/regdisp.h

+
+#define VOLATILE_REG_METHODS(reg) \
+ inline PDWORD Get##reg##Location(void) { return &pCurrentContext->reg; } \
+ inline void Set##reg##Location(PDWORD p##reg) { pCurrentContext->reg = *p##reg; }


We don't need to distinguish between the volatile and non-volatile cases. We can add Eax, Ecx and Edx to the KNONVOLATILE_CONTEXT_POINTERS instead. If you look at the structure for AMD64, you can see that the same thing is done there.

The SetXXXLocation should really just set the context pointer without touching the register value in the context and that we should update the context as an additional step after the unwinding (in OOPStackUnwinderX86::VirtualUnwind and EECodeManager::QuickUnwindStackFrame). Letting the function have a side effect that's not obvious based on its name is confusing when reading the code that uses this function.

It also seems that it would be better to keep the XXXFrame::UpdateRegDisplay code for x86 Linux as close as possible to the x64 version. And these functions also update context and context pointers separately on x64. And thinking about it more, it seems it would make sense to use the new GetXXXLocation / SetXXXLocation only in code that's common for WIN64EXCEPTIONS and !WIN64EXCEPTIONS, but let places where the code is split into separate WIN64EXCEPTIONS and !WIN64EXCEPTIONS versions access the specific pXXX or pCurrentContextPointers->XXX explicitly for the reason of consistency with the x64 version.

@janvorli Could you let me know the role of pCurrentContext? It seems that pCurrentContextPointers is enough for the purpose of unwind.

The pCurrentContext->XXX contain values that the pCurrentContextPointers->XXX point to. Please note that there are actually two context and two context pointer structures. To one of them points the pCurrentContext / pCurrentContextPointers. To the other points the pParentContext / pParentContextPointers. That allows unwinding at some places to eliminate some context / context pointers copying by just switching the the pCurrentXXX and pParentXXX pointers.

While it might theoretically be possible to modify the existing code to not to use pCurrentContext for x86, it would be error prone and make the x86 Linux code even more different from the other targets using WIN64EXCEPTIONS.

janvorli · 2017-01-31T09:59:58Z

src/inc/regdisp.h

+
+#define NONVOLATILE_REG_METHODS(reg) \
+ inline PDWORD Get##reg##Location(void) { return (pCurrentContextPointers) ? pCurrentContextPointers->reg : &pCurrentContext->reg; } \
+ inline void Set##reg##Location(PDWORD p##reg) { if (pCurrentContextPointers) { pCurrentContextPointers->reg = p##reg; } pCurrentContext->reg = *p##reg; }


Also, let's not check the pCurrentContextPointers here and ensure the pCurrentContextPointers is always valid when calling these functions instead. I think that the only place where this could be called with the context pointers set to NULL is from OOPStackUnwinderX86::VirtualUnwind with the way you've modified the function. If you didn't reassign the pCurrentContextPointers and pCurrentContext and just copied the values as you were doing before, you'd not have this issue. Or is there another place that I am missing?

janvorli · 2017-01-31T13:04:15Z

src/vm/eetwain.cpp

@@ -3207,12 +3211,12 @@ const RegMask CALLEE_SAVED_REGISTERS_MASK[] =
 RM_EBP // last register to be pushed
 };

-const SIZE_T REGDISPLAY_OFFSET_OF_CALLEE_SAVED_REGISTERS[] =
+const REGDISPLAY::TAG CALLEE_SAVED_REGISTERS_TAG[] =


I would prefer to keep using offsets instead of a tag and switch in the SetLocation. It should work if we let the offsets array for WIN64EXCEPTIONS to keep offsets in the context pointers structure and the code accessing it would use pContext->pCurrentContextPointers as the base.
Since there are five places where we use the offset array the same way, it would then make sense to factor this out into a separate function, e.g. SetRegisterLocation(i, addr) so that the WIN64EXCEPTIONS changes would be localized.

…into SetXXXLocation

janvorli

Thank you, we are almost there! Just a few little things remaining.

janvorli · 2017-02-01T10:10:52Z

src/inc/regdisp.h

@@ -430,6 +423,10 @@ inline void FillRegDisplay(const PREGDISPLAY pRD, PT_CONTEXT pctx, PT_CONTEXT pC
 pRD->ctxPtrsOne.Esi = &pctx->Esi;
 pRD->ctxPtrsOne.Edi = &pctx->Edi;
 pRD->ctxPtrsOne.Ebp = &pctx->Ebp;
+


A nit - for consistency with other target, you could now revert back to using a loop for copying the registers if you reordered them in the KNONVOLATILE_CONTEXT_POINTERS to match their order in the CONTEXT.

janvorli · 2017-02-01T10:12:13Z

src/inc/regdisp.h

- inline void Set##reg##Location(PDWORD p##reg) { pCurrentContext->reg = *p##reg; }
-
-#define NONVOLATILE_REG_METHODS(reg) \
+#define REG_METHODS(reg) \
 inline PDWORD Get##reg##Location(void) { return (pCurrentContextPointers) ? pCurrentContextPointers->reg : &pCurrentContext->reg; } \


The GetXXXLocation should also assume the pCurrentContextPointers is not NULL.

janvorli · 2017-02-01T10:14:05Z

src/unwinder/i386/unwinder_i386.cpp

@@ -97,6 +97,10 @@ OOPStackUnwinderX86::VirtualUnwind(

 ContextRecord->ContextFlags |= CONTEXT_UNWOUND_TO_CALL;

+#define CALLER_SAVED_REGISTER(reg) if (rd.pCurrentContextPointers->reg) ContextRecord->reg = *rd.pCurrentContextPointers->reg;


This is not needed. The UnwindStackFrame never sets context pointers of the caller saved registers.

UnwindEbpDoubleAlignFrame sets Eax, Ecx, Edx for debugging purpose:

3832 #ifdef _DEBUG 3833 /* The filter has to be called by the VM. So we dont need to 3834 update callee-saved registers. 3835 */ 3836 3837 if (flags & UpdateAllRegs) 3838 { 3839 static DWORD s_badData = 0xDEADBEEF; 3840 3841 pContext->SetEaxLocation(&s_badData); 3842 pContext->SetEcxLocation(&s_badData); 3843 pContext->SetEdxLocation(&s_badData); 3844 3845 pContext->SetEbxLocation(&s_badData); 3846 pContext->SetEsiLocation(&s_badData); 3847 pContext->SetEdiLocation(&s_badData); 3848 } 3849 #endif

I'm not sure which one is better: enclosing this region with _DEBUG, or simply removing it. Please let me know your opinion.

I am sorry, I have missed this. So please leave your change as it is.

janvorli · 2017-02-01T10:26:00Z

src/vm/i386/cgenx86.cpp


-#else // WIN64EXCEPTIONS
+#define CALLEE_SAVED_REGISTER(regname) pRD->pCurrentContextPointers->regname = &m_ctx.regname;


Looking at the amd64 version, we should also set the context pointers for caller saved registers here.

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

src/vm/i386/cgenx86.cpp

+ ENUM_CALLEE_SAVED_REGISTERS();
+#undef CALLEE_SAVED_REGISTER
+
+#define CALLER_SAVED_REGISTER(reg) pRD->pCurrentContextPointers->reg = &m_Args->reg;


Looking at the amd64 version, context pointers to argument registers (ecx, edx) should be NULLed and only the pointer to the return register (eax) should be set.

janvorli · 2017-02-01T10:37:14Z

src/vm/i386/cgenx86.cpp

+ pRD->pCurrentContext->Eip = *PTR_PCODE(pRD->PCTAddr);
+ pRD->pCurrentContext->Esp = (DWORD)(pRD->PCTAddr + sizeof(TADDR));
+
+#define CALLEE_SAVED_REGISTER(reg) pRD->pCurrentContext->reg = m_Args->reg;


It seems we can use the UpdateRegDisplayFromCalleeSavedRegisters that you've added now.

That was my first trial, but UpdateRegDisplayFromCalleeSavedRegisters cannot be used here as m_Args is of type HijackArgs (not CalleeSavedRegisters).

Ah, I see, the HijackArgs differ from the amd64 version.

janvorli · 2017-02-01T10:43:32Z

src/vm/i386/cgencpu.h

@@ -154,6 +154,11 @@ typedef INT32 StackElemType;
 // This represents some of the FramedMethodFrame fields that are
 // stored at negative offsets.
 //--------------------------------------------------------------------
+#define ENUM_CALLER_SAVED_REGISTERS() \


A nit. These are argument and scratch registers, so it seems it might be better to call it ENUM_ARGUMENT_AND_SCRATCH_REGISTERS. We have ENUM_ARGUMENT_REGISTERS for amd64, so that would make it similar.

janvorli

LGTM, thank you a lot!

janvorli · 2017-02-01T23:59:12Z

@parjong I'll be OOF on Thursday and Friday, so if you create PRs during that time, please ask @jkotas to look at them instead.

parjong · 2017-02-02T00:02:08Z

@janvorli Oh, I see. Thank you for your consideration.

parjong · 2017-02-02T01:31:56Z

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

* [x86/Linux] Do NOT use pXXX fields * Fix x86/Windows build error * Fix another x86/Windows build error * Fix typo * Do NOT trash EBP * Reflect the original semantics of EHContext::UpdateFrame * Unify ReadXXX/LocateXXX into GetXXXLocation, and RestoreXXX/TrashXXX into SetXXXLocation * Revert the order of pXXX fields * Revise cgenx86.cpp * Revert unnecessary changes * Remove direct accesses to CALLEE_SAVED_REGISTERS_TAG * Do NOT update pCurrentContext inside SetXXXLocation * Update RegPtr via offset * Unify REG_METHODS (and revise UpdateRegDisplay methods accordingly) * Revise per feedback * Fix x86/Windows build error Commit migrated from dotnet/coreclr@009c70f

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

janvorli suggested changes Jan 31, 2017

View reviewed changes

parjong mentioned this pull request Feb 1, 2017

[x86/Linux] Get Frame Pointer from CallerSp #9235

Merged

parjong added 8 commits February 1, 2017 11:09

[x86/Linux] Do NOT use pXXX fields

1f880a0

Fix x86/Windows build error

308426e

Fix another x86/Windows build error

3f3571e

Fix typo

838e78f

Do NOT trash EBP

be0e57c

Reflect the original semantics of EHContext::UpdateFrame

f286a67

Unify ReadXXX/LocateXXX into GetXXXLocation, and RestoreXXX/TrashXXX …

ac268da

…into SetXXXLocation

Revert the order of pXXX fields

88cd11d

parjong force-pushed the fix/x86_revised_unwinder branch from 1310d8c to 8206dc4 Compare February 1, 2017 04:00

parjong added 6 commits February 1, 2017 13:02

Revise cgenx86.cpp

8206dc4

Revert unnecessary changes

71552d4

Remove direct accesses to CALLEE_SAVED_REGISTERS_TAG

bd0362f

Do NOT update pCurrentContext inside SetXXXLocation

f3cfaec

Update RegPtr via offset

876b33e

Unify REG_METHODS (and revise UpdateRegDisplay methods accordingly)

eeedf6c

janvorli suggested changes Feb 1, 2017

View reviewed changes

parjong added 2 commits February 2, 2017 08:24

Revise per feedback

346f111

Fix x86/Windows build error

cbb1de5

janvorli approved these changes Feb 1, 2017

View reviewed changes

janvorli merged commit 009c70f into dotnet:master Feb 2, 2017

parjong deleted the fix/x86_revised_unwinder branch February 2, 2017 09:42

parjong mentioned this pull request Feb 20, 2017

[x86/Linux] Fix framepointer while unwinding #9678

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[x86/Linux] Do NOT use pXXX fields #9121

[x86/Linux] Do NOT use pXXX fields #9121

parjong commented Jan 26, 2017 •

edited

Loading

parjong commented Jan 26, 2017

parjong commented Jan 26, 2017

parjong commented Jan 26, 2017

parjong commented Jan 26, 2017

janvorli commented Jan 26, 2017 •

edited

Loading

janvorli commented Jan 26, 2017

parjong commented Jan 30, 2017 •

edited

Loading

janvorli commented Jan 30, 2017

parjong commented Jan 30, 2017 •

edited

Loading

janvorli commented Jan 30, 2017

parjong commented Jan 31, 2017

parjong commented Jan 31, 2017

janvorli Jan 31, 2017

janvorli Jan 31, 2017

parjong Jan 31, 2017

janvorli Feb 1, 2017

janvorli Jan 31, 2017

janvorli Jan 31, 2017

janvorli left a comment

janvorli Feb 1, 2017

janvorli Feb 1, 2017

janvorli Feb 1, 2017

parjong Feb 1, 2017 •

edited

Loading

janvorli Feb 1, 2017

janvorli Feb 1, 2017

janvorli Feb 1, 2017

janvorli Feb 1, 2017

parjong Feb 1, 2017 •

edited

Loading

janvorli Feb 1, 2017

janvorli Feb 1, 2017

janvorli left a comment

janvorli commented Feb 1, 2017

parjong commented Feb 2, 2017 •

edited

Loading

parjong commented Feb 2, 2017

		@@ -97,6 +97,10 @@ OOPStackUnwinderX86::VirtualUnwind(

		ContextRecord->ContextFlags \|= CONTEXT_UNWOUND_TO_CALL;

		#define CALLER_SAVED_REGISTER(reg) if (rd.pCurrentContextPointers->reg) ContextRecord->reg = *rd.pCurrentContextPointers->reg;


		#else // WIN64EXCEPTIONS
		#define CALLEE_SAVED_REGISTER(regname) pRD->pCurrentContextPointers->regname = &m_ctx.regname;

[x86/Linux] Do NOT use pXXX fields #9121

[x86/Linux] Do NOT use pXXX fields #9121

Conversation

parjong commented Jan 26, 2017 • edited Loading

parjong commented Jan 26, 2017

parjong commented Jan 26, 2017

parjong commented Jan 26, 2017

parjong commented Jan 26, 2017

janvorli commented Jan 26, 2017 • edited Loading

janvorli commented Jan 26, 2017

parjong commented Jan 30, 2017 • edited Loading

janvorli commented Jan 30, 2017

parjong commented Jan 30, 2017 • edited Loading

janvorli commented Jan 30, 2017

parjong commented Jan 31, 2017

parjong commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvorli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parjong Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parjong Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvorli left a comment

Choose a reason for hiding this comment

janvorli commented Feb 1, 2017

parjong commented Feb 2, 2017 • edited Loading

parjong commented Feb 2, 2017

parjong commented Jan 26, 2017 •

edited

Loading

janvorli commented Jan 26, 2017 •

edited

Loading

parjong commented Jan 30, 2017 •

edited

Loading

parjong commented Jan 30, 2017 •

edited

Loading

parjong Feb 1, 2017 •

edited

Loading

parjong Feb 1, 2017 •

edited

Loading

parjong commented Feb 2, 2017 •

edited

Loading