1687417 - [MIPS] '(__builtin_offsetof(js::wasm::DebugFrame, frame_) + sizeof(js::wasm::Frame)) % Alignment == 0' "Aligned after pushing DebugFrame"

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P5)

Description

[Mike Hommey [:glandium]]

Build fails on mips and mipsel with:

/builds/worker/checkouts/gecko/js/src/wasm/WasmTypes.cpp:1142:3: error: static_assert failed due to requirement '(__builtin_offsetof(js::wasm::DebugFrame, frame_) + sizeof(js::wasm::Frame)) % Alignment == 0' "Aligned after pushing DebugFrame"

offsetof(DebugFrame, frame_) is 28
sizeof(Frame) is 8

It seems to me the right test would be alignof(DebugFrame) >= Alignment, but I don't really know what's being tested here.

Comment 1

[Lars T Hansen [:lth]]

Mips builds are generally somewhat broken at the moment, there are a number of open issues.

Comment 2

[Zhao Jiazhong]

Hi, may I ask is it necessary to maintain the mips32 port? Since our company only have mips64el machine and os, I don't think we can make sure the mips32 port is good.

Comment 3

[Lars T Hansen [:lth]]

No, it's not necessary to maintain mips32 as far as I know, provided we can remove the mips32 backend. Several people in our Jit team have been interested in removing the mips port (both 32-bit and 64-bit) for some time because it's not being maintained very actively. Removing at least the 32-bit version would probably reduce everyone's maintenance burden, simplify development (because the 64-bit mips simulator should run on 64-bit macOS, while the 32-bit does not), improve quality, simplify code, etc. I'd be interested in seeing that happen. cc nbp, jandem.

An alternative for mips development of wasm in firefox is to use the Cranelift compiler, which is part of wasmtime (https://github.com/bytecodealliance/wasmtime). Cranelift for arm64 currently works in firefox, and the x64 port will work "soon". You may get more active contributors on Cranelift than you get on SpiderMonkey directly, and over time you will probably experience lower complexity and less maintenance work with Cranelift. Of course it would take a bit of work for you to port to Cranelift in the first place; it doesn't look like any work has been done yet on MIPS: https://github.com/bytecodealliance/wasmtime/tree/main/cranelift/codegen/src/isa. cc cfallin.

Comment 4

[Zhao Jiazhong]

Thanks for your information, and since the mips64 and mips32 share some files, I will have a look at how to remove the mips32 codes.

And I will conduct an investigation on the Cranelift compiler, see if I can port it to mips64el.

Comment 5

[Mike Hommey [:glandium]]

32-bits mipsel is still a supported platform on Debian, but due to bug 1406999 we had to disable the JIT. I don't know if the resulting build actually works, but this bug is the only thing preventing the build from going through (and is new to 85).

The question from comment 0 still applies.

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

Ah, sorry I misinterpreted the conversation. Okay here's what I was able to find out.

1. DebugFrame is supposed to have an alignment of 8 so that it can be used behind a tagged pointer (AbstractFramePtr) [1]
2. The wasm baseline compiler prologue to allocate a DebugFrame allocates sizeof(Frame) stack space first, followed by offsetof(DebugFrame, frame). This is why that assertion is written the way it is. [2]

So we have sizeof(Frame) = 8, offsetof(DebugFrame, frame) == 28. This leads the baseline compiler to allocate a DebugFrame that isn't 8 byte aligned, which would cause problems for AbstractFramePtr.

This is odd, because I see that MIPS32 already has some padding allocated in DebugFrame presumably for this reason. [3] So I don't know why this would have just broke. Maybe it just needs to be tweaked or dropped?

[1] https://searchfox.org/mozilla-central/rev/2c06b16a0c15ae340a0532e319cbf89ef9d21b68/js/src/vm/Stack.h#132
[2] https://searchfox.org/mozilla-central/rev/2c06b16a0c15ae340a0532e319cbf89ef9d21b68/js/src/wasm/WasmBaselineCompile.cpp#5323
[3] https://searchfox.org/mozilla-central/rev/2c06b16a0c15ae340a0532e319cbf89ef9d21b68/js/src/wasm/WasmTypes.h#3833

Comment 7

[Mike Hommey [:glandium]]

(In reply to Ryan Hunt [:rhunt] from comment #6)

2. The wasm baseline compiler prologue to allocate a DebugFrame allocates sizeof(Frame) stack space first, followed by offsetof(DebugFrame, frame). This is why that assertion is written the way it is. [2]

Is there a specific reason it's not allocating sizeof(DebugFrame) directly?

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

I think the problem with that would be any padding at the end of sizeof(DebugFrame) would have to be pushed at the beginning of the function prologue before the previous frame pointer and return address, due to stack direction. And that's not possible when the return address is pushed as part of the call instruction.

So really my previous statement was too loose. sizeof(Frame) is allocated as a side effect of the wasm calling convention first, then the offsetof(DebugFrame, frame_) is allocated after that when in debug. And that normally works, except for alignment in this case.

Comment 9

[Lars T Hansen [:lth]]

Gonna start marking all mips errors as p5.

Comment 10

[Lars T Hansen [:lth]]

No longer relevant because MIPS32 wasm support has been removed (bug 1728973).

Comment 11

[Mike Hommey [:glandium]]

Surprisingly... it's actually still happening...

Comment 12

[Lars T Hansen [:lth]]

On mips64? Then I need STR. As discussed earlier, mips32 support has been removed from wasm and will be removed from JS.

I guess we could ifdef the assertion with the supported platforms. (Assert has moved to WasmDebugFrame.cpp.)

Comment 13

[Lars T Hansen [:lth]]

nm, I can repro with --disable-jit and sufficient persistence...

Comment 14

[Lars T Hansen [:lth]]

Attachment: Bug 1687417 - MIPS32 nojit, silence an assert and define a couple stubs. r?yury

A static_assert is failing but this is not relevant for nojit builds,
so silence it.

Two stub functions need to be defined for compilation to succeed,
but they need to be ifdeffed carefully to avoid unused-function
errors on CI.

Comment 15

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2f420f9620ba0b7efd14493300953424d4c73c

Comment 16

[Mike Hommey [:glandium]]

I can confirm the patch works for mips32.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9408195567a2
MIPS32 nojit, silence an assert and define a couple stubs. r=yury

Comment 18

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/9408195567a2

Comment 19

[Marco Trevisan (Treviño)]

Can be please backported to firefox-91 esr branch?

Comment 20

[Lars T Hansen [:lth]]

Based on a quick look at the code, it is only the silencing of the alert that matters? (The other change doesn't appear to have counterpart on ESR91.)

If we're going to uplift we need a reason; are you building ESR for a distro or is this a personal thing?

Comment 21

[Sam James]

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

Based on a quick look at the code, it is only the silencing of the alert that matters? (The other change doesn't appear to have counterpart on ESR91.)

If we're going to uplift we need a reason; are you building ESR for a distro or is this a personal thing?

We're hitting this in Gentoo which results in a failed build on PPC32: https://bugs.gentoo.org/835575.

It looks like openembedded hit it too: https://lists.openembedded.org/g/openembedded-devel/message/93761.

Comment 22

[Lars T Hansen [:lth]]

I'll set up an uplift.

Comment 23

[Lars T Hansen [:lth]]

Attachment: Bug 1687417 - Disable static_assert on NOJIT 32-bit. r?jseward

Backport for ESR91, see original patch for more discussion.

Comment 24

[Lars T Hansen [:lth]]

Comment on attachment 9268617 [details]
Bug 1687417 - Disable static_assert on NOJIT 32-bit. r?jseward

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Pain for distros on various obscure no-jit platforms, see bug for discussions.
• User impact if declined: None, this is strictly on the build / distro side.
• Fix Landed on Version: 94
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Strictly affects no-jit platforms, and then only removes a static_assert - no behavior change at all. This patch is pruned from the patch that landed originally.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9268617 [details]
Bug 1687417 - Disable static_assert on NOJIT 32-bit. r?jseward

No-op for Tier 1 platforms but helpful for ones that don't have the jit enabled. Approved for 91.8esr.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/93640595d8e5

Comment 27

[Sam James]

Thank you!