1316332 - RaBaldr: Explicit drops sometimes don't release stack memory

Status: RESOLVED FIXED

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

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: drop.patch

An explicit drop should not only pop values off the value stack if the value is on the CPU stack. The initial test case is... interesting :)

(it really calls for differential testing, because the fuzzers would have found this so many times)

Comment 1

[Lars T Hansen [:lth]]

Comment on attachment 8809041 [details] [diff] [review]
drop.patch

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

Thank you!  We need another round here but this is a good catch and another reason for me to scrutinize the 0xC patch more closely.

::: js/src/jit-test/tests/wasm/drop.js
@@ +1,1 @@
> +// |jit-test| test-also-wasm-baseline

This line is no longer needed :-)

Should this file go into regress/ btw?  Not sure.

@@ +6,5 @@
> +        (module
> +         (func $test (result ${type}) (param $p ${type}) (param $p2 ${type})
> +            get_local $p
> +            get_local $p2
> +            (block)

Nice!

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +5200,5 @@
>      return true;
>  }
>  
> +bool
> +BaseCompiler::emitDrop()

This is the right idea but the wrong fix because it breaks abstraction boundaries in the compiler.  emitWhatever() functions do not get to look at the value stack like this and make decisions about what to do with it, as a rule.  Instead, you want an instruction roughly called popStackIfMemory that would sit next to popStackOnBlockExit, for example.  So then the body here would become

  ...
  popStackIfMemory()
  popValueStackBy(1)

In addition, the implementation of popStackIfMemory should not need to allocate temp registers and read values into them as it's popping the stack; it should just adjust the stack pointer in the way that eg popStackOnBlockExit does.

The amount to adjust by can be computed by stackConsumed(1), so indeed the implementation of popStackIfMemory is simply:

 v = stackConsumed(1)
 if (v > 0)
   add to stack pointer(v)

and changes to introduce isMem etc should not be required.

Comment 2

[Lars T Hansen [:lth]]

There may be a slightly systemic problem here: Expr::Unreachable may have the same issue, though I don't know if it matters, since unreachableTrap presumably never returns.

I'll prioritize this investigation today.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: drop.patch

Sweet! Using stackConsumed even fixes a bug in the previous patch (32 bits are pushed for float32 on ARM). I've used the main dir instead of the regress dir cause I like to have fuzz bugs there only; but it doesn't matter much to me, so if you feel strongly, I can move it there.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8809350 [details] [diff] [review]
drop.patch

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

Ship it!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a67f9e4b5faf
Release stack slots in case of explicit drops; r=lth

Comment 6

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

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