1507785 - Assertion failure: masm.framePushed() >= currentFramePushed_, at /js/src/wasm/WasmBaselineCompile.cpp:1401

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5a955ff4b0dd4e44a5363fcd41d18cf2.js

The following program crashes on a debug optimized ARM 64 bits build with arguments --no-wasm-ion 5a955ff4b0dd4e44a5363fcd41d18cf2.js:
Assertion failure: masm.framePushed() >= currentFramePushed_, at /home/ben/code/mozilla-inbound/js/src/wasm/WasmBaselineCompile.cpp:1401

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5a955ff4b0dd4e44a5363fcd41d18cf2.wasm

Comment 2

[Lars T Hansen [:lth]]

At some point, we no longer account correctly for what's on the machine stack.  We should be able to assert the stack size between every instruction, let's try that first.

Comment 3

[Chris Peterson [:cpeterson]]

[arm64:m3] because this assertion failure should probably block ARM64 Fennec from riding the trains.

Comment 7

[Lars T Hansen [:lth]]

The error is in popStackOnBlockExit(), in the case of the chunky stack used only on ARM64.  The virtual SP (currentFramePushed_) is not updated properly in the case of dead code, which is how we found the bug; but even when the code is not dead, it is not updated properly when the branch does not cause a chunk to be popped.  With the assert on the stack size in place this is easy to find.

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1507785-use-logical-frame-size.patch

This turned out to be an obvious error and an easy fix.

Benjamin, I have not yet included the test cases, we should chat about which of these four bugs' tests it makes most sense to include.  The one for this bug is clearly a candidate; the one for the differential output problem probably less so.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9026016 [details] [diff] [review]
bug1507785-use-logical-frame-size.patch

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

Is there a chance the test case could be reduced to a smaller test case? Otherwise, just getting one of the reproducing tests should be enough.

A question below before I can finish my review.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +1604,5 @@
>      // stack.
>  
>      void popStackOnBlockExit(StackHeight destStackHeight, bool deadCode) {
> +        uint32_t framePushedHere = currentFramePushed();
> +        uint32_t framePushedThere = destStackHeight.height;

Why don't we keep the framePushedForHeight here? Also, don't the neighbor functions above (popStackBeforeBranch, willPopStackBeforeBranch) need the same change?

@@ +3415,5 @@
>              }
>          }
>      }
> +
> +    void assertStackInvariants() {

could be made const

@@ +3418,5 @@
> +
> +    void assertStackInvariants() {
> +        size_t size = 0;
> +        for (uint32_t i = 0; i < stk_.length(); i++) {
> +            Stk& v = stk_[i];

nit: for (const Stk& v : stk_) {

@@ +3425,5 @@
> +              case Stk::MemI32: size += BaseStackFrame::StackSizeOfPtr;    break;
> +              case Stk::MemI64: size += BaseStackFrame::StackSizeOfInt64;  break;
> +              case Stk::MemF64: size += BaseStackFrame::StackSizeOfDouble; break;
> +              case Stk::MemF32: size += BaseStackFrame::StackSizeOfFloat;  break;
> +              default: break;

I don't know if we have a function for this, but could we assert here something like !isMem(v.kind()), in case we add new Mem types later?

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9026016 [details] [diff] [review]
bug1507785-use-logical-frame-size.patch

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

(clearing review until question is answered)

Comment 11

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> Comment on attachment 9026016 [details] [diff] [review]
> bug1507785-use-logical-frame-size.patch
> 
> Review of attachment 9026016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a chance the test case could be reduced to a smaller test case?
> Otherwise, just getting one of the reproducing tests should be enough.

OK.  I'll see how much time I have, I think a small test is possible but if it doesn't work out quickly I'll probably just  use the test case from this bug since it had such a clear symptom.

> ::: js/src/wasm/WasmBaselineCompile.cpp
> @@ +1604,5 @@
> >      // stack.
> >  
> >      void popStackOnBlockExit(StackHeight destStackHeight, bool deadCode) {
> > +        uint32_t framePushedHere = currentFramePushed();
> > +        uint32_t framePushedThere = destStackHeight.height;
> 
> Why don't we keep the framePushedForHeight here?

We're supposed to be operating on logical frame height here, but framePushedForHeight is not that, it is about the actual ("physical") SP.  See below.  I had hoped the commit message was clear enough on this but I will add more prose, probably similar to the explanation below.

> Also, don't the neighbor
> functions above (popStackBeforeBranch, willPopStackBeforeBranch) need the
> same change?

I believe these are still correct.

The logic for these three functions should be as follows:

When we branch out of a block we must adjust the physical stack pointer along the edge so that it will be equal to the physical stack pointer as it is when we fall out of the block at the bottom.  But along the edge we do not do anything to adjust the logical stack pointer, because the logical stack pointer is determined by the non-branching path through the code.

When we fall out of a block we must adjust both the physical and the logical stack pointers: the logical stack pointer may need to pop a few items to get to where it was on entry to the block (the stack can be higher because of pushes followed by an UNREACHABLE), and if the code is not dead we may also have to deallocate some stack memory by changing the physical stack pointer.

Previously, we used values for the physical stack pointers as guards on the block-exit code, but this is wrong since it prevents us from doing something when the physical stack pointer does not change, as the case is on ARM64 when we do need to pop something logically but the stack stays within the currently allocated chunk - we would end up doing nothing when we should adjust the logical stack pointer.  Instead, we should use the logical stack pointers for the guard, and then popChunkyBytes will take care of translating the difference in logical pointers for us so that a chunk is popped if that is required by the amount of deallocation.

Comment 12

[Lars T Hansen [:lth]]

An argument might be made that BaseStackFrame has two aspects; it has a completely primitive aspect, that handles stack allocation and deallocation; and it has a logical aspect that doesn't need to know the details.  These two are intermingled a bit in the code in that methods doing the one thing are mixed with methods doing the other thing.  This makes the code obscure, and it could be cleaned up; I'll file a bug.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Lars T Hansen [:lth] from comment #12)
> An argument might be made that BaseStackFrame has two aspects; it has a
> completely primitive aspect, that handles stack allocation and deallocation;
> and it has a logical aspect that doesn't need to know the details.  These
> two are intermingled a bit in the code in that methods doing the one thing
> are mixed with methods doing the other thing.  This makes the code obscure,
> and it could be cleaned up; I'll file a bug.

Great. To be honest, I know I was the one who reviewed this code in the first place (at least partially), but it was hard enough that when looking at it again the other day, it felt very obscure indeed, so I concur with your sentiment that it needs some changes. Now, talking about logical vs physical stack makes it all very clear, at least much clearer than the "height" vs "stack" distinction.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9026016 [details] [diff] [review]
bug1507785-use-logical-frame-size.patch

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

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +1604,5 @@
>      // stack.
>  
>      void popStackOnBlockExit(StackHeight destStackHeight, bool deadCode) {
> +        uint32_t framePushedHere = currentFramePushed();
> +        uint32_t framePushedThere = destStackHeight.height;

Then maybe can we just rename `framePushedHere/There` to `stackHeightHere/There`? (so we know it's about the logical stack size, if/when we update the names here)

Comment 15

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #14)

> Then maybe can we just rename `framePushedHere/There` to
> `stackHeightHere/There`? (so we know it's about the logical stack size,
> if/when we update the names here)

Will do.

Comment 16

[Lars T Hansen [:lth]]

Attachment: bug1507785-use-logical-frame-size-v2.patch

Turns out I could remove almost all the JS from the TC for this bug; that's good enough.  The wasm is valid and wasm2wat will disassemble it, for those who are curious.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9026408 [details] [diff] [review]
bug1507785-use-logical-frame-size-v2.patch

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

Thanks!

::: js/src/jit-test/tests/wasm/regress/baseline-stack-height.js
@@ +1,1 @@
> +if (typeof os != "object" || typeof os.file != "object" || typeof os.file.readFile != "function")

Isn't it always defined in the shell?

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +1611,4 @@
>  
>      void popStackOnBlockExit(StackHeight destStackHeight, bool deadCode) {
> +        uint32_t stackHeightHere = currentFramePushed();
> +        uint32_t stackHeightThere = destStackHeight.height;

I've been confused by Here and There; how about Current and Dest?

@@ +3421,5 @@
>              }
>          }
>      }
> +
> +    void assertStackInvariants() const {

Should it be named assertHeightInvariants? (so we can spot it and replace it with logicalStack in the next bug?)

Comment 18

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #17)
> Comment on attachment 9026408 [details] [diff] [review]
> bug1507785-use-logical-frame-size-v2.patch
> 
> Review of attachment 9026408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/wasm/WasmBaselineCompile.cpp
> @@ +1611,4 @@
> >  
> >      void popStackOnBlockExit(StackHeight destStackHeight, bool deadCode) {
> > +        uint32_t stackHeightHere = currentFramePushed();
> > +        uint32_t stackHeightThere = destStackHeight.height;
> 
> I've been confused by Here and There; how about Current and Dest?

Since I don't find Current and Dest any more intuitive, I'm keeping these for now.  Point noted, though.

> @@ +3421,5 @@
> >              }
> >          }
> >      }
> > +
> > +    void assertStackInvariants() const {
> 
> Should it be named assertHeightInvariants? (so we can spot it and replace it
> with logicalStack in the next bug?)

I want this method to be about more than the stack height, so I'm keeping the current name here as well.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b378c260d9
Use logical, not physical, frame size at block exit. r=bbouvier

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/45b378c260d9