From 7f508ba86c9ac232460630dead1f835cad6058d3 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 21 Jul 2023 20:02:12 +0300 Subject: [PATCH] linker: use `OutputFilenames::temp_path_ext` for critical dumping, even without `--dump-*`. --- crates/rustc_codegen_spirv/src/link.rs | 5 ++ crates/rustc_codegen_spirv/src/linker/mod.rs | 65 +++++++++++++++---- .../src/linker/spirt_passes/diagnostics.rs | 41 +++++------- crates/rustc_codegen_spirv/src/linker/test.rs | 18 ++++- 4 files changed, 92 insertions(+), 37 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/link.rs b/crates/rustc_codegen_spirv/src/link.rs index 84ee4db4d5..aba8f3c366 100644 --- a/crates/rustc_codegen_spirv/src/link.rs +++ b/crates/rustc_codegen_spirv/src/link.rs @@ -78,6 +78,7 @@ pub fn link( crate_type, &out_filename, codegen_results, + outputs, &disambiguated_crate_name_for_dumps, ); } @@ -123,6 +124,7 @@ fn link_exe( crate_type: CrateType, out_filename: &Path, codegen_results: &CodegenResults, + outputs: &OutputFilenames, disambiguated_crate_name_for_dumps: &OsStr, ) { let mut objects = Vec::new(); @@ -152,6 +154,7 @@ fn link_exe( &cg_args, &objects, &rlibs, + outputs, disambiguated_crate_name_for_dumps, ); let compile_result = match link_result { @@ -517,6 +520,7 @@ fn do_link( cg_args: &CodegenArgs, objects: &[PathBuf], rlibs: &[PathBuf], + outputs: &OutputFilenames, disambiguated_crate_name_for_dumps: &OsStr, ) -> linker::LinkResult { let load_modules_timer = sess.timer("link_load_modules"); @@ -570,6 +574,7 @@ fn do_link( sess, modules, &cg_args.linker_opts, + outputs, disambiguated_crate_name_for_dumps, ); diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index 1ef6509eed..dc540a3686 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -27,6 +27,7 @@ use rspirv::dr::{Block, Instruction, Loader, Module, ModuleHeader, Operand}; use rspirv::spirv::{Op, StorageClass, Word}; use rustc_data_structures::fx::FxHashMap; use rustc_errors::ErrorGuaranteed; +use rustc_session::config::OutputFilenames; use rustc_session::Session; use std::collections::BTreeMap; use std::ffi::{OsStr, OsString}; @@ -151,6 +152,7 @@ pub fn link( sess: &Session, mut inputs: Vec, opts: &Options, + outputs: &OutputFilenames, disambiguated_crate_name_for_dumps: &OsStr, ) -> Result { let mut output = { @@ -383,9 +385,13 @@ pub fn link( } }; + let spv_words; let spv_bytes = { let _timer = sess.timer("assemble-to-spv_bytes-for-spirt"); - spirv_tools::binary::from_binary(&output.assemble()).to_vec() + spv_words = output.assemble(); + // FIXME(eddyb) this is wastefully cloning all the bytes, but also + // `spirt::Module` should have a method that takes `Vec`. + spirv_tools::binary::from_binary(&spv_words).to_vec() }; let cx = std::rc::Rc::new(spirt::Context::new()); let mut module = { @@ -393,14 +399,20 @@ pub fn link( match spirt::Module::lower_from_spv_bytes(cx.clone(), spv_bytes) { Ok(module) => module, Err(e) => { - use rspirv::binary::Disassemble; + let spv_path = outputs.temp_path_ext("spirt-lower-from-spv-input.spv", None); + + let was_saved_msg = match std::fs::write( + &spv_path, + spirv_tools::binary::from_binary(&spv_words), + ) { + Ok(()) => format!("was saved to {}", spv_path.display()), + Err(e) => format!("could not be saved: {e}"), + }; return Err(sess .struct_err(format!("{e}")) - .note(format!( - "while lowering this SPIR-V module to SPIR-T:\n{}", - output.disassemble() - )) + .note("while lowering SPIR-V module to SPIR-T (spirt::spv::lower)") + .note(format!("input SPIR-V module {was_saved_msg}")) .emit()); } } @@ -438,14 +450,30 @@ pub fn link( let _timer = sess.timer("spirt_passes::diagnostics::report_diagnostics"); spirt_passes::diagnostics::report_diagnostics(sess, opts, &module) }; + let any_spirt_bugs = report_diagnostics_result + .as_ref() + .err() + .map_or(false, |e| e.any_errors_were_spirt_bugs); + + let mut dump_spirt_file_path = opts.dump_spirt_passes.as_ref().map(|dump_dir| { + dump_dir + .join(disambiguated_crate_name_for_dumps) + .with_extension("spirt") + }); + + // FIXME(eddyb) this won't allow seeing the individual passes, but it's + // better than nothing (we could theoretically put this whole block in + // a loop so that we redo everything but keeping `Module` clones?). + if any_spirt_bugs && dump_spirt_file_path.is_none() { + if per_pass_module_for_dumping.is_empty() { + per_pass_module_for_dumping.push(("", module.clone())); + } + dump_spirt_file_path = Some(outputs.temp_path_ext("spirt", None)); + } // NOTE(eddyb) this should be *before* `lift_to_spv` below, // so if that fails, the dump could be used to debug it. - if let Some(dump_dir) = &opts.dump_spirt_passes { - let dump_spirt_file_path = dump_dir - .join(disambiguated_crate_name_for_dumps) - .with_extension("spirt"); - + if let Some(dump_spirt_file_path) = &dump_spirt_file_path { // HACK(eddyb) unless requested otherwise, clean up the pretty-printed // SPIR-T output by converting our custom extended instructions, to // standard SPIR-V debuginfo (which SPIR-T knows how to pretty-print). @@ -464,7 +492,7 @@ pub fn link( let pretty = plan.pretty_print(); // FIXME(eddyb) don't allocate whole `String`s here. - std::fs::write(&dump_spirt_file_path, pretty.to_string()).unwrap(); + std::fs::write(dump_spirt_file_path, pretty.to_string()).unwrap(); std::fs::write( dump_spirt_file_path.with_extension("spirt.html"), pretty @@ -475,6 +503,19 @@ pub fn link( .unwrap(); } + if any_spirt_bugs { + let mut note = sess.struct_note_without_error("SPIR-T bugs were reported"); + note.help(format!( + "pretty-printed SPIR-T was saved to {}.html", + dump_spirt_file_path.as_ref().unwrap().display() + )); + if opts.dump_spirt_passes.is_none() { + note.help("re-run with `RUSTGPU_CODEGEN_ARGS=\"--dump-spirt-passes=$PWD\"` for more details"); + } + note.note("pretty-printed SPIR-T is preferred when reporting Rust-GPU issues") + .emit(); + } + // NOTE(eddyb) this is late so that `--dump-spirt-passes` is processed, // even/especially when errors were reported, but lifting to SPIR-V is // skipped (since it could very well fail due to reported errors). diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs index 6dc7bec717..73ca78c74c 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs @@ -17,11 +17,22 @@ use spirt::{ use std::marker::PhantomData; use std::{mem, str}; +pub(crate) struct ReportedDiagnostics { + pub rustc_errors_guarantee: rustc_errors::ErrorGuaranteed, + pub any_errors_were_spirt_bugs: bool, +} + +impl From for rustc_errors::ErrorGuaranteed { + fn from(r: ReportedDiagnostics) -> Self { + r.rustc_errors_guarantee + } +} + pub(crate) fn report_diagnostics( sess: &Session, linker_options: &crate::linker::Options, module: &Module, -) -> crate::linker::Result<()> { +) -> Result<(), ReportedDiagnostics> { let cx = &module.cx(); let mut reporter = DiagnosticReporter { @@ -68,28 +79,12 @@ pub(crate) fn report_diagnostics( exportee.inner_visit_with(&mut reporter); } - if reporter.any_spirt_bugs { - let mut note = sess.struct_note_without_error("SPIR-T bugs were reported"); - match &linker_options.dump_spirt_passes { - Some(dump_dir) => { - note.help(format!( - "pretty-printed SPIR-T will be saved to `{}`, as `.spirt.html` files", - dump_dir.display() - )); - } - None => { - // FIXME(eddyb) maybe just always generate the files in a tmpdir? - note.help( - "re-run with `RUSTGPU_CODEGEN_ARGS=\"--dump-spirt-passes=$PWD\"` to \ - get pretty-printed SPIR-T (`.spirt.html`)", - ); - } - } - note.note("pretty-printed SPIR-T is preferred when reporting Rust-GPU issues") - .emit(); - } - - reporter.overall_result + reporter + .overall_result + .map_err(|rustc_errors_guarantee| ReportedDiagnostics { + rustc_errors_guarantee, + any_errors_were_spirt_bugs: reporter.any_spirt_bugs, + }) } // HACK(eddyb) version of `decorations::LazilyDecoded` that works for SPIR-T. diff --git a/crates/rustc_codegen_spirv/src/linker/test.rs b/crates/rustc_codegen_spirv/src/linker/test.rs index 0dfbcade1c..4ae6f2ba40 100644 --- a/crates/rustc_codegen_spirv/src/linker/test.rs +++ b/crates/rustc_codegen_spirv/src/linker/test.rs @@ -2,7 +2,8 @@ use super::{link, LinkResult}; use pipe::pipe; use rspirv::dr::{Loader, Module}; use rustc_errors::{registry::Registry, TerminalUrl}; -use rustc_session::{config::Input, CompilerIO}; +use rustc_session::config::{Input, OutputFilenames, OutputTypes}; +use rustc_session::CompilerIO; use rustc_span::FileName; use std::io::Read; @@ -148,7 +149,20 @@ fn link_with_linker_opts( ) }; - let res = link(&sess, modules, opts, Default::default()); + let res = link( + &sess, + modules, + opts, + &OutputFilenames::new( + std::env::current_dir().unwrap_or_default(), + "".into(), + None, + None, + "".into(), + OutputTypes::new(&[]), + ), + Default::default(), + ); assert_eq!(sess.has_errors(), res.as_ref().err().copied()); res.map(|res| match res { LinkResult::SingleModule(m) => *m,