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.

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.)

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.

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?

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd986e5c3c856b81a5b09fdd69925555ec5a93f Bug 1311294 - wasm baseline, check register sets against leaks. r=bbouvier