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)
Tracking
()
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.
Reporter | ||
Comment 1•5 years ago
|
||
Reporter | ||
Comment 2•5 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 3•5 years ago
|
||
Reporter | ||
Comment 4•5 years ago
|
||
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.
Reporter | ||
Comment 5•5 years ago
|
||
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?
Comment 6•5 years ago
|
||
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?
Assignee | ||
Comment 7•5 years ago
|
||
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?
Comment 8•5 years ago
|
||
(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 inisa::x86::binemit::emit_inst
needs to callput_rexop1
rather thanput_op1
as as present. Looking further, it seems that the text{{PUT_OP}}
in the recipe forumr_reg_to_ssa
should be substituted withput_rexop1
instead ofput_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 changinge.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?
Assignee | ||
Comment 9•5 years ago
|
||
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?
Comment 10•5 years ago
|
||
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?)
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
This is, we believe, fixed in upstream Cranelift, now that https://github.com/CraneStation/cranelift/pull/966 has landed.
Reporter | ||
Comment 12•5 years ago
|
||
(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?
Comment 13•5 years ago
|
||
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.
Reporter | ||
Comment 14•5 years ago
|
||
While this isn't a hard blocker per se, funbind fuzzing + cranelift is pretty much hitting this all the time.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•4 years ago
|
Description
•