Closed Bug 1311294 Opened 5 years ago Closed 5 years ago

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


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




Tracking Status
firefox54 --- fixed


(Reporter: lth, Assigned: lth)




(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
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]

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+
Bug 1311294 - wasm baseline, check register sets against leaks. r=bbouvier
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.