Closed Bug 1310158 Opened 6 years ago Closed 6 years ago

Wasm baseline regalloc failure on MacOS x86 with Clang-3.9


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




Tracking Status
firefox52 --- fixed


(Reporter: lth, Assigned: lth)




(2 files)

Consider this self-contained test case:

let bin = wasmTextToBinary(`
 (module (func (export "i64.test") (result i64)
	  (return (i64.const 0x0CABBA6E0ba66a6e))))`);
let mod = new WebAssembly.Module(bin);

The baseline compiler generates the following code using gcc 5.2.1 on Linux (where the jump is a jump to code that cleans up the stack and returns):

  [Codegen] movl       $0xba66a6e, %eax
  [Codegen] movl       $0xcabba6e, %edi
  [Codegen] jmp        .Lfrom75

It generates the following code using Clang-3.9 on Mac OS X:

  [Codegen] movl       $0xba66a6e, %ecx
  [Codegen] movl       $0xcabba6e, %eax
  [Codegen] movl       %ecx, %eax
  [Codegen] movl       %eax, %edi
  [Codegen] jmp        .Lfrom79

The second snipped is clearly wrong since eax is overwritten before its value can be obtained; this is a straightforward bug in how the return register is handled.

The real mystery is why different code is being generated on the two platforms.  The reason is that on 32-bit systems, the allocator for 64-bit registers (ie a pair) looks like this:

    Register64 allocInt64() {
        return Register64(availGPR_.takeAny(), availGPR_.takeAny());

and the order of those calls is implementation-dependent.  That should be fixed; we want determinism when we can have it.

So, two fixes here: one for the regalloc bug, and one to curb the nondeterminism.
I should note that EDI:EAX is the 64-bit return register pair, of course.
Make register allocation deterministic again.

This patch makes the test case fail on all platforms.  The patch can't land without its companion that fixes the regalloc bug.
Attachment #8801078 - Flags: review?(jdemooij)
Clean up the handling of return values, both when returning them and when capturing them.  This code was probably correct up until 64-bit on 32-bit, but with register pairs it was becoming shaky.  This is better.
Attachment #8801090 - Flags: review?(hv1989)
Attachment #8801078 - Flags: review?(jdemooij) → review?(hv1989)
Priority: -- → P1
Comment on attachment 8801090 [details] [diff] [review]

Review of attachment 8801090 [details] [diff] [review]:

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +1492,5 @@
>          return r;
>      }
> +    // Note, the stack top can be in one half of "specific" on 32-bit
> +    // systems.  We can optimize, but for simplicity, if the register

s/  / /
Attachment #8801090 - Flags: review?(hv1989) → review+
Comment on attachment 8801078 [details] [diff] [review]

Review of attachment 8801078 [details] [diff] [review]:

Good catch!

::: js/src/asmjs/WasmBaselineCompile.cpp
@@ +698,5 @@
>          MOZ_ASSERT(hasInt64());
>  #ifdef JS_PUNBOX64
>          return Register64(availGPR_.takeAny());
>  #else
> +        // Determinism, please.

Please remove comment.
Attachment #8801078 - Flags: review?(hv1989) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.