Fix mis-aligned access issues with s390x (#4702)

This fixes two problems: minimum symbol alignment for the LARL
instruction, and alignment requirements for LRL/LGRL etc.

The first problem is that the LARL instruction used to load a
symbol address (PC relative) requires that the target symbol
is at least 2-byte aligned.  This is always guaranteed for code
symbols (all instructions must be 2-aligned anyway), but not
necessarily for data symbols.

Other s390x compilers fix this problem by ensuring that all
global symbols are always emitted with a minimum 2-byte
alignment.  This patch introduces an equivalent mechanism
for cranelift:
- Add a symbol_alignment routine to TargetIsa, similar to the
  existing code_section_alignment routine.
- Respect symbol_alignment as minimum alignment for all symbols
  emitted in the object backend (code and data).

The second problem is that PC-relative instructions that
directly *access* data (like LRL/LGRL, STRL/STGRL etc.)
not only have the 2-byte requirement like LARL, but actually
require that their memory operand is *naturally* aligned
(i.e. alignment is at least the size of the access).

This property (natural alignment for memory accesses) is
supposed to be provided by the "aligned" flag in MemFlags;
however, this is not implemented correctly at the moment.

To fix this, this patch:
- Only emits PC-relative memory access instructions if the
  "aligned" flag is set in the associated MemFlags.
- Fixes a bug in emit_small_memory_copy and emit_small_memset
  which currently set the aligned flag unconditionally, ignoring
  the actual alignment info passed by their caller.

Tested with wasmtime and cg_clif.

cranelift/codegen/src/isa/mod.rs

@@ -312,6 +312,15 @@ impl<'a> dyn TargetIsa + 'a {
         }
     }
 
+    /// Returns the minimum symbol alignment for this ISA.
+    pub fn symbol_alignment(&self) -> u64 {
+        match self.triple().architecture {
+            // All symbols need to be aligned to at least 2 on s390x.
+            Architecture::S390x => 2,
+            _ => 1,
+        }
+    }
+
     /// Get the pointer type of this ISA.
     pub fn pointer_type(&self) -> ir::Type {
         ir::Type::int(self.pointer_bits() as u16).unwrap()

cranelift/codegen/src/isa/s390x/inst/emit.rs

@@ -11,17 +11,28 @@ use crate::trace;
 use core::convert::TryFrom;
 use regalloc2::Allocation;
 
+/// Type(s) of memory instructions available for mem_finalize.
+pub struct MemInstType {
+    /// True if 12-bit unsigned displacement is supported.
+    pub have_d12: bool,
+    /// True if 20-bit signed displacement is supported.
+    pub have_d20: bool,
+    /// True if PC-relative addressing is supported (memory access).
+    pub have_pcrel: bool,
+    /// True if PC-relative addressing is supported (load address).
+    pub have_unaligned_pcrel: bool,
+    /// True if an index register is supported.
+    pub have_index: bool,
+}
+
 /// Memory addressing mode finalization: convert "special" modes (e.g.,
 /// generic arbitrary stack offset) into real addressing modes, possibly by
 /// emitting some helper instructions that come immediately before the use
 /// of this amode.
 pub fn mem_finalize(
     mem: &MemArg,
     state: &EmitState,
-    have_d12: bool,
-    have_d20: bool,
-    have_pcrel: bool,
-    have_index: bool,
+    mi: MemInstType,
 ) -> (SmallVec<[Inst; 4]>, MemArg) {
     let mut insts = SmallVec::new();
 
@@ -70,9 +81,10 @@ pub fn mem_finalize(
 
     // If this addressing mode cannot be handled by the instruction, use load-address.
     let need_load_address = match &mem {
-        &MemArg::Label { .. } | &MemArg::Symbol { .. } if !have_pcrel => true,
-        &MemArg::BXD20 { .. } if !have_d20 => true,
-        &MemArg::BXD12 { index, .. } | &MemArg::BXD20 { index, .. } if !have_index => {
+        &MemArg::Label { .. } | &MemArg::Symbol { .. } if !mi.have_pcrel => true,
+        &MemArg::Symbol { flags, .. } if !mi.have_unaligned_pcrel && !flags.aligned() => true,
+        &MemArg::BXD20 { .. } if !mi.have_d20 => true,
+        &MemArg::BXD12 { index, .. } | &MemArg::BXD20 { index, .. } if !mi.have_index => {
             index != zero_reg()
         }
         _ => false,
@@ -93,8 +105,8 @@ pub fn mem_finalize(
             index,
             disp,
             flags,
-        } if !have_d12 => {
-            assert!(have_d20);
+        } if !mi.have_d12 => {
+            assert!(mi.have_d20);
             MemArg::BXD20 {
                 base,
                 index,
@@ -122,10 +134,13 @@ pub fn mem_emit(
     let (mem_insts, mem) = mem_finalize(
         mem,
         state,
-        opcode_rx.is_some(),
-        opcode_rxy.is_some(),
-        opcode_ril.is_some(),
-        true,
+        MemInstType {
+            have_d12: opcode_rx.is_some(),
+            have_d20: opcode_rxy.is_some(),
+            have_pcrel: opcode_ril.is_some(),
+            have_unaligned_pcrel: opcode_ril.is_some() && !add_trap,
+            have_index: true,
+        },
     );
     for inst in mem_insts.into_iter() {
         inst.emit(&[], sink, emit_info, state);
@@ -190,10 +205,13 @@ pub fn mem_rs_emit(
     let (mem_insts, mem) = mem_finalize(
         mem,
         state,
-        opcode_rs.is_some(),
-        opcode_rsy.is_some(),
-        false,
-        false,
+        MemInstType {
+            have_d12: opcode_rs.is_some(),
+            have_d20: opcode_rsy.is_some(),
+            have_pcrel: false,
+            have_unaligned_pcrel: false,
+            have_index: false,
+        },
     );
     for inst in mem_insts.into_iter() {
         inst.emit(&[], sink, emit_info, state);
@@ -236,7 +254,17 @@ pub fn mem_imm8_emit(
     emit_info: &EmitInfo,
     state: &mut EmitState,
 ) {
-    let (mem_insts, mem) = mem_finalize(mem, state, true, true, false, false);
+    let (mem_insts, mem) = mem_finalize(
+        mem,
+        state,
+        MemInstType {
+            have_d12: true,
+            have_d20: true,
+            have_pcrel: false,
+            have_unaligned_pcrel: false,
+            have_index: false,
+        },
+    );
     for inst in mem_insts.into_iter() {
         inst.emit(&[], sink, emit_info, state);
     }
@@ -274,7 +302,17 @@ pub fn mem_imm16_emit(
     emit_info: &EmitInfo,
     state: &mut EmitState,
 ) {
-    let (mem_insts, mem) = mem_finalize(mem, state, true, false, false, false);
+    let (mem_insts, mem) = mem_finalize(
+        mem,
+        state,
+        MemInstType {
+            have_d12: true,
+            have_d20: false,
+            have_pcrel: false,
+            have_unaligned_pcrel: false,
+            have_index: false,
+        },
+    );
     for inst in mem_insts.into_iter() {
         inst.emit(&[], sink, emit_info, state);
     }
@@ -336,7 +374,17 @@ pub fn mem_vrx_emit(
     emit_info: &EmitInfo,
     state: &mut EmitState,
 ) {
-    let (mem_insts, mem) = mem_finalize(mem, state, true, false, false, true);
+    let (mem_insts, mem) = mem_finalize(
+        mem,
+        state,
+        MemInstType {
+            have_d12: true,
+            have_d20: false,
+            have_pcrel: false,
+            have_unaligned_pcrel: false,
+            have_index: true,
+        },
+    );
     for inst in mem_insts.into_iter() {
         inst.emit(&[], sink, emit_info, state);
     }