Fix s390x regressions (#3330)

- Add relocation handling needed after PR #3275 - Fix incorrect handling of signed constants detected by PR #3056 test - Fix LabelUse max pos/neg ranges; fix overflow in buffers.rs - Disable fuzzing tests that require pre-built v8 binaries - Disable cranelift test that depends on i128 - Temporarily disable memory64 tests
bytecodealliance · Sep 20, 2021 · 51131a3 · 51131a3
1 parent 9eae88a
commit 51131a3
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 22 deletions.
diff --git a/build.rs b/build.rs
@@ -201,6 +201,8 @@ fn ignore(testsuite: &str, testname: &str, strategy: &str) -> bool {
             ("reference_types", _) if cfg!(feature = "old-x86-backend") => return true,
             // No simd support yet for s390x.
             ("simd", _) if platform_is_s390x() => return true,
+            // No memory64 support yet for s390x.
+            ("memory64", _) if platform_is_s390x() => return true,
             _ => {}
         },
         _ => panic!("unrecognized strategy"),

diff --git a/cranelift/codegen/src/isa/s390x/inst/mod.rs b/cranelift/codegen/src/isa/s390x/inst/mod.rs
@@ -3607,6 +3607,9 @@ pub enum LabelUse {
     /// 32-bit PC relative constant offset (from address of constant itself),
     /// signed. Used in jump tables.
     PCRel32,
+    /// 32-bit PC relative constant offset (from address of call instruction),
+    /// signed. Offset is imm << 1.  Used for call relocations.
+    PCRel32Dbl,
 }
 
 impl MachInstLabelUse for LabelUse {
@@ -3617,21 +3620,29 @@ impl MachInstLabelUse for LabelUse {
     fn max_pos_range(self) -> CodeOffset {
         match self {
             // 16-bit signed immediate, left-shifted by 1.
-            LabelUse::BranchRI => (1 << 20) - 1,
-            // This can address any valid CodeOffset.
-            LabelUse::BranchRIL => 0x7fff_ffff,
+            LabelUse::BranchRI => ((1 << 15) - 1) << 1,
+            // 32-bit signed immediate, left-shifted by 1.
+            LabelUse::BranchRIL => 0xffff_fffe,
+            // 32-bit signed immediate.
             LabelUse::PCRel32 => 0x7fff_ffff,
+            // 32-bit signed immediate, left-shifted by 1, offset by 2.
+            LabelUse::PCRel32Dbl => 0xffff_fffc,
         }
     }
 
     /// Maximum PC-relative range (negative).
     fn max_neg_range(self) -> CodeOffset {
         match self {
             // 16-bit signed immediate, left-shifted by 1.
-            LabelUse::BranchRI => 1 << 20,
-            // This can address any valid CodeOffset.
-            LabelUse::BranchRIL => 0x8000_0000,
+            LabelUse::BranchRI => (1 << 15) << 1,
+            // 32-bit signed immediate, left-shifted by 1.
+            // NOTE: This should be 4GB, but CodeOffset is only u32.
+            LabelUse::BranchRIL => 0xffff_ffff,
+            // 32-bit signed immediate.
             LabelUse::PCRel32 => 0x8000_0000,
+            // 32-bit signed immediate, left-shifted by 1, offset by 2.
+            // NOTE: This should be 4GB + 2, but CodeOffset is only u32.
+            LabelUse::PCRel32Dbl => 0xffff_ffff,
         }
     }
 
@@ -3641,6 +3652,7 @@ impl MachInstLabelUse for LabelUse {
             LabelUse::BranchRI => 4,
             LabelUse::BranchRIL => 6,
             LabelUse::PCRel32 => 4,
+            LabelUse::PCRel32Dbl => 4,
         }
     }
 
@@ -3664,6 +3676,11 @@ impl MachInstLabelUse for LabelUse {
                 let insn_word = insn_word.wrapping_add(pc_rel as u32);
                 buffer[0..4].clone_from_slice(&u32::to_be_bytes(insn_word));
             }
+            LabelUse::PCRel32Dbl => {
+                let insn_word = u32::from_be_bytes([buffer[0], buffer[1], buffer[2], buffer[3]]);
+                let insn_word = insn_word.wrapping_add((pc_rel_shifted + 1) as u32);
+                buffer[0..4].clone_from_slice(&u32::to_be_bytes(insn_word));
+            }
         }
     }
 
@@ -3687,7 +3704,10 @@ impl MachInstLabelUse for LabelUse {
         unreachable!();
     }
 
-    fn from_reloc(_reloc: Reloc, _addend: Addend) -> Option<Self> {
-        None
+    fn from_reloc(reloc: Reloc, addend: Addend) -> Option<Self> {
+        match (reloc, addend) {
+            (Reloc::S390xPCRel32Dbl, 2) => Some(LabelUse::PCRel32Dbl),
+            _ => None,
+        }
     }
 }
diff --git a/cranelift/codegen/src/isa/s390x/lower.rs b/cranelift/codegen/src/isa/s390x/lower.rs
@@ -61,6 +61,16 @@ fn input_matches_const<C: LowerCtx<I = Inst>>(ctx: &mut C, input: InsnInput) ->
     input.constant
 }
 
+/// Lower an instruction input to a 64-bit signed constant, if possible.
+fn input_matches_sconst<C: LowerCtx<I = Inst>>(ctx: &mut C, input: InsnInput) -> Option<i64> {
+    if let Some(imm) = input_matches_const(ctx, input) {
+        let ty = ctx.input_ty(input.insn, input.input);
+        Some(sign_extend_to_u64(imm, ty_bits(ty) as u8) as i64)
+    } else {
+        None
+    }
+}
+
 /// Return false if instruction input cannot have the value Imm, true otherwise.
 fn input_maybe_imm<C: LowerCtx<I = Inst>>(ctx: &mut C, input: InsnInput, imm: u64) -> bool {
     if let Some(c) = input_matches_const(ctx, input) {
@@ -79,8 +89,8 @@ fn input_maybe_imm<C: LowerCtx<I = Inst>>(ctx: &mut C, input: InsnInput, imm: u6
 
 /// Lower an instruction input to a 16-bit signed constant, if possible.
 fn input_matches_simm16<C: LowerCtx<I = Inst>>(ctx: &mut C, input: InsnInput) -> Option<i16> {
-    if let Some(imm_value) = input_matches_const(ctx, input) {
-        if let Ok(imm) = i16::try_from(imm_value as i64) {
+    if let Some(imm_value) = input_matches_sconst(ctx, input) {
+        if let Ok(imm) = i16::try_from(imm_value) {
             return Some(imm);
         }
     }
@@ -89,8 +99,8 @@ fn input_matches_simm16<C: LowerCtx<I = Inst>>(ctx: &mut C, input: InsnInput) ->
 
 /// Lower an instruction input to a 32-bit signed constant, if possible.
 fn input_matches_simm32<C: LowerCtx<I = Inst>>(ctx: &mut C, input: InsnInput) -> Option<i32> {
-    if let Some(imm_value) = input_matches_const(ctx, input) {
-        if let Ok(imm) = i32::try_from(imm_value as i64) {
+    if let Some(imm_value) = input_matches_sconst(ctx, input) {
+        if let Ok(imm) = i32::try_from(imm_value) {
             return Some(imm);
         }
     }
@@ -112,8 +122,8 @@ fn negated_input_matches_simm16<C: LowerCtx<I = Inst>>(
     ctx: &mut C,
     input: InsnInput,
 ) -> Option<i16> {
-    if let Some(imm_value) = input_matches_const(ctx, input) {
-        if let Ok(imm) = i16::try_from(-(imm_value as i64)) {
+    if let Some(imm_value) = input_matches_sconst(ctx, input) {
+        if let Ok(imm) = i16::try_from(-imm_value) {
             return Some(imm);
         }
     }
@@ -125,8 +135,8 @@ fn negated_input_matches_simm32<C: LowerCtx<I = Inst>>(
     ctx: &mut C,
     input: InsnInput,
 ) -> Option<i32> {
-    if let Some(imm_value) = input_matches_const(ctx, input) {
-        if let Ok(imm) = i32::try_from(-(imm_value as i64)) {
+    if let Some(imm_value) = input_matches_sconst(ctx, input) {
+        if let Ok(imm) = i32::try_from(-imm_value) {
             return Some(imm);
         }
     }

diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs
@@ -597,7 +597,7 @@ impl<I: VCodeInst> MachBuffer<I> {
             self.island_worst_case_size += kind.veneer_size();
             self.island_worst_case_size &= !(I::LabelUse::ALIGN - 1);
         }
-        let deadline = offset + kind.max_pos_range();
+        let deadline = offset.saturating_add(kind.max_pos_range());
         if deadline < self.island_deadline {
             self.island_deadline = deadline;
         }

diff --git a/cranelift/filetests/filetests/runtests/i128-concat-split.clif b/cranelift/filetests/filetests/runtests/i128-concat-split.clif
@@ -1,7 +1,6 @@
 test interpret
 test run
 target aarch64
-target s390x
 target x86_64 machinst
 
 function %iconcat_isplit(i64, i64) -> i64, i64 {

diff --git a/crates/fuzzing/Cargo.toml b/crates/fuzzing/Cargo.toml
@@ -25,8 +25,8 @@ wasmi = "0.7.0"
 # We rely on precompiled v8 binaries, but rusty-v8 doesn't have a precompiled
 # binary for MinGW which is built on our CI. It does have one for Windows-msvc,
 # though, so we could use that if we wanted. For now though just simplify a bit
-# and don't depend on this on Windows.
-[target.'cfg(not(windows))'.dependencies]
+# and don't depend on this on Windows.  The same applies on s390x.
+[target.'cfg(not(any(windows, target_arch = "s390x")))'.dependencies]
 rusty_v8 = "0.27"
 
 [dev-dependencies]

diff --git a/crates/fuzzing/src/oracles.rs b/crates/fuzzing/src/oracles.rs
@@ -21,9 +21,9 @@ use std::time::{Duration, Instant};
 use wasmtime::*;
 use wasmtime_wast::WastContext;
 
-#[cfg(not(windows))]
+#[cfg(not(any(windows, target_arch = "s390x")))]
 pub use v8::*;
-#[cfg(not(windows))]
+#[cfg(not(any(windows, target_arch = "s390x")))]
 mod v8;
 
 static CNT: AtomicUsize = AtomicUsize::new(0);