From 7f109a51987fa03f8fbacf80358cb40358c363d8 Mon Sep 17 00:00:00 2001
From: Benjamin Bouvier <public@benj.me>
Date: Tue, 28 Jul 2020 11:07:16 +0200
Subject: [PATCH] machinst x64: use a sign-extension when loading jump table
 offsets;

The jump table offset that's loaded out of the jump table could be
signed (if it's an offset to before the jump table itself), so we should
use a signed extension there, not an unsigned extension.
---
 cranelift/codegen/src/isa/x64/inst/emit.rs | 7 ++++---
 cranelift/codegen/src/isa/x64/inst/mod.rs  | 1 +
 cranelift/codegen/src/isa/x64/lower.rs     | 9 +++++++--
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/cranelift/codegen/src/isa/x64/inst/emit.rs b/cranelift/codegen/src/isa/x64/inst/emit.rs
index 7194ef92ab..91fd5a1176 100644
--- a/cranelift/codegen/src/isa/x64/inst/emit.rs
+++ b/cranelift/codegen/src/isa/x64/inst/emit.rs
@@ -1462,7 +1462,7 @@ pub(crate) fn emit(
             // jnb $default_target
             // movl %idx, %tmp2
             // lea start_of_jump_table_offset(%rip), %tmp1
-            // movzlq [%tmp1, %tmp2], %tmp2
+            // movslq [%tmp1, %tmp2, 4], %tmp2 ;; shift of 2, viz. multiply index by 4
             // addq %tmp2, %tmp1
             // j *%tmp1
             // $start_of_jump_table:
@@ -1485,8 +1485,9 @@ pub(crate) fn emit(
             );
             inst.emit(sink, flags, state);
 
-            // Load value out of jump table.
-            let inst = Inst::movzx_rm_r(
+            // Load value out of the jump table. It's a relative offset to the target block, so it
+            // might be negative; use a sign-extension.
+            let inst = Inst::movsx_rm_r(
                 ExtMode::LQ,
                 RegMem::mem(Amode::imm_reg_reg_shift(0, tmp1.to_reg(), tmp2.to_reg(), 2)),
                 *tmp2,
diff --git a/cranelift/codegen/src/isa/x64/inst/mod.rs b/cranelift/codegen/src/isa/x64/inst/mod.rs
index 0558c3f98e..9fcabaaf0f 100644
--- a/cranelift/codegen/src/isa/x64/inst/mod.rs
+++ b/cranelift/codegen/src/isa/x64/inst/mod.rs
@@ -350,6 +350,7 @@ pub enum Inst {
     /// Jump-table sequence, as one compound instruction (see note in lower.rs for rationale).
     /// The generated code sequence is described in the emit's function match arm for this
     /// instruction.
+    /// See comment in lowering about the temporaries signedness.
     JmpTableSeq {
         idx: Reg,
         tmp1: Writable<Reg>,
diff --git a/cranelift/codegen/src/isa/x64/lower.rs b/cranelift/codegen/src/isa/x64/lower.rs
index 9f1f4f4f1b..11f6e59392 100644
--- a/cranelift/codegen/src/isa/x64/lower.rs
+++ b/cranelift/codegen/src/isa/x64/lower.rs
@@ -1989,8 +1989,13 @@ impl LowerBackend for X64Backend {
                     // is to introduce a relocation pass for inlined jumptables, which is much
                     // worse.)
 
-                    let tmp1 = ctx.alloc_tmp(RegClass::I64, I32);
-                    let tmp2 = ctx.alloc_tmp(RegClass::I64, I32);
+                    // This temporary is used as a signed integer of 64-bits (to hold addresses).
+                    let tmp1 = ctx.alloc_tmp(RegClass::I64, I64);
+                    // This temporary is used as a signed integer of 32-bits (for the wasm-table
+                    // index) and then 64-bits (address addend). The small lie about the I64 type
+                    // is benign, since the temporary is dead after this instruction (and its
+                    // Cranelift type is thus unused).
+                    let tmp2 = ctx.alloc_tmp(RegClass::I64, I64);
 
                     let targets_for_term: Vec<MachLabel> = targets.to_vec();
                     let default_target = BranchTarget::Label(targets[0]);