Skip to content

Commit 1f2f290

Browse files
committed
x64: Fix encoding of RIP-relative addressing
This commit fixes a bug in the new assembler which was surfaced through the changes in bytecodealliance#10782 but was a pre-existing issue. Specifically the encoding of a RIP-relative addressing mode required knowing the number of bytes at the end of an instruction but this was accidentally hardcoded to 0. In bytecodealliance#10782 `imul` instructions were added where a RIP-relative address mode can be used in conjunction with an immediate which cause the RIP-relative addressing to load from the wrong address. This bug can in theory affect other instructions in the new assembler as well, but auditing the list of instructions it looks like `imul` is the only one that can possibly have an immediate after a RIP-relative addressing mode. That means that prior instructions using the new assembler should not be affected.
1 parent b226c23 commit 1f2f290

File tree

3 files changed

+94
-2
lines changed

3 files changed

+94
-2
lines changed

cranelift/assembler-x64/meta/src/generate/format.rs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,10 @@ impl dsl::Format {
146146
f.empty_line();
147147
f.comment("Emit ModR/M byte.");
148148
}
149+
let bytes_at_end = match self.operands_by_kind().as_slice() {
150+
[.., Imm(imm)] => imm.bytes(),
151+
_ => 0,
152+
};
149153

150154
match self.operands_by_kind().as_slice() {
151155
[FixedReg(_)] | [FixedReg(_), FixedReg(_)] | [FixedReg(_), Imm(_)] => {
@@ -163,7 +167,10 @@ impl dsl::Format {
163167
| [FixedReg(_), FixedReg(_), RegMem(mem)] => {
164168
let digit = rex.digit.unwrap();
165169
fmtln!(f, "let digit = 0x{digit:x};");
166-
fmtln!(f, "self.{mem}.encode_rex_suffixes(buf, off, digit, 0);");
170+
fmtln!(
171+
f,
172+
"self.{mem}.encode_rex_suffixes(buf, off, digit, {bytes_at_end});"
173+
);
167174
}
168175
[Reg(reg), RegMem(mem)]
169176
| [Reg(reg), RegMem(mem), Imm(_)]
@@ -172,7 +179,10 @@ impl dsl::Format {
172179
| [RegMem(mem), Reg(reg), Imm(_)]
173180
| [RegMem(mem), Reg(reg), FixedReg(_)] => {
174181
fmtln!(f, "let reg = self.{reg}.enc();");
175-
fmtln!(f, "self.{mem}.encode_rex_suffixes(buf, off, reg, 0);");
182+
fmtln!(
183+
f,
184+
"self.{mem}.encode_rex_suffixes(buf, off, reg, {bytes_at_end});"
185+
);
176186
}
177187
unknown => unimplemented!("unknown pattern: {unknown:?}"),
178188
}

cranelift/filetests/filetests/isa/x64/mul.clif

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -547,3 +547,67 @@ block0(v0: i8, v1: i8):
547547
; popq %rbp
548548
; retq
549549

550+
function %imul_and_constant_pool_small_immediate() -> i32 {
551+
block0:
552+
v0 = iconst.i32 7
553+
v1 = iconst.i32 0x22222222
554+
v2 = imul v0, v1
555+
return v2
556+
}
557+
558+
; VCode:
559+
; pushq %rbp
560+
; movq %rsp, %rbp
561+
; block0:
562+
; imull $0x7, (%rip), %eax
563+
; movq %rbp, %rsp
564+
; popq %rbp
565+
; ret
566+
;
567+
; Disassembled:
568+
; block0: ; offset 0x0
569+
; pushq %rbp
570+
; movq %rsp, %rbp
571+
; block1: ; offset 0x4
572+
; imull $7, 6(%rip), %eax
573+
; movq %rbp, %rsp
574+
; popq %rbp
575+
; retq
576+
; andb (%rdx), %ah
577+
; andb (%rdx), %ah
578+
; addb %al, (%rax)
579+
; addb %al, (%rax)
580+
581+
function %imul_and_constant_pool_big_immediate() -> i32 {
582+
block0:
583+
v0 = iconst.i32 0x11111111
584+
v1 = iconst.i32 0x22222222
585+
v2 = imul v0, v1
586+
return v2
587+
}
588+
589+
; VCode:
590+
; pushq %rbp
591+
; movq %rsp, %rbp
592+
; block0:
593+
; imull $0x11111111, (%rip), %eax
594+
; movq %rbp, %rsp
595+
; popq %rbp
596+
; ret
597+
;
598+
; Disassembled:
599+
; block0: ; offset 0x0
600+
; pushq %rbp
601+
; movq %rsp, %rbp
602+
; block1: ; offset 0x4
603+
; imull $0x11111111, 0xe(%rip), %eax
604+
; movq %rbp, %rsp
605+
; popq %rbp
606+
; retq
607+
; addb %al, (%rax)
608+
; addb %al, (%rax)
609+
; addb %ah, (%rdx)
610+
; andb (%rdx), %ah
611+
; andb (%rax), %al
612+
; addb %al, (%rax)
613+

cranelift/filetests/filetests/runtests/arithmetic.clif

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,3 +556,21 @@ block0(v0: i8):
556556
; run: %udiv_i8_const(0) == 0
557557
; run: %udiv_i8_const(-1) == 1
558558
; run: %udiv_i8_const(0xFE) == 1
559+
560+
function %imul_small_constant() -> i32 {
561+
block0:
562+
v0 = iconst.i32 7
563+
v1 = iconst.i32 0x22222222
564+
v2 = imul v0, v1
565+
return v2
566+
}
567+
; run: %imul_small_constant() == -286331154
568+
569+
function %imul_big_constant() -> i32 {
570+
block0:
571+
v0 = iconst.i32 0x11111111
572+
v1 = iconst.i32 0x22222222
573+
v2 = imul v0, v1
574+
return v2
575+
}
576+
; run: %imul_big_constant() == 248153666

0 commit comments

Comments
 (0)