RaBaldr: Explicit drops sometimes don't release stack memory

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

3 years ago
Posted 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)
Assignee

Updated

3 years ago
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.
Assignee

Comment 3

3 years ago
Posted 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+

Comment 5

3 years ago
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

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a67f9e4b5faf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.