From ea04aa5b9876171eeb389c77fb6a34831c08db1b Mon Sep 17 00:00:00 2001 From: Andrew Brown Date: Sat, 16 Nov 2019 11:42:17 -0800 Subject: [PATCH] Improve error messages received from Cranelift (#583) As discussed in https://github.com/bytecodealliance/cranelift/pull/1226, the context of Cranelift errors is lost after exiting the scope containing the Cranelift function. `CodegenError` then only contains something like `inst2: arg 0 (v4) has type i16x8, expected i8x16`, which is rarely enough information for investigating a codegen failure. This change uses Cranelift's `pretty_error` function to improve the error messages wrapped in `CompileError`; `CompileError` has lost the reference to `CodegenError` due to `pretty_error` taking ownership but this seems preferable since no backtrace is attached and losing the pretty-printed context would be worse (if `CodegenError` gains a `Backtrace` or implements `Clone` we can revisit this). --- crates/api/src/trampoline/func.rs | 2 ++ crates/environ/src/compilation.rs | 6 +++--- crates/environ/src/cranelift.rs | 33 +++++++++++++++++++++++-------- crates/jit/src/compiler.rs | 9 ++++++++- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/crates/api/src/trampoline/func.rs b/crates/api/src/trampoline/func.rs index a2272a16d2..de3b4b2e3d 100644 --- a/crates/api/src/trampoline/func.rs +++ b/crates/api/src/trampoline/func.rs @@ -7,6 +7,7 @@ use alloc::{boxed::Box, rc::Rc, string::ToString, vec::Vec}; use anyhow::Result; use core::cmp; use cranelift_codegen::ir::{types, InstBuilder, StackSlotData, StackSlotKind, TrapCode}; +use cranelift_codegen::print_errors::pretty_error; use cranelift_codegen::{binemit, ir, isa, Context}; use cranelift_entity::{EntityRef, PrimaryMap}; use cranelift_frontend::{FunctionBuilder, FunctionBuilderContext}; @@ -174,6 +175,7 @@ fn make_trampoline( &mut trap_sink, &mut stackmap_sink, ) + .map_err(|error| pretty_error(&context.func, Some(isa), error)) .expect("compile_and_emit"); let mut unwind_info = Vec::new(); diff --git a/crates/environ/src/compilation.rs b/crates/environ/src/compilation.rs index acb349ac83..c0da59ffbf 100644 --- a/crates/environ/src/compilation.rs +++ b/crates/environ/src/compilation.rs @@ -5,7 +5,7 @@ use crate::address_map::{ModuleAddressMap, ValueLabelsRanges}; use crate::module; use crate::module_environ::FunctionBodyData; use alloc::vec::Vec; -use cranelift_codegen::{binemit, ir, isa, CodegenError}; +use cranelift_codegen::{binemit, ir, isa}; use cranelift_entity::PrimaryMap; use cranelift_wasm::{DefinedFuncIndex, FuncIndex, ModuleTranslationState, WasmError}; use serde::{Deserialize, Serialize}; @@ -156,8 +156,8 @@ pub enum CompileError { Wasm(#[from] WasmError), /// A compilation error occured. - #[error("Compilation error")] - Codegen(#[from] CodegenError), + #[error("Compilation error: {0}")] + Codegen(String), /// A compilation error occured. #[error("Debug info is not supported with this configuration")] diff --git a/crates/environ/src/cranelift.rs b/crates/environ/src/cranelift.rs index f4503f6cd9..b48925f86b 100644 --- a/crates/environ/src/cranelift.rs +++ b/crates/environ/src/cranelift.rs @@ -19,6 +19,7 @@ use cranelift_codegen::binemit; use cranelift_codegen::ir; use cranelift_codegen::ir::ExternalName; use cranelift_codegen::isa; +use cranelift_codegen::print_errors::pretty_error; use cranelift_codegen::Context; use cranelift_entity::PrimaryMap; use cranelift_wasm::{DefinedFuncIndex, FuncIndex, FuncTranslator, ModuleTranslationState}; @@ -239,13 +240,21 @@ impl crate::compilation::Compiler for Cranelift { let mut reloc_sink = RelocSink::new(func_index); let mut trap_sink = TrapSink::new(); let mut stackmap_sink = binemit::NullStackmapSink {}; - context.compile_and_emit( - isa, - &mut code_buf, - &mut reloc_sink, - &mut trap_sink, - &mut stackmap_sink, - )?; + context + .compile_and_emit( + isa, + &mut code_buf, + &mut reloc_sink, + &mut trap_sink, + &mut stackmap_sink, + ) + .map_err(|error| { + CompileError::Codegen(pretty_error( + &context.func, + Some(isa), + error, + )) + })?; context.emit_unwind_info(isa, &mut unwind_info); @@ -257,7 +266,15 @@ impl crate::compilation::Compiler for Cranelift { }; let ranges = if generate_debug_info { - Some(context.build_value_labels_ranges(isa)?) + let ranges = + context.build_value_labels_ranges(isa).map_err(|error| { + CompileError::Codegen(pretty_error( + &context.func, + Some(isa), + error, + )) + })?; + Some(ranges) } else { None }; diff --git a/crates/jit/src/compiler.rs b/crates/jit/src/compiler.rs index 993ba050e6..cbe25f219d 100644 --- a/crates/jit/src/compiler.rs +++ b/crates/jit/src/compiler.rs @@ -10,6 +10,7 @@ use alloc::vec::Vec; use core::convert::TryFrom; use cranelift_codegen::ir::InstBuilder; use cranelift_codegen::isa::{TargetFrontendConfig, TargetIsa}; +use cranelift_codegen::print_errors::pretty_error; use cranelift_codegen::Context; use cranelift_codegen::{binemit, ir}; use cranelift_entity::{EntityRef, PrimaryMap}; @@ -336,7 +337,13 @@ fn make_trampoline( &mut trap_sink, &mut stackmap_sink, ) - .map_err(|error| SetupError::Compile(CompileError::Codegen(error)))?; + .map_err(|error| { + SetupError::Compile(CompileError::Codegen(pretty_error( + &context.func, + Some(isa), + error, + ))) + })?; context.emit_unwind_info(isa, &mut unwind_info);