1855960 - MIPS: change `MacroAssembler::storeUncanonicalized{Float32,Double} to return a `FaultingCodeOffset`.

Status: RESOLVED FIXED

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

Description

[Julian Seward [:jseward]]

Without this change, the build will fail, due to the assembler changes made by
bug 1846474 (D187462).

Comment 1

[Jiangjin Wang]

Attachment: Bug 1855960 - [MIPS64] Make some assembler routines return FaultingCodeOffset. r=jseward

Comment 2

[Zhao Jiazhong]

Attachment: mips64_trapsite.diff

I also just finish the patch today, may could be a reference for you about the SummarizeTrapInstruction part.

Comment 3

[Julian Seward [:jseward]]

(In reply to Zhao Jiazhong from comment #2)

Created attachment 9365788 [details] [diff] [review]
mips64_trapsite.diff

Thanks for the patch. Although it is needed for compilation, it will have
no effect until you enable Trapsite checking for MIPS at
https://searchfox.org/mozilla-central/source/js/src/wasm/WasmGenerator.cpp#1077
(in ModuleGenerator::finishCodeTier). When you enable that, and run the
jit-tests, you may get some incorrect Trapsites detected, and you will need to fix those.

Comment 4

[Jiangjin Wang]

(In reply to Zhao Jiazhong from comment #2)

Created attachment 9365788 [details] [diff] [review]
mips64_trapsite.diff

I also just finish the patch today, may could be a reference for you about the SummarizeTrapInstruction part.

Thanks for the patch! I only did the FaultingCodeOffset part because I'm not familiar with MIPS instruction encoding. The FaultingCodeOffset fix alone is enough to get Firefox 120.0 building on MIPS64 (I build packages for AOSC OS). Your SummarizeTrapInstruction implementation is extremely helpful for me.

(In reply to Julian Seward [:jseward] from comment #3)

Thanks for the patch. Although it is needed for compilation, it will have
no effect until you enable Trapsite checking for MIPS at
https://searchfox.org/mozilla-central/source/js/src/wasm/WasmGenerator.cpp#1077
(in ModuleGenerator::finishCodeTier).

I believe his patch already includes that.

When you enable that, and run the
jit-tests, you may get some incorrect Trapsites detected, and you will need to fix those.

I'll try to do that and maybe submit the SummarizeTrapInstruction part in a separate review.

Comment 5

[Julian Seward [:jseward]]

(In reply to Jiangjin Wang from comment #4)

Thanks for the patch! I only did the FaultingCodeOffset part because I'm not familiar with MIPS instruction encoding. The FaultingCodeOffset fix alone is enough to get Firefox 120.0 building on MIPS64 (I build packages for AOSC OS). Your SummarizeTrapInstruction implementation is extremely helpful for me.

I should explain .. the TrapSite checking machinery (in ModuleGenerator::finishCodeTier)
is needed to detect incorrect FaultingCodeOffsets that the assembler layers create.
In short, all FaultingCodeOffsets must point to an instruction that can actually
fault (SIGSEGV or SIGILL); if that happens, SpiderMonkey catches the signal and
continues. If these FaultingCodeOffsets are incorrect, these signals will not be
caught and so can cause a tab crash in Firefox.

The checking machinery uses SummariseTrapInstruction to look at each instruction
pointed to by a FaultingCodeOffset, to check that it really will trap. This has
found bugs in our macroassembler stack. So you need to enable it to have the
most reliable wasm support on MIPS.

Thanks for the patch. Although it is needed for compilation, it will have
no effect until you enable Trapsite checking for MIPS at
https://searchfox.org/mozilla-central/source/js/src/wasm/WasmGenerator.cpp#1077
(in ModuleGenerator::finishCodeTier).

I believe his patch already includes that.

I don't see that in https://phabricator.services.mozilla.com/D194834; did you
mean some other patch?

Comment 6

[Jiangjin Wang]

(In reply to Julian Seward [:jseward] from comment #5)

I don't see that in https://phabricator.services.mozilla.com/D194834; did you
mean some other patch?

Yea, I thought you were talking about the patch from Jiazhong's patch in the attachments. I submitted that review before Jiazhong finished his.

As a status update, I just got the review comments addressed and Firefox building with SummarizeTrapInstruction locally. I'm running jit-test now.

Comment 7

[Jiangjin Wang]

Attachment: Bug 1855960 - [MIPS64] Implement SummarizeTrapInstruction. r=jseward

Ref: https://bugzilla.mozilla.org/attachment.cgi?id=9365788&action=diff

Co-Authored-By: Zhao Jiazhong <[e-mail]>

Comment 8

[Mike Hommey [:glandium]]

Note the mips32 code is also affected.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:the, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:rhunt, since the bug has recent activity, could you have a look please?

For more information, please visit BugBot documentation.

Comment 14

[Jiaxun Yang [:flygoat]]

Any chance to get this landed?
:jseward

Comment 15

[Julian Seward [:jseward]]

(In reply to Jiaxun Yang [:flygoat] from comment #14)

Any chance to get this landed?

Do the patches (D195052 and D194834) apply and build successfully on the current mozilla-central code?

Comment 16

[Jiaxun Yang [:flygoat]]

(In reply to Julian Seward [:jseward] from comment #15)

(In reply to Jiaxun Yang [:flygoat] from comment #14)

Any chance to get this landed?

Do the patches (D195052 and D194834) apply and build successfully on the current mozilla-central code?

Yes they still applies with moz-phab and works.

Comment 17

[Julian Seward [:jseward]]

Ok. I will land it after the current soft code freeze ends, on 8 August.

Comment 18

[Mike Hommey [:glandium]]

They don't apply cleanly on current central. Bug 1905058 and bug 1835034 introduced conflicts in the first one and bug 1898153 in the second one.

Comment 19

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #18)

They don't apply cleanly on current central.

One of the patches, D195052, doesn't even build; there is a syntax error
in the definition of macro INSN(..). I fixed it.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b89fcb3c99c7
[MIPS64] Make some assembler routines return FaultingCodeOffset. r=jseward
https://hg.mozilla.org/integration/autoland/rev/5a0b16abb1e7
[MIPS64] Implement SummarizeTrapInstruction. r=jseward

Comment 21

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/b89fcb3c99c7
https://hg.mozilla.org/mozilla-central/rev/5a0b16abb1e7

Comment 22

[Julian Seward [:jseward]]

Attachment: bug1855960-D194834-for-esr128

D194834 as rebased for esr128.

Comment 23

[Julian Seward [:jseward]]

Attachment: bug1855960-D195052-for-esr128

D195052 as rebased for esr128.