Closed Bug 1316181 Opened 3 years ago Closed 3 years ago

emitBrIf no longer pops the value stack along the taken edge

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(1 file)

Changeset 315115 / bug 1287220, which updated us to version 0xC, made a change in emitBrIf() which transformed this code:

    masm.branch32(Assembler::Equal, rc.reg, Imm32(0), &notTaken);
    popStackBeforeBranch(target.framePushed);
    masm.jump(target.label);
    masm.bind(&notTaken);

to this code:

    masm.branch32(Assembler::NotEqual, rc.reg, Imm32(0), target.label);

The latter is only correct if there are no values to pop off the CPU stack.  Indeed emitBr() retains the old behavior, popping along the taken edge, even after the update to 0xC.

I've not found any reason to believe that the above change is correct, though of course all tests continue to pass, so... maybe there's something special here.  Odds are, however, the change was spurious.

Perhaps with drop in place, test cases that trigger the failure will need to have fairly contorted expression nesting, and will need to have caused spilling to the CPU stack.
Actually, this was a change in the actual 0xc semantics and validation: br_if keeps its branch value on the stack along both successors edges.
(In reply to Luke Wagner [:luke] from comment #1)
> Actually, this was a change in the actual 0xc semantics and validation:
> br_if keeps its branch value on the stack along both successors edges.

That's a different matter (and I see the code does that, and it's fine).  The problem here is that (I believe) there could be still-unconsumed values that have been pushed onto the CPU stack since we entered the block that will be consumed if we don't take the branch; they will need to be popped along the taken edge.  Unless 0xC semantics somehow prevent that, say, by disallowing brIf nested arbitrarily within other expressions.  That would surprise me, but I'm probably no longer up-to-date on the instruction semantics.
Oh sorry, I read this too quickly, I think you're right.
Restore the code that adjusts SP along the taken edge + test case.

The included test case crashes both ARM-sim and x64 debug builds without the fix.  (Input on how this test could be better or stronger is most welcome.)
Attachment #8808960 - Flags: review?(luke)
Attachment #8808960 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/9d15c879fb12
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → 1316850
You need to log in before you can comment on or make changes to this bug.