Closed Bug 1311294 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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
Attached 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/integration/mozilla-inbound/rev/3bd986e5c3c856b81a5b09fdd69925555ec5a93f
Bug 1311294 - wasm baseline, check register sets against leaks. r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/3bd986e5c3c8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.