Include identifying information (e.g. crate names) in --dump-* filenames.

CHANGELOG.md

@@ -33,7 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [PR#959](https://github.com/EmbarkStudios/rust-gpu/pull/959) added two `spirv-builder` environment variables to customize *only* the `rustc` invocations for shader crates and their dependencies:
   - `RUSTGPU_RUSTFLAGS="..."` for shader `RUSTFLAGS="..."`
   - `RUSTGPU_CODEGEN_ARGS="..."` for shader "codegen args" (i.e. `RUSTFLAGS=-Cllvm-args="..."`)  
-    (check out ["codegen args" docs](docs/src/codegen-args.md), or run with `RUSTGPU_CODEGEN_ARGS=--help` to see the full list of options)
+    (check out [the "codegen args" docs](docs/src/codegen-args.md), or run with `RUSTGPU_CODEGEN_ARGS=--help` to see the full list of options)
 - [PR#940](https://github.com/EmbarkStudios/rust-gpu/pull/940) integrated the experimental [`SPIR-🇹` shader IR framework](https://github.com/EmbarkStudios/spirt) into the linker  
   (opt-in via `RUSTGPU_CODEGEN_ARGS=--spirt`, see also [the `--spirt` docs](docs/src/codegen-args.md#--spirt), for more details)
 
@@ -41,6 +41,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [PR#958](https://github.com/EmbarkStudios/rust-gpu/pull/958) updated toolchain to `nightly-2022-10-29`
 - [PR#941](https://github.com/EmbarkStudios/rust-gpu/pull/941) applied workspace inheritance to `Cargo.toml` files
 - [PR#959](https://github.com/EmbarkStudios/rust-gpu/pull/959) moved `rustc_codegen_spirv` debugging functionality from environment variables to "codegen args" options/flags (see [the updated docs](docs/src/codegen-args.md) for more details)
+- [PR#967](https://github.com/EmbarkStudios/rust-gpu/pull/967) made `--dump-*` ["codegen args"](docs/src/codegen-args.md) include identifying information (e.g. crate names) in the names of files they emit
 
 ### Removed 🔥
 - [PR#946](https://github.com/EmbarkStudios/rust-gpu/pull/946) removed the `fn`/closure `#[spirv(unroll_loops)]` attribute, as it has no users, is becoming non-trivial to support, and requires redesign for better ergonomics (e.g. `#[spirv(unroll)]` applied to individual loops, not the whole `fn`/closure)

Cargo.lock

@@ -679,9 +679,9 @@ checksum = "9ea835d29036a4087793836fa931b08837ad5e957da9e23886b29586fb9b6650"
 
 [[package]]
 name = "either"
-version = "1.6.1"
+version = "1.8.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e78d4f1cc4ae33bbfc157ed5d5a5ef3bc29227303d595861deb238fcec4e9457"
+checksum = "90e5c1c8368803113bf0c9584fc495a58b86dc8a29edbf8fe877d21d9507e797"
 
 [[package]]
 name = "elsa"
@@ -2011,6 +2011,7 @@ name = "rustc_codegen_spirv"
 version = "0.4.0-alpha.17"
 dependencies = [
  "ar",
+ "either",
  "hashbrown",
  "indexmap",
  "libc",

crates/rustc_codegen_spirv-types/src/compile_result.rs

@@ -32,8 +32,8 @@ impl ModuleResult {
 
 #[derive(Debug, Serialize, Deserialize)]
 pub struct CompileResult {
-    pub module: ModuleResult,
     pub entry_points: Vec<String>,
+    pub module: ModuleResult,
 }
 
 impl CompileResult {

crates/rustc_codegen_spirv/Cargo.toml

@@ -47,6 +47,7 @@ regex = { version = "1", features = ["perf"] }
 
 # Normal dependencies.
 ar = "0.9.0"
+either = "1.8.0"
 indexmap = "1.6.0"
 rspirv = "0.11"
 rustc-demangle = "0.1.21"

crates/rustc_codegen_spirv/src/codegen_cx/mod.rs

@@ -362,8 +362,8 @@ impl CodegenArgs {
             opts.optopt(
                 "",
                 "dump-post-merge",
-                "dump the merged module immediately after merging, to FILE",
-                "FILE",
+                "dump the merged module immediately after merging, to a file in DIR",
+                "DIR",
             );
             opts.optopt(
                 "",
@@ -374,8 +374,8 @@ impl CodegenArgs {
             opts.optopt(
                 "",
                 "dump-spirt-passes",
-                "dump the SPIR-T module across passes, to FILE and FILE.html",
-                "FILE",
+                "dump the SPIR-T module across passes, to a (pair of) file(s) in DIR",
+                "DIR",
             );
             opts.optflag(
                 "",
@@ -532,9 +532,9 @@ impl CodegenArgs {
 
             // NOTE(eddyb) these are debugging options that used to be env vars
             // (for more information see `docs/src/codegen-args.md`).
-            dump_post_merge: matches_opt_path("dump-post-merge"),
+            dump_post_merge: matches_opt_dump_dir_path("dump-post-merge"),
             dump_post_split: matches_opt_dump_dir_path("dump-post-split"),
-            dump_spirt_passes: matches_opt_path("dump-spirt-passes"),
+            dump_spirt_passes: matches_opt_dump_dir_path("dump-spirt-passes"),
             specializer_debug: matches.opt_present("specializer-debug"),
             specializer_dump_instances: matches_opt_path("specializer-dump-instances"),
             print_all_zombie: matches.opt_present("print-all-zombie"),

crates/rustc_codegen_spirv/src/link.rs

@@ -2,6 +2,7 @@ use crate::codegen_cx::{CodegenArgs, SpirvMetadata};
 use crate::{linker, SpirvCodegenBackend, SpirvModuleBuffer, SpirvThinBuffer};
 use ar::{Archive, GnuBuilder, Header};
 use rspirv::binary::Assemble;
+use rspirv::dr::Module;
 use rustc_ast::CRATE_NODE_ID;
 use rustc_codegen_spirv_types::{CompileResult, ModuleResult};
 use rustc_codegen_ssa::back::lto::{LtoModuleCodegen, SerializedModule, ThinModule, ThinShared};
@@ -17,7 +18,8 @@ use rustc_session::config::{CrateType, DebugInfo, Lto, OptLevel, OutputFilenames
 use rustc_session::output::{check_file_is_writeable, invalid_output_for_target, out_filename};
 use rustc_session::utils::NativeLibKind;
 use rustc_session::Session;
-use std::ffi::CString;
+use std::collections::BTreeMap;
+use std::ffi::{CString, OsStr};
 use std::fs::File;
 use std::io::{BufWriter, Read};
 use std::iter;
@@ -62,7 +64,21 @@ pub fn link<'a>(
                     link_rlib(sess, codegen_results, &out_filename);
                 }
                 CrateType::Executable | CrateType::Cdylib | CrateType::Dylib => {
-                    link_exe(sess, crate_type, &out_filename, codegen_results);
+                    // HACK(eddyb) there's no way way to access `outputs.filestem`,
+                    // so we pay the cost of building a whole `PathBuf` instead.
+                    let disambiguated_crate_name_for_dumps = outputs
+                        .with_extension("")
+                        .file_name()
+                        .unwrap()
+                        .to_os_string();
+
+                    link_exe(
+                        sess,
+                        crate_type,
+                        &out_filename,
+                        codegen_results,
+                        &disambiguated_crate_name_for_dumps,
+                    );
                 }
                 other => {
                     sess.err(format!("CrateType {:?} not supported yet", other));
@@ -117,6 +133,7 @@ fn link_exe(
     crate_type: CrateType,
     out_filename: &Path,
     codegen_results: &CodegenResults,
+    disambiguated_crate_name_for_dumps: &OsStr,
 ) {
     let mut objects = Vec::new();
     let mut rlibs = Vec::new();
@@ -140,38 +157,49 @@ fn link_exe(
     // HACK(eddyb) this removes the `.json` in `.spv.json`, from `out_filename`.
     let out_path_spv = out_filename.with_extension("");
 
-    let compile_result = match do_link(sess, &cg_args, &objects, &rlibs) {
+    let link_result = do_link(
+        sess,
+        &cg_args,
+        &objects,
+        &rlibs,
+        disambiguated_crate_name_for_dumps,
+    );
+    let compile_result = match link_result {
         linker::LinkResult::SingleModule(module) => {
-            let module_filename = out_path_spv;
-            post_link_single_module(sess, &cg_args, module.assemble(), &module_filename);
-            cg_args.do_disassemble(&module);
-            let module_result = ModuleResult::SingleModule(module_filename);
+            let entry_points = entry_points(&module);
+            post_link_single_module(sess, &cg_args, *module, &out_path_spv, None);
             CompileResult {
-                module: module_result,
-                entry_points: entry_points(&module),
+                entry_points,
+                module: ModuleResult::SingleModule(out_path_spv),
             }
         }
-        linker::LinkResult::MultipleModules(map) => {
+        linker::LinkResult::MultipleModules {
+            file_stem_to_entry_name_and_module,
+        } => {
             let out_dir = out_path_spv.with_extension("spvs");
             if !out_dir.is_dir() {
                 std::fs::create_dir_all(&out_dir).unwrap();
             }
 
-            let entry_points = map.keys().cloned().collect();
-            let map = map
+            let entry_name_to_file_path: BTreeMap<_, _> = file_stem_to_entry_name_and_module
                 .into_iter()
-                .map(|(name, module)| {
-                    let mut module_filename = out_dir.clone();
-                    module_filename.push(sanitize_filename::sanitize(&name));
-                    module_filename.set_extension("spv");
-                    post_link_single_module(sess, &cg_args, module.assemble(), &module_filename);
-                    (name, module_filename)
+                .map(|(file_stem, (entry_name, module))| {
+                    let mut out_file_name = file_stem;
+                    out_file_name.push(".spv");
+                    let out_file_path = out_dir.join(out_file_name);
+                    post_link_single_module(
+                        sess,
+                        &cg_args,
+                        module,
+                        &out_file_path,
+                        Some(disambiguated_crate_name_for_dumps),
+                    );
+                    (entry_name, out_file_path)
                 })
                 .collect();
-            let module_result = ModuleResult::MultiModule(map);
             CompileResult {
-                module: module_result,
-                entry_points,
+                entry_points: entry_name_to_file_path.keys().cloned().collect(),
+                module: ModuleResult::MultiModule(entry_name_to_file_path),
             }
         }
     };
@@ -192,15 +220,21 @@ fn entry_points(module: &rspirv::dr::Module) -> Vec<String> {
 fn post_link_single_module(
     sess: &Session,
     cg_args: &CodegenArgs,
-    spv_binary: Vec<u32>,
+    module: Module,
     out_filename: &Path,
+    dump_prefix: Option<&OsStr>,
 ) {
+    cg_args.do_disassemble(&module);
+    let spv_binary = module.assemble();
+
     if let Some(dir) = &cg_args.dump_post_link {
-        std::fs::write(
-            dir.join(out_filename.file_name().unwrap()),
-            spirv_tools::binary::from_binary(&spv_binary),
-        )
-        .unwrap();
+        // FIXME(eddyb) rename `filename` with `file_path` to make this less confusing.
+        let out_filename_file_name = out_filename.file_name().unwrap();
+        let dump_path = match dump_prefix {
+            Some(prefix) => dir.join(prefix).with_extension(out_filename_file_name),
+            None => dir.join(out_filename_file_name),
+        };
+        std::fs::write(dump_path, spirv_tools::binary::from_binary(&spv_binary)).unwrap();
     }
 
     let val_options = spirv_tools::val::ValidatorOptions {
@@ -514,20 +548,34 @@ fn do_link(
     cg_args: &CodegenArgs,
     objects: &[PathBuf],
     rlibs: &[PathBuf],
+    disambiguated_crate_name_for_dumps: &OsStr,
 ) -> linker::LinkResult {
-    fn load(bytes: &[u8]) -> rspirv::dr::Module {
-        let mut loader = rspirv::dr::Loader::new();
-        rspirv::binary::parse_bytes(bytes, &mut loader).unwrap();
-        loader.module()
-    }
-
     let load_modules_timer = sess.timer("link_load_modules");
+
     let mut modules = Vec::new();
+    let mut add_module = |file_name: &OsStr, bytes: &[u8]| {
+        let module = {
+            let mut loader = rspirv::dr::Loader::new();
+            rspirv::binary::parse_bytes(bytes, &mut loader).unwrap();
+            loader.module()
+        };
+        if let Some(dir) = &cg_args.dump_pre_link {
+            // FIXME(eddyb) is it a good idea to re-`assemble` the `rspirv::dr`
+            // module, or should this just save the original bytes?
+            std::fs::write(
+                dir.join(file_name).with_extension("spv"),
+                spirv_tools::binary::from_binary(&module.assemble()),
+            )
+            .unwrap();
+        }
+        modules.push(module);
+    };
+
     // `objects` are the plain obj files we need to link - usually produced by the final crate.
     for obj in objects {
-        let bytes = std::fs::read(obj).unwrap();
-        modules.push(load(&bytes));
+        add_module(obj.file_name().unwrap(), &std::fs::read(obj).unwrap());
     }
+
     // `rlibs` are archive files we've created in `create_archive`, usually produced by crates that are being
     // referenced. We need to unpack them and add the modules inside.
     for rlib in rlibs {
@@ -539,24 +587,22 @@ fn do_link(
                 // https://github.com/rust-lang/rust/blob/72868e017bdade60603a25889e253f556305f996/library/std/src/fs.rs#L200-L202
                 let mut bytes = Vec::with_capacity(entry.header().size() as usize + 1);
                 entry.read_to_end(&mut bytes).unwrap();
-                modules.push(load(&bytes));
+
+                let file_name = std::str::from_utf8(entry.header().identifier()).unwrap();
+                add_module(OsStr::new(file_name), &bytes);
             }
         }
     }
 
-    if let Some(dir) = &cg_args.dump_pre_link {
-        for (num, module) in modules.iter().enumerate() {
-            std::fs::write(
-                dir.join(format!("mod_{}.spv", num)),
-                spirv_tools::binary::from_binary(&module.assemble()),
-            )
-            .unwrap();
-        }
-    }
     drop(load_modules_timer);
 
     // Do the link...
-    let link_result = linker::link(sess, modules, &cg_args.linker_opts);
+    let link_result = linker::link(
+        sess,
+        modules,
+        &cg_args.linker_opts,
+        disambiguated_crate_name_for_dumps,
+    );
 
     match link_result {
         Ok(v) => v,

crates/rustc_codegen_spirv/src/linker/mod.rs

@@ -19,12 +19,15 @@ mod zombies;
 use std::borrow::Cow;
 
 use crate::codegen_cx::SpirvMetadata;
+use either::Either;
 use rspirv::binary::{Assemble, Consumer};
 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::Session;
+use std::collections::BTreeMap;
+use std::ffi::{OsStr, OsString};
 use std::path::PathBuf;
 
 pub type Result<T> = std::result::Result<T, ErrorGuaranteed>;
@@ -58,7 +61,13 @@ pub struct Options {
 
 pub enum LinkResult {
     SingleModule(Box<Module>),
-    MultipleModules(FxHashMap<String, Module>),
+    MultipleModules {
+        /// The "file stem" key is computed from the "entry name" in the value
+        /// (through `sanitize_filename`, replacing invalid chars with `-`),
+        /// but it's used as the map key because it *has to* be unique, even if
+        /// lossy sanitization could have erased distinctions between entry names.
+        file_stem_to_entry_name_and_module: BTreeMap<OsString, (String, Module)>,
+    },
 }
 
 fn id(header: &mut ModuleHeader) -> Word {
@@ -131,7 +140,12 @@ fn get_name<'a>(names: &FxHashMap<Word, &'a str>, id: Word) -> Cow<'a, str> {
     )
 }
 
-pub fn link(sess: &Session, mut inputs: Vec<Module>, opts: &Options) -> Result<LinkResult> {
+pub fn link(
+    sess: &Session,
+    mut inputs: Vec<Module>,
+    opts: &Options,
+    disambiguated_crate_name_for_dumps: &OsStr,
+) -> Result<LinkResult> {
     let mut output = {
         let _timer = sess.timer("link_merge");
         // shift all the ids
@@ -167,8 +181,13 @@ pub fn link(sess: &Session, mut inputs: Vec<Module>, opts: &Options) -> Result<L
         output
     };
 
-    if let Some(path) = &opts.dump_post_merge {
-        std::fs::write(path, spirv_tools::binary::from_binary(&output.assemble())).unwrap();
+    if let Some(dir) = &opts.dump_post_merge {
+        std::fs::write(
+            dir.join(disambiguated_crate_name_for_dumps)
+                .with_extension("spv"),
+            spirv_tools::binary::from_binary(&output.assemble()),
+        )
+        .unwrap();
     }
 
     // remove duplicates (https://github.com/KhronosGroup/SPIRV-Tools/blob/e7866de4b1dc2a7e8672867caeb0bdca49f458d3/source/opt/remove_duplicates_pass.cpp)
@@ -379,11 +398,10 @@ pub fn link(sess: &Session, mut inputs: Vec<Module>, opts: &Options) -> Result<L
 
         // 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_file) = &opts.dump_spirt_passes {
-            // HACK(eddyb) using `.with_extension(...)` may replace part of a
-            // `.`-containing file name, but we want to always append `.html`.
-            let mut dump_file_html = dump_file.as_os_str().to_owned();
-            dump_file_html.push(".html");
+        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");
 
             let plan = spirt::print::Plan::for_versions(
                 &cx,
@@ -394,9 +412,9 @@ pub fn link(sess: &Session, mut inputs: Vec<Module>, opts: &Options) -> Result<L
             let pretty = plan.pretty_print();
 
             // FIXME(eddyb) don't allocate whole `String`s here.
-            std::fs::write(dump_file, pretty.to_string()).unwrap();
+            std::fs::write(&dump_spirt_file_path, pretty.to_string()).unwrap();
             std::fs::write(
-                dump_file_html,
+                dump_spirt_file_path.with_extension("spirt.html"),
                 pretty
                     .render_to_html()
                     .with_dark_mode_support()
@@ -443,30 +461,73 @@ pub fn link(sess: &Session, mut inputs: Vec<Module>, opts: &Options) -> Result<L
     }
 
     let mut output = if opts.emit_multiple_modules {
-        let modules = output
-            .entry_points
-            .iter()
-            .map(|entry| {
-                let mut module = output.clone();
-                module.entry_points.clear();
-                module.entry_points.push(entry.clone());
-                let name = entry.operands[2].unwrap_literal_string();
-                (name.to_string(), module)
-            })
-            .collect();
-        LinkResult::MultipleModules(modules)
+        let mut file_stem_to_entry_name_and_module = BTreeMap::new();
+        for (i, entry) in output.entry_points.iter().enumerate() {
+            let mut module = output.clone();
+            module.entry_points.clear();
+            module.entry_points.push(entry.clone());
+            let entry_name = entry.operands[2].unwrap_literal_string().to_string();
+            let mut file_stem = OsString::from(
+                sanitize_filename::sanitize_with_options(
+                    &entry_name,
+                    sanitize_filename::Options {
+                        replacement: "-",
+                        ..Default::default()
+                    },
+                )
+                .replace("--", "-"),
+            );
+            // It's always possible to find an unambiguous `file_stem`, but it
+            // may take two tries (or more, in bizzare/adversarial cases).
+            let mut disambiguator = Some(i);
+            loop {
+                use std::collections::btree_map::Entry;
+                match file_stem_to_entry_name_and_module.entry(file_stem) {
+                    Entry::Vacant(entry) => {
+                        entry.insert((entry_name, module));
+                        break;
+                    }
+                    Entry::Occupied(entry) => {
+                        // FIXME(eddyb) there's no way to access the owned key
+                        // passed to `BTreeMap::entry` from `OccupiedEntry`.
+                        file_stem = entry.key().clone();
+                        file_stem.push(".");
+                        match disambiguator.take() {
+                            Some(d) => file_stem.push(d.to_string()),
+                            None => file_stem.push("next"),
+                        }
+                    }
+                }
+            }
+        }
+        LinkResult::MultipleModules {
+            file_stem_to_entry_name_and_module,
+        }
     } else {
         LinkResult::SingleModule(Box::new(output))
     };
 
-    let output_module_iter: Box<dyn Iterator<Item = &mut Module>> = match output {
-        LinkResult::SingleModule(ref mut m) => Box::new(std::iter::once(&mut *m as &mut Module)),
-        LinkResult::MultipleModules(ref mut m) => Box::new(m.values_mut()),
+    let output_module_iter = match &mut output {
+        LinkResult::SingleModule(m) => Either::Left(std::iter::once((None, &mut **m))),
+        LinkResult::MultipleModules {
+            file_stem_to_entry_name_and_module,
+        } => Either::Right(
+            file_stem_to_entry_name_and_module
+                .iter_mut()
+                .map(|(file_stem, (_, m))| (Some(file_stem), m)),
+        ),
     };
-    for (i, output) in output_module_iter.enumerate() {
+    for (file_stem, output) in output_module_iter {
         if let Some(dir) = &opts.dump_post_split {
+            let mut file_name = disambiguated_crate_name_for_dumps.to_os_string();
+            if let Some(file_stem) = file_stem {
+                file_name.push(".");
+                file_name.push(file_stem);
+            }
+            file_name.push(".spv");
+
             std::fs::write(
-                dir.join(format!("mod_{}.spv", i)),
+                dir.join(file_name),
                 spirv_tools::binary::from_binary(&output.assemble()),
             )
             .unwrap();

crates/rustc_codegen_spirv/src/linker/test.rs

@@ -166,11 +166,11 @@ fn link_with_linker_opts(
                 )
             };
 
-            let res = link(&sess, modules, opts);
+            let res = link(&sess, modules, opts, Default::default());
             assert_eq!(sess.has_errors(), res.as_ref().err().copied());
             res.map(|res| match res {
                 LinkResult::SingleModule(m) => *m,
-                LinkResult::MultipleModules(_) => unreachable!(),
+                LinkResult::MultipleModules { .. } => unreachable!(),
             })
         })
     })

docs/src/codegen-args.md

@@ -72,9 +72,9 @@ for codegen, if the linker panics, this option does nothing, sadly.
 
 Dumps all input modules to the linker, to files in `DIR`, before the linker touches them at all.
 
-### `--dump-post-merge FILE`
+### `--dump-post-merge DIR`
 
-Dumps the merged module, to `FILE`, immediately after merging, but before the linker has done anything else
+Dumps the merged module, to a file in `DIR`, immediately after merging, but before the linker has done anything else
 (including, well, linking the methods - `LinkageAttributes` will still exist, etc.). This is very
 similar to `--dump-pre-link`, except it outputs only a single file, which might make grepping through
 for stuff easier.
@@ -149,7 +149,7 @@ Enables using the experimental [`SPIR-🇹` shader IR framework](https://github.
 
 For more information, also see [the `SPIR-🇹` repository](https://github.com/EmbarkStudios/spirt).
 
-### `--dump-spirt-passes FILE`
+### `--dump-spirt-passes DIR`
 
-Dump the `SPIR-🇹` module across passes (i.e. all of the versions before/after each pass), as a combined report, to `FILE` and `FILE.html`.  
-<sub>(the `.html` version of the report is the recommended form for viewing, as it uses tabling for versions, syntax-highlighting-like styling, and use->def linking)</sub>
+Dump the `SPIR-🇹` module across passes (i.e. all of the versions before/after each pass), as a combined report, to a pair of files (`.spirt` and `.spirt.html`) in `DIR`.  
+<sub>(the `.spirt.html` version of the report is the recommended form for viewing, as it uses tabling for versions, syntax-highlighting-like styling, and use->def linking)</sub>