From 92fec417643fadbd0044bda9dc66d6e76bda7bd5 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Thu, 20 Jul 2023 04:28:10 +0300 Subject: [PATCH 1/2] Allow `-C debuginfo=2`, but require `-Zinline-mir=off`, for `panic!` `format_args!` removal. --- .../src/builder/builder_methods.rs | 290 ++++++++++++------ .../src/codegen_cx/declare.rs | 32 +- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 13 +- crates/spirv-builder/src/lib.rs | 3 + tests/src/main.rs | 12 +- 5 files changed, 246 insertions(+), 104 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index 430650b482..80415b1a67 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -4,6 +4,7 @@ use crate::builder_spirv::{BuilderCursor, SpirvConst, SpirvValue, SpirvValueExt, use crate::custom_insts::{CustomInst, CustomOp}; use crate::rustc_codegen_ssa::traits::BaseTypeMethods; use crate::spirv_type::SpirvType; +use itertools::Itertools; use rspirv::dr::{InsertPoint, Instruction, Operand}; use rspirv::spirv::{Capability, MemoryModel, MemorySemantics, Op, Scope, StorageClass, Word}; use rustc_apfloat::{ieee, Float, Round, Status}; @@ -24,6 +25,7 @@ use rustc_span::Span; use rustc_target::abi::call::FnAbi; use rustc_target::abi::{Abi, Align, Scalar, Size, WrappingRange}; use smallvec::SmallVec; +use std::cell::Cell; use std::convert::TryInto; use std::iter::{self, empty}; @@ -2411,18 +2413,49 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { // more complex and neither immediate (`fmt::Arguments` is too big) // nor simplified in MIR (e.g. promoted to a constant) in any way, // so we have to try and remove the `fmt::Arguments::new` call here. + #[derive(Default)] + struct DecodedFormatArgs {} + struct FormatArgsNotRecognized(String); + // HACK(eddyb) this is basically a `try` block. - let remove_format_args_if_possible = || -> Option<()> { + let try_decode_and_remove_format_args = || { + let decoded_format_args = DecodedFormatArgs::default(); + + let const_u32_as_usize = |ct_id| match self.builder.lookup_const_by_id(ct_id)? { + SpirvConst::U32(x) => Some(x as usize), + _ => None, + }; + let const_slice_as_elem_ids = |slice_ptr_and_len_ids: &[Word]| { + if let [ptr_id, len_id] = slice_ptr_and_len_ids[..] { + if let SpirvConst::PtrTo { pointee } = + self.builder.lookup_const_by_id(ptr_id)? + { + if let SpirvConst::Composite(elems) = + self.builder.lookup_const_by_id(pointee)? + { + if elems.len() == const_u32_as_usize(len_id)? { + return Some(elems); + } + } + } + } + None + }; + let format_args_id = match args { &[ SpirvValue { kind: SpirvValueKind::Def(format_args_id), .. }, - _, + _, // `&'static panic::Location<'static>` ] => format_args_id, - _ => return None, + _ => { + return Err(FormatArgsNotRecognized( + "panic entry-point call args".into(), + )); + } }; let custom_ext_inst_set_import = self.ext_inst.borrow_mut().import_custom(self); @@ -2446,8 +2479,11 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { .take_while(|inst| inst.class.opcode == Op::Variable) .map(|inst| inst.result_id.unwrap()) .collect(); - let require_local_var = - |ptr_id| Some(()).filter(|()| local_var_ids.contains(&ptr_id)); + let require_local_var = |ptr_id, var| { + Some(()) + .filter(|()| local_var_ids.contains(&ptr_id)) + .ok_or_else(|| FormatArgsNotRecognized(format!("{var} storage not local"))) + }; let mut non_debug_insts = func.blocks[block_idx] .instructions @@ -2474,14 +2510,14 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { Call(ID, ID, SmallVec<[ID; 4]>), } - let mut taken_inst_idx_range = func.blocks[block_idx].instructions.len()..; + let taken_inst_idx_range = Cell::new(func.blocks[block_idx].instructions.len())..; // Take `count` instructions, advancing backwards, but returning // instructions in their original order (and decoded to `Inst`s). let mut try_rev_take = |count| { let maybe_rev_insts = (0..count).map(|_| { let (i, inst) = non_debug_insts.next_back()?; - taken_inst_idx_range = i..; + taken_inst_idx_range.start.set(i); // HACK(eddyb) all instructions accepted below // are expected to take no more than 4 operands, @@ -2519,102 +2555,143 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { insts.reverse(); Some(insts) }; + let fmt_args_new_call_insts = try_rev_take(3).ok_or_else(|| { + FormatArgsNotRecognized( + "fmt::Arguments::new call: ran out of instructions".into(), + ) + })?; + let ((pieces_slice_ptr_id, pieces_len_id), (rt_args_slice_ptr_id, rt_args_count)) = + match fmt_args_new_call_insts[..] { + [ + Inst::Call(call_ret_id, callee_id, ref call_args), + Inst::Store(st_dst_id, st_val_id), + Inst::Load(ld_val_id, ld_src_id), + ] if self.fmt_args_new_fn_ids.borrow().contains(&callee_id) + && call_ret_id == st_val_id + && st_dst_id == ld_src_id + && ld_val_id == format_args_id => + { + require_local_var(st_dst_id, "fmt::Arguments::new destination")?; + + match call_args[..] { + // `::new_v1` + [ + pieces_slice_ptr_id, + pieces_len_id, + rt_args_slice_ptr_id, + rt_args_len_id, + ] => ( + (pieces_slice_ptr_id, pieces_len_id), + ( + Some(rt_args_slice_ptr_id), + const_u32_as_usize(rt_args_len_id).ok_or_else(|| { + FormatArgsNotRecognized( + "fmt::Arguments::new: args.len() not constant" + .into(), + ) + })?, + ), + ), - let (rt_args_slice_ptr_id, rt_args_count) = match try_rev_take(3)?[..] { - [ - Inst::Call(call_ret_id, callee_id, ref call_args), - Inst::Store(st_dst_id, st_val_id), - Inst::Load(ld_val_id, ld_src_id), - ] if self.fmt_args_new_fn_ids.borrow().contains(&callee_id) - && call_ret_id == st_val_id - && st_dst_id == ld_src_id - && ld_val_id == format_args_id => - { - require_local_var(st_dst_id)?; - match call_args[..] { - // `::new_v1` - [_, _, rt_args_slice_ptr_id, rt_args_len_id] => ( - Some(rt_args_slice_ptr_id), - self.builder - .lookup_const_by_id(rt_args_len_id) - .and_then(|ct| match ct { - SpirvConst::U32(x) => Some(x as usize), - _ => None, - })?, - ), - - // `::new_const` - [_, _] => (None, 0), - - _ => return None, + // `::new_const` + [pieces_slice_ptr_id, pieces_len_id] => { + ((pieces_slice_ptr_id, pieces_len_id), (None, 0)) + } + + _ => { + return Err(FormatArgsNotRecognized( + "fmt::Arguments::new call args".into(), + )); + } + } } - } - _ => return None, - }; + _ => { + // HACK(eddyb) this gathers more context before reporting. + let mut insts = fmt_args_new_call_insts; + insts.reverse(); + while let Some(extra_inst) = try_rev_take(1) { + insts.extend(extra_inst); + if insts.len() >= 32 { + break; + } + } + insts.reverse(); + + return Err(FormatArgsNotRecognized(format!( + "fmt::Arguments::new call sequence ({insts:?})", + ))); + } + }; // HACK(eddyb) this is the worst part: if we do have runtime // arguments (from e.g. new `assert!`s being added to `core`), // we have to confirm their many instructions for removal. if rt_args_count > 0 { let rt_args_slice_ptr_id = rt_args_slice_ptr_id.unwrap(); - let rt_args_array_ptr_id = match try_rev_take(1)?[..] { + let rt_args_array_ptr_id = match try_rev_take(1).ok_or_else(|| { + FormatArgsNotRecognized( + "&[fmt::rt::Argument] bitcast: ran out of instructions".into(), + ) + })?[..] + { [Inst::Bitcast(out_id, in_id)] if out_id == rt_args_slice_ptr_id => in_id, - _ => return None, + _ => { + return Err(FormatArgsNotRecognized( + "&[fmt::rt::Argument] bitcast".into(), + )); + } }; - require_local_var(rt_args_array_ptr_id); + require_local_var(rt_args_array_ptr_id, "[fmt::rt::Argument; N]")?; - // Each runtime argument has its own variable, 6 instructions - // to initialize it, and 9 instructions to copy it to the - // appropriate slot in the array. The groups of 6 and 9 + // Each runtime argument has 3 instructions to call one of + // the `fmt::rt::Argument::new_*` functions (and split its + // scalar pair result), and 5 instructions to store it into + // the appropriate slot in the array. The groups of 3 and 5 // instructions, for all runtime args, are each separate. - let copies_from_rt_arg_vars_to_rt_args_array = try_rev_take(rt_args_count * 9)?; - let copies_from_rt_arg_vars_to_rt_args_array = - copies_from_rt_arg_vars_to_rt_args_array.chunks(9); - let inits_of_rt_arg_vars = try_rev_take(rt_args_count * 6)?; - let inits_of_rt_arg_vars = inits_of_rt_arg_vars.chunks(6); - - for ( - rt_arg_idx, - (init_of_rt_arg_var_insts, copy_from_rt_arg_var_to_rt_args_array_insts), - ) in inits_of_rt_arg_vars - .zip(copies_from_rt_arg_vars_to_rt_args_array) - .enumerate() + let stores_to_rt_args_array = + try_rev_take(rt_args_count * 5).ok_or_else(|| { + FormatArgsNotRecognized( + "[fmt::rt::Argument; N] stores: ran out of instructions".into(), + ) + })?; + let stores_to_rt_args_array = stores_to_rt_args_array.chunks(5); + let rt_arg_new_calls = try_rev_take(rt_args_count * 3).ok_or_else(|| { + FormatArgsNotRecognized( + "fmt::rt::Argument::new calls: ran out of instructions".into(), + ) + })?; + let rt_arg_new_calls = rt_arg_new_calls.chunks(3); + + for (rt_arg_idx, (rt_arg_new_call_insts, store_to_rt_args_array_insts)) in + rt_arg_new_calls.zip(stores_to_rt_args_array).enumerate() { - let rt_arg_var_id = match init_of_rt_arg_var_insts[..] { + let (a, b) = match rt_arg_new_call_insts[..] { [ - Inst::Bitcast(b, _), - Inst::Bitcast(a, _), - Inst::AccessChain(a_ptr, a_base_ptr, SpirvConst::U32(0)), - Inst::Store(a_st_dst, a_st_val), - Inst::AccessChain(b_ptr, b_base_ptr, SpirvConst::U32(1)), - Inst::Store(b_st_dst, b_st_val), - ] if a_base_ptr == b_base_ptr - && (a, b) == (a_st_val, b_st_val) - && (a_ptr, b_ptr) == (a_st_dst, b_st_dst) => - { - require_local_var(a_base_ptr); - a_base_ptr - } - _ => return None, - }; + Inst::Call(call_ret_id, callee_id, ref call_args), + Inst::CompositeExtract(a, a_parent_pair, 0), + Inst::CompositeExtract(b, b_parent_pair, 1), + ] if [a_parent_pair, b_parent_pair] == [call_ret_id; 2] => self + .fmt_rt_arg_new_fn_ids_to_ty_and_spec + .borrow() + .get(&callee_id) + .and_then(|&(ty, spec)| match call_args[..] { + [x] => { + decoded_format_args + .ref_arg_ids_with_ty_and_spec + .push((x, ty, spec)); + Some((a, b)) + } + _ => None, + }), + _ => None, + } + .ok_or_else(|| { + FormatArgsNotRecognized(format!( + "fmt::rt::Argument::new call sequence ({rt_arg_new_call_insts:?})" + )) + })?; - // HACK(eddyb) this is only split to allow variable name reuse. - let (copy_loads, copy_stores) = - copy_from_rt_arg_var_to_rt_args_array_insts.split_at(4); - let (a, b) = match copy_loads[..] { - [ - Inst::AccessChain(a_ptr, a_base_ptr, SpirvConst::U32(0)), - Inst::Load(a_ld_val, a_ld_src), - Inst::AccessChain(b_ptr, b_base_ptr, SpirvConst::U32(1)), - Inst::Load(b_ld_val, b_ld_src), - ] if [a_base_ptr, b_base_ptr] == [rt_arg_var_id; 2] - && (a_ptr, b_ptr) == (a_ld_src, b_ld_src) => - { - (a_ld_val, b_ld_val) - } - _ => return None, - }; - match copy_stores[..] { + match store_to_rt_args_array_insts[..] { [ Inst::InBoundsAccessChain( array_slot_ptr, @@ -2630,7 +2707,11 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { && [a_base_ptr, b_base_ptr] == [array_slot_ptr; 2] && (a, b) == (a_st_val, b_st_val) && (a_ptr, b_ptr) == (a_st_dst, b_st_dst) => {} - _ => return None, + _ => { + return Err(FormatArgsNotRecognized(format!( + "[fmt::rt::Argument; N] stores sequence ({store_to_rt_args_array_insts:?})" + ))); + } } } } @@ -2639,11 +2720,36 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { // confirmed above to be the first instruction of `format_args!`. func.blocks[block_idx] .instructions - .truncate(taken_inst_idx_range.start); + .truncate(taken_inst_idx_range.start.get()); - None + Ok(decoded_format_args) }; - remove_format_args_if_possible(); + + match try_decode_and_remove_format_args() { + Ok(DecodedFormatArgs {}) => {} + Err(FormatArgsNotRecognized(step)) => { + if let Some(current_span) = self.current_span { + let mut warn = self.tcx.sess.struct_span_warn( + current_span, + "failed to find and remove `format_args!` construction for this `panic!`", + ); + + warn.note( + "compilation may later fail due to leftover `format_args!` internals", + ); + + if self.tcx.sess.opts.unstable_opts.inline_mir != Some(true) { + warn.note("missing `-Zinline-mir=on` flag (should've been set by `spirv-builder`)") + .help("check `.cargo` and environment variables for potential overrides") + .help("(or, if not using `spirv-builder` at all, add the flag manually)"); + } else { + warn.note(format!("[RUST-GPU BUG] bailed from {step}")); + } + + warn.emit(); + } + } + } // HACK(eddyb) redirect any possible panic call to an abort, to avoid // needing to materialize `&core::panic::Location` or `format_args!`. diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs b/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs index ce28a66f54..0065c6a095 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs @@ -4,6 +4,7 @@ use crate::attr::AggregatedSpirvAttributes; use crate::builder_spirv::{SpirvConst, SpirvValue, SpirvValueExt}; use crate::custom_decorations::{CustomDecoration, SrcLocDecoration}; use crate::spirv_type::SpirvType; +use itertools::Itertools; use rspirv::spirv::{FunctionControl, LinkageType, StorageClass, Word}; use rustc_attr::InlineAttr; use rustc_codegen_ssa::traits::{BaseTypeMethods, PreDefineMethods, StaticMethods}; @@ -177,15 +178,13 @@ impl<'tcx> CodegenCx<'tcx> { if [ self.tcx.lang_items().panic_fn(), self.tcx.lang_items().panic_fmt(), - self.tcx.lang_items().panic_display(), - self.tcx.lang_items().panic_bounds_check_fn(), ] .contains(&Some(instance_def_id)) { self.panic_entry_point_ids.borrow_mut().insert(fn_id); } - // HACK(eddyb) there is no good way to identify this definition + // HACK(eddyb) there is no good way to identify these definitions // (e.g. no `#[lang = "..."]` attribute), but this works well enough. if [ "::new_v1", @@ -196,6 +195,33 @@ impl<'tcx> CodegenCx<'tcx> { self.fmt_args_new_fn_ids.borrow_mut().insert(fn_id); } + // HACK(eddyb) there is no good way to identify these definitions + // (e.g. no `#[lang = "..."]` attribute), but this works well enough. + if let Some(suffix) = demangled_symbol_name.strip_prefix("::new_") + { + let spec = suffix.split_once("::<").and_then(|(method_suffix, _)| { + Some(match method_suffix { + "display" => ' ', + "debug" => '?', + "octal" => 'o', + "lower_hex" => 'x', + "upper_hex" => 'X', + "pointer" => 'p', + "binary" => 'b', + "lower_exp" => 'e', + "upper_exp" => 'E', + _ => return None, + }) + }); + if let Some(spec) = spec { + if let Some((ty,)) = instance.substs.types().collect_tuple() { + self.fmt_rt_arg_new_fn_ids_to_ty_and_spec + .borrow_mut() + .insert(fn_id, (ty, spec)); + } + } + } + declared } diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index 7c2aece463..04e3942a42 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -61,17 +61,17 @@ pub struct CodegenCx<'tcx> { pub libm_intrinsics: RefCell>, /// All `panic!(...)`s and builtin panics (from MIR `Assert`s) call into one - /// of these lang items, which we always replace with an "abort", erasing - /// anything passed in. - // - // FIXME(eddyb) we should not erase anywhere near as much, but `format_args!` - // is not representable due to containg Rust slices, and Rust 2021 has made - // it mandatory even for `panic!("...")` (that were previously separate). + /// of these lang items, which we always replace with an "abort". pub panic_entry_point_ids: RefCell>, /// `core::fmt::Arguments::new_{v1,const}` instances (for Rust 2021 panics). pub fmt_args_new_fn_ids: RefCell>, + /// `core::fmt::rt::Argument::new_*::` instances (for panics' `format_args!`), + /// with their `T` type (i.e. of the value being formatted), and formatting + /// "specifier" as a `char` (' ' for `Display`, `x` for `LowerHex`, etc.) + pub fmt_rt_arg_new_fn_ids_to_ty_and_spec: RefCell, char)>>, + /// Intrinsic for loading a from a &[u32]. The PassMode is the mode of the . pub buffer_load_intrinsic_fn_id: RefCell>, /// Intrinsic for storing a into a &[u32]. The PassMode is the mode of the . @@ -131,6 +131,7 @@ impl<'tcx> CodegenCx<'tcx> { libm_intrinsics: Default::default(), panic_entry_point_ids: Default::default(), fmt_args_new_fn_ids: Default::default(), + fmt_rt_arg_new_fn_ids_to_ty_and_spec: Default::default(), buffer_load_intrinsic_fn_id: Default::default(), buffer_store_intrinsic_fn_id: Default::default(), i8_i16_atomics_allowed: false, diff --git a/crates/spirv-builder/src/lib.rs b/crates/spirv-builder/src/lib.rs index 5f516e3edd..57ef35ccbd 100644 --- a/crates/spirv-builder/src/lib.rs +++ b/crates/spirv-builder/src/lib.rs @@ -562,6 +562,9 @@ fn invoke_rustc(builder: &SpirvBuilder) -> Result { // ensures no unwanted surprises from e.g. `core` debug assertions. "-Coverflow-checks=off".to_string(), "-Cdebug-assertions=off".to_string(), + // HACK(eddyb) we need this for `core::fmt::rt::Argument::new_*` calls + // to *never* be inlined, so we can pattern-match the calls themselves. + "-Zinline-mir=off".to_string(), ]; // Wrapper for `env::var` that appropriately informs Cargo of the dependency. diff --git a/tests/src/main.rs b/tests/src/main.rs index 21e0c1aa99..46d12bfbc7 100644 --- a/tests/src/main.rs +++ b/tests/src/main.rs @@ -346,14 +346,20 @@ fn rust_flags(codegen_backend_path: &Path) -> String { // to rebuild crates compiled with it when it changes (this used to be // the default until https://github.com/rust-lang/rust/pull/93969). "-Zbinary-dep-depinfo", + "-Csymbol-mangling-version=v0", + "-Zcrate-attr=feature(register_tool)", + "-Zcrate-attr=register_tool(rust_gpu)", + // HACK(eddyb) this is the same configuration that we test with, and + // ensures no unwanted surprises from e.g. `core` debug assertions. "-Coverflow-checks=off", "-Cdebug-assertions=off", + // HACK(eddyb) we need this for `core::fmt::rt::Argument::new_*` calls + // to *never* be inlined, so we can pattern-match the calls themselves. + "-Zinline-mir=off", + // NOTE(eddyb) flags copied from `spirv-builder` are all above this line. "-Cdebuginfo=2", "-Cembed-bitcode=no", &format!("-Ctarget-feature=+{}", target_features.join(",+")), - "-Csymbol-mangling-version=v0", - "-Zcrate-attr=feature(register_tool)", - "-Zcrate-attr=register_tool(rust_gpu)", ] .join(" ") } From 1de52f2bb17ae380920b3dd7d1162b38fd3524da Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Thu, 20 Jul 2023 20:18:14 +0300 Subject: [PATCH 2/2] Show `panic!` messages via `debugPrintf`, even including some runtime arguments (`{u,i,f}32` as `{}` or `{:?}`). --- CHANGELOG.md | 4 + .../src/builder/builder_methods.rs | 146 ++++++++++++++++-- .../src/builder/intrinsics.rs | 16 +- .../src/codegen_cx/constant.rs | 8 +- .../rustc_codegen_spirv/src/custom_insts.rs | 21 ++- .../src/linker/spirt_passes/controlflow.rs | 37 +++-- 6 files changed, 201 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4c84d5431..f5ed85dc6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added ⭐ +- [PR#1082](https://github.com/EmbarkStudios/rust-gpu/pull/1082) added partial + support for extracting `format_args!` from `panic!`s, and converting them to + `debugPrintf` calls (if enabled via `ShaderPanicStrategy`), including runtime + arguments (`u32`/`i32`/`f32` with `Display`/`Debug` formatting, for now) - [PR#1081](https://github.com/EmbarkStudios/rust-gpu/pull/1081) added the ability to access SPIR-V specialization constants (`OpSpecConstant`) via entry-point inputs declared as `#[spirv(spec_constant(id = ..., default = ...))] x: u32` diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index 80415b1a67..ef92f02da3 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -20,11 +20,13 @@ use rustc_codegen_ssa::MemFlags; use rustc_data_structures::fx::FxHashSet; use rustc_middle::bug; use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs; +use rustc_middle::ty::layout::LayoutOf; use rustc_middle::ty::Ty; use rustc_span::Span; use rustc_target::abi::call::FnAbi; use rustc_target::abi::{Abi, Align, Scalar, Size, WrappingRange}; use smallvec::SmallVec; +use std::borrow::Cow; use std::cell::Cell; use std::convert::TryInto; use std::iter::{self, empty}; @@ -2414,12 +2416,25 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { // nor simplified in MIR (e.g. promoted to a constant) in any way, // so we have to try and remove the `fmt::Arguments::new` call here. #[derive(Default)] - struct DecodedFormatArgs {} + struct DecodedFormatArgs<'tcx> { + /// If fully constant, the `pieces: &'a [&'static str]` input + /// of `fmt::Arguments<'a>` (i.e. the strings between args). + const_pieces: Option>, + + /// Original references for `fmt::Arguments<'a>` dynamic arguments, + /// i.e. the `&'a T` passed to `fmt::rt::Argument::<'a>::new_*`, + /// tracking the type `T` and `char` formatting specifier. + /// + /// E.g. for `format_args!("{a} {b:x}")` they'll be: + /// * `&a` with `typeof a` and ' ', + /// *`&b` with `typeof b` and 'x' + ref_arg_ids_with_ty_and_spec: SmallVec<[(Word, Ty<'tcx>, char); 2]>, + } struct FormatArgsNotRecognized(String); // HACK(eddyb) this is basically a `try` block. let try_decode_and_remove_format_args = || { - let decoded_format_args = DecodedFormatArgs::default(); + let mut decoded_format_args = DecodedFormatArgs::default(); let const_u32_as_usize = |ct_id| match self.builder.lookup_const_by_id(ct_id)? { SpirvConst::U32(x) => Some(x as usize), @@ -2441,6 +2456,32 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { } None }; + let const_str_as_utf8 = |str_ptr_and_len_ids: &[Word]| { + let piece_str_bytes = const_slice_as_elem_ids(str_ptr_and_len_ids)? + .iter() + .map(|&id| u8::try_from(const_u32_as_usize(id)?).ok()) + .collect::>>()?; + String::from_utf8(piece_str_bytes).ok() + }; + + // HACK(eddyb) some entry-points only take a `&str`, not `fmt::Arguments`. + if let [ + SpirvValue { + kind: SpirvValueKind::Def(a_id), + .. + }, + SpirvValue { + kind: SpirvValueKind::Def(b_id), + .. + }, + _, // `&'static panic::Location<'static>` + ] = args[..] + { + if let Some(const_msg) = const_str_as_utf8(&[a_id, b_id]) { + decoded_format_args.const_pieces = Some([const_msg].into_iter().collect()); + return Ok(decoded_format_args); + } + } let format_args_id = match args { &[ @@ -2503,6 +2544,7 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { #[derive(Debug)] enum Inst<'tcx, ID> { Bitcast(ID, ID), + CompositeExtract(ID, ID, u32), AccessChain(ID, ID, SpirvConst<'tcx>), InBoundsAccessChain(ID, ID, SpirvConst<'tcx>), Store(ID, ID), @@ -2519,6 +2561,15 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { let (i, inst) = non_debug_insts.next_back()?; taken_inst_idx_range.start.set(i); + // HACK(eddyb) avoid the logic below that assumes only ID operands + if inst.class.opcode == Op::CompositeExtract { + if let (Some(r), &[Operand::IdRef(x), Operand::LiteralInt32(i)]) = + (inst.result_id, &inst.operands[..]) + { + return Some(Inst::CompositeExtract(r, x, i)); + } + } + // HACK(eddyb) all instructions accepted below // are expected to take no more than 4 operands, // and this is easier to use than an iterator. @@ -2716,6 +2767,26 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { } } + // If the `pieces: &[&str]` slice needs a bitcast, it'll be here. + let pieces_slice_ptr_id = match try_rev_take(1).as_deref() { + Some(&[Inst::Bitcast(out_id, in_id)]) if out_id == pieces_slice_ptr_id => in_id, + _ => pieces_slice_ptr_id, + }; + decoded_format_args.const_pieces = + const_slice_as_elem_ids(&[pieces_slice_ptr_id, pieces_len_id]).and_then( + |piece_ids| { + piece_ids + .iter() + .map(|&piece_id| { + match self.builder.lookup_const_by_id(piece_id)? { + SpirvConst::Composite(piece) => const_str_as_utf8(piece), + _ => None, + } + }) + .collect::>() + }, + ); + // Keep all instructions up to (but not including) the last one // confirmed above to be the first instruction of `format_args!`. func.blocks[block_idx] @@ -2725,8 +2796,61 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { Ok(decoded_format_args) }; - match try_decode_and_remove_format_args() { - Ok(DecodedFormatArgs {}) => {} + let mut debug_printf_args = SmallVec::<[_; 2]>::new(); + let message = match try_decode_and_remove_format_args() { + Ok(DecodedFormatArgs { + const_pieces, + ref_arg_ids_with_ty_and_spec, + }) => { + match const_pieces { + Some(const_pieces) => { + const_pieces + .into_iter() + .map(|s| Cow::Owned(s.replace('%', "%%"))) + .interleave(ref_arg_ids_with_ty_and_spec.iter().map( + |&(ref_id, ty, spec)| { + use rustc_target::abi::{Integer::*, Primitive::*}; + + let layout = self.layout_of(ty); + + let scalar = match layout.abi { + Abi::Scalar(scalar) => Some(scalar.primitive()), + _ => None, + }; + let debug_printf_fmt = match (spec, scalar) { + // FIXME(eddyb) support more of these, + // potentially recursing to print ADTs. + (' ' | '?', Some(Int(I32, false))) => "%u", + ('x', Some(Int(I32, false))) => "%x", + (' ' | '?', Some(Int(I32, true))) => "%i", + (' ' | '?', Some(F32)) => "%f", + + _ => "", + }; + + if debug_printf_fmt.is_empty() { + return Cow::Owned( + format!("{{/* unprintable {ty} */:{spec}}}") + .replace('%', "%%"), + ); + } + + let spirv_type = layout.spirv_type(self.span(), self); + debug_printf_args.push( + self.emit() + .load(spirv_type, None, ref_id, None, []) + .unwrap() + .with_type(spirv_type), + ); + Cow::Borrowed(debug_printf_fmt) + }, + )) + .collect::() + } + None => "".into(), + } + } + Err(FormatArgsNotRecognized(step)) => { if let Some(current_span) = self.current_span { let mut warn = self.tcx.sess.struct_span_warn( @@ -2738,8 +2862,8 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { "compilation may later fail due to leftover `format_args!` internals", ); - if self.tcx.sess.opts.unstable_opts.inline_mir != Some(true) { - warn.note("missing `-Zinline-mir=on` flag (should've been set by `spirv-builder`)") + if self.tcx.sess.opts.unstable_opts.inline_mir != Some(false) { + warn.note("missing `-Zinline-mir=off` flag (should've been set by `spirv-builder`)") .help("check `.cargo` and environment variables for potential overrides") .help("(or, if not using `spirv-builder` at all, add the flag manually)"); } else { @@ -2748,13 +2872,17 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { warn.emit(); } + " (failed to find/decode `format_args!` expansion)".into() } - } + }; // HACK(eddyb) redirect any possible panic call to an abort, to avoid // needing to materialize `&core::panic::Location` or `format_args!`. - // FIXME(eddyb) find a way to extract the original message. - self.abort_with_message("panicked: ".into()); + self.abort_with_message_and_debug_printf_args( + // HACK(eddyb) `|` is an ad-hoc convention of `linker::spirt_passes::controlflow`. + format!("panicked|{message}"), + debug_printf_args, + ); self.undef(result_type) } else if let Some(mode) = buffer_load_intrinsic { self.codegen_buffer_load_intrinsic(result_type, args, mode) diff --git a/crates/rustc_codegen_spirv/src/builder/intrinsics.rs b/crates/rustc_codegen_spirv/src/builder/intrinsics.rs index b94475c5dc..982e47f13d 100644 --- a/crates/rustc_codegen_spirv/src/builder/intrinsics.rs +++ b/crates/rustc_codegen_spirv/src/builder/intrinsics.rs @@ -339,7 +339,11 @@ impl<'a, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'a, 'tcx> { } fn abort(&mut self) { - self.abort_with_message("aborted: intrinsics::abort() called".into()); + // HACK(eddyb) `|` is an ad-hoc convention of `linker::spirt_passes::controlflow`. + self.abort_with_message_and_debug_printf_args( + "aborted|intrinsics::abort() called".into(), + [], + ); } fn assume(&mut self, _val: Self::Value) { @@ -374,7 +378,11 @@ impl<'a, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'a, 'tcx> { } impl Builder<'_, '_> { - pub fn abort_with_message(&mut self, message: String) { + pub fn abort_with_message_and_debug_printf_args( + &mut self, + message: String, + debug_printf_args: impl IntoIterator, + ) { // FIXME(eddyb) this should be cached more efficiently. let void_ty = SpirvType::Void.def(rustc_span::DUMMY_SP, self); @@ -385,6 +393,10 @@ impl Builder<'_, '_> { void_ty, CustomInst::Abort { message: Operand::IdRef(message_id), + debug_printf_args: debug_printf_args + .into_iter() + .map(|arg| Operand::IdRef(arg.def(self))) + .collect(), }, ); self.unreachable(); diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs b/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs index 4f53fab9bc..65fe219e00 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs @@ -172,12 +172,16 @@ impl<'tcx> ConstMethods<'tcx> for CodegenCx<'tcx> { let str_ty = self .layout_of(self.tcx.types.str_) .spirv_type(DUMMY_SP, self); - // FIXME(eddyb) include the actual byte data. ( self.def_constant( self.type_ptr_to(str_ty), SpirvConst::PtrTo { - pointee: self.undef(str_ty).def_cx(self), + pointee: self + .constant_composite( + str_ty, + s.bytes().map(|b| self.const_u8(b).def_cx(self)), + ) + .def_cx(self), }, ), self.const_usize(len as u64), diff --git a/crates/rustc_codegen_spirv/src/custom_insts.rs b/crates/rustc_codegen_spirv/src/custom_insts.rs index 4242d5802d..4ec02c5baa 100644 --- a/crates/rustc_codegen_spirv/src/custom_insts.rs +++ b/crates/rustc_codegen_spirv/src/custom_insts.rs @@ -49,9 +49,9 @@ lazy_static! { } macro_rules! def_custom_insts { - ($($num:literal => $name:ident $({ $($field:ident),+ $(,)? })?),+ $(,)?) => { + ($($num:literal => $name:ident $({ $($field:ident),+ $(, ..$variadic_field:ident)? $(,)? })?),+ $(,)?) => { const SCHEMA: &[(u32, &str, &[&str])] = &[ - $(($num, stringify!($name), &[$($(stringify!($field)),+)?])),+ + $(($num, stringify!($name), &[$($(stringify!($field),)+ $(stringify!(..$variadic_field),)?)?])),+ ]; #[repr(u32)] @@ -74,16 +74,19 @@ macro_rules! def_custom_insts { pub fn with_operands(self, operands: &[T]) -> CustomInst { match self { $(Self::$name => match operands { - [$($($field),+)?] => CustomInst::$name $({ $($field: $field.clone()),+ })?, + [$($($field,)+ $(ref $variadic_field @ ..)?)?] => CustomInst::$name $({ + $($field: $field.clone(),)+ + $($variadic_field: $variadic_field.iter().cloned().collect())? + })?, _ => unreachable!("{self:?} does not have the right number of operands"), }),+ } } } - #[derive(Copy, Clone, Debug)] + #[derive(Clone, Debug)] pub enum CustomInst { - $($name $({ $($field: T),+ })?),+ + $($name $({ $($field: T,)+ $($variadic_field: SmallVec<[T; 4]>)? })?),+ } impl CustomInst { @@ -96,7 +99,9 @@ macro_rules! def_custom_insts { // HACK(eddyb) this should return an iterator, but that's too much effort. pub fn into_operands(self) -> SmallVec<[T; 8]> { match self { - $(Self::$name $({ $($field),+ })? => [$($($field),+)?].into_iter().collect()),+ + $(Self::$name $({ $($field,)+ $($variadic_field)? })? => { + [$($($field),+)?].into_iter() $($(.chain($variadic_field))?)? .collect() + })+ } } } @@ -146,7 +151,9 @@ def_custom_insts! { // users to do `catch_unwind` at the top-level of their shader to handle // panics specially (e.g. by appending to a custom buffer, or using some // specific color in a fragment shader, to indicate a panic happened). - 4 => Abort { message }, + // NOTE(eddyb) the `message` string follows `debugPrintf` rules, with remaining + // operands (collected into `debug_printf_args`) being its formatting arguments. + 4 => Abort { message, ..debug_printf_args }, } impl CustomOp { diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs index e22c94beab..21e263052a 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs @@ -240,8 +240,13 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( .map(|(func_at_inst, custom)| (func_at_inst, custom.unwrap())) .find(|(_, custom)| !custom.op().is_debuginfo()) .filter(|(_, custom)| custom.op().is_terminator()); - if let Some((func_at_abort_inst, CustomInst::Abort { message })) = - custom_terminator_inst + if let Some(( + func_at_abort_inst, + CustomInst::Abort { + message, + debug_printf_args: message_debug_printf_args, + }, + )) = custom_terminator_inst { let abort_inst = func_at_abort_inst.position; terminator.kind = cfg::ControlInstKind::Return; @@ -332,29 +337,38 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( // HACK(eddyb) this improves readability w/ very verbose Vulkan loggers. fmt += "\n"; - fmt += "[RUST-GPU] "; - fmt += &cx[const_str(message)].replace('%', "%%"); + fmt += "["; + + // NB: `message` has its own `message_debug_printf_args` + // it formats, and as such any escaping is already done. + let message = &cx[const_str(message)]; + let (message_kind, message) = + message.split_once('|').unwrap_or(("", message)); - // FIXME(eddyb) deduplicate with "called at" form below - // (not trivial becasue both closures would borrow `fmt`). if let Some((file, line, col)) = current_debug_src_loc.take() { - fmt += &format!("\n at {file}:{line}:{col}").replace('%', "%%"); + fmt += &format!("Rust {message_kind} at {file}:{line}:{col}") + .replace('%', "%%"); + } else { + fmt += message_kind; } + fmt += "]\n "; + fmt += &message.replace('\n', "\n "); + let mut innermost = true; let mut append_call = |callsite_debug_src_loc, callee: &str| { if innermost { innermost = false; - fmt += "\n in "; + fmt += "\n in "; } else if current_debug_src_loc.is_some() { - fmt += "\n by "; + fmt += "\n by "; } else { // HACK(eddyb) previous call didn't have a `called at` line. - fmt += "\n called by "; + fmt += "\n called by "; } fmt += callee; if let Some((file, line, col)) = callsite_debug_src_loc { - fmt += &format!("\n called at {file}:{line}:{col}") + fmt += &format!("\n called at {file}:{line}:{col}") .replace('%', "%%"); } current_debug_src_loc = callsite_debug_src_loc; @@ -373,6 +387,7 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( }; abort_inst_def.inputs = [Value::Const(mk_const_str(cx.intern(fmt)))] .into_iter() + .chain(message_debug_printf_args) .chain(debug_printf_context_inputs.iter().copied()) .collect();