[x86/Linux] Fix ResumeEsp before resume #10749

parjong · 2017-04-05T23:48:24Z

This commit fixes RseumeEsp just before resuming, which resolves 4 test failures:

JIT/IL_Conformance/Old/Conformance_Base/conv_ovf_r8_i
JIT/IL_Conformance/Old/Conformance_Base/conv_ovf_r8_i4
JIT/Methodical/Arrays/misc/_il_relinitializearray
JIT/Regression/VS-ia64-JIT/V1.2-M01/b12390

parjong · 2017-04-06T03:32:31Z

parjong · 2017-04-06T08:33:09Z

@jkotas @janvorli Is there any issue related with OSX CI?

janvorli · 2017-04-06T08:36:09Z

@parjong yes, there is some issue with network connection speed to the OSX machines. I believe people are looking into it.

parjong · 2017-04-06T08:39:23Z

Thanks you for reply 👍

janvorli · 2017-04-06T08:49:48Z

src/vm/eetwain.cpp

+    }
+
+    const size_t curEBP = *pRD->GetEbpLocation();
+    return GetOutermostBaseFP(curEBP, info);


Does this work correctly for the cases where we have added stack alignment before pushing the arguments?

As I understand jitter records stack changes (including stack alignment padding) for ESP frame, and thus this approach works for ESP frames. For EBP frame, jitter reports frame size, and thus this approach seems to work as we could derive resume SP from ebp and frame size.

I am currently not sure about funclets in ebp framed function.

Can you verify that it works for the funclets too? I have looked for a good candidate of a fcall that throws and can be easily forced to throw and has at least one stack argument and found ArrayNative::ArrayClear. It throws if the passed iIndex is below the lower bound of the array or if the iIndex + iLength is over the end of the array.
It looks like this FCall is invoked when you call Array.Clear static method.
So if you write a little test app and placed the following piece of code into a funclet, you should be able to put a breakpoint inside the if (codeInfo.IsFunclet()) in the EECodeManager::GetResumeSp and then step through your code to see if it works as expected.

byte[] b = new byte[4]; Array.Clear(b, 0, 8);

Sorry for unclear comment.

I already found a case that GetPushedArgSize becomes incorrect while working on #10685. I tried to set baseSP as curESP + GetPushedArgSize(...) instead of pContext->pCurrentContext->ResumeEsp, but JIT.Methodical.flowgraph.dev10_bug679008.fgloop fails with that approach.

It seems that we need to emit full arg info for all managed methods in order to fix this case. I'm not sure about the amount of work for this task.

I have a quick idea. Maybe we can solve it in an easy way, based on the fact that we know that a funclet is called from native code and we know what the prolog of a funclet looks like. Since funclet cannot allocate any locals, we know that the ESP we are looking for is the ESP after the prolog of the funclet, which is ESP at which the asm helper called the funclet minus the return address size and that aligned to 16 bytes.

I'm not sure whether we could get ESP at which the asm helper called correctly.

I submitted #10782 to address this issue. Could you please take a look?

I have quickly looked at the #10782, but my first impression is that it should be our last resort if there is no other way. I feel uneasy about adding the full argument info even for EBP frames. But I'll think about it more.
But as for my idea how to solve the funclet issue differently, the CallEHFunclet and CallEHFilterFunclet records the funclet caller SP in ExceptionTracker::m_EHClauseInfo.GetCallerStackFrameForEHClauseReference(). So in ExceptionTracker::ResumeExecution, we could read that value from the right exception tracker and pass it to the EECodeManager::GetResumeSp. If there are no issues that I don't see at this late time at night, I think that it would work.

I guess that the suggested approach works for resuming, but I'm not sure whether this approach could address incorrect unwinding during GC and 1st pass EH.

Could you let me know your opinion?

One approach to optimize #10782 is to emit full arg info only for functions with funclet. Is it possible to add some flag on GCInfo header (to indicate arg info type)?

janvorli · 2017-04-06T09:12:08Z

src/vm/i386/cgenx86.cpp


    pRD->pCurrentContext->Eip = pRD->ControlPC = m_MachState.GetRetAddr();
-    pRD->pCurrentContext->Esp = pRD->pCurrentContext->ResumeEsp = pRD->SP = (DWORD) m_MachState.esp();
+    pRD->pCurrentContext->Esp = pRD->SP = (DWORD) m_MachState.esp();


Should we make the same change in TailCallFrame::UpdateRegDisplay and HijackFrame::UpdateRegDisplay? They set the Esp and ResumeEsp the same way too.

I think so, but I would like to isolate those changes from this change.

janvorli · 2017-04-06T09:14:25Z

src/vm/exceptionhandling.cpp

    UPDATEREG(Edi);
    UPDATEREG(Ebp);

+    UPDATEREG_VAL(ResumeEsp);


I am not excited about the updating of ResumeEsp at this place since it is a not a non-volatile register and the function is called UpdateNonvolatileRegisters. Unfortunately the only other place where we have the necessary data to update the ResumeESP is the caller and that's called FixNonvolatileRegisters, so it would not be any better. And I don't really want to rename the functions.

We may revise GetResumeSp to take CONTEXT instead REGDISPLAY.

Then, we could fix ResumeEsp inside ProcessCLRException just before calling ExceptionTracker::ResumeExecution.

Great, that sounds much better to me! And maybe we can just change the method to set the ResumeEsp internally in the context instead of returning it back. And maybe rename the method to something like FixResumeEsp then.

It seems that the correctness of GetPushedArgSize should be guaranteed before implementing this change. I'll take a look.

jkotas · 2017-04-06T13:55:16Z

src/vm/i386/cgenx86.cpp

+    //
+    // Fix up ResumeSp
+    //
+    pRD->pCurrentContext->ResumeEsp = EECodeManager::GetResumeSp(pRD);


This assumes that the FCall is always called from JITed code. It is not always the case. For example, the FCall can be called by CallDescrWorker.

It is possible, but does not matter as we use ResumeEsp only when resuming from managed code.

I think it will be more obvious after you make the change that you've suggested.

parjong · 2017-04-07T04:34:30Z

@janvorli Revised PR as I mentioned, but it introduces baseservices.compilerservices.RuntimeWrappedException regression. I'll take a look into this issue.

parjong · 2017-04-07T08:03:28Z

955fbc2 resolves baseservices.compilerservices.RuntimeWrappedException regression.

janvorli · 2017-04-07T10:00:19Z

I guess that the suggested approach works for resuming, but I'm not sure whether this approach could address incorrect unwinding during GC and 1st pass EH.
@parjong it should work in all of these cases. Whenever there is a funclet on the stack, there must be an exception tracker corresponding to the exception that caused the funclet to be called. And that tracker contains the caller SP of the funclet.
The tricky part though is how to find the right exception tracker. It seems the unwinder would need to keep track of a "current" tracker and whenever we hit a funclet frame during unwinding, we would move to the next tracker.

But I was thinking about it and it seems we have two more options to solve the issue:

Do it by adding the full args info even for EBP frames, but only explicitly for calls to FCALLs if that's possible. That would seem to be the ideal way with minimal growth of the GC info.
Use the idea that was discussed a long time ago and create a lookup table for stack args size for each FCALL. As was pointed out, that size is already passed to the macros that are used to create FCALL table, but currently ignored. So we could start using it and whenever unwind goes from native to managed frame, we would locate the appropriate FCALL's entry in the lookup and get the size from there.
Maybe we should focus on completing the FEATURE_FIXED_OUT_ARGS work instead. That would remove the need to solve this problem.

@parjong can you please tell me your opinions on these options?

parjong · 2017-04-07T14:29:12Z

Do it by adding the full args info even for EBP frames, but only explicitly for calls to FCALLs if that's possible. That would seem to be the ideal way with minimal growth of the GC info.

Full arg info is required only for functions with funclets. FCALL itself does not matter with the current approach. All the function with funclet are ebp-framed, but we need to treat its funclets as esp-framed. Actually, #10782 already introduced emitFullArgInfo to control when we emit full arg info. Currently, it is always true when fullPtrMap which means that function itself is fully interruptible (changes in morph.cpp enforce all functions with funclet to be fully interruptible), or esp-framed. It is easy to change this in JIT. Unfortunately, there is no way to distinguish fully interruptible function with full arg info and with partial arg info from VM, which enforces #10782 to emit full arg info for all fully interruptible funtions. We could optimize this if we could introduce an additional flag in GCInfo header that allows VM to distinguish two types of fully interruptible functions.

Use the idea that was discussed a long time ago and create a lookup table for stack args size for each FCALL. As was pointed out, that size is already passed to the macros that are used to create FCALL table, but currently ignored. So we could start using it and whenever unwind goes from native to managed frame, we would locate the appropriate FCALL's entry in the lookup and get the size from there.

I once considered this approach, but concluded that it will not work. Recently, it turns out that x86 JIT emits nested calls. The presence of nested calls makes it impossible to estimate stack alignment padding without additional meta data such as GCInfo.

Maybe we should focus on completing the FEATURE_FIXED_OUT_ARGS work instead. That would remove the need to solve this problem.

I totally agree that enabling FEATURE_FIXED_OUT_ARGS will be the best solution for this issue, but I overhear that it will require non-trivial amount of work: JIT itself, GC Info encoder/decoder, and unwinder should be revised all together.

janvorli · 2017-04-07T15:44:36Z

@parjong it seems I got confused by the title of this PR. So the problem you are solving is not just FCALLs in a funclet, but all calls made from a funclet? That when we unwind from any such call back to the funclet, we cannot get the correct resume ESP without a fix?

danmoseley · 2017-04-08T16:32:03Z

@dotnet-bot test OSX10.12 x64 Checked Build and Test (network)

parjong · 2017-04-09T23:38:36Z

@janvorli Sorry for the confusion. Yes, this PR attempts to address general incorrect resume esp issue due to stack alignment padding (via using GCInfo). This issue currently appears only for FCALL as managed unwinder uses a heuristic to adjust resume esp (which may be incorrect in the presence of nested calls).

I guess that we will suffer from incorrect resume esp issue when we unwind from any nested call even for managed functions without a fix like this.

This PR is based on the assumption that GetPushedArgSize is correct for ESP-framed functions and funclets, but unfortunately it turns out that GetPushedArgSize is incorrect for funclets.

#10782 attempts to fix this issue (makes GetPushedArgSize correct for funclets).

[x86/Linux] Correctly unwind FCALL frames

25637dd

dnfclas added the cla-already-signed label Apr 5, 2017

janvorli reviewed Apr 6, 2017

View reviewed changes

jkotas reviewed Apr 6, 2017

View reviewed changes

parjong changed the title ~~[x86/Linux] Correctly unwind FCALL frames~~ [WIP][x86/Linux] Correctly unwind FCALL frames Apr 7, 2017

parjong force-pushed the fix/x86_fcall_unwind branch from a586896 to 21db4ce Compare April 7, 2017 04:47

parjong added 2 commits April 7, 2017 13:48

Fix Up ResumeEsp inside ProcessCLRException

21db4ce

Fix Context Before PC Update

955fbc2

parjong changed the title ~~[WIP][x86/Linux] Correctly unwind FCALL frames~~ [WIP][x86/Linux] Fix ResumeEsp before resume Apr 9, 2017

parjong changed the title ~~[WIP][x86/Linux] Fix ResumeEsp before resume~~ [x86/Linux] Fix ResumeEsp before resume Apr 9, 2017

janvorli approved these changes Apr 12, 2017

View reviewed changes

janvorli merged commit 03d0d24 into dotnet:master Apr 12, 2017

parjong deleted the fix/x86_fcall_unwind branch April 12, 2017 22:13

parjong mentioned this pull request Apr 24, 2017

[x86/Linux] Remove unnecessary ResumeEsp field #11167

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

[x86/Linux] Fix ResumeEsp before resume #10749

[x86/Linux] Fix ResumeEsp before resume #10749

Uh oh!

Conversation

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

Uh oh!

Uh oh!

parjong commented Apr 6, 2017

Uh oh!

parjong commented Apr 6, 2017

Uh oh!

janvorli commented Apr 6, 2017

Uh oh!

parjong commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parjong Apr 6, 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!

parjong Apr 6, 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!

Choose a reason for hiding this comment

Uh oh!

parjong Apr 7, 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!

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 Apr 6, 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!

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

Uh oh!

Uh oh!

parjong commented Apr 7, 2017

Uh oh!

janvorli commented Apr 7, 2017

Uh oh!

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

Uh oh!

Uh oh!

janvorli commented Apr 7, 2017

Uh oh!

danmoseley commented Apr 8, 2017

Uh oh!

parjong commented Apr 9, 2017

Uh oh!

Reviewers

Assignees

Labels

parjong commented Apr 5, 2017 •

edited

Loading

parjong Apr 6, 2017 •

edited

Loading

parjong Apr 6, 2017 •

edited

Loading

parjong Apr 7, 2017 •

edited

Loading

parjong Apr 6, 2017 •

edited

Loading

parjong commented Apr 7, 2017 •

edited

Loading

parjong commented Apr 7, 2017 •

edited

Loading