Closed Bug 1316850 Opened 3 years ago Closed 3 years ago

Wasm baseline: Vet the 0xC changes

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(6 files, 1 obsolete file)

When the wasm instruction set was updated to version 0xC and the machine model changed from an ast to a stack machine, several errors were introduced in the baseline compiler: bug 1316181, bug 1316332.

In addition, the handling of the Unreachable opcode only pops the value stack, not the CPU stack.  This may in a sense be unproblematic if the trap never returns, but it's not completely obvious that it will not affect code generation further down in the control flow.  (I don't know for a fact that this problem was introduced by that change.)

Finally, the allocJoinReg() abstraction that is introduced is suspicious because it counts on the join register being available.  It may be correct, but it's not obvious.

Landed on asmjs/WasmBaselineCompile.cpp [sic] here:

changeset:   315115:4119fba22f7f
user:        Dan Gohman <sunfish@mozilla.com>
date:        Fri Sep 23 09:13:15 2016 -0500
summary:     Bug 1287220 - Baldr: update to binary version 0xc (r=luke)
The baseline compiler's newLabel() is fallible.  It should not be - that's bug 1316819.  But until that's changed we need a null pointer check here, cf all the other uses of newLabel in the compiler.
Attachment #8809776 - Flags: review?(bbouvier)
Keywords: leave-open
Changeset 315115 did indeed introduce popValueStackTo() for Unreachable, where there was nothing before.  So this was at least half-a-fix for a perceived problem.

It also introduced popValueStackTo() in many other places, most of which surprise me, so that needs to be investigated.  Clearly this could have been the right thing to do, as we went from an AST to a stack machine.
Comment on attachment 8809776 [details] [diff] [review]
bug1316850-fallible-newLabel.patch

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

lgtm
Attachment #8809776 - Flags: review?(bbouvier) → review+
Very tiny cleanup item: The first label to a ControlItem can no longer be null, so remove the guard that allowed that.
Attachment #8810355 - Flags: review?(bbouvier)
Very tiny cleanup item: Now that we can br to the function block I became a little paranoid and decided it would be good to make sure we don't fall into the code generated by endFunction.
Attachment #8810358 - Flags: review?(bbouvier)
Cleanup: Starting with 0xC a function ends with just a regular block 'end', but to generate correct code we must know whether a block is the function block or not.  (In the former case we must jump to the code that returns.)  The 0xC patch would sniff the iterator's control stack to determine that, but it's cleaner to just track it in the control item.
Attachment #8810359 - Flags: review?(bbouvier)
Cleanup: The 0xC patch introduced functionality for teeSetLocal, teeSetGlobal, and teeStore, but in doing so ended up duplicating quite a bit of code.  We can unduplicate the code with little bit of templating, so let's do that.  (No functional change is intended here.)
Attachment #8810360 - Flags: review?(bbouvier)
Comment on attachment 8810355 [details] [diff] [review]
bug1316850-allow-null-label.patch

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

Cool
Attachment #8810355 - Flags: review?(bbouvier) → review+
Comment on attachment 8810358 [details] [diff] [review]
bug1316850-no-fallthrough.patch

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

I like this idea.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +2053,5 @@
>      bool endFunction() {
> +#ifdef DEBUG
> +        // Always branch to outOfLinePrologue_ or returnLabel_.
> +        masm.breakpoint();
> +#endif

For more efficiency against real-world attackers, how about generating it also in opt builds?
Attachment #8810358 - Flags: review?(bbouvier) → review+
Comment on attachment 8810359 [details] [diff] [review]
bug1316850-track-function-block.patch

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

Is it used in a subsequent patch? I do prefer the old way, since adding the boolean in the general case doesn't seem much useful. If there's a way to do it with the old way and explicit the invariant with MOZ_ASSERT (e.g. once we've read that (supposed to be) final endBlock, set a DebugOnly<bool> hasReachedFuncEnd at the BaselineCompiler level and MOZ_ASSERT(!hasReachedFuncEnd) in some control-flow functions?), we could do that. Or the BaselineCompiler struct could keep a ControlFlow pointer to the top level item, and check against this when needed (since there's only one of these)?
Attachment #8810359 - Flags: review?(bbouvier)
Comment on attachment 8810360 [details] [diff] [review]
bug1316850-avoid-duplication.patch

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

This is better, thanks!
Attachment #8810360 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> Comment on attachment 8810359 [details] [diff] [review]
> bug1316850-track-function-block.patch
> 
> Review of attachment 8810359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is it used in a subsequent patch? I do prefer the old way, since adding the
> boolean in the general case doesn't seem much useful.

It is useful because it's fairly gross to be sniffing the iterator's control stack, which is used
nowhere else in the baseline compiler...  The flag is used in the compiler as of now, so
I'm not sure what you're asking, really.

All that said, I'm trying to revert the code for maintaining the value stack, and if that succeeds
then the flag disappears althogether.  More to come.
Version 0xC changed the handling of Void, and Void no longer appears on the baseline compiler's value stack.  But the cleanup that landed in changeset 315115 was incomplete, so let's finish the job here.  Most of this is straightforward.  We want to keep Stk::None as a value because it is a useful default value; I have left it in the switches to signal that it is not a value we expect to see, but I could remove those cases too, if you like.

In addition, this patch moves the proliferating IsVoid checks into the routines that care about them, since that works very well; popJoinReg() becomes popJoinRegUnlessVoid(t), which checks t.  It makes good use of t, because it can be used to check against what's actually on the stack.  And so on for several other routines.  This makes the callers of popJoinReg et al more streamlined.

allocJoinReg has been renamed as captureJoinReg to parallel captureReturned{I,F}{32,64} -- it does the same work, namely, it returns a predefined register that must be free, or else.
Attachment #8810438 - Flags: review?(bbouvier)
This patch reverts the change made in changeset 315115 that moved popValueStackTo calls from just before popControl to (a) inside popControl with an additional complex condition and (b) to near the end of certain control flow constructs.  I'm not sure why that was done.  It looks like it might have been done to move logic from emitFunction into emitBlock/endBlock, since the function logically carries a "block" that can be a branch target and that has some implications for which labels get bound and so on, but I don't think the change was an improvement.  The old location of popValueStackTo coupled it very nicely with popControl; the two belong together.

However, with this reversion comes a need to handle the doReturn called from emitFunction slightly differently, so there's an extra flag to pass to doReturn to tell it whether to try to clean up the CPU stack before branching to the exit code.

This passes all tests, and I'm pretty confident it's right, but I will test AngryBots etc before I land the queue.
Attachment #8810359 - Attachment is obsolete: true
Attachment #8810440 - Flags: review?(bbouvier)
Priority: P2 → P1
Comment on attachment 8810438 [details] [diff] [review]
bug1316850-cleanup-void.patch

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

Nice cleanup!

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +5033,5 @@
>      if (ifThenElse.label->used())
>          masm.bind(ifThenElse.label);
>  
> +    bool elseLive = !ifThenElse.deadOnArrival &&
> +        (!ifThenElse.deadThenBranch || !deadCode_ || ifThenElse.label->bound());

nit: can you align the opening parenthesis with !ifThenElse.deadOnArrival, please?

Also, I don't think this means the `else` is live: it seems to rather mean that the join itself is live (the if/else wasn't dead on arrival, and either the then isn't dead, or we're not in dead code (~= the else isn't dead), or the join label is used). Can you rename this variable, please? (endLive, joinLive...)
Attachment #8810438 - Flags: review?(bbouvier) → review+
The first try run burned on linux64-opt because of an internal compiler error related to the new templates (looks like a register allocator failure).

A second try run on linux64-opt with the offending templates inlined in the class defn is better:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc09bc4cee95bae1022af643888cc6ec13096bd3
(In reply to Lars T Hansen [:lth] from comment #17)
> The first try run burned on linux64-opt because of an internal compiler
> error related to the new templates (looks like a register allocator failure).

Looks like this is gcc 4.8.5.
Comment on attachment 8810440 [details] [diff] [review]
bug1316850-pop-value-stack-as-before.patch

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

So iiuc, unreachable/br/br_table enter dead code, but now we cleanup afterwards when we actually pop a Control? That makes sense to me. Nice that we can get rid of the bool param in endBlock. Thanks for the patch.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +155,5 @@
>  typedef bool IsSigned;
>  typedef bool ZeroOnOverflow;
>  typedef bool IsKnownNotZero;
>  typedef bool HandleNaNSpecially;
> +typedef bool DoPopStack;

Maybe simply PopStack (and in the function arguments too)?

@@ +7119,5 @@
>      if (!emitBody())
>          return false;
>  
> +    if (!deadCode_)
> +        doReturn(sig.ret(), DoPopStack(false));

Could it be put in the emitBody function instead? There seems to be only way emitBody can return true.
Attachment #8810440 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #20)
> Comment on attachment 8810440 [details] [diff] [review]
> bug1316850-pop-value-stack-as-before.patch
> 
> Review of attachment 8810440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +7119,5 @@
> >      if (!emitBody())
> >          return false;
> >  
> > +    if (!deadCode_)
> > +        doReturn(sig.ret(), DoPopStack(false));
> 
> Could it be put in the emitBody function instead? There seems to be only way
> emitBody can return true.

I'm torn about that.  I agree with the sentiment, but it doesn't feel like a simplification: it means passing the return type into emitBody and it means adding logic in the middle of emitBody that feels out of place there.  I will keep this as it is for now, I think.
We're done here.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.