BaselineCompiler: Fix jit-test failures

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments)

17.65 KB, patch
djvj
: review+
Details | Diff | Splinter Review
5.53 KB, patch
djvj
: review+
Details | Diff | Splinter Review
2.45 KB, patch
djvj
: review+
Details | Diff | Splinter Review
1.02 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
1.42 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
1.92 KB, patch
djvj
: review+
Details | Diff | Splinter Review
2.83 KB, patch
djvj
: review+
Details | Diff | Splinter Review
6.92 KB, patch
djvj
: review+
Details | Diff | Splinter Review
8.42 KB, patch
djvj
: review+
Details | Diff | Splinter Review
3.15 KB, patch
djvj
: review+
Details | Diff | Splinter Review
904 bytes, patch
djvj
: review+
Details | Diff | Splinter Review
5.91 KB, patch
djvj
: review+
Details | Diff | Splinter Review
4.91 KB, patch
djvj
: review+
Details | Diff | Splinter Review
Going to attach some patches to fix all remaining jit-test failures.
When we bailout, we have to make sure the bailed-out frames have arguments objects if script->needsArgsObj(). The attached patch does that by calling a VM function right before we jump into baseline JIT code. Also changes frameSize_ from size_t to uint32_t, slightly more efficient on 64-bit.
Attachment #712422 - Flags: review?(kvijayan)
On x86 we were using ebx as scratch, but this could clobber R1. This patch changes the function so that we don't need a scratch register.
Attachment #712423 - Flags: review?(kvijayan)
This just matches the bailout-to-interpreter code that landed on m-c recently.
Attachment #712424 - Flags: review?(kvijayan)
Changes JSOP_CASE to pop the switch value if the case matches. (JM does not need this because it does not have to keep the stack pointer updated between VM calls.) Fixes a bunch of CONDSWITCH test failures, especially in debug mode, where it's more likely to crash (leave-frame debugger hooks use StackIter).
Attachment #712425 - Flags: review?(bhackett1024)
Like we discussed last week, NEWARRAY and NEWOBJECT are JOF_TYPESET, but don't have a type monitor IC because the TypeObject is statically known.
Attachment #712427 - Flags: review?(bhackett1024)
Patch for m-c. Moves the code to create the |this| object in IonBuilder up, before we create the resuem point. Fixes some test failures because Ion -> baseline bailouts assume the |this| slot in the stub frame holds the constructed object (the loadValue in ICCall_Fallback). This seems to be the easiest fix, and nbp couldn't think of any problems with it.
Attachment #712431 - Flags: review?(kvijayan)
Fixes some test failures caused by us not creating a call object after a bailout from Ion.
Attachment #712432 - Flags: review?(kvijayan)
AbstractFramePtr::evalPrev() did the right thing for baseline frames, but not for StackFrame if the previous frame was a baseline frame. This patch fixes it so that it returns the baseline frame in this case.
Attachment #712434 - Flags: review?(kvijayan)
BaselineStackBuilder can reallocate its buffer, while we still have a BaselineFrame * pointer pointing into the old buffer, resulting in a dangling pointer. We really want to have everything in a single buffer, and calculating the buffer size upfront is complicated, so this patch wraps pointers into the buffer in a safe pointer class that calculates the pointer from the header + offset when we use it.
Attachment #712438 - Flags: review?(kvijayan)
We have to use sub32 instead of subPtr, fixes 2 jit-tests that use tableswitch with negative numbers.
Attachment #712460 - Flags: review?(kvijayan)
Fixes a jstest failure, ecma_5/Expressions/primitive-this-boxing-behavior.js.
Attachment #712472 - Flags: review?(kvijayan)
Attachment #712422 - Flags: review?(kvijayan) → review+
Attachment #712423 - Flags: review?(kvijayan) → review+
Comment on attachment 712424 [details] [diff] [review]
Part 3: Fix inlined FUNAPPLY bailouts

Review of attachment 712424 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/BaselineBailouts.cpp
@@ +563,5 @@
> +    // will have the real arguments in the slots and not always be equal.
> +#ifdef DEBUG
> +    uint32_t expectedDepth = js_ReconstructStackDepth(cx, script,
> +                                                      resumeAfter ? GetNextPc(pc) : pc);
> +    JS_ASSERT_IF(op != JSOP_FUNAPPLY, exprStackSlots == expectedDepth);

Mostly a nit:
It seems like it should be possible to make this ASSERT more precise.

This property should only be the case if this frame is not a terminal frame (i.e. iter.moreFrames() == true), or maybe we're doing a resumeAfter on a type-monitor bailout when "returning" from the inlined funapply.

Does it seem reasonable to add those to the condition in JS_ASSERT_IF?
Attachment #712424 - Flags: review?(kvijayan) → review+
Attachment #712431 - Flags: review?(kvijayan) → review+
Attachment #712432 - Flags: review?(kvijayan) → review+
Comment on attachment 712434 [details] [diff] [review]
Part 8: Fix StackFrame::evalPrev()

Review of attachment 712434 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Stack.cpp
@@ +1636,5 @@
> +    /* Pop native and Ion frames until we reach the target frame. */
> +    while (data_.state_ == NATIVE) {
> +        popCall();
> +        settleOnNewState();
> +    }

I don't understand what the above is meant to fix.  Could you explain?
Attachment #712434 - Flags: review?(kvijayan) → review+
Comment on attachment 712438 [details] [diff] [review]
Part 9: Avoid dangling pointers into bailout buffer

Review of attachment 712438 [details] [diff] [review]:
-----------------------------------------------------------------

Nice find.
Attachment #712438 - Flags: review?(kvijayan) → review+
Attachment #712460 - Flags: review?(kvijayan) → review+
Attachment #712472 - Flags: review?(kvijayan) → review+
Attachment #712425 - Flags: review?(bhackett1024) → review+
Attachment #712427 - Flags: review?(bhackett1024) → review+
Part 1-11, except part 6:

https://hg.mozilla.org/projects/ionmonkey/rev/1bd122f04e75
https://hg.mozilla.org/projects/ionmonkey/rev/9bb7b9ee9ca4
https://hg.mozilla.org/projects/ionmonkey/rev/3ea6a8da3c13
https://hg.mozilla.org/projects/ionmonkey/rev/dc7e4175d9fc
https://hg.mozilla.org/projects/ionmonkey/rev/567c22d8fade
https://hg.mozilla.org/projects/ionmonkey/rev/e851560bc266
https://hg.mozilla.org/projects/ionmonkey/rev/1afacbe26e79
https://hg.mozilla.org/projects/ionmonkey/rev/071bfa61dbe4
https://hg.mozilla.org/projects/ionmonkey/rev/47fade8de5bd
https://hg.mozilla.org/projects/ionmonkey/rev/87bf57667483

(In reply to Kannan Vijayan [:djvj] from comment #12)
> Does it seem reasonable to add those to the condition in JS_ASSERT_IF?

Yeah, I used the same assert as the bailout-to-interpreter code, but we can tighten it a bit. I added the cases you suggested to the condition.

(In reply to Kannan Vijayan [:djvj] from comment #13)
> I don't understand what the above is meant to fix.  Could you explain?

If we do an eval-in-frame in a baseline JIT frame, StackIter will reach the entry StackFrame, and then we have to pop Ion frames until we reach our baseline frame. In some cases js::Invoke can push a CallArgs list and we have to skip past these as well.
Some tests with overrecursion still fail on TBPL. These are hard to reproduce, but fortunately I could reproduce one of them with an x64 build.

The problem is that if the overrecursion check fails during bailout, we can't report it immediately because we have no exit frame (StackIter will crash when we try to create a stack trace).

This patch makes us behave like the interpreter: return a special bailout code, ensure there's an exit frame and then report the exception.
Attachment #712865 - Flags: review?(kvijayan)
We were not toggling barriers for the main script at the end of compilation. Also fixes an issue with the JSOP_NEW stub, GC triggered by CreateThis can destroy the callee IonScript or BaselineScript.

I think that's enough patches for one bug, will file new bugs for other problems.
Attachment #712874 - Flags: review?(kvijayan)
Attachment #712865 - Flags: review?(kvijayan) → review+
Attachment #712874 - Flags: review?(kvijayan) → review+
https://hg.mozilla.org/mozilla-central/rev/7b06c456f336
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.