1518785 - wasm via Ion: incorrect logic to decide on whether to omit a stack overflow check

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Julian Seward [:jseward]]

This surfaced while I was investigating some stack misalignment issues
possibly relating to sizeof(wasm::Frame).

The Ion pipeline for Wasm decides in CodeGenerator::generateWasm what kind of
stack reservation to make:

  if (omitOverRecursedCheck()) {
    masm.reserveStack(frameSize());
  } else {
    masm.wasmReserveStackChecked(frameSize(), trapOffset);
  }

Assuming the 'else' clause is taken, we end up here:

  void
  MacroAssembler::wasmReserveStackChecked(uint32_t amount,
                                          wasm::BytecodeOffset trapOffset) {
    if (!amount) {
      return;
    }

so if |amount == 0| the stack check is omitted.  I think this early exit
is incorrect and should be removed.

My guess about what happened is as follows: it appears that |amount|,
which == CodeGeneratorShared::frameSize(), holds the number of extra bytes
*in addition* to the sizeof(Wasm::frame), that are needed.  However, this
number is rounded up so that (it + sizeof(Wasm::frame)) are 0 % 16.

As an example, on x86(32)-linux, in an infinite-recursion test that should end
by a stack overflow check failing ...

   (module (func (call 0)) (export "" 0))

.. we have:

   sizeof(Frame) == 12
   no extra stack allocation needed
   so |amount| == 4 because it is the smallest number giving
        |amount| + sizeof(Frame) being 0 % 16.
   So the stack overflow is included (correctly).

When I increase sizeof(Frame) to 16, then

   sizeof(Frame) == 16
   no extra stack allocation needed
   so |amount| == 0
   So the stack overflow is incorrectly omitted.

and printfs verify the |amount| numbers in these cases.

Looking back at the early exit in MacroAssembler::wasmReserveStackChecked,
there's no way any non-leaf call could really use zero stack in total -- else
where would he return address be stored?  So the early exit can't be right
as-is.

I looked also at CodeGeneratorShared::frameSize(), but failed to deduce the
intended semantics from it:

  uint32_t frameSize() const {
    return frameClass_ == FrameSizeClass::None() ? frameDepth_
                                                 : frameClass_.frameSize();
  }

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1518785-retain-overflow-check-1.diff

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 9045252 [details] [diff] [review]
bug1518785-retain-overflow-check-1.diff

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

Thanks!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb1ed3f3dfd
wasm-via-Ion: incorrect logic to decide on whether to omit a stack overflow check.  r=luke.

Comment 4

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/bbb1ed3f3dfd