Merge pull request #956 from vext01/compile-error-propagation

Compile error propagation

ykrt/Cargo.toml

@@ -22,6 +22,7 @@ strum_macros = "0.26.1"
 yktracec = { path = "../yktracec" }
 static_assertions = "1.1.0"
 typed-index-collections = "3.1.0"
+thiserror = "1.0.56"
 
 [dependencies.llvm-sys]
 # note: using a git version to get llvm linkage features in llvm-sys (not in a

ykrt/src/compile/jitc_llvm.rs

@@ -2,7 +2,7 @@
 //! to be compiled with LLVM.
 
 use crate::{
-    compile::{CompiledTrace, Compiler},
+    compile::{CompilationError, CompiledTrace, Compiler},
     location::HotLocation,
     mt::{SideTraceInfo, MT},
     trace::TracedAOTBlock,
@@ -13,7 +13,6 @@ use parking_lot::Mutex;
 use std::os::unix::io::AsRawFd;
 use std::{
     env,
-    error::Error,
     ffi::{c_char, c_int},
     ptr,
     sync::{Arc, LazyLock},
@@ -36,7 +35,7 @@ impl Compiler for JITCLLVM {
         irtrace: Vec<TracedAOTBlock>,
         sti: Option<SideTraceInfo>,
         hl: Arc<Mutex<HotLocation>>,
-    ) -> Result<CompiledTrace, Box<dyn Error>> {
+    ) -> Result<CompiledTrace, CompilationError> {
         let (func_names, bbs, trace_len) = self.encode_trace(&irtrace);
 
         let llvmbc = llvmbc_section();
@@ -62,16 +61,19 @@ impl Compiler for JITCLLVM {
             )
         };
         if ret.is_null() {
-            Err("Could not compile trace.".into())
+            // The LLVM backend is now legacy code and is pending deletion, so it's not worth us
+            // spending time auditing all of the failure modes and categorising them into
+            // recoverable/temporary. So for now we say any error is temporary.
+            Err(CompilationError::Temporary("llvm backend error".into()))
         } else {
             Ok(CompiledTrace::new(mt, ret, di_tmp, Arc::downgrade(&hl)))
         }
     }
 }
 
 impl JITCLLVM {
-    pub(crate) fn new() -> Result<Arc<Self>, Box<dyn Error>> {
-        Ok(Arc::new(JITCLLVM))
+    pub(crate) fn new() -> Arc<Self> {
+        Arc::new(JITCLLVM)
     }
 
     fn encode_trace(&self, irtrace: &Vec<TracedAOTBlock>) -> (Vec<*const i8>, Vec<usize>, usize) {

ykrt/src/compile/jitc_yk/aot_ir.rs

@@ -3,9 +3,10 @@
 //! This is a parser for the on-disk (in the ELF binary) IR format used to express the
 //! (immutable) ahead-of-time compiled interpreter.
 
+use super::CompilationError;
 use byteorder::{NativeEndian, ReadBytesExt};
 use deku::prelude::*;
-use std::{cell::RefCell, error::Error, ffi::CStr, fs, io::Cursor, path::PathBuf};
+use std::{cell::RefCell, ffi::CStr, fs, io::Cursor, path::PathBuf};
 use typed_index_collections::TiVec;
 
 /// A magic number that all bytecode payloads begin with.
@@ -609,6 +610,7 @@ pub(crate) struct FuncType {
 }
 
 impl FuncType {
+    #[cfg(debug_assertions)]
     pub(crate) fn num_args(&self) -> usize {
         self.arg_ty_idxs.len()
     }
@@ -789,13 +791,19 @@ impl Module {
         *self.var_names_computed.borrow_mut() = true;
     }
 
-    pub(crate) fn func_idx(&self, find_func: &str) -> Option<FuncIdx> {
+    /// Find a function by its name.
+    ///
+    /// # Panics
+    ///
+    /// Panics if no function exists with that name.
+    pub(crate) fn func_idx(&self, find_func: &str) -> FuncIdx {
         // OPT: create a cache in the Module.
         self.funcs
             .iter()
             .enumerate()
             .find(|(_, f)| f.name == find_func)
             .map(|(f_idx, _)| FuncIdx(f_idx))
+            .unwrap()
     }
 
     /// Look up a `FuncType` by its index.
@@ -804,6 +812,7 @@ impl Module {
     ///
     /// Panics if the type index is either out of bounds, or the corresponding type is not a
     /// function type.
+    #[cfg(debug_assertions)]
     pub(crate) fn func_ty(&self, func_idx: FuncIdx) -> &FuncType {
         match self.types[self.funcs[func_idx].type_idx] {
             Type::Func(ref ft) => &ft,
@@ -882,21 +891,21 @@ impl Module {
 }
 
 /// Deserialise an AOT module from the slice `data`.
-pub(crate) fn deserialise_module(data: &[u8]) -> Result<Module, Box<dyn Error>> {
+pub(crate) fn deserialise_module(data: &[u8]) -> Result<Module, CompilationError> {
     match Module::from_bytes((data, 0)) {
         Ok(((_, _), mut modu)) => {
             modu.compute_local_operand_func_indices();
             Ok(modu)
         }
-        Err(e) => Err(e.to_string().into()),
+        Err(e) => Err(CompilationError::Unrecoverable(e.to_string())),
     }
 }
 
 /// Deserialise and print IR from an on-disk file.
 ///
 /// Used for support tooling (in turn used by tests too).
-pub fn print_from_file(path: &PathBuf) -> Result<(), Box<dyn Error>> {
-    let data = fs::read(path)?;
+pub fn print_from_file(path: &PathBuf) -> Result<(), CompilationError> {
+    let data = fs::read(path).map_err(|e| CompilationError::Unrecoverable(e.to_string()))?;
     let ir = deserialise_module(&data)?;
     println!("{}", ir.to_str());
     Ok(())

ykrt/src/compile/jitc_yk/jit_ir.rs

@@ -6,6 +6,8 @@
 // FIXME: eventually delete.
 #![allow(dead_code)]
 
+use crate::compile::CompilationError;
+
 use super::aot_ir;
 use std::{fmt, mem, ptr};
 
@@ -52,13 +54,20 @@ impl U24 {
     }
 }
 
+/// Helper to create index overflow errors.
+fn index_overflow(typ: &str) -> CompilationError {
+    CompilationError::Temporary(format!("index overflow: {}", typ))
+}
+
 // Generate common methods for 24-bit index types.
 macro_rules! index_24bit {
     ($struct:ident) => {
         impl $struct {
             /// Convert an AOT index to a reduced-size JIT index (if possible).
-            pub(crate) fn from_aot(aot_idx: aot_ir::$struct) -> $struct {
-                Self(U24::from_usize(usize::from(aot_idx)).unwrap()) // FIXME: propagate error
+            pub(crate) fn from_aot(aot_idx: aot_ir::$struct) -> Result<$struct, CompilationError> {
+                U24::from_usize(usize::from(aot_idx))
+                    .ok_or(index_overflow(stringify!($struct)))
+                    .map(|u| Self(u))
             }
 
             /// Convert a JIT index to an AOT index.
@@ -73,8 +82,10 @@ macro_rules! index_24bit {
 macro_rules! index_16bit {
     ($struct:ident) => {
         impl $struct {
-            pub(crate) fn new(v: usize) -> Self {
-                Self(u16::try_from(v).unwrap()) // FIXME: propagate error
+            pub(crate) fn new(v: usize) -> Result<Self, CompilationError> {
+                u16::try_from(v)
+                    .map_err(|_| index_overflow(stringify!($struct)))
+                    .map(|u| Self(u))
             }
 
             pub(crate) fn to_u16(&self) -> u16 {
@@ -325,21 +336,21 @@ impl CallInstruction {
         m: &mut Module,
         target: aot_ir::FuncIdx,
         args: &[Operand],
-    ) -> CallInstruction {
+    ) -> Result<CallInstruction, CompilationError> {
         let mut arg1 = PackedOperand::default();
         let mut extra = ExtraArgsIdx::default();
 
         if args.len() >= 1 {
             arg1 = PackedOperand::new(&args[0]);
         }
         if args.len() >= 2 {
-            extra = m.push_extra_args(&args[1..]);
+            extra = m.push_extra_args(&args[1..])?;
         }
-        Self {
-            target: FuncIdx::from_aot(target),
+        Ok(Self {
+            target: FuncIdx::from_aot(target)?,
             arg1,
             extra,
-        }
+        })
     }
 
     fn arg1(&self) -> PackedOperand {
@@ -352,19 +363,19 @@ impl CallInstruction {
     /// It is undefined behaviour to provide an out-of-bounds index.
     pub(crate) fn operand(
         &self,
-        aot_mod: &aot_ir::Module,
+        _aot_mod: &aot_ir::Module,
         jit_mod: &Module,
         idx: usize,
     ) -> Option<Operand> {
         #[cfg(debug_assertions)]
         {
-            let ft = aot_mod.func_ty(self.target.into_aot());
+            let ft = _aot_mod.func_ty(self.target.into_aot());
             debug_assert!(ft.num_args() > idx);
         }
         if idx == 0 {
             Some(self.arg1().get())
         } else {
-            Some(jit_mod.extra_args[usize::try_from(self.extra.0).unwrap() + idx - 1].clone())
+            Some(jit_mod.extra_args[usize::from(self.extra.0) + idx - 1].clone())
         }
     }
 }
@@ -423,26 +434,26 @@ impl Module {
     }
 
     /// Push a slice of extra arguments into the extra arg table.
-    fn push_extra_args(&mut self, ops: &[Operand]) -> ExtraArgsIdx {
+    fn push_extra_args(&mut self, ops: &[Operand]) -> Result<ExtraArgsIdx, CompilationError> {
         let idx = self.extra_args.len();
         self.extra_args.extend_from_slice(ops); // FIXME: this clones.
-        ExtraArgsIdx(u16::try_from(idx).unwrap()) // FIXME: propagate error
+        ExtraArgsIdx::new(idx)
     }
 
     /// Push a new constant into the constant table and return its index.
-    pub(crate) fn push_const(&mut self, constant: Constant) -> ConstIdx {
+    pub(crate) fn push_const(&mut self, constant: Constant) -> Result<ConstIdx, CompilationError> {
         let idx = self.consts.len();
         self.consts.push(constant);
-        ConstIdx(u16::try_from(idx).unwrap()) // FIXME: propagate error
+        ConstIdx::new(idx)
     }
 
     /// Get the index of a type, inserting it in the type table if necessary.
-    pub fn const_idx(&mut self, c: &Constant) -> ConstIdx {
+    pub fn const_idx(&mut self, c: &Constant) -> Result<ConstIdx, CompilationError> {
         // FIXME: can we optimise this?
         for (idx, tc) in self.consts.iter().enumerate() {
             if tc == c {
                 // const table hit.
-                return ConstIdx(u16::try_from(idx).unwrap()); // FIXME: propagate error
+                return ConstIdx::new(idx);
             }
         }
         // type table miss, we need to insert it.
@@ -532,7 +543,7 @@ mod tests {
             Operand::Local(InstrIdx(1)), // first extra arg
             Operand::Local(InstrIdx(2)),
         ];
-        let ci = CallInstruction::new(&mut jit_mod, aot_func_idx, &args);
+        let ci = CallInstruction::new(&mut jit_mod, aot_func_idx, &args).unwrap();
 
         assert_eq!(
             ci.operand(&aot_mod, &jit_mod, 0),
@@ -571,9 +582,9 @@ mod tests {
             Operand::Local(InstrIdx(1)), // first extra arg
             Operand::Local(InstrIdx(2)),
         ];
-        let ci = CallInstruction::new(&mut jit_mod, aot_func_idx, &args);
+        let ci = CallInstruction::new(&mut jit_mod, aot_func_idx, &args).unwrap();
 
-        ci.operand(&aot_mod, &jit_mod, 3);
+        ci.operand(&aot_mod, &jit_mod, 3).unwrap();
     }
 
     #[test]
@@ -599,4 +610,37 @@ mod tests {
         assert_eq!(U24::from_usize(0x123456).unwrap().to_usize(), 0x123456);
         assert_eq!(U24::from_usize(0xffffff).unwrap().to_usize(), 0xffffff);
     }
+
+    #[test]
+    fn index24_fits() {
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(0)).is_ok());
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(1)).is_ok());
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(0x1234)).is_ok());
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(0x123456)).is_ok());
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(0xffffff)).is_ok());
+    }
+
+    #[test]
+    fn index24_doesnt_fit() {
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(0x1000000)).is_err());
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(0x1234567)).is_err());
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(0xeddedde)).is_err());
+        assert!(TypeIdx::from_aot(aot_ir::TypeIdx::new(usize::MAX)).is_err());
+    }
+
+    #[test]
+    fn index16_fits() {
+        assert!(ExtraArgsIdx::new(0).is_ok());
+        assert!(ExtraArgsIdx::new(1).is_ok());
+        assert!(ExtraArgsIdx::new(0x1234).is_ok());
+        assert!(ExtraArgsIdx::new(0xffff).is_ok());
+    }
+
+    #[test]
+    fn index16_doesnt_fit() {
+        assert!(ExtraArgsIdx::new(0x10000).is_err());
+        assert!(ExtraArgsIdx::new(0x12345).is_err());
+        assert!(ExtraArgsIdx::new(0xffffff).is_err());
+        assert!(ExtraArgsIdx::new(usize::MAX).is_err());
+    }
 }

ykrt/src/compile/jitc_yk/mod.rs

@@ -1,5 +1,6 @@
 //! Yk's built-in trace compiler.
 
+use super::CompilationError;
 use crate::{
     compile::{CompiledTrace, Compiler},
     location::HotLocation,
@@ -10,7 +11,6 @@ use parking_lot::Mutex;
 use std::{
     collections::HashSet,
     env,
-    error::Error,
     ffi::CString,
     slice,
     sync::{Arc, LazyLock},
@@ -25,14 +25,16 @@ enum IRPhase {
 }
 
 impl IRPhase {
-    fn from_str(s: &str) -> Result<Self, String> {
-        let ret = match s {
-            "aot" => Self::AOT,
-            "jit-pre-opt" => Self::PreOpt,
-            "jit-post-opt" => Self::PostOpt,
-            _ => return Err(format!("Invalid YKD_PRINT_IR value: {}", s)),
-        };
-        Ok(ret)
+    fn from_str(s: &str) -> Result<Self, CompilationError> {
+        match s {
+            "aot" => Ok(Self::AOT),
+            "jit-pre-opt" => Ok(Self::PreOpt),
+            "jit-post-opt" => Ok(Self::PostOpt),
+            _ => Err(CompilationError::Unrecoverable(format!(
+                "Invalid YKD_PRINT_IR value: {}",
+                s
+            ))),
+        }
     }
 }
 
@@ -61,11 +63,11 @@ impl Compiler for JITCYk {
         mtrace: Vec<TracedAOTBlock>,
         sti: Option<SideTraceInfo>,
         _hl: Arc<Mutex<HotLocation>>,
-    ) -> Result<CompiledTrace, Box<dyn Error>> {
+    ) -> Result<CompiledTrace, CompilationError> {
         if sti.is_some() {
             todo!();
         }
-        let ir_slice = yk_ir_section();
+        let ir_slice = yk_ir_section()?;
         // FIXME: Cache deserialisation, so we don't load it afresh each time.
         let aot_mod = aot_ir::deserialise_module(ir_slice)?;
 
@@ -88,13 +90,17 @@ impl Compiler for JITCYk {
 }
 
 impl JITCYk {
-    pub(crate) fn new() -> Result<Arc<Self>, Box<dyn Error>> {
+    pub(crate) fn new() -> Result<Arc<Self>, CompilationError> {
         Ok(Arc::new(Self {}))
     }
 }
 
-pub(crate) fn yk_ir_section() -> &'static [u8] {
-    let start = symbol_vaddr(&CString::new("ykllvm.yk_ir.start").unwrap()).unwrap();
-    let stop = symbol_vaddr(&CString::new("ykllvm.yk_ir.stop").unwrap()).unwrap();
-    unsafe { slice::from_raw_parts(start as *const u8, stop - start) }
+pub(crate) fn yk_ir_section() -> Result<&'static [u8], CompilationError> {
+    let start = symbol_vaddr(&CString::new("ykllvm.yk_ir.start").unwrap()).ok_or(
+        CompilationError::Unrecoverable("couldn't find ykllvm.yk_ir.start".into()),
+    )?;
+    let stop = symbol_vaddr(&CString::new("ykllvm.yk_ir.stop").unwrap()).ok_or(
+        CompilationError::Unrecoverable("couldn't find ykllvm.yk_ir.stop".into()),
+    )?;
+    Ok(unsafe { slice::from_raw_parts(start as *const u8, stop - start) })
 }