1507730 - Assertion failure: Code(reg_) < Registers::Total, at /js/src/jit/Registers.h:62

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

The following test case (reduced from awsm) found a crash in x86 Ion codegen:

new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(`
(module
    (func (param i64))
    (func (export "f")
        i64.const 2
        i64.const -9223372036854775808
        i64.mul
        call 0
    )
)
`))).exports.f();

Lowering and codegen aren't properly sync'd, causing a invalid register to be used. Not sec critical, because this means eax is used in place of the temp register, and eax *is* a fixed register input for this operation anyway, so this just implies correctness issue. Would be nice to have uplifted though.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9025594 [details] [diff] [review]
fix.patch

Oh great, same bug on ARM32.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98b78581ff76
Generate a temporary for negative power-of-two constants in mul64 on 32 bits platforms; r=lth

Comment 5

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/98b78581ff76

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. Also, can you please provide some context for the tracking request? It's not clear to me what the impact of this bug is for our users.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9025599 [details] [diff] [review]
fix.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: wasm

User impact if declined: Incorrect behavior in wasm code on 32 bits platforms.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Very small patch.

String changes made/needed: 

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 9025599 [details] [diff] [review]
fix.patch

wasm fix for 32bit x86 and arm, approved for 64.0b11 and 60.4.0esr

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/dd0f01818b9c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3910d176d5c2