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