Show panic! messages via debugPrintf, even including some runtime arguments ({u,i,f}32 as {} or {:?}).

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`  

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<Item = SpirvValue>,
+    ) {
         // 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();

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),

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 [
             "<core::fmt::Arguments>::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("<core::fmt::rt::Argument>::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
     }
 

crates/rustc_codegen_spirv/src/codegen_cx/mod.rs

@@ -61,17 +61,17 @@ pub struct CodegenCx<'tcx> {
     pub libm_intrinsics: RefCell<FxHashMap<Word, super::builder::libm_intrinsics::LibmIntrinsic>>,
 
     /// 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<FxHashSet<Word>>,
 
     /// `core::fmt::Arguments::new_{v1,const}` instances (for Rust 2021 panics).
     pub fmt_args_new_fn_ids: RefCell<FxHashSet<Word>>,
 
+    /// `core::fmt::rt::Argument::new_*::<T>` 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<FxHashMap<Word, (Ty<'tcx>, char)>>,
+
     /// Intrinsic for loading a <T> from a &[u32]. The PassMode is the mode of the <T>.
     pub buffer_load_intrinsic_fn_id: RefCell<FxHashMap<Word, &'tcx PassMode>>,
     /// Intrinsic for storing a <T> into a &[u32]. The PassMode is the mode of the <T>.
@@ -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,

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<T: Clone>(self, operands: &[T]) -> CustomInst<T> {
                 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<T> {
-            $($name $({ $($field: T),+ })?),+
+            $($name $({ $($field: T,)+ $($variadic_field: SmallVec<[T; 4]>)? })?),+
         }
 
         impl<T> CustomInst<T> {
@@ -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 {

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();
 

crates/spirv-builder/src/lib.rs

@@ -562,6 +562,9 @@ fn invoke_rustc(builder: &SpirvBuilder) -> Result<PathBuf, SpirvBuilderError> {
         // 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.

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(" ")
 }