Closed Bug 1576969 Opened 5 years ago Closed 5 years ago

thread '<unnamed>' panicked at 'assertion failed: `(left == right)` left: `68`, right: `64`: Invalid registers for REX-less Op1 encoding', third_party/rust/cranelift-codegen/src/isa/x86/binemit.rs:75:5

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- disabled
firefox71 --- fixed

People

(Reporter: gkw, Assigned: jseward)

References

(Regression)

Details

(6 keywords, Whiteboard: [jsbugmon:][post-critsmash-triage])

Attachments

(4 files)

The following testcase crashes on mozilla-central revision 993cf82f6bbf (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion --wasm-compiler=cranelift 374.wrapper 374.wasm):

See attachment.

Backtrace:

#0  mozalloc_abort (msg=<optimized out>) at memory/mozalloc/mozalloc_abort.cpp:33
#1  0x0000562f63aface0 in abort () at memory/mozalloc/mozalloc_abort.cpp:79
#2  0x0000562f6501e4d7 in panic_abort::__rust_start_panic::abort () at src/libpanic_abort/lib.rs:49
#3  0x0000562f6501e4c6 in __rust_start_panic () at src/libpanic_abort/lib.rs:45
#4  0x0000562f6500dfb9 in rust_panic () at src/libstd/panicking.rs:526
#5  0x0000562f6500dedc in std::panicking::rust_panic_with_hook () at src/libstd/panicking.rs:497
#6  0x0000562f6500d922 in std::panicking::continue_panic_fmt () at src/libstd/panicking.rs:384
#7  0x0000562f6500d86f in std::panicking::begin_panic_fmt () at src/libstd/panicking.rs:339
#8  0x0000562f64f8969b in cranelift_codegen::isa::x86::binemit::put_op1 (bits=<optimized out>, rex=<optimized out>, sink=<optimized out>) at third_party/rust/cranelift-codegen/src/isa/x86/binemit.rs:75
#9  0x0000562f64f8d3c5 in cranelift_codegen::isa::x86::binemit::emit_inst (func=0x7fee9415a020, inst=..., divert=0x7ffd42d9dd18, sink=0x7ffd42d9de40, isa=...) at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-x86_64-993cf82f6bbf/objdir-js/x86_64-unknown-linux-gnu/debug/build/cranelift-codegen-3099c906cb900757/out/binemit-x86.rs:2250
#10 0x0000562f64f7f47a in cranelift_codegen::binemit::emit_function (func=0x7fee9415a020, emit_inst=<optimized out>, sink=<optimized out>, isa=...) at third_party/rust/cranelift-codegen/src/binemit/mod.rs:179
#11 0x0000562f64f34150 in cranelift_codegen::context::Context::emit_to_memory (self=0x7fee9478a8b0 <_IO_stdfile_2_lock>, isa=..., mem=<optimized out>, relocs=..., traps=..., stackmaps=...) at third_party/rust/cranelift-codegen/src/context.rs:185
#12 0x0000562f64d6c04a in baldrdash::compile::BatchCompiler::binemit (self=0x7fee9415a000, info=...) at js/src/wasm/cranelift/src/compile.rs:189
/snip

For detailed crash information, see attachment.

Setting s-s as a start as I'm not sure how bad this memory allocator/rust panic issue in Cranelift is.

Component: JavaScript Engine → Javascript: WebAssembly

Pernosco trace of a --enable-debug --disable-optimize build:

https://pernos.co/debug/fJUl3FqdrlVeKapMJPCNbA/index.html

(Please comment on whether the Pernosco recording - rr on the cloud helps you in debugging this issue!)

autobisectjs is running now, so not sure who to needinfo? yet.

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/8d66ae95b161
user: Benjamin Bouvier
date: Mon Aug 26 10:18:17 2019 +0000
summary: Bug 1576591: Bump Cranelift to 164f91a1f473e582e18e48d056c51787d9a1c24d; r=jseward

Benjamin, is bug 1576591 a likely regressor?

Flags: needinfo?(bbouvier)
Regressed by: 1576591

Great find. Note that the Cranelift bump covers many Cranelift commits, but it's definitely one of these that caused this.

(Please comment on whether the Pernosco recording - rr on the cloud helps you in debugging this issue!)

It is useful! It allowed me to look at the backtrace, see that the recipe being emitted is umr_reg_to_ssa, introduced by Julian for RRR. The issue here is that the src register (which is an immediate operand in the instruction) is 15, so r15, which requires a REX prefix. So we need to change the emit function of this recipe, so it inserts a rex prefix when needed.

Julian, can you take a look, please?

Flags: needinfo?(bbouvier) → needinfo?(jseward)

From poking at this in gdb, I can see that the // Recipe Op1umr_reg_to_ssa case in isa::x86::binemit::emit_inst needs to call put_rexop1 rather than put_op1 as as present. Looking further, it seems that the text {{PUT_OP}} in the recipe for umr_reg_to_ssa should be substituted with put_rexop1 instead of put_op1. But I couldn't figure out how to do that.

I tried adding a trailing .rex() at the points in encodings.rs where the recipe is used, eg changing

  e.enc_i32_i64(copy_to_ssa, rec_umr_reg_to_ssa.opcodes(vec![0x89]));

to

  e.enc_i32_i64(copy_to_ssa, rec_umr_reg_to_ssa.opcodes(vec![0x89]).rex());

as one sees in various other places in this file. But that doesn't help either, and I see that enc_i32_i64 instantiates the template with .rex() and .rex().w() anyway. So I suspect I'm looking in the wrong place. Benjamin, any suggestions?

Flags: needinfo?(jseward) → needinfo?(bbouvier)
Keywords: sec-high

(Regarding security evaluation: this is Cranelift only, which is disabled by default and Nightly-only.)

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

From poking at this in gdb, I can see that the // Recipe Op1umr_reg_to_ssa case in isa::x86::binemit::emit_inst needs to call put_rexop1 rather than put_op1 as as present. Looking further, it seems that the text {{PUT_OP}} in the recipe for umr_reg_to_ssa should be substituted with put_rexop1 instead of put_op1. But I couldn't figure out how to do that.

I tried adding a trailing .rex() at the points in encodings.rs where the recipe is used, eg changing

  e.enc_i32_i64(copy_to_ssa, rec_umr_reg_to_ssa.opcodes(vec![0x89]));

to

  e.enc_i32_i64(copy_to_ssa, rec_umr_reg_to_ssa.opcodes(vec![0x89]).rex());

as one sees in various other places in this file. But that doesn't help either, and I see that enc_i32_i64 instantiates the template with .rex() and .rex().w() anyway. So I suspect I'm looking in the wrong place. Benjamin, any suggestions?

Yeah, I think the relaxation phase sees that it could use the nonrex variant, since the only register involved in constraints is the output one, and it selects it. Instead, we might need to expand the call to enc_i32_i64, repeating everything but the enc64 I32 nonrex line. There'll always be a REX prefix even when it's not needed, but it seems less hacky than manual dispatching in the emit function. Can you try this?

Flags: needinfo?(bbouvier) → needinfo?(jseward)

Benjamin, your proposal works. (Thanks!) However, to make it work
consistently for all types, I had to make _rex_only clones not only for
enc_i32_i64 but also for enc_both and enc_r32_r64. The resulting
patch is attached. What do you think?

Assignee: nobody → jseward
Flags: needinfo?(jseward) → needinfo?(bbouvier)

Yes, it's not pretty but it's not your fault :). Thanks. (Could you please remove the encoding for r32 on x64, as you noted on IRC?)

Flags: needinfo?(bbouvier)
Status: NEW → ASSIGNED
Priority: -- → P1

This is, we believe, fixed in upstream Cranelift, now that https://github.com/CraneStation/cranelift/pull/966 has landed.

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

This is, we believe, fixed in upstream Cranelift, now that https://github.com/CraneStation/cranelift/pull/966 has landed.

Thus, I guess to fix this, we need another import of Cranelift into SpiderMonkey?

Flags: needinfo?(jseward)
Flags: needinfo?(bbouvier)

Yes indeed. There's a couple more patches I'd like to see in Cranelift before bumping, but we could do it now if that's high priority for you.

Flags: needinfo?(bbouvier)

While this isn't a hard blocker per se, funbind fuzzing + cranelift is pretty much hitting this all the time.

Ok, I'll try to bump today.

Flags: needinfo?(jseward)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [jsbugmon:] → [jsbugmon:][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: