Wasm baseline: Assert the register set is invariant to guard against leaks

RESOLVED FIXED in Firefox 54

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

For background, see bug 1311287.

We want an assertion in the main decoding loop in the baseline compiler that checks that the compiler's register set is invariant, as follows.

Let R be the registers available after initialization.

After each iteration of the main decoding loop:
- Let U be the union of the currently available registers and the 
  registers on the evaluation stack.
- Assert that R and U are equal.

This should quickly pin down leaks when the emitters forget to free a working register.  These leaks will otherwise seem arbitrary.
A similar application is to assert, after the sync() that currently precedes every function call and every block, that all registers are available, because the code for function calls and CF joins actually assumes that.  There may be other cases.
Blocks: wasm-baseline-perf
No longer blocks: wasm
Summary: wasm: Baseline JIT should assert its register set is invariant to guard against leaks → Wasm baseline: Assert the register set is invariant to guard against leaks
Priority: P2 → P3
Posted patch bug1311294-no-leaks.patch (obsolete) — Splinter Review
Tentative fix for this.  It uncovers the second problem in bug 1337060 and also flags two other test cases in the test suite as having problems, will investigate to make sure it's not a bug in the test.

On my system, this code does /not/ slow down test suite running much, so we should be able to leave it enabled in debug builds.  Yay!

(Yes, it can be abstracted a bit, and I will do so.)
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Priority: P3 → P1
Those failing tests become non-failing with Benjamin's fix for the register leak.
The obvious code: capture the allocatable register set early, then between opcodes regenerate it from available information and make sure it matches the captured value.
Attachment #8834444 - Attachment is obsolete: true
Attachment #8834450 - Flags: review?(bbouvier)
Depends on: 1337060
Comment on attachment 8834450 [details] [diff] [review]
bug1311294-no-leaks.patch

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

Good to have this. Probably this should help spot issues with the fuzzer eagerly too. Thanks!

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +504,5 @@
>      AllocatableFloatRegisterSet availFPU_;
>  #ifdef DEBUG
>      bool                        scratchRegisterTaken_;
> +    AllocatableGeneralRegisterSet allGPR_;
> +    AllocatableFloatRegisterSet allFPU_;

Can you maybe re-align the names on the same column, up to availGPR_ above?

@@ +2037,5 @@
> +
> +    // Call this before compiling any code.
> +    void setupRegisterLeakCheck() {
> +        allGPR_ = availGPR_;
> +        allFPU_ = availFPU_;

How about using `previousAvailGPR_/FPU_` instead of `allGPR_/allFPU_`? To me, `all` sounds like they should indeed *all* be there, that is, the full set of all the registers available at the start of a function's body.

Other prefixes might be good too.

@@ +2044,5 @@
> +    // Call this between opcodes.
> +    void performRegisterLeakCheck() {
> +        AllocatableGeneralRegisterSet knownGPR_ = availGPR_;
> +        AllocatableFloatRegisterSet knownFPU_ = availFPU_;
> +        for ( size_t i=0 ; i < stk_.length() ; i++ ) {

nit: for (size_t i = 0; i < stk_.length(); i++) {

@@ +2048,5 @@
> +        for ( size_t i=0 ; i < stk_.length() ; i++ ) {
> +	    Stk& item = stk_[i];
> +	    switch (item.kind_) {
> +	      case Stk::RegisterI32:
> +		knownGPR_.add(item.i32reg());

Using `add` will assert also if the register argument is *already* in the knownGPR_ set, right?
Attachment #8834450 - Flags: review?(bbouvier) → review+
https://hg.mozilla.org/mozilla-central/rev/3bd986e5c3c8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.