Remove duplication of map_reg; fixes #1245

Both cranelift-codegen and wasmtime-debug need to map Cranelift registers to Gimli registers. Previously both crates had an almost-identical `map_reg` implementation. This change:
 - removes the wasmtime-debug implementation
 - improves the cranelift-codegen implementation with custom errors
 - exposes map_reg in `cranelift_codegen::isa::fde::map_reg` and subsequently `wasmtime_environ::isa::fde::map_reg`

cranelift/codegen/src/isa/mod.rs

@@ -74,6 +74,12 @@ mod riscv;
 #[cfg(feature = "x86")]
 mod x86;
 
+#[cfg(all(feature = "x86", feature = "unwind"))]
+/// Expose the register-mapping functionality necessary for exception handling, debug, etc.
+pub mod fde {
+    pub use super::x86::map_reg;
+}
+
 #[cfg(feature = "arm32")]
 mod arm32;
 

cranelift/codegen/src/isa/x86/fde.rs

@@ -10,6 +10,7 @@ use gimli::write::{
     FrameDescriptionEntry, FrameTable, Result, Writer,
 };
 use gimli::{Encoding, Format, LittleEndian, Register, X86_64};
+use thiserror::Error;
 
 pub type FDERelocEntry = (FrameUnwindOffset, Reloc);
 
@@ -74,8 +75,15 @@ fn return_address_reg(isa: &dyn TargetIsa) -> Register {
     X86_64::RA
 }
 
-fn map_reg(isa: &dyn TargetIsa, reg: RegUnit) -> Register {
+/// Map Cranelift registers to their corresponding Gimli registers.
-    assert!(isa.name() == "x86" && isa.pointer_bits() == 64);
+pub fn map_reg(
+    isa: &dyn TargetIsa,
+    reg: RegUnit,
+) -> core::result::Result<Register, RegisterMappingError> {
+    if isa.name() != "x86" || isa.pointer_bits() != 64 {
+        return Err(RegisterMappingError::UnsupportedArchitecture);
+    }
+
     // Mapping from https://github.com/bytecodealliance/cranelift/pull/902 by @iximeow
     const X86_GP_REG_MAP: [gimli::Register; 16] = [
         X86_64::RAX,
@@ -113,21 +121,32 @@ fn map_reg(isa: &dyn TargetIsa, reg: RegUnit) -> Register {
         X86_64::XMM14,
         X86_64::XMM15,
     ];
+
     let reg_info = isa.register_info();
-    let bank = reg_info.bank_containing_regunit(reg).unwrap();
+    let bank = reg_info
+        .bank_containing_regunit(reg)
+        .ok_or_else(|| RegisterMappingError::MissingBank)?;
     match bank.name {
         "IntRegs" => {
             // x86 GP registers have a weird mapping to DWARF registers, so we use a
             // lookup table.
-            X86_GP_REG_MAP[(reg - bank.first_unit) as usize]
+            Ok(X86_GP_REG_MAP[(reg - bank.first_unit) as usize])
-        }
-        "FloatRegs" => X86_XMM_REG_MAP[(reg - bank.first_unit) as usize],
-        _ => {
-            panic!("unsupported register bank: {}", bank.name);
         }
+        "FloatRegs" => Ok(X86_XMM_REG_MAP[(reg - bank.first_unit) as usize]),
+        _ => Err(RegisterMappingError::UnsupportedRegisterBank(bank.name)),
     }
 }
 
+#[derive(Error, Debug)]
+pub enum RegisterMappingError {
+    #[error("unable to find bank for register info")]
+    MissingBank,
+    #[error("register mapping is currently only implemented for x86_64")]
+    UnsupportedArchitecture,
+    #[error("unsupported register bank: {0}")]
+    UnsupportedRegisterBank(&'static str),
+}
+
 fn to_cfi(
     isa: &dyn TargetIsa,
     change: &FrameLayoutChange,
@@ -136,7 +155,7 @@ fn to_cfi(
 ) -> Option<CallFrameInstruction> {
     Some(match change {
         FrameLayoutChange::CallFrameAddressAt { reg, offset } => {
-            let mapped = map_reg(isa, *reg);
+            let mapped = map_reg(isa, *reg).expect("a register mapping from cranelift to gimli");
             let offset = (*offset) as i32;
             if mapped != *cfa_def_reg && offset != *cfa_def_offset {
                 *cfa_def_reg = mapped;
@@ -155,7 +174,7 @@ fn to_cfi(
         FrameLayoutChange::RegAt { reg, cfa_offset } => {
             assert!(cfa_offset % -8 == 0);
             let cfa_offset = *cfa_offset as i32;
-            let mapped = map_reg(isa, *reg);
+            let mapped = map_reg(isa, *reg).expect("a register mapping from cranelift to gimli");
             CallFrameInstruction::Offset(mapped, cfa_offset)
         }
         FrameLayoutChange::ReturnAddressAt { cfa_offset } => {

cranelift/codegen/src/isa/x86/mod.rs

@@ -10,6 +10,9 @@ pub mod settings;
 #[cfg(feature = "unwind")]
 mod unwind;
 
+#[cfg(feature = "unwind")]
+pub use fde::map_reg;
+
 use super::super::settings as shared_settings;
 #[cfg(feature = "testing_hooks")]
 use crate::binemit::CodeSink;

crates/debug/src/frame.rs

@@ -1,6 +1,6 @@
-use crate::transform::map_reg;
 use std::collections::HashMap;
 use wasmtime_environ::entity::EntityRef;
+use wasmtime_environ::isa::fde::map_reg;
 use wasmtime_environ::isa::{CallConv, TargetIsa};
 use wasmtime_environ::wasm::DefinedFuncIndex;
 use wasmtime_environ::{FrameLayoutChange, FrameLayouts};

crates/debug/src/transform/expression.rs

@@ -1,11 +1,11 @@
 use super::address_transform::AddressTransform;
-use super::map_reg::map_reg;
 use anyhow::{Context, Error, Result};
 use gimli::{self, write, Expression, Operation, Reader, ReaderOffset, X86_64};
 use more_asserts::{assert_le, assert_lt};
 use std::collections::{HashMap, HashSet};
 use wasmtime_environ::entity::EntityRef;
 use wasmtime_environ::ir::{StackSlots, ValueLabel, ValueLabelsRanges, ValueLoc};
+use wasmtime_environ::isa::fde::map_reg;
 use wasmtime_environ::isa::TargetIsa;
 use wasmtime_environ::wasm::{get_vmctx_value_label, DefinedFuncIndex};
 use wasmtime_environ::ModuleMemoryOffset;

crates/debug/src/transform/map_reg.rs

@@ -1,58 +0,0 @@
-use anyhow::{bail, Result};
-use gimli::{Register, X86_64};
-use wasmtime_environ::isa::{RegUnit, TargetIsa};
-
-pub(crate) fn map_reg(isa: &dyn TargetIsa, reg: RegUnit) -> Result<Register> {
-    // TODO avoid duplication with fde.rs
-    assert!(isa.name() == "x86" && isa.pointer_bits() == 64);
-    // Mapping from https://github.com/bytecodealliance/cranelift/pull/902 by @iximeow
-    const X86_GP_REG_MAP: [Register; 16] = [
-        X86_64::RAX,
-        X86_64::RCX,
-        X86_64::RDX,
-        X86_64::RBX,
-        X86_64::RSP,
-        X86_64::RBP,
-        X86_64::RSI,
-        X86_64::RDI,
-        X86_64::R8,
-        X86_64::R9,
-        X86_64::R10,
-        X86_64::R11,
-        X86_64::R12,
-        X86_64::R13,
-        X86_64::R14,
-        X86_64::R15,
-    ];
-    const X86_XMM_REG_MAP: [Register; 16] = [
-        X86_64::XMM0,
-        X86_64::XMM1,
-        X86_64::XMM2,
-        X86_64::XMM3,
-        X86_64::XMM4,
-        X86_64::XMM5,
-        X86_64::XMM6,
-        X86_64::XMM7,
-        X86_64::XMM8,
-        X86_64::XMM9,
-        X86_64::XMM10,
-        X86_64::XMM11,
-        X86_64::XMM12,
-        X86_64::XMM13,
-        X86_64::XMM14,
-        X86_64::XMM15,
-    ];
-    let reg_info = isa.register_info();
-    let bank = reg_info.bank_containing_regunit(reg).unwrap();
-    match bank.name {
-        "IntRegs" => {
-            // x86 GP registers have a weird mapping to DWARF registers, so we use a
-            // lookup table.
-            Ok(X86_GP_REG_MAP[(reg - bank.first_unit) as usize])
-        }
-        "FloatRegs" => Ok(X86_XMM_REG_MAP[(reg - bank.first_unit) as usize]),
-        bank_name => {
-            bail!("unsupported register bank: {}", bank_name);
-        }
-    }
-}

crates/debug/src/transform/mod.rs

@@ -14,13 +14,11 @@ use wasmtime_environ::isa::TargetIsa;
 use wasmtime_environ::{ModuleAddressMap, ModuleVmctxInfo, ValueLabelsRanges};
 
 pub use address_transform::AddressTransform;
-pub(crate) use map_reg::map_reg;
 
 mod address_transform;
 mod attr;
 mod expression;
 mod line_program;
-mod map_reg;
 mod range_info_builder;
 mod refs;
 mod simulate;

crates/environ/src/data_structures.rs

@@ -14,6 +14,9 @@ pub mod settings {
 
 pub mod isa {
     pub use cranelift_codegen::isa::{CallConv, RegUnit, TargetFrontendConfig, TargetIsa};
+    pub mod fde {
+        pub use cranelift_codegen::isa::fde::map_reg;
+    }
 }
 
 pub mod entity {