Closed Bug 1316332 Opened 4 years ago Closed 4 years ago

RaBaldr: Explicit drops sometimes don't release stack memory

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch drop.patch (obsolete) — Splinter Review
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)
Attachment #8809041 - Flags: review?(lhansen)
Blocks: wasm
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.
Attachment #8809041 - Flags: review?(lhansen)
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.
Attached patch drop.patchSplinter Review
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.
Attachment #8809041 - Attachment is obsolete: true
Attachment #8809350 - Flags: review?(lhansen)
Comment on attachment 8809350 [details] [diff] [review]
drop.patch

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

Ship it!
Attachment #8809350 - Flags: review?(lhansen) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a67f9e4b5faf
Release stack slots in case of explicit drops; r=lth
Priority: -- → P1
See Also: → 1316850
https://hg.mozilla.org/mozilla-central/rev/a67f9e4b5faf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.