1316850 - Wasm baseline: Vet the 0xC changes

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

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 <[e-mail]>
date:        Fri Sep 23 09:13:15 2016 -0500
summary:     Bug 1287220 - Baldr: update to binary version 0xc (r=luke)

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1316850-fallible-newLabel.patch

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.

Comment 2

[Lars T Hansen [:lth]]

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 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8809776 [details] [diff] [review]
bug1316850-fallible-newLabel.patch

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

lgtm

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1316850-allow-null-label.patch

Very tiny cleanup item: The first label to a ControlItem can no longer be null, so remove the guard that allowed that.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1316850-no-fallthrough.patch

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.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1316850-track-function-block.patch

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.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1316850-avoid-duplication.patch

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.)

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8810355 [details] [diff] [review]
bug1316850-allow-null-label.patch

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

Cool

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

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)?

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8810360 [details] [diff] [review]
bug1316850-avoid-duplication.patch

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

This is better, thanks!

Comment 12

[Lars T Hansen [:lth]]

(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.

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1316850-cleanup-void.patch

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.

Comment 14

[Lars T Hansen [:lth]]

Attachment: bug1316850-pop-value-stack-as-before.patch

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.

Comment 15

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e0eb206fec4

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

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...)

Comment 17

[Lars T Hansen [:lth]]

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

Comment 18

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=356b09fa6502

Comment 19

[Lars T Hansen [:lth]]

(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 20

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 21

[Lars T Hansen [:lth]]

(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.

Comment 22

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc3650c7a4e2e691980d7ce9ad89445b799b9f8
Bug 1316850 - check return from fallible allocator. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b36809646cecbc04bdb09e6cf86d46cf3d4e93
Bug 1316850 - no need to allow null label in pushControl. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f086350aaf4da3625b9dc520c4a0d4f9027625
Bug 1316850 - make sure we don't fall through into endFunction. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8873fe7b7c83793a1015555a7b641e0577094c2
Bug 1316850 - reduce code duplication. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/0da56750be97361ba349e591b699b66911a0ce9c
Bug 1316850 - clean up void handling. r=bbouvier

https://hg.mozilla.org/integration/mozilla-inbound/rev/df748be239499d9a8f88df38988d89acab774f7a
Bug 1316850 - restore old value stack popping regime, with a twist. r=bbouvier

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2fc3650c7a4e
https://hg.mozilla.org/mozilla-central/rev/e9b36809646c
https://hg.mozilla.org/mozilla-central/rev/b0f086350aaf
https://hg.mozilla.org/mozilla-central/rev/a8873fe7b7c8
https://hg.mozilla.org/mozilla-central/rev/0da56750be97
https://hg.mozilla.org/mozilla-central/rev/df748be23949

Comment 24

[Lars T Hansen [:lth]]

We're done here.