1730161 - Wasm instance calls on x86-32 can't return int64 because the ABIs don't match

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

The Linux C++ ABI returns int64 in edx:eax (per observation, to be verified), win32 looks the same (also to be verified). The Wasm ABI uses edi:eax (per x86/Assembler-x86.h), clearly a poor match. We either need to change our result register conventions or we need an adapter somewhere along the call path from wasm to an instance method that returns i64, as will eg memory.size for memory64. (Or we need to pass in a pointer to a cell for the result, but why would we?)

Comment 1

[Nicolas B. Pierron [:nbp]]

Note, our ABI calls are tested using testJITABICalls.cpp.
are only testing that arguments are correctly interpreted. However, it lacks checking that return values are correctly interpreted as well.

If we were to make changes to the system ABI, we should also add such tests.

Comment 2

[Lars T Hansen [:lth]]

Hm, good point. I think my preference would be to insert a shim where i need it, rather than changing any ABIs, and to assert that the return type is not i64 in places that would need a shim if the issue came up, but hasn't. Specifically, we need this for wasm Instance calls but I'm guessing it's not an issue for JS.

Comment 3

[Lars T Hansen [:lth]]

Old comment in jit/shared/AtomicOperations-shared-jit.cpp under an ifdef JS_CODEGEN_X86:

      // The return register edx:eax is a compiler/ABI assumption that is *not*
      // the same as ReturnReg64, ...

On x86, we have:

static constexpr Register JSReturnReg_Type = ecx;
static constexpr Register JSReturnReg_Data = edx;
static constexpr Register ReturnReg = eax;
static constexpr Register64 ReturnReg64(edi, eax);

There's a slight chance that there's code somewhere that depends on JSReturnReg_Data having a byte personality, though I've not seen such code. But it's easiest to assume that there is such code.

The SMDOC for callWithABI in MacroAssembler.h is also subtly wrong, depending on your interpretation of "mostly":

  //    setupWasmABICall is a special case of setupAlignedABICall as
  //    SpiderMonkey's WebAssembly implementation mostly follow the system
  //    ABI, except for float/double arguments, which always use floating
  //    point registers, even if this is not supported by the system ABI.

(Also incomplete is step 5 in that algorithm which does not handle the i64 case on 32-bit systems.)

Comment 4

[Lars T Hansen [:lth]]

Attachment: Bug 1730161 - Capture i64 results from system ABI calls on x86

On x86, the i64 return register in the Wasm ABI (ResultReg64) does not
equal the i64 return register in the system ABI: the former is
edi:eax, while the latter is edx:eax.

Only Wasm appears to care about this (there were no i64-returning ABI
calls until now, and only wasm gets them now, with the Wasm64 work),
so the easiest fix is to adjust the results from i64 wasm builtin
calls on x86, moving them from the system ABI registers to the wasm
ABI registers.

The only tricky bit is that the adjustment must come directly after
the call instruction, as edx is a non-return temp register in the Wasm
ABI that is clobbered by the call teardown.

Comment 5

[Lars T Hansen [:lth]]

Attachment: WIP: Bug 1730161 - Atomic-rmw baseline operations

Depends on D125183

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Lars T Hansen [:lth] from comment #3)

Old comment in jit/shared/AtomicOperations-shared-jit.cpp under an ifdef JS_CODEGEN_X86:

      // The return register edx:eax is a compiler/ABI assumption that is *not*
      // the same as ReturnReg64, ...

This comment was added by Waldo.

On x86, we have:

static constexpr Register JSReturnReg_Type = ecx;
static constexpr Register JSReturnReg_Data = edx;
static constexpr Register ReturnReg = eax;
static constexpr Register64 ReturnReg64(edi, eax);

There's a slight chance that there's code somewhere that depends on JSReturnReg_Data having a byte personality, though I've not seen such code. But it's easiest to assume that there is such code.

I had the same feeling a while ago. Retrospectively, if there is any issue this would be in the wrapping of a WASM call or a JS call, but not in the interleaving of calls as all registers are virtual registers which are clobbered by a call.

The SMDOC for callWithABI in MacroAssembler.h is also subtly wrong, depending on your interpretation of "mostly":

  //    setupWasmABICall is a special case of setupAlignedABICall as
  //    SpiderMonkey's WebAssembly implementation mostly follow the system
  //    ABI, except for float/double arguments, which always use floating
  //    point registers, even if this is not supported by the system ABI.

(Also incomplete is step 5 in that algorithm which does not handle the i64 case on 32-bit systems.)

I probably wrote this comment before it got into the SMDOC, or suggested it over some communication channel, this was the intent that Luke had when designing ASM.js implementation, and that WASM inherited this part from ASM.js.
In the past, the WASMABIArg generator did not exists and the setup call was using ABIArgGenerator, which had a few conditions, IIRC.

Comment 7

[Lars T Hansen [:lth]]

A related concern is that

static constexpr Register ABINonArgReturnReg1 = edx;

and there may dependencies on that register having a byte personality.

I'll swap edx and edi and run as many tests as I can, but given that this is x86 and our test coverage (on Nightly) is relatively poor I'm still going to be a little nervous...

Comment 8

[Lars T Hansen [:lth]]

Attachment: Bug 1730161 - Change the ReturnReg64 on x86 (take two). r?nbp

Change the ReturnReg64 on x86 to conform to system ABI conventions: it
needs to be edx:eax for wasm-to-C++ calls that return i64 to work out
properly.

For this to work, the second return-point temp register needs to be
edi and not edx, so change that too.

Fix a comment that pertains to this situation to be more precise.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06dc9594aed8
Change the ReturnReg64 on x86 (take two). r=nbp

Comment 10

[Phabricator Automation]

Comment on attachment 9241085 [details]
WIP: Bug 1730161 - Atomic-rmw baseline operations

Revision D125524 was moved to bug 1727084. Setting attachment 9241085 [details] to obsolete.

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/06dc9594aed8