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

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: bbouvier, Assigned: lth)

Tracking

(Blocks 2 bugs)

Trunk
mozilla65
ARM64
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [arm64:m3])

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

6 months ago
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
Reporter

Comment 1

6 months ago
Flags: needinfo?(lhansen)
Assignee

Updated

6 months ago
Assignee: nobody → lhansen
Blocks: Fennec-ARM64
Flags: needinfo?(lhansen)
Priority: -- → P2
Hardware: Unspecified → ARM64
Assignee

Comment 2

6 months ago
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.
[arm64:m3] because this assertion failure should probably block ARM64 Fennec from riding the trains.
Whiteboard: [arm64:m3]
Assignee

Updated

6 months ago
Duplicate of this bug: 1507778
Assignee

Updated

6 months ago
Duplicate of this bug: 1507780
Assignee

Updated

6 months ago
Duplicate of this bug: 1507786
Assignee

Comment 7

6 months ago
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.
Assignee

Comment 8

6 months ago
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.
Attachment #9026016 - Flags: review?(bbouvier)
Reporter

Comment 9

6 months ago
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?
Reporter

Comment 10

6 months ago
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)
Attachment #9026016 - Flags: review?(bbouvier)
Assignee

Comment 11

6 months ago
(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.
Assignee

Comment 12

6 months ago
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.
Reporter

Comment 13

6 months ago
(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.
Reporter

Comment 14

6 months ago
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)
Assignee

Comment 15

6 months ago
(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.
Assignee

Updated

6 months ago
Blocks: 1508665
Assignee

Comment 16

6 months ago
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.
Attachment #9026016 - Attachment is obsolete: true
Attachment #9026408 - Flags: review?(bbouvier)
Reporter

Comment 17

6 months ago
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?)
Attachment #9026408 - Flags: review?(bbouvier) → review+
Reporter

Updated

6 months ago
Status: NEW → ASSIGNED
Assignee

Comment 18

6 months ago
(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

6 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b378c260d9
Use logical, not physical, frame size at block exit. r=bbouvier

Comment 20

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45b378c260d9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1526568
You need to log in before you can comment on or make changes to this bug.