From 6232fc03a66eee3fec0b16b8ee1a6d2d60ba64e4 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Mon, 17 Apr 2023 09:29:01 +0300 Subject: [PATCH 01/13] Emit `OpSource` for every source file referenced via `OpLine`. --- .../src/builder/builder_methods.rs | 11 +- .../rustc_codegen_spirv/src/builder_spirv.rs | 139 ++++++++++++++++-- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 2 +- .../src/linker/duplicates.rs | 3 + tests/ui/dis/asm_op_decorate.not_spirt.rs | 1 + tests/ui/dis/asm_op_decorate.spirt.rs | 1 + tests/ui/dis/custom_entry_point.not_spirt.rs | 1 + tests/ui/dis/custom_entry_point.spirt.rs | 1 + tests/ui/dis/generic-fn-op-name.not_spirt.rs | 1 + tests/ui/dis/generic-fn-op-name.spirt.rs | 1 + tests/ui/dis/issue-723-output.not_spirt.rs | 1 + tests/ui/dis/issue-723-output.spirt.rs | 1 + .../non-writable-storage_buffer.not_spirt.rs | 1 + .../dis/non-writable-storage_buffer.spirt.rs | 1 + 14 files changed, 145 insertions(+), 20 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index 312b6f5366..dd49b94888 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -657,11 +657,12 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { fn set_span(&mut self, span: Span) { self.current_span = Some(span); let loc = self.cx.tcx.sess.source_map().lookup_char_pos(span.lo()); - let file = self - .builder - .def_string(format!("{}", loc.file.name.prefer_remapped())); - self.emit() - .line(file, loc.line as u32, loc.col_display as u32); + let file = self.builder.def_debug_file(loc.file); + self.emit().line( + file.file_name_op_string_id, + loc.line as u32, + loc.col_display as u32, + ); } // FIXME(eddyb) change `Self::Function` to be more like a function index. diff --git a/crates/rustc_codegen_spirv/src/builder_spirv.rs b/crates/rustc_codegen_spirv/src/builder_spirv.rs index 0c5e660e38..6e91a7897b 100644 --- a/crates/rustc_codegen_spirv/src/builder_spirv.rs +++ b/crates/rustc_codegen_spirv/src/builder_spirv.rs @@ -5,14 +5,20 @@ use crate::symbols::Symbols; use crate::target::SpirvTarget; use crate::target_feature::TargetFeature; use rspirv::dr::{Block, Builder, Module, Operand}; -use rspirv::spirv::{AddressingModel, Capability, MemoryModel, Op, StorageClass, Word}; +use rspirv::spirv::{ + AddressingModel, Capability, MemoryModel, Op, SourceLanguage, StorageClass, Word, +}; use rspirv::{binary::Assemble, binary::Disassemble}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::sync::Lrc; use rustc_middle::bug; +use rustc_span::source_map::SourceMap; use rustc_span::symbol::Symbol; -use rustc_span::{Span, DUMMY_SP}; +use rustc_span::{SourceFile, Span, DUMMY_SP}; use std::assert_matches::assert_matches; use std::cell::{RefCell, RefMut}; +use std::hash::{Hash, Hasher}; +use std::iter; use std::{fs::File, io::Write, path::Path}; #[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq, Hash)] @@ -321,6 +327,42 @@ struct WithConstLegality { legal: Result<(), IllegalConst>, } +/// `HashMap` key type (for `debug_file_cache` in `BuilderSpirv`), which is +/// equivalent to a whole `rustc` `SourceFile`, but has O(1) `Eq` and `Hash` +/// implementations (i.e. not involving the path or the contents of the file). +/// +/// This is possible because we can compare `Lrc`s by equality, as +/// `rustc`'s `SourceMap` already ensures that only one `SourceFile` will be +/// allocated for some given file. For hashing, we could hash the address, or +/// +struct DebugFileKey(Lrc); + +impl PartialEq for DebugFileKey { + fn eq(&self, other: &Self) -> bool { + let (Self(self_sf), Self(other_sf)) = (self, other); + Lrc::ptr_eq(self_sf, other_sf) + } +} +impl Eq for DebugFileKey {} + +impl Hash for DebugFileKey { + fn hash(&self, state: &mut H) { + let Self(sf) = self; + sf.name_hash.hash(state); + sf.src_hash.hash(state); + } +} + +#[derive(Copy, Clone)] +pub struct DebugFileSpirv { + /// The SPIR-V ID for the result of the `OpString` instruction containing + /// the file name - this is what e.g. `OpLine` uses to identify the file. + /// + /// All other details about the file are also attached to this ID, using + /// other instructions that don't produce their own IDs (e.g. `OpSource`). + pub file_name_op_string_id: Word, +} + /// Cursor system: /// /// The LLVM module builder model (and therefore `codegen_ssa`) assumes that there is a central @@ -351,6 +393,8 @@ pub struct BuilderCursor { } pub struct BuilderSpirv<'tcx> { + source_map: &'tcx SourceMap, + builder: RefCell, // Bidirectional maps between `SpirvConst` and the ID of the defined global @@ -359,14 +403,20 @@ pub struct BuilderSpirv<'tcx> { // allows getting that legality information without additional lookups. const_to_id: RefCell>, WithConstLegality>>, id_to_const: RefCell>>>, - string_cache: RefCell>, + + debug_file_cache: RefCell>, enabled_capabilities: FxHashSet, enabled_extensions: FxHashSet, } impl<'tcx> BuilderSpirv<'tcx> { - pub fn new(sym: &Symbols, target: &SpirvTarget, features: &[TargetFeature]) -> Self { + pub fn new( + source_map: &'tcx SourceMap, + sym: &Symbols, + target: &SpirvTarget, + features: &[TargetFeature], + ) -> Self { let version = target.spirv_version(); let memory_model = target.memory_model(); @@ -427,10 +477,11 @@ impl<'tcx> BuilderSpirv<'tcx> { builder.memory_model(AddressingModel::Logical, memory_model); Self { + source_map, builder: RefCell::new(builder), const_to_id: Default::default(), id_to_const: Default::default(), - string_cache: Default::default(), + debug_file_cache: Default::default(), enabled_capabilities, enabled_extensions, } @@ -637,15 +688,75 @@ impl<'tcx> BuilderSpirv<'tcx> { } } - pub fn def_string(&self, s: String) -> Word { - use std::collections::hash_map::Entry; - match self.string_cache.borrow_mut().entry(s) { - Entry::Occupied(entry) => *entry.get(), - Entry::Vacant(entry) => { - let key = entry.key().clone(); - *entry.insert(self.builder(Default::default()).string(key)) - } - } + pub fn def_debug_file(&self, sf: Lrc) -> DebugFileSpirv { + *self + .debug_file_cache + .borrow_mut() + .entry(DebugFileKey(sf)) + .or_insert_with_key(|DebugFileKey(sf)| { + let mut builder = self.builder(Default::default()); + + // FIXME(eddyb) remapping might be really bad for being able to + // load the sources the later, maybe it should be done at the + // very end? (just before linking outputting the final SPIR-V) + let file_name_op_string_id = builder.string(sf.name.prefer_remapped().to_string()); + + let file_contents = self + .source_map + .span_to_snippet(Span::with_root_ctxt(sf.start_pos, sf.end_pos)) + .ok(); + + // HACK(eddyb) this logic is duplicated from `spirt::spv::lift`. + let op_source_and_continued_chunks = file_contents.as_ref().map(|contents| { + // The maximum word count is `2**16 - 1`, the first word is + // taken up by the opcode & word count, and one extra byte is + // taken up by the nil byte at the end of the LiteralString. + const MAX_OP_SOURCE_CONT_CONTENTS_LEN: usize = (0xffff - 1) * 4 - 1; + + // `OpSource` has 3 more operands than `OpSourceContinued`, + // and each of them take up exactly one word. + const MAX_OP_SOURCE_CONTENTS_LEN: usize = + MAX_OP_SOURCE_CONT_CONTENTS_LEN - 3 * 4; + + let (op_source_str, mut all_op_source_continued_str) = + contents.split_at(contents.len().min(MAX_OP_SOURCE_CONTENTS_LEN)); + + // FIXME(eddyb) `spirt::spv::lift` should use this. + let all_op_source_continued_str_chunks = iter::from_fn(move || { + let contents_rest = &mut all_op_source_continued_str; + if contents_rest.is_empty() { + return None; + } + + // FIXME(eddyb) test with UTF-8! this `split_at` should + // actually take *less* than the full possible size, to + // avoid cutting a UTF-8 sequence. + let (cont_chunk, rest) = contents_rest + .split_at(contents_rest.len().min(MAX_OP_SOURCE_CONT_CONTENTS_LEN)); + *contents_rest = rest; + Some(cont_chunk) + }); + (op_source_str, all_op_source_continued_str_chunks) + }); + + if let Some((op_source_str, all_op_source_continued_str_chunks)) = + op_source_and_continued_chunks + { + builder.source( + SourceLanguage::Unknown, + 0, + Some(file_name_op_string_id), + Some(op_source_str), + ); + for cont_chunk in all_op_source_continued_str_chunks { + builder.source_continued(cont_chunk); + } + } + + DebugFileSpirv { + file_name_op_string_id, + } + }) } pub fn set_global_initializer(&self, global: Word, initializer: Word) { diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index d1e7add8b4..87bc233abf 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -117,7 +117,7 @@ impl<'tcx> CodegenCx<'tcx> { Self { tcx, codegen_unit, - builder: BuilderSpirv::new(&sym, &target, &features), + builder: BuilderSpirv::new(tcx.sess.source_map(), &sym, &target, &features), instances: Default::default(), function_parameter_values: Default::default(), type_cache: Default::default(), diff --git a/crates/rustc_codegen_spirv/src/linker/duplicates.rs b/crates/rustc_codegen_spirv/src/linker/duplicates.rs index 7e9e0f7ff0..24ccfe8802 100644 --- a/crates/rustc_codegen_spirv/src/linker/duplicates.rs +++ b/crates/rustc_codegen_spirv/src/linker/duplicates.rs @@ -5,6 +5,9 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_middle::bug; use std::collections::hash_map; +// FIXME(eddyb) consider deduplicating the `OpString` and `OpSource` created for +// file-level debuginfo (but using SPIR-T for linking might be better?). + pub fn remove_duplicate_extensions(module: &mut Module) { let mut set = FxHashSet::default(); diff --git a/tests/ui/dis/asm_op_decorate.not_spirt.rs b/tests/ui/dis/asm_op_decorate.not_spirt.rs index 9b32e9871f..f8460b8f4d 100644 --- a/tests/ui/dis/asm_op_decorate.not_spirt.rs +++ b/tests/ui/dis/asm_op_decorate.not_spirt.rs @@ -6,6 +6,7 @@ // compile-flags: -C target-feature=+RuntimeDescriptorArray,+ext:SPV_EXT_descriptor_indexing // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/asm_op_decorate.spirt.rs b/tests/ui/dis/asm_op_decorate.spirt.rs index 973b80a609..be47ede266 100644 --- a/tests/ui/dis/asm_op_decorate.spirt.rs +++ b/tests/ui/dis/asm_op_decorate.spirt.rs @@ -6,6 +6,7 @@ // compile-flags: -C target-feature=+RuntimeDescriptorArray,+ext:SPV_EXT_descriptor_indexing // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/custom_entry_point.not_spirt.rs b/tests/ui/dis/custom_entry_point.not_spirt.rs index b2e8391214..8d1db083f4 100644 --- a/tests/ui/dis/custom_entry_point.not_spirt.rs +++ b/tests/ui/dis/custom_entry_point.not_spirt.rs @@ -5,6 +5,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/custom_entry_point.spirt.rs b/tests/ui/dis/custom_entry_point.spirt.rs index f5117322ca..6ef6a7dbe9 100644 --- a/tests/ui/dis/custom_entry_point.spirt.rs +++ b/tests/ui/dis/custom_entry_point.spirt.rs @@ -5,6 +5,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/generic-fn-op-name.not_spirt.rs b/tests/ui/dis/generic-fn-op-name.not_spirt.rs index 48ef785d67..a7629f5908 100644 --- a/tests/ui/dis/generic-fn-op-name.not_spirt.rs +++ b/tests/ui/dis/generic-fn-op-name.not_spirt.rs @@ -7,6 +7,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" #![feature(adt_const_params)] diff --git a/tests/ui/dis/generic-fn-op-name.spirt.rs b/tests/ui/dis/generic-fn-op-name.spirt.rs index ce29fb428b..4fbf96678e 100644 --- a/tests/ui/dis/generic-fn-op-name.spirt.rs +++ b/tests/ui/dis/generic-fn-op-name.spirt.rs @@ -7,6 +7,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" #![feature(adt_const_params)] diff --git a/tests/ui/dis/issue-723-output.not_spirt.rs b/tests/ui/dis/issue-723-output.not_spirt.rs index aab775a377..24dd50b82d 100644 --- a/tests/ui/dis/issue-723-output.not_spirt.rs +++ b/tests/ui/dis/issue-723-output.not_spirt.rs @@ -17,6 +17,7 @@ // build-pass // compile-flags: -C debuginfo=0 -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/issue-723-output.spirt.rs b/tests/ui/dis/issue-723-output.spirt.rs index 4a9a46b238..96e056b04b 100644 --- a/tests/ui/dis/issue-723-output.spirt.rs +++ b/tests/ui/dis/issue-723-output.spirt.rs @@ -17,6 +17,7 @@ // build-pass // compile-flags: -C debuginfo=0 -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs b/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs index ba25af924f..0b89f048dd 100644 --- a/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs +++ b/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs @@ -7,6 +7,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/non-writable-storage_buffer.spirt.rs b/tests/ui/dis/non-writable-storage_buffer.spirt.rs index 5548788222..c9c0d083c0 100644 --- a/tests/ui/dis/non-writable-storage_buffer.spirt.rs +++ b/tests/ui/dis/non-writable-storage_buffer.spirt.rs @@ -7,6 +7,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" From d63ab26055d5a317250f0ce0f216ecfb0aae8016 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 18 Apr 2023 03:44:34 +0300 Subject: [PATCH 02/13] linker/zombies: regenerate rustc `SourceFile`s from `OpSource`. --- .../rustc_codegen_spirv/src/builder_spirv.rs | 6 +- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 2 +- crates/rustc_codegen_spirv/src/decorations.rs | 232 ++++++++++++------ .../rustc_codegen_spirv/src/linker/zombies.rs | 77 +++--- 4 files changed, 206 insertions(+), 111 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/builder_spirv.rs b/crates/rustc_codegen_spirv/src/builder_spirv.rs index 6e91a7897b..f513f3f160 100644 --- a/crates/rustc_codegen_spirv/src/builder_spirv.rs +++ b/crates/rustc_codegen_spirv/src/builder_spirv.rs @@ -393,7 +393,8 @@ pub struct BuilderCursor { } pub struct BuilderSpirv<'tcx> { - source_map: &'tcx SourceMap, + // HACK(eddyb) public only for `decorations`. + pub(crate) source_map: &'tcx SourceMap, builder: RefCell, @@ -696,9 +697,6 @@ impl<'tcx> BuilderSpirv<'tcx> { .or_insert_with_key(|DebugFileKey(sf)| { let mut builder = self.builder(Default::default()); - // FIXME(eddyb) remapping might be really bad for being able to - // load the sources the later, maybe it should be done at the - // very end? (just before linking outputting the final SPIR-V) let file_name_op_string_id = builder.string(sf.name.prefer_remapped().to_string()); let file_contents = self diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index 87bc233abf..6c5943eb99 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -189,7 +189,7 @@ impl<'tcx> CodegenCx<'tcx> { word, ZombieDecoration { reason: reason.to_string(), - span: SerializedSpan::from_rustc(span, self.tcx.sess.source_map()), + span: SerializedSpan::from_rustc(span, &self.builder), }, ); } diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index b4a937bab7..2a317ecc7a 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -1,10 +1,16 @@ //! SPIR-V decorations specific to `rustc_codegen_spirv`, produced during //! the original codegen of a crate, and consumed by the `linker`. +use crate::builder_spirv::BuilderSpirv; use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Decoration, Op, Word}; -use rustc_span::{source_map::SourceMap, FileName, Pos, Span}; +use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; +use rustc_data_structures::sync::Lrc; +use rustc_span::{source_map::SourceMap, Pos, Span}; +use rustc_span::{FileName, SourceFile}; use serde::{Deserialize, Serialize}; +use smallvec::SmallVec; +use std::borrow::Cow; use std::marker::PhantomData; use std::path::PathBuf; use std::{iter, slice}; @@ -56,7 +62,7 @@ pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize { Some(( id, LazilyDeserialized { - json, + json: json.into(), _marker: PhantomData, }, )) @@ -87,15 +93,32 @@ type DecodeAllIter<'a, D> = iter::FilterMap< >; /// Helper allowing full deserialization to be avoided where possible. -#[derive(Copy, Clone)] pub struct LazilyDeserialized<'a, D> { - json: &'a str, + json: Cow<'a, str>, _marker: PhantomData, } -impl<'a, D: Deserialize<'a>> LazilyDeserialized<'a, D> { - pub fn deserialize(self) -> D { - serde_json::from_str(self.json).unwrap() +impl Clone for LazilyDeserialized<'_, D> { + fn clone(&self) -> Self { + let Self { ref json, _marker } = *self; + Self { + json: json.clone(), + _marker, + } + } +} + +impl Deserialize<'a>> LazilyDeserialized<'_, D> { + pub fn deserialize(&self) -> D { + serde_json::from_str(&self.json).unwrap() + } + + pub fn into_owned(self) -> LazilyDeserialized<'static, D> { + let Self { json, _marker } = self; + LazilyDeserialized { + json: json.into_owned().into(), + _marker, + } } } @@ -112,59 +135,16 @@ impl CustomDecoration for ZombieDecoration { } /// Representation of a `rustc` `Span` that can be turned into a `Span` again -/// in another compilation, by reloading the file. However, note that this will -/// fail if the file changed since, which is detected using the serialized `hash`. +/// in another compilation, by regenerating the `rustc` `SourceFile`. #[derive(Deserialize, Serialize)] pub struct SerializedSpan { - file: PathBuf, - hash: serde_adapters::SourceFileHash, + file_name: String, lo: u32, hi: u32, } -// HACK(eddyb) `rustc_span` types implement only `rustc_serialize` traits, but -// not `serde` traits, and the easiest workaround is to have our own types. -mod serde_adapters { - use serde::{Deserialize, Serialize}; - - #[derive(Copy, Clone, PartialEq, Eq, Deserialize, Serialize)] - pub enum SourceFileHashAlgorithm { - Md5, - Sha1, - Sha256, - } - - impl From for SourceFileHashAlgorithm { - fn from(kind: rustc_span::SourceFileHashAlgorithm) -> Self { - match kind { - rustc_span::SourceFileHashAlgorithm::Md5 => Self::Md5, - rustc_span::SourceFileHashAlgorithm::Sha1 => Self::Sha1, - rustc_span::SourceFileHashAlgorithm::Sha256 => Self::Sha256, - } - } - } - - #[derive(Copy, Clone, PartialEq, Eq, Deserialize, Serialize)] - pub struct SourceFileHash { - kind: SourceFileHashAlgorithm, - value: [u8; 32], - } - - impl From for SourceFileHash { - fn from(hash: rustc_span::SourceFileHash) -> Self { - let bytes = hash.hash_bytes(); - let mut hash = Self { - kind: hash.kind.into(), - value: Default::default(), - }; - hash.value[..bytes.len()].copy_from_slice(bytes); - hash - } - } -} - impl SerializedSpan { - pub fn from_rustc(span: Span, source_map: &SourceMap) -> Option { + pub fn from_rustc(span: Span, builder: &BuilderSpirv<'_>) -> Option { // Decorations may not always have valid spans. // FIXME(eddyb) reduce the sources of this as much as possible. if span.is_dummy() { @@ -177,42 +157,150 @@ impl SerializedSpan { return None; } - let file = source_map.lookup_source_file(lo); + let file = builder.source_map.lookup_source_file(lo); if !(file.start_pos <= lo && hi <= file.end_pos) { // FIXME(eddyb) broken `Span` - potentially turn this into an assert? return None; } + // NOTE(eddyb) this emits necessary `OpString`/`OpSource` instructions. + builder.def_debug_file(file.clone()); + Some(Self { - file: match &file.name { - // We can only support real files, not "synthetic" ones (which - // are almost never exposed to the compiler backend anyway). - FileName::Real(real_name) => real_name.local_path()?.to_path_buf(), - _ => return None, - }, - hash: file.src_hash.into(), + file_name: file.name.prefer_remapped().to_string(), lo: (lo - file.start_pos).to_u32(), hi: (hi - file.start_pos).to_u32(), }) } +} - pub fn to_rustc(&self, source_map: &SourceMap) -> Option { - let file = source_map.load_file(&self.file).ok()?; +/// Helper type to delay most of the work necessary to turn a `SerializedSpan` +/// back into an usable `Span`, until it's actually needed (i.e. for an error). +pub struct SpanRegenerator<'a> { + source_map: &'a SourceMap, + module: &'a Module, - // If the file has changed since serializing, there's not much we can do, - // other than avoid creating invalid/confusing `Span`s. - // FIXME(eddyb) we could still indicate some of this to the user. - if self.hash != file.src_hash.into() { - return None; + // HACK(eddyb) this is mostly replicating SPIR-T's module-level debuginfo. + spv_debug_files: Option>>, +} + +// HACK(eddyb) this is mostly replicating SPIR-T's module-level debuginfo. +#[derive(Default)] +struct SpvDebugFile<'a> { + /// Source strings from one `OpSource`, and any number of `OpSourceContinued`. + op_source_parts: SmallVec<[&'a str; 1]>, + + regenerated_rustc_source_file: Option>, +} + +impl<'a> SpanRegenerator<'a> { + pub fn new(source_map: &'a SourceMap, module: &'a Module) -> Self { + Self { + source_map, + module, + spv_debug_files: None, } + } + + fn regenerate_rustc_source_file(&mut self, file_name: &str) -> Option<&SourceFile> { + let spv_debug_files = self.spv_debug_files.get_or_insert_with(|| { + let mut op_string_by_id = FxHashMap::default(); + let mut spv_debug_files = FxIndexMap::default(); + let mut insts = self.module.debug_string_source.iter().peekable(); + while let Some(inst) = insts.next() { + match inst.class.opcode { + Op::String => { + op_string_by_id.insert( + inst.result_id.unwrap(), + inst.operands[0].unwrap_literal_string(), + ); + } + Op::Source if inst.operands.len() == 4 => { + let file_name_id = inst.operands[2].unwrap_id_ref(); + if let Some(&file_name) = op_string_by_id.get(&file_name_id) { + let mut file = SpvDebugFile::default(); + file.op_source_parts + .push(inst.operands[3].unwrap_literal_string()); + while let Some(&next_inst) = insts.peek() { + if next_inst.class.opcode != Op::SourceContinued { + break; + } + insts.next(); + + file.op_source_parts + .push(next_inst.operands[0].unwrap_literal_string()); + } + + // FIXME(eddyb) what if the file is already present, + // should it be considered ambiguous overall? + spv_debug_files.insert(file_name, file); + } + } + _ => {} + } + } + spv_debug_files + }); + let spv_debug_file = spv_debug_files.get_mut(file_name)?; + + let file = &mut spv_debug_file.regenerated_rustc_source_file; + if file.is_none() { + // FIXME(eddyb) reduce allocations here by checking if the file is + // already loaded, and not allocating just to compare the source, + // but at least it's cheap when `OpSourceContinued` isn't used. + let src = match &spv_debug_file.op_source_parts[..] { + &[part] => Cow::Borrowed(part), + parts => parts.concat().into(), + }; + + // HACK(eddyb) in case the file has changed, and because `SourceMap` + // is strictly monotonic, we need to come up with some other name. + let mut sm_file_name_candidates = [PathBuf::from(file_name).into()] + .into_iter() + .chain((0..).map(|i| FileName::Custom(format!("outdated({i}) {file_name}")))); + + *file = sm_file_name_candidates.find_map(|sm_file_name_candidate| { + let sf = self + .source_map + .new_source_file(sm_file_name_candidate, src.clone().into_owned()); + + // Only use this `FileName` candidate if we either: + // 1. reused a `SourceFile` with the right `src`/`external_src` + // 2. allocated a new `SourceFile` with our choice of `src` + self.source_map + .ensure_source_file_source_present(sf.clone()); + let sf_src_matches = sf + .src + .as_ref() + .map(|sf_src| sf_src[..] == src[..]) + .or_else(|| { + sf.external_src + .borrow() + .get_source() + .map(|sf_src| sf_src[..] == src[..]) + }) + .unwrap_or(false); + + if sf_src_matches { + Some(sf) + } else { + None + } + }); + } + file.as_deref() + } + + pub fn serialized_span_to_rustc(&mut self, span: &SerializedSpan) -> Option { + let file = self.regenerate_rustc_source_file(&span.file_name[..])?; // Sanity check - assuming `SerializedSpan` isn't corrupted, this assert - // could only ever fail because of a hash collision. - assert!(self.lo <= self.hi && self.hi <= (file.end_pos.0 - file.start_pos.0)); + // could only ever fail because of the file name being ambiguous. + assert!(span.lo <= span.hi && span.hi <= (file.end_pos.0 - file.start_pos.0)); Some(Span::with_root_ctxt( - file.start_pos + Pos::from_u32(self.lo), - file.start_pos + Pos::from_u32(self.hi), + file.start_pos + Pos::from_u32(span.lo), + file.start_pos + Pos::from_u32(span.hi), )) } } diff --git a/crates/rustc_codegen_spirv/src/linker/zombies.rs b/crates/rustc_codegen_spirv/src/linker/zombies.rs index d2e4c14dad..0a90ffd56c 100644 --- a/crates/rustc_codegen_spirv/src/linker/zombies.rs +++ b/crates/rustc_codegen_spirv/src/linker/zombies.rs @@ -1,33 +1,25 @@ //! See documentation on `CodegenCx::zombie` for a description of the zombie system. use super::{get_name, get_names}; -use crate::decorations::{CustomDecoration, ZombieDecoration}; +use crate::decorations::{CustomDecoration, LazilyDeserialized, SpanRegenerator, ZombieDecoration}; use rspirv::dr::{Instruction, Module}; use rspirv::spirv::{Op, Word}; use rustc_data_structures::fx::FxHashMap; use rustc_session::Session; -use rustc_span::{Span, DUMMY_SP}; +use rustc_span::DUMMY_SP; use std::iter::once; +// FIXME(eddyb) change this to chain through IDs instead of wasting allocations. #[derive(Clone)] struct ZombieInfo<'a> { - reason: &'a str, - span: Span, + serialized: &'a LazilyDeserialized<'static, ZombieDecoration>, stack: Vec, } impl<'a> ZombieInfo<'a> { - fn new(reason: &'a str, span: Span) -> Self { - Self { - reason, - span, - stack: Vec::new(), - } - } fn push_stack(&self, word: Word) -> Self { Self { - reason: self.reason, - span: self.span, + serialized: self.serialized, stack: self.stack.iter().cloned().chain(once(word)).collect(), } } @@ -66,7 +58,7 @@ fn is_or_contains_zombie<'h, 'a>( result_zombie.or_else(|| contains_zombie(inst, zombie)) } -fn spread_zombie(module: &mut Module, zombie: &mut FxHashMap>) -> bool { +fn spread_zombie(module: &Module, zombie: &mut FxHashMap>) -> bool { let mut any = false; // globals are easy for inst in module.global_inst_iter() { @@ -123,12 +115,18 @@ fn report_error_zombies( module: &Module, zombie: &FxHashMap>, ) -> super::Result<()> { + let mut span_regen = SpanRegenerator::new(sess.source_map(), module); + let mut result = Ok(()); let mut names = None; for root in super::dce::collect_roots(module) { - if let Some(reason) = zombie.get(&root) { + if let Some(zombie_info) = zombie.get(&root) { + let ZombieDecoration { reason, span } = zombie_info.serialized.deserialize(); + let span = span + .and_then(|span| span_regen.serialized_span_to_rustc(&span)) + .unwrap_or(DUMMY_SP); let names = names.get_or_insert_with(|| get_names(module)); - let stack = reason + let stack = zombie_info .stack .iter() .map(|&s| get_name(names, s).into_owned()); @@ -136,10 +134,7 @@ fn report_error_zombies( .chain(stack) .collect::>() .join("\n"); - result = Err(sess - .struct_span_err(reason.span, reason.reason) - .note(&stack_note) - .emit()); + result = Err(sess.struct_span_err(span, reason).note(&stack_note).emit()); } } result @@ -150,20 +145,25 @@ pub fn remove_zombies( opts: &super::Options, module: &mut Module, ) -> super::Result<()> { + // FIXME(eddyb) combine these two steps to take the original strings, + // instead of effectively cloning them (via `.into_owned()`). let zombies_owned = ZombieDecoration::decode_all(module) - .map(|(id, zombie)| { - let ZombieDecoration { reason, span } = zombie.deserialize(); - let span = span - .and_then(|span| span.to_rustc(sess.source_map())) - .unwrap_or(DUMMY_SP); - (id, (reason, span)) - }) + .map(|(id, zombie)| (id, zombie.into_owned())) .collect::>(); + ZombieDecoration::remove_all(module); + let mut zombies = zombies_owned .iter() - .map(|(id, (reason, span))| (*id, ZombieInfo::new(reason, *span))) + .map(|(id, serialized)| { + ( + *id, + ZombieInfo { + serialized, + stack: vec![], + }, + ) + }) .collect(); - ZombieDecoration::remove_all(module); // Note: This is O(n^2). while spread_zombie(module, &mut zombies) {} @@ -171,22 +171,31 @@ pub fn remove_zombies( // FIXME(eddyb) use `log`/`tracing` instead. if opts.print_all_zombie { - for (&zomb, reason) in &zombies { - let orig = if zombies_owned.iter().any(|&(z, _)| z == zomb) { + for (&zombie_id, zombie_info) in &zombies { + let orig = if zombies_owned.iter().any(|&(z, _)| z == zombie_id) { "original" } else { "infected" }; - println!("zombie'd {} because {} ({})", zomb, reason.reason, orig); + println!( + "zombie'd {} because {} ({})", + zombie_id, + zombie_info.serialized.deserialize().reason, + orig + ); } } if opts.print_zombie { let names = get_names(module); for f in &module.functions { - if let Some(reason) = is_zombie(f.def.as_ref().unwrap(), &zombies) { + if let Some(zombie_info) = is_zombie(f.def.as_ref().unwrap(), &zombies) { let name = get_name(&names, f.def_id().unwrap()); - println!("Function removed {:?} because {:?}", name, reason.reason); + println!( + "Function removed {:?} because {:?}", + name, + zombie_info.serialized.deserialize().reason + ); } } } From cc818dc109e816422386277e218363add4d50c60 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 18 Apr 2023 04:01:09 +0300 Subject: [PATCH 03/13] decorations: limit zombie `SerializedSpan`s to an `OpLine` equivalent. --- crates/rustc_codegen_spirv/src/decorations.rs | 20 +++++++------------ tests/ui/dis/ptr_copy.normal.stderr | 2 +- .../consts/nested-ref-in-composite.stderr | 4 ++-- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index 2a317ecc7a..322d3067dc 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -139,8 +139,9 @@ impl CustomDecoration for ZombieDecoration { #[derive(Deserialize, Serialize)] pub struct SerializedSpan { file_name: String, + // NOTE(eddyb) by keeping `lo` but not `hi`, we mimick `OpLine` limitations + // (which could be lifted in the future using custom SPIR-T debuginfo). lo: u32, - hi: u32, } impl SerializedSpan { @@ -151,14 +152,10 @@ impl SerializedSpan { return None; } - let (lo, hi) = (span.lo(), span.hi()); - if lo > hi { - // FIXME(eddyb) broken `Span` - potentially turn this into an assert? - return None; - } + let lo = span.lo(); let file = builder.source_map.lookup_source_file(lo); - if !(file.start_pos <= lo && hi <= file.end_pos) { + if !(file.start_pos..=file.end_pos).contains(&lo) { // FIXME(eddyb) broken `Span` - potentially turn this into an assert? return None; } @@ -169,7 +166,6 @@ impl SerializedSpan { Some(Self { file_name: file.name.prefer_remapped().to_string(), lo: (lo - file.start_pos).to_u32(), - hi: (hi - file.start_pos).to_u32(), }) } } @@ -296,11 +292,9 @@ impl<'a> SpanRegenerator<'a> { // Sanity check - assuming `SerializedSpan` isn't corrupted, this assert // could only ever fail because of the file name being ambiguous. - assert!(span.lo <= span.hi && span.hi <= (file.end_pos.0 - file.start_pos.0)); + assert!(span.lo <= (file.end_pos.0 - file.start_pos.0)); - Some(Span::with_root_ctxt( - file.start_pos + Pos::from_u32(span.lo), - file.start_pos + Pos::from_u32(span.hi), - )) + let lo = file.start_pos + Pos::from_u32(span.lo); + Some(Span::with_root_ctxt(lo, lo)) } } diff --git a/tests/ui/dis/ptr_copy.normal.stderr b/tests/ui/dis/ptr_copy.normal.stderr index c9f66e2d67..2e27465411 100644 --- a/tests/ui/dis/ptr_copy.normal.stderr +++ b/tests/ui/dis/ptr_copy.normal.stderr @@ -2,7 +2,7 @@ error: Cannot memcpy dynamically sized data --> $CORE_SRC/intrinsics.rs:2460:9 | 2460 | copy(src, dst, count) - | ^^^^^^^^^^^^^^^^^^^^^ + | ^ | = note: Stack: core::intrinsics::copy:: diff --git a/tests/ui/lang/consts/nested-ref-in-composite.stderr b/tests/ui/lang/consts/nested-ref-in-composite.stderr index 0844de7a6b..9f5c2a7aba 100644 --- a/tests/ui/lang/consts/nested-ref-in-composite.stderr +++ b/tests/ui/lang/consts/nested-ref-in-composite.stderr @@ -2,7 +2,7 @@ error: constant arrays/structs cannot contain pointers to other constants --> $DIR/nested-ref-in-composite.rs:20:17 | 20 | *pair_out = pair_deep_load(&(&123, &3.14)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^ | = note: Stack: nested_ref_in_composite::main_pair @@ -12,7 +12,7 @@ error: constant arrays/structs cannot contain pointers to other constants --> $DIR/nested-ref-in-composite.rs:25:19 | 25 | *array3_out = array3_deep_load(&[&0, &1, &2]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^ | = note: Stack: nested_ref_in_composite::main_array3 From 78934d7dbb87d04fd279a77c951fc5ab5b0a7d4e Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 18 Apr 2023 12:07:49 +0300 Subject: [PATCH 04/13] decorations: allow zero-copy deserialization of strings. --- .../src/builder/builder_methods.rs | 9 +-- .../rustc_codegen_spirv/src/builder_spirv.rs | 61 ++++++++++++++++--- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 8 ++- crates/rustc_codegen_spirv/src/decorations.rs | 35 ++++++----- crates/rustc_codegen_spirv/src/lib.rs | 1 + .../rustc_codegen_spirv/src/linker/zombies.rs | 2 +- 6 files changed, 81 insertions(+), 35 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index dd49b94888..439f39a304 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -656,13 +656,8 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { fn set_span(&mut self, span: Span) { self.current_span = Some(span); - let loc = self.cx.tcx.sess.source_map().lookup_char_pos(span.lo()); - let file = self.builder.def_debug_file(loc.file); - self.emit().line( - file.file_name_op_string_id, - loc.line as u32, - loc.col_display as u32, - ); + let (file, line, col) = self.builder.file_line_col_for_op_line(span); + self.emit().line(file.file_name_op_string_id, line, col); } // FIXME(eddyb) change `Self::Function` to be more like a function index. diff --git a/crates/rustc_codegen_spirv/src/builder_spirv.rs b/crates/rustc_codegen_spirv/src/builder_spirv.rs index f513f3f160..ef95f3c823 100644 --- a/crates/rustc_codegen_spirv/src/builder_spirv.rs +++ b/crates/rustc_codegen_spirv/src/builder_spirv.rs @@ -9,16 +9,19 @@ use rspirv::spirv::{ AddressingModel, Capability, MemoryModel, Op, SourceLanguage, StorageClass, Word, }; use rspirv::{binary::Assemble, binary::Disassemble}; +use rustc_arena::DroplessArena; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::sync::Lrc; use rustc_middle::bug; +use rustc_middle::ty::TyCtxt; use rustc_span::source_map::SourceMap; use rustc_span::symbol::Symbol; -use rustc_span::{SourceFile, Span, DUMMY_SP}; +use rustc_span::{FileName, FileNameDisplayPreference, SourceFile, Span, DUMMY_SP}; use std::assert_matches::assert_matches; use std::cell::{RefCell, RefMut}; use std::hash::{Hash, Hasher}; use std::iter; +use std::str; use std::{fs::File, io::Write, path::Path}; #[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq, Hash)] @@ -354,9 +357,11 @@ impl Hash for DebugFileKey { } #[derive(Copy, Clone)] -pub struct DebugFileSpirv { +pub struct DebugFileSpirv<'tcx> { + pub file_name: &'tcx str, + /// The SPIR-V ID for the result of the `OpString` instruction containing - /// the file name - this is what e.g. `OpLine` uses to identify the file. + /// `file_name` - this is what e.g. `OpLine` uses to identify the file. /// /// All other details about the file are also attached to this ID, using /// other instructions that don't produce their own IDs (e.g. `OpSource`). @@ -395,6 +400,7 @@ pub struct BuilderCursor { pub struct BuilderSpirv<'tcx> { // HACK(eddyb) public only for `decorations`. pub(crate) source_map: &'tcx SourceMap, + dropless_arena: &'tcx DroplessArena, builder: RefCell, @@ -405,7 +411,7 @@ pub struct BuilderSpirv<'tcx> { const_to_id: RefCell>, WithConstLegality>>, id_to_const: RefCell>>>, - debug_file_cache: RefCell>, + debug_file_cache: RefCell>>, enabled_capabilities: FxHashSet, enabled_extensions: FxHashSet, @@ -413,7 +419,7 @@ pub struct BuilderSpirv<'tcx> { impl<'tcx> BuilderSpirv<'tcx> { pub fn new( - source_map: &'tcx SourceMap, + tcx: TyCtxt<'tcx>, sym: &Symbols, target: &SpirvTarget, features: &[TargetFeature], @@ -478,7 +484,8 @@ impl<'tcx> BuilderSpirv<'tcx> { builder.memory_model(AddressingModel::Logical, memory_model); Self { - source_map, + source_map: tcx.sess.source_map(), + dropless_arena: &tcx.arena.dropless, builder: RefCell::new(builder), const_to_id: Default::default(), id_to_const: Default::default(), @@ -689,7 +696,17 @@ impl<'tcx> BuilderSpirv<'tcx> { } } - pub fn def_debug_file(&self, sf: Lrc) -> DebugFileSpirv { + pub fn file_line_col_for_op_line(&self, span: Span) -> (DebugFileSpirv<'tcx>, u32, u32) { + let loc = self.source_map.lookup_char_pos(span.lo()); + ( + self.def_debug_file(loc.file), + loc.line as u32, + loc.col_display as u32, + ) + } + + // HACK(eddyb) public only for `decorations`. + pub(crate) fn def_debug_file(&self, sf: Lrc) -> DebugFileSpirv<'tcx> { *self .debug_file_cache .borrow_mut() @@ -697,7 +714,34 @@ impl<'tcx> BuilderSpirv<'tcx> { .or_insert_with_key(|DebugFileKey(sf)| { let mut builder = self.builder(Default::default()); - let file_name_op_string_id = builder.string(sf.name.prefer_remapped().to_string()); + // FIXME(eddyb) it would be nicer if we could just rely on + // `RealFileName::to_string_lossy` returning `Cow<'_, str>`, + // but sadly that `'_` is the lifetime of the temporary `Lrc`, + // not `'tcx`, so we have to arena-allocate to get `&'tcx str`. + let file_name = match &sf.name { + FileName::Real(name) => { + name.to_string_lossy(FileNameDisplayPreference::Remapped) + } + _ => sf.name.prefer_remapped().to_string().into(), + }; + let file_name = { + // FIXME(eddyb) it should be possible to arena-allocate a + // `&str` directly, but it would require upstream changes, + // and strings are handled by string interning in `rustc`. + fn arena_alloc_slice<'tcx, T: Copy>( + dropless_arena: &'tcx DroplessArena, + xs: &[T], + ) -> &'tcx [T] { + if xs.is_empty() { + &[] + } else { + dropless_arena.alloc_slice(xs) + } + } + str::from_utf8(arena_alloc_slice(self.dropless_arena, file_name.as_bytes())) + .unwrap() + }; + let file_name_op_string_id = builder.string(file_name.to_owned()); let file_contents = self .source_map @@ -752,6 +796,7 @@ impl<'tcx> BuilderSpirv<'tcx> { } DebugFileSpirv { + file_name, file_name_op_string_id, } }) diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index 6c5943eb99..1152bb3b57 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -53,7 +53,7 @@ pub struct CodegenCx<'tcx> { /// Invalid spir-v IDs that should be stripped from the final binary, /// each with its own reason and span that should be used for reporting /// (in the event that the value is actually needed) - zombie_decorations: RefCell>, + zombie_decorations: RefCell>>, /// Cache of all the builtin symbols we need pub sym: Rc, pub instruction_table: InstructionTable, @@ -117,7 +117,7 @@ impl<'tcx> CodegenCx<'tcx> { Self { tcx, codegen_unit, - builder: BuilderSpirv::new(tcx.sess.source_map(), &sym, &target, &features), + builder: BuilderSpirv::new(tcx, &sym, &target, &features), instances: Default::default(), function_parameter_values: Default::default(), type_cache: Default::default(), @@ -188,7 +188,9 @@ impl<'tcx> CodegenCx<'tcx> { self.zombie_decorations.borrow_mut().insert( word, ZombieDecoration { - reason: reason.to_string(), + // FIXME(eddyb) this could take advantage of `Cow` and use + // either `&'static str` or `String`, on a case-by-case basis. + reason: reason.to_string().into(), span: SerializedSpan::from_rustc(span, &self.builder), }, ); diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index 322d3067dc..3ac395b709 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -108,8 +108,11 @@ impl Clone for LazilyDeserialized<'_, D> { } } -impl Deserialize<'a>> LazilyDeserialized<'_, D> { - pub fn deserialize(&self) -> D { +impl LazilyDeserialized<'_, D> { + pub fn deserialize<'a>(&'a self) -> D + where + D: Deserialize<'a>, + { serde_json::from_str(&self.json).unwrap() } @@ -123,29 +126,29 @@ impl Deserialize<'a>> LazilyDeserialized<'_, D> { } #[derive(Deserialize, Serialize)] -pub struct ZombieDecoration { - pub reason: String, +pub struct ZombieDecoration<'a> { + pub reason: Cow<'a, str>, #[serde(flatten)] - pub span: Option, + pub span: Option>, } -impl CustomDecoration for ZombieDecoration { +impl CustomDecoration for ZombieDecoration<'_> { const ENCODING_PREFIX: &'static str = "Z"; } /// Representation of a `rustc` `Span` that can be turned into a `Span` again /// in another compilation, by regenerating the `rustc` `SourceFile`. #[derive(Deserialize, Serialize)] -pub struct SerializedSpan { - file_name: String, +pub struct SerializedSpan<'a> { + file_name: Cow<'a, str>, // NOTE(eddyb) by keeping `lo` but not `hi`, we mimick `OpLine` limitations // (which could be lifted in the future using custom SPIR-T debuginfo). lo: u32, } -impl SerializedSpan { - pub fn from_rustc(span: Span, builder: &BuilderSpirv<'_>) -> Option { +impl<'tcx> SerializedSpan<'tcx> { + pub fn from_rustc(span: Span, builder: &BuilderSpirv<'tcx>) -> Option { // Decorations may not always have valid spans. // FIXME(eddyb) reduce the sources of this as much as possible. if span.is_dummy() { @@ -154,18 +157,18 @@ impl SerializedSpan { let lo = span.lo(); - let file = builder.source_map.lookup_source_file(lo); - if !(file.start_pos..=file.end_pos).contains(&lo) { + let sf = builder.source_map.lookup_source_file(lo); + if !(sf.start_pos..=sf.end_pos).contains(&lo) { // FIXME(eddyb) broken `Span` - potentially turn this into an assert? return None; } // NOTE(eddyb) this emits necessary `OpString`/`OpSource` instructions. - builder.def_debug_file(file.clone()); + let file = builder.def_debug_file(sf.clone()); Some(Self { - file_name: file.name.prefer_remapped().to_string(), - lo: (lo - file.start_pos).to_u32(), + file_name: file.file_name.into(), + lo: (lo - sf.start_pos).to_u32(), }) } } @@ -287,7 +290,7 @@ impl<'a> SpanRegenerator<'a> { file.as_deref() } - pub fn serialized_span_to_rustc(&mut self, span: &SerializedSpan) -> Option { + pub fn serialized_span_to_rustc(&mut self, span: &SerializedSpan<'_>) -> Option { let file = self.regenerate_rustc_source_file(&span.file_name[..])?; // Sanity check - assuming `SerializedSpan` isn't corrupted, this assert diff --git a/crates/rustc_codegen_spirv/src/lib.rs b/crates/rustc_codegen_spirv/src/lib.rs index 071cfc8523..705a69386f 100644 --- a/crates/rustc_codegen_spirv/src/lib.rs +++ b/crates/rustc_codegen_spirv/src/lib.rs @@ -40,6 +40,7 @@ compile_error!( ); extern crate rustc_apfloat; +extern crate rustc_arena; extern crate rustc_ast; extern crate rustc_attr; extern crate rustc_codegen_ssa; diff --git a/crates/rustc_codegen_spirv/src/linker/zombies.rs b/crates/rustc_codegen_spirv/src/linker/zombies.rs index 0a90ffd56c..0707796589 100644 --- a/crates/rustc_codegen_spirv/src/linker/zombies.rs +++ b/crates/rustc_codegen_spirv/src/linker/zombies.rs @@ -12,7 +12,7 @@ use std::iter::once; // FIXME(eddyb) change this to chain through IDs instead of wasting allocations. #[derive(Clone)] struct ZombieInfo<'a> { - serialized: &'a LazilyDeserialized<'static, ZombieDecoration>, + serialized: &'a LazilyDeserialized<'static, ZombieDecoration<'a>>, stack: Vec, } From ed622fdaa0de4090033d14e5cdc34d6520dc171a Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 18 Apr 2023 12:17:33 +0300 Subject: [PATCH 05/13] decorations: use line & column instead of a `Span`'s `BytePos`. --- Cargo.lock | 1 + crates/rustc_codegen_spirv/Cargo.toml | 1 + .../rustc_codegen_spirv/src/builder_spirv.rs | 6 +- crates/rustc_codegen_spirv/src/decorations.rs | 84 ++++++++++++++----- 4 files changed, 69 insertions(+), 23 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9460318f89..93538bd4d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1973,6 +1973,7 @@ dependencies = [ "either", "hashbrown 0.11.2", "indexmap", + "itertools", "lazy_static", "libc", "num-traits", diff --git a/crates/rustc_codegen_spirv/Cargo.toml b/crates/rustc_codegen_spirv/Cargo.toml index 97f317bb2d..d24bd78143 100644 --- a/crates/rustc_codegen_spirv/Cargo.toml +++ b/crates/rustc_codegen_spirv/Cargo.toml @@ -60,6 +60,7 @@ spirv-tools = { version = "0.9", default-features = false } rustc_codegen_spirv-types.workspace = true spirt = "0.1.0" lazy_static = "1.4.0" +itertools = "0.10.5" [dev-dependencies] pipe = "0.4" diff --git a/crates/rustc_codegen_spirv/src/builder_spirv.rs b/crates/rustc_codegen_spirv/src/builder_spirv.rs index ef95f3c823..86f1477888 100644 --- a/crates/rustc_codegen_spirv/src/builder_spirv.rs +++ b/crates/rustc_codegen_spirv/src/builder_spirv.rs @@ -398,8 +398,7 @@ pub struct BuilderCursor { } pub struct BuilderSpirv<'tcx> { - // HACK(eddyb) public only for `decorations`. - pub(crate) source_map: &'tcx SourceMap, + source_map: &'tcx SourceMap, dropless_arena: &'tcx DroplessArena, builder: RefCell, @@ -705,8 +704,7 @@ impl<'tcx> BuilderSpirv<'tcx> { ) } - // HACK(eddyb) public only for `decorations`. - pub(crate) fn def_debug_file(&self, sf: Lrc) -> DebugFileSpirv<'tcx> { + fn def_debug_file(&self, sf: Lrc) -> DebugFileSpirv<'tcx> { *self .debug_file_cache .borrow_mut() diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index 3ac395b709..94aad34abb 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -2,16 +2,18 @@ //! the original codegen of a crate, and consumed by the `linker`. use crate::builder_spirv::BuilderSpirv; +use itertools::Itertools; use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Decoration, Op, Word}; use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; use rustc_data_structures::sync::Lrc; -use rustc_span::{source_map::SourceMap, Pos, Span}; +use rustc_span::{source_map::SourceMap, Span}; use rustc_span::{FileName, SourceFile}; use serde::{Deserialize, Serialize}; use smallvec::SmallVec; use std::borrow::Cow; use std::marker::PhantomData; +use std::ops::Range; use std::path::PathBuf; use std::{iter, slice}; @@ -139,12 +141,15 @@ impl CustomDecoration for ZombieDecoration<'_> { /// Representation of a `rustc` `Span` that can be turned into a `Span` again /// in another compilation, by regenerating the `rustc` `SourceFile`. +// +// FIXME(eddyb) encode/decode this using a `file:line:col` string format. #[derive(Deserialize, Serialize)] pub struct SerializedSpan<'a> { file_name: Cow<'a, str>, - // NOTE(eddyb) by keeping `lo` but not `hi`, we mimick `OpLine` limitations + // NOTE(eddyb) by keeping `line`+`col`, we mimick `OpLine` limitations // (which could be lifted in the future using custom SPIR-T debuginfo). - lo: u32, + line: u32, + col: u32, } impl<'tcx> SerializedSpan<'tcx> { @@ -155,20 +160,12 @@ impl<'tcx> SerializedSpan<'tcx> { return None; } - let lo = span.lo(); - - let sf = builder.source_map.lookup_source_file(lo); - if !(sf.start_pos..=sf.end_pos).contains(&lo) { - // FIXME(eddyb) broken `Span` - potentially turn this into an assert? - return None; - } - - // NOTE(eddyb) this emits necessary `OpString`/`OpSource` instructions. - let file = builder.def_debug_file(sf.clone()); + let (file, line, col) = builder.file_line_col_for_op_line(span); Some(Self { file_name: file.file_name.into(), - lo: (lo - sf.start_pos).to_u32(), + line, + col, }) } } @@ -293,11 +290,60 @@ impl<'a> SpanRegenerator<'a> { pub fn serialized_span_to_rustc(&mut self, span: &SerializedSpan<'_>) -> Option { let file = self.regenerate_rustc_source_file(&span.file_name[..])?; - // Sanity check - assuming `SerializedSpan` isn't corrupted, this assert - // could only ever fail because of the file name being ambiguous. - assert!(span.lo <= (file.end_pos.0 - file.start_pos.0)); + let line_bpos_range = file.line_bounds(span.line.checked_sub(1)? as usize); + + // Find the special cases (`MultiByteChar`s/`NonNarrowChar`s) in the line. + let multibyte_chars = { + let find = |bpos| { + file.multibyte_chars + .binary_search_by_key(&bpos, |mbc| mbc.pos) + .unwrap_or_else(|x| x) + }; + let Range { start, end } = line_bpos_range; + file.multibyte_chars[find(start)..find(end)].iter() + }; + let non_narrow_chars = { + let find = |bpos| { + file.non_narrow_chars + .binary_search_by_key(&bpos, |nnc| nnc.pos()) + .unwrap_or_else(|x| x) + }; + let Range { start, end } = line_bpos_range; + file.non_narrow_chars[find(start)..find(end)].iter() + }; + let mut special_chars = multibyte_chars + .merge_join_by(non_narrow_chars, |mbc, nnc| mbc.pos.cmp(&nnc.pos())) + .peekable(); + + // Increment the `BytePos` until we reach the right `col_display`, using + // `MultiByteChar`s/`NonNarrowChar`s to track non-trivial contributions + // (this may look inefficient, but lines tend to be short, and `rustc` + // itself is even worse than this, when it comes to `BytePos` lookups). + let (mut cur_bpos, mut cur_col_display) = (line_bpos_range.start, 0); + while cur_bpos < line_bpos_range.end && cur_col_display < span.col { + let next_special_bpos = special_chars.peek().map(|special| { + special + .as_ref() + .map_any(|mbc| mbc.pos, |nnc| nnc.pos()) + .reduce(|x, _| x) + }); + + // Batch trivial chars (i.e. chars 1:1 wrt `BytePos` vs `col_display`). + let following_trivial_chars = + next_special_bpos.unwrap_or(line_bpos_range.end).0 - cur_bpos.0; + if following_trivial_chars > 0 { + let wanted_trivial_chars = following_trivial_chars.min(span.col - cur_col_display); + cur_bpos.0 += wanted_trivial_chars; + cur_col_display += wanted_trivial_chars; + continue; + } + + // Add a special char's `BytePos` and `col_display` contributions. + let mbc_nnc = special_chars.next().unwrap(); + cur_bpos.0 += mbc_nnc.as_ref().left().map_or(1, |mbc| mbc.bytes as u32); + cur_col_display += mbc_nnc.as_ref().right().map_or(1, |nnc| nnc.width() as u32); + } - let lo = file.start_pos + Pos::from_u32(span.lo); - Some(Span::with_root_ctxt(lo, lo)) + Some(Span::with_root_ctxt(cur_bpos, cur_bpos)) } } From 0c2740076c39a5e3ffdfe5d7cb4ea170d829d5d0 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 18 Apr 2023 13:34:41 +0300 Subject: [PATCH 06/13] decorations: split off `SrcLocDecoration` from `ZombieDecoration`. --- .../src/builder/builder_methods.rs | 11 +- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 36 +++--- crates/rustc_codegen_spirv/src/decorations.rs | 99 ++++++++------- crates/rustc_codegen_spirv/src/linker/mod.rs | 9 ++ .../rustc_codegen_spirv/src/linker/zombies.rs | 120 ++++++++---------- 5 files changed, 146 insertions(+), 129 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index 439f39a304..d81a171dc8 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -656,8 +656,15 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { fn set_span(&mut self, span: Span) { self.current_span = Some(span); - let (file, line, col) = self.builder.file_line_col_for_op_line(span); - self.emit().line(file.file_name_op_string_id, line, col); + + // We may not always have valid spans. + // FIXME(eddyb) reduce the sources of this as much as possible. + if span.is_dummy() { + self.emit().no_line(); + } else { + let (file, line, col) = self.builder.file_line_col_for_op_line(span); + self.emit().line(file.file_name_op_string_id, line, col); + } } // FIXME(eddyb) change `Self::Function` to be more like a function index. diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index 1152bb3b57..3cc435f9e4 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -5,7 +5,7 @@ mod type_; use crate::builder::{ExtInst, InstructionTable}; use crate::builder_spirv::{BuilderCursor, BuilderSpirv, SpirvConst, SpirvValue, SpirvValueKind}; -use crate::decorations::{CustomDecoration, SerializedSpan, ZombieDecoration}; +use crate::decorations::{CustomDecoration, SrcLocDecoration, ZombieDecoration}; use crate::spirv_type::{SpirvType, SpirvTypePrinter, TypeCache}; use crate::symbols::Symbols; use crate::target::SpirvTarget; @@ -50,10 +50,11 @@ pub struct CodegenCx<'tcx> { /// Cache generated vtables pub vtables: RefCell, Option>), SpirvValue>>, pub ext_inst: RefCell, - /// Invalid spir-v IDs that should be stripped from the final binary, + /// Invalid SPIR-V IDs that should be stripped from the final binary, /// each with its own reason and span that should be used for reporting /// (in the event that the value is actually needed) - zombie_decorations: RefCell>>, + zombie_decorations: + RefCell, Option>)>>, /// Cache of all the builtin symbols we need pub sym: Rc, pub instruction_table: InstructionTable, @@ -187,12 +188,14 @@ impl<'tcx> CodegenCx<'tcx> { pub fn zombie_even_in_user_code(&self, word: Word, span: Span, reason: &str) { self.zombie_decorations.borrow_mut().insert( word, - ZombieDecoration { - // FIXME(eddyb) this could take advantage of `Cow` and use - // either `&'static str` or `String`, on a case-by-case basis. - reason: reason.to_string().into(), - span: SerializedSpan::from_rustc(span, &self.builder), - }, + ( + ZombieDecoration { + // FIXME(eddyb) this could take advantage of `Cow` and use + // either `&'static str` or `String`, on a case-by-case basis. + reason: reason.to_string().into(), + }, + SrcLocDecoration::from_rustc_span(span, &self.builder), + ), ); } @@ -224,12 +227,15 @@ impl<'tcx> CodegenCx<'tcx> { pub fn finalize_module(self) -> Module { let mut result = self.builder.finalize(); - result.annotations.extend( - self.zombie_decorations - .into_inner() - .into_iter() - .map(|(id, zombie)| zombie.encode(id)), - ); + result + .annotations + .extend(self.zombie_decorations.into_inner().into_iter().flat_map( + |(id, (zombie, src_loc))| { + [zombie.encode(id)] + .into_iter() + .chain(src_loc.map(|src_loc| src_loc.encode(id))) + }, + )); result } diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index 94aad34abb..b1075425e7 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -64,7 +64,7 @@ pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize { Some(( id, LazilyDeserialized { - json: json.into(), + json, _marker: PhantomData, }, )) @@ -96,65 +96,44 @@ type DecodeAllIter<'a, D> = iter::FilterMap< /// Helper allowing full deserialization to be avoided where possible. pub struct LazilyDeserialized<'a, D> { - json: Cow<'a, str>, + json: &'a str, _marker: PhantomData, } -impl Clone for LazilyDeserialized<'_, D> { - fn clone(&self) -> Self { - let Self { ref json, _marker } = *self; - Self { - json: json.clone(), - _marker, - } - } -} - -impl LazilyDeserialized<'_, D> { - pub fn deserialize<'a>(&'a self) -> D - where - D: Deserialize<'a>, - { - serde_json::from_str(&self.json).unwrap() - } - - pub fn into_owned(self) -> LazilyDeserialized<'static, D> { - let Self { json, _marker } = self; - LazilyDeserialized { - json: json.into_owned().into(), - _marker, - } +impl<'a, D: Deserialize<'a>> LazilyDeserialized<'a, D> { + pub fn deserialize(&self) -> D { + serde_json::from_str(self.json).unwrap() } } #[derive(Deserialize, Serialize)] pub struct ZombieDecoration<'a> { pub reason: Cow<'a, str>, - - #[serde(flatten)] - pub span: Option>, } impl CustomDecoration for ZombieDecoration<'_> { const ENCODING_PREFIX: &'static str = "Z"; } -/// Representation of a `rustc` `Span` that can be turned into a `Span` again -/// in another compilation, by regenerating the `rustc` `SourceFile`. +/// Equivalent of `OpLine`, for the places where `rspirv` currently doesn't let +/// us actually emit a real `OpLine`, the way generating SPIR-T directly might. // -// FIXME(eddyb) encode/decode this using a `file:line:col` string format. +// NOTE(eddyb) by keeping `line`+`col`, we mimick `OpLine` limitations +// (which could be lifted in the future using custom SPIR-T debuginfo). #[derive(Deserialize, Serialize)] -pub struct SerializedSpan<'a> { +pub struct SrcLocDecoration<'a> { file_name: Cow<'a, str>, - // NOTE(eddyb) by keeping `line`+`col`, we mimick `OpLine` limitations - // (which could be lifted in the future using custom SPIR-T debuginfo). line: u32, col: u32, } -impl<'tcx> SerializedSpan<'tcx> { - pub fn from_rustc(span: Span, builder: &BuilderSpirv<'tcx>) -> Option { - // Decorations may not always have valid spans. +impl CustomDecoration for SrcLocDecoration<'_> { + const ENCODING_PREFIX: &'static str = "L"; +} + +impl<'tcx> SrcLocDecoration<'tcx> { + pub fn from_rustc_span(span: Span, builder: &BuilderSpirv<'tcx>) -> Option { + // We may not always have valid spans. // FIXME(eddyb) reduce the sources of this as much as possible. if span.is_dummy() { return None; @@ -170,12 +149,18 @@ impl<'tcx> SerializedSpan<'tcx> { } } -/// Helper type to delay most of the work necessary to turn a `SerializedSpan` +/// Helper type to delay most of the work necessary to turn a `SrcLocDecoration` /// back into an usable `Span`, until it's actually needed (i.e. for an error). pub struct SpanRegenerator<'a> { source_map: &'a SourceMap, module: &'a Module, + src_loc_decorations: Option>>>, + + // HACK(eddyb) this has no really good reason to belong here, but it's easier + // to handle it together with `SrcLocDecoration`, than separately. + zombie_decorations: Option>>>, + // HACK(eddyb) this is mostly replicating SPIR-T's module-level debuginfo. spv_debug_files: Option>>, } @@ -194,10 +179,30 @@ impl<'a> SpanRegenerator<'a> { Self { source_map, module, + + src_loc_decorations: None, + zombie_decorations: None, + spv_debug_files: None, } } + pub fn src_loc_for_id(&mut self, id: Word) -> Option> { + self.src_loc_decorations + .get_or_insert_with(|| SrcLocDecoration::decode_all(self.module).collect()) + .get(&id) + .map(|src_loc| src_loc.deserialize()) + } + + // HACK(eddyb) this has no really good reason to belong here, but it's easier + // to handle it together with `SrcLocDecoration`, than separately. + pub(crate) fn zombie_for_id(&mut self, id: Word) -> Option> { + self.zombie_decorations + .get_or_insert_with(|| ZombieDecoration::decode_all(self.module).collect()) + .get(&id) + .map(|zombie| zombie.deserialize()) + } + fn regenerate_rustc_source_file(&mut self, file_name: &str) -> Option<&SourceFile> { let spv_debug_files = self.spv_debug_files.get_or_insert_with(|| { let mut op_string_by_id = FxHashMap::default(); @@ -287,10 +292,16 @@ impl<'a> SpanRegenerator<'a> { file.as_deref() } - pub fn serialized_span_to_rustc(&mut self, span: &SerializedSpan<'_>) -> Option { - let file = self.regenerate_rustc_source_file(&span.file_name[..])?; + pub fn src_loc_to_rustc(&mut self, src_loc: &SrcLocDecoration<'_>) -> Option { + let SrcLocDecoration { + ref file_name, + line, + col, + } = *src_loc; + + let file = self.regenerate_rustc_source_file(file_name)?; - let line_bpos_range = file.line_bounds(span.line.checked_sub(1)? as usize); + let line_bpos_range = file.line_bounds(line.checked_sub(1)? as usize); // Find the special cases (`MultiByteChar`s/`NonNarrowChar`s) in the line. let multibyte_chars = { @@ -320,7 +331,7 @@ impl<'a> SpanRegenerator<'a> { // (this may look inefficient, but lines tend to be short, and `rustc` // itself is even worse than this, when it comes to `BytePos` lookups). let (mut cur_bpos, mut cur_col_display) = (line_bpos_range.start, 0); - while cur_bpos < line_bpos_range.end && cur_col_display < span.col { + while cur_bpos < line_bpos_range.end && cur_col_display < col { let next_special_bpos = special_chars.peek().map(|special| { special .as_ref() @@ -332,7 +343,7 @@ impl<'a> SpanRegenerator<'a> { let following_trivial_chars = next_special_bpos.unwrap_or(line_bpos_range.end).0 - cur_bpos.0; if following_trivial_chars > 0 { - let wanted_trivial_chars = following_trivial_chars.min(span.col - cur_col_display); + let wanted_trivial_chars = following_trivial_chars.min(col - cur_col_display); cur_bpos.0 += wanted_trivial_chars; cur_col_display += wanted_trivial_chars; continue; diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index 1536c307b5..9611d26fbe 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -20,6 +20,7 @@ mod zombies; use std::borrow::Cow; use crate::codegen_cx::SpirvMetadata; +use crate::decorations::{CustomDecoration, SrcLocDecoration, ZombieDecoration}; use either::Either; use rspirv::binary::{Assemble, Consumer}; use rspirv::dr::{Block, Instruction, Loader, Module, ModuleHeader, Operand}; @@ -572,6 +573,14 @@ pub fn link( // compact the ids https://github.com/KhronosGroup/SPIRV-Tools/blob/e02f178a716b0c3c803ce31b9df4088596537872/source/opt/compact_ids_pass.cpp#L43 output.header.as_mut().unwrap().bound = simple_passes::compact_ids(output); }; + + // FIXME(eddyb) convert these into actual `OpLine`s with a SPIR-T pass, + // but that'd require keeping the modules in SPIR-T form (once lowered), + // and never loading them back into `rspirv` once lifted back to SPIR-V. + SrcLocDecoration::remove_all(output); + + // FIXME(eddyb) might make more sense to rewrite these away on SPIR-T. + ZombieDecoration::remove_all(output); } Ok(output) diff --git a/crates/rustc_codegen_spirv/src/linker/zombies.rs b/crates/rustc_codegen_spirv/src/linker/zombies.rs index 0707796589..eb0dda7c47 100644 --- a/crates/rustc_codegen_spirv/src/linker/zombies.rs +++ b/crates/rustc_codegen_spirv/src/linker/zombies.rs @@ -1,7 +1,7 @@ //! See documentation on `CodegenCx::zombie` for a description of the zombie system. use super::{get_name, get_names}; -use crate::decorations::{CustomDecoration, LazilyDeserialized, SpanRegenerator, ZombieDecoration}; +use crate::decorations::{CustomDecoration, SpanRegenerator, ZombieDecoration}; use rspirv::dr::{Instruction, Module}; use rspirv::spirv::{Op, Word}; use rustc_data_structures::fx::FxHashMap; @@ -11,64 +11,61 @@ use std::iter::once; // FIXME(eddyb) change this to chain through IDs instead of wasting allocations. #[derive(Clone)] -struct ZombieInfo<'a> { - serialized: &'a LazilyDeserialized<'static, ZombieDecoration<'a>>, +struct Zombie { + leaf_id: Word, stack: Vec, } -impl<'a> ZombieInfo<'a> { +impl Zombie { fn push_stack(&self, word: Word) -> Self { Self { - serialized: self.serialized, + leaf_id: self.leaf_id, stack: self.stack.iter().cloned().chain(once(word)).collect(), } } } -fn contains_zombie<'h, 'a>( +fn contains_zombie<'h>( inst: &Instruction, - zombie: &'h FxHashMap>, -) -> Option<&'h ZombieInfo<'a>> { + zombies: &'h FxHashMap, +) -> Option<&'h Zombie> { if let Some(result_type) = inst.result_type { - if let Some(reason) = zombie.get(&result_type) { + if let Some(reason) = zombies.get(&result_type) { return Some(reason); } } inst.operands .iter() - .find_map(|op| op.id_ref_any().and_then(|w| zombie.get(&w))) + .find_map(|op| op.id_ref_any().and_then(|w| zombies.get(&w))) } -fn is_zombie<'h, 'a>( - inst: &Instruction, - zombie: &'h FxHashMap>, -) -> Option<&'h ZombieInfo<'a>> { +fn is_zombie<'h>(inst: &Instruction, zombies: &'h FxHashMap) -> Option<&'h Zombie> { if let Some(result_id) = inst.result_id { - zombie.get(&result_id) + zombies.get(&result_id) } else { - contains_zombie(inst, zombie) + contains_zombie(inst, zombies) } } -fn is_or_contains_zombie<'h, 'a>( +fn is_or_contains_zombie<'h>( inst: &Instruction, - zombie: &'h FxHashMap>, -) -> Option<&'h ZombieInfo<'a>> { - let result_zombie = inst.result_id.and_then(|result_id| zombie.get(&result_id)); - result_zombie.or_else(|| contains_zombie(inst, zombie)) + zombies: &'h FxHashMap, +) -> Option<&'h Zombie> { + let result_zombie = inst.result_id.and_then(|result_id| zombies.get(&result_id)); + result_zombie.or_else(|| contains_zombie(inst, zombies)) } -fn spread_zombie(module: &Module, zombie: &mut FxHashMap>) -> bool { +fn spread_zombie(module: &Module, zombies: &mut FxHashMap) -> bool { let mut any = false; // globals are easy for inst in module.global_inst_iter() { if let Some(result_id) = inst.result_id { - if let Some(reason) = contains_zombie(inst, zombie) { - if zombie.contains_key(&result_id) { + if let Some(reason) = contains_zombie(inst, zombies) { + if zombies.contains_key(&result_id) { continue; } let reason = reason.clone(); - zombie.insert(result_id, reason); + zombies.insert(result_id, reason); any = true; } } @@ -81,24 +78,24 @@ fn spread_zombie(module: &Module, zombie: &mut FxHashMap>) for func in &module.functions { let func_id = func.def_id().unwrap(); // Can't use zombie.entry() here, due to using the map in contains_zombie - if zombie.contains_key(&func_id) { + if zombies.contains_key(&func_id) { // Func is already zombie, no need to scan it again. continue; } for inst in func.all_inst_iter() { if inst.class.opcode == Op::Variable { let result_id = inst.result_id.unwrap(); - if let Some(reason) = contains_zombie(inst, zombie) { - if zombie.contains_key(&result_id) { + if let Some(reason) = contains_zombie(inst, zombies) { + if zombies.contains_key(&result_id) { continue; } let reason = reason.clone(); - zombie.insert(result_id, reason); + zombies.insert(result_id, reason); any = true; } - } else if let Some(reason) = is_or_contains_zombie(inst, zombie) { + } else if let Some(reason) = is_or_contains_zombie(inst, zombies) { let pushed_reason = reason.push_stack(func_id); - zombie.insert(func_id, pushed_reason); + zombies.insert(func_id, pushed_reason); any = true; break; } @@ -113,20 +110,21 @@ fn spread_zombie(module: &Module, zombie: &mut FxHashMap>) fn report_error_zombies( sess: &Session, module: &Module, - zombie: &FxHashMap>, + zombies: &FxHashMap, ) -> super::Result<()> { let mut span_regen = SpanRegenerator::new(sess.source_map(), module); let mut result = Ok(()); let mut names = None; for root in super::dce::collect_roots(module) { - if let Some(zombie_info) = zombie.get(&root) { - let ZombieDecoration { reason, span } = zombie_info.serialized.deserialize(); - let span = span - .and_then(|span| span_regen.serialized_span_to_rustc(&span)) + if let Some(zombie) = zombies.get(&root) { + let reason = span_regen.zombie_for_id(zombie.leaf_id).unwrap().reason; + let span = span_regen + .src_loc_for_id(zombie.leaf_id) + .and_then(|src_loc| span_regen.src_loc_to_rustc(&src_loc)) .unwrap_or(DUMMY_SP); let names = names.get_or_insert_with(|| get_names(module)); - let stack = zombie_info + let stack = zombie .stack .iter() .map(|&s| get_name(names, s).into_owned()); @@ -145,20 +143,12 @@ pub fn remove_zombies( opts: &super::Options, module: &mut Module, ) -> super::Result<()> { - // FIXME(eddyb) combine these two steps to take the original strings, - // instead of effectively cloning them (via `.into_owned()`). - let zombies_owned = ZombieDecoration::decode_all(module) - .map(|(id, zombie)| (id, zombie.into_owned())) - .collect::>(); - ZombieDecoration::remove_all(module); - - let mut zombies = zombies_owned - .iter() - .map(|(id, serialized)| { + let mut zombies = ZombieDecoration::decode_all(module) + .map(|(id, _)| { ( - *id, - ZombieInfo { - serialized, + id, + Zombie { + leaf_id: id, stack: vec![], }, ) @@ -171,31 +161,25 @@ pub fn remove_zombies( // FIXME(eddyb) use `log`/`tracing` instead. if opts.print_all_zombie { - for (&zombie_id, zombie_info) in &zombies { - let orig = if zombies_owned.iter().any(|&(z, _)| z == zombie_id) { - "original" - } else { - "infected" - }; - println!( - "zombie'd {} because {} ({})", - zombie_id, - zombie_info.serialized.deserialize().reason, - orig - ); + let mut span_regen = SpanRegenerator::new(sess.source_map(), module); + for (&zombie_id, zombie) in &zombies { + let reason = span_regen.zombie_for_id(zombie.leaf_id).unwrap().reason; + eprint!("zombie'd %{zombie_id} because {reason}"); + if zombie_id != zombie.leaf_id { + eprint!(" (infected by %{})", zombie.leaf_id); + } + eprintln!(); } } if opts.print_zombie { + let mut span_regen = SpanRegenerator::new(sess.source_map(), module); let names = get_names(module); for f in &module.functions { - if let Some(zombie_info) = is_zombie(f.def.as_ref().unwrap(), &zombies) { + if let Some(zombie) = is_zombie(f.def.as_ref().unwrap(), &zombies) { let name = get_name(&names, f.def_id().unwrap()); - println!( - "Function removed {:?} because {:?}", - name, - zombie_info.serialized.deserialize().reason - ); + let reason = span_regen.zombie_for_id(zombie.leaf_id).unwrap().reason; + eprintln!("function removed {name:?} because {reason:?}"); } } } From 3f091530fba14769ca383e7df71f24971115b91a Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 18 Apr 2023 14:06:14 +0300 Subject: [PATCH 07/13] decorations: use custom string encodings instead of JSON. --- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 4 +- crates/rustc_codegen_spirv/src/decorations.rs | 109 +++++++++++------- .../rustc_codegen_spirv/src/linker/zombies.rs | 2 +- 3 files changed, 73 insertions(+), 42 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index 3cc435f9e4..d1a6297c70 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -231,9 +231,9 @@ impl<'tcx> CodegenCx<'tcx> { .annotations .extend(self.zombie_decorations.into_inner().into_iter().flat_map( |(id, (zombie, src_loc))| { - [zombie.encode(id)] + [zombie.encode_to_inst(id)] .into_iter() - .chain(src_loc.map(|src_loc| src_loc.encode(id))) + .chain(src_loc.map(|src_loc| src_loc.encode_to_inst(id))) }, )); result diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index b1075425e7..89f7b95364 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -9,17 +9,16 @@ use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; use rustc_data_structures::sync::Lrc; use rustc_span::{source_map::SourceMap, Span}; use rustc_span::{FileName, SourceFile}; -use serde::{Deserialize, Serialize}; use smallvec::SmallVec; use std::borrow::Cow; use std::marker::PhantomData; use std::ops::Range; use std::path::PathBuf; -use std::{iter, slice}; +use std::{fmt, iter, slice}; /// Decorations not native to SPIR-V require some form of encoding into existing /// SPIR-V constructs, for which we use `OpDecorateString` with decoration type -/// `UserTypeGOOGLE` and a JSON-encoded Rust value as the decoration string. +/// `UserTypeGOOGLE` and some encoded Rust value as the decoration string. /// /// Each decoration type has to implement this trait, and use a different /// `ENCODING_PREFIX` from any other decoration type, to disambiguate them. @@ -30,16 +29,15 @@ use std::{iter, slice}; /// /// TODO: uses `non_semantic` instead of piggybacking off of `UserTypeGOOGLE` /// -pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize { +pub trait CustomDecoration<'a>: Sized { const ENCODING_PREFIX: &'static str; - fn encode(self, id: Word) -> Instruction { - // FIXME(eddyb) this allocates twice, because there is no functionality - // in `serde_json` for writing to something that impls `fmt::Write`, - // only for `io::Write`, which would require performing redundant UTF-8 - // (re)validation, or relying on `unsafe` code, to use with `String`. - let json = serde_json::to_string(&self).unwrap(); - let encoded = [Self::ENCODING_PREFIX, &json].concat(); + fn encode(self, w: &mut impl fmt::Write) -> fmt::Result; + fn decode(s: &'a str) -> Self; + + fn encode_to_inst(self, id: Word) -> Instruction { + let mut encoded = Self::ENCODING_PREFIX.to_string(); + self.encode(&mut encoded).unwrap(); Instruction::new( Op::DecorateString, @@ -53,18 +51,18 @@ pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize { ) } - fn try_decode(inst: &Instruction) -> Option<(Word, LazilyDeserialized<'_, Self>)> { + fn try_decode_from_inst(inst: &Instruction) -> Option<(Word, LazilyDecoded<'_, Self>)> { if inst.class.opcode == Op::DecorateString && inst.operands[1].unwrap_decoration() == Decoration::UserTypeGOOGLE { let id = inst.operands[0].unwrap_id_ref(); - let encoded = inst.operands[2].unwrap_literal_string(); - let json = encoded.strip_prefix(Self::ENCODING_PREFIX)?; + let prefixed_encoded = inst.operands[2].unwrap_literal_string(); + let encoded = prefixed_encoded.strip_prefix(Self::ENCODING_PREFIX)?; Some(( id, - LazilyDeserialized { - json, + LazilyDecoded { + encoded, _marker: PhantomData, }, )) @@ -77,42 +75,51 @@ pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize { module .annotations .iter() - .filter_map(Self::try_decode as fn(_) -> _) + .filter_map(Self::try_decode_from_inst as fn(_) -> _) } fn remove_all(module: &mut Module) { module .annotations - .retain(|inst| Self::try_decode(inst).is_none()); + .retain(|inst| Self::try_decode_from_inst(inst).is_none()); } } // HACK(eddyb) return type of `CustomDecoration::decode_all`, in lieu of -// `-> impl Iterator)` in the trait. +// `-> impl Iterator)` in the trait. type DecodeAllIter<'a, D> = iter::FilterMap< slice::Iter<'a, Instruction>, - fn(&'a Instruction) -> Option<(Word, LazilyDeserialized<'a, D>)>, + fn(&'a Instruction) -> Option<(Word, LazilyDecoded<'a, D>)>, >; -/// Helper allowing full deserialization to be avoided where possible. -pub struct LazilyDeserialized<'a, D> { - json: &'a str, +/// Helper allowing full decoding to be avoided where possible. +// +// FIXME(eddyb) is this even needed? (decoding impls are now much cheaper) +pub struct LazilyDecoded<'a, D> { + encoded: &'a str, _marker: PhantomData, } -impl<'a, D: Deserialize<'a>> LazilyDeserialized<'a, D> { - pub fn deserialize(&self) -> D { - serde_json::from_str(self.json).unwrap() +impl<'a, D: CustomDecoration<'a>> LazilyDecoded<'a, D> { + pub fn decode(&self) -> D { + D::decode(self.encoded) } } -#[derive(Deserialize, Serialize)] pub struct ZombieDecoration<'a> { pub reason: Cow<'a, str>, } -impl CustomDecoration for ZombieDecoration<'_> { +impl<'a> CustomDecoration<'a> for ZombieDecoration<'a> { const ENCODING_PREFIX: &'static str = "Z"; + + fn encode(self, w: &mut impl fmt::Write) -> fmt::Result { + let Self { reason } = self; + w.write_str(&reason) + } + fn decode(s: &'a str) -> Self { + Self { reason: s.into() } + } } /// Equivalent of `OpLine`, for the places where `rspirv` currently doesn't let @@ -120,15 +127,39 @@ impl CustomDecoration for ZombieDecoration<'_> { // // NOTE(eddyb) by keeping `line`+`col`, we mimick `OpLine` limitations // (which could be lifted in the future using custom SPIR-T debuginfo). -#[derive(Deserialize, Serialize)] +#[derive(Copy, Clone)] pub struct SrcLocDecoration<'a> { - file_name: Cow<'a, str>, + file_name: &'a str, line: u32, col: u32, } -impl CustomDecoration for SrcLocDecoration<'_> { +impl<'a> CustomDecoration<'a> for SrcLocDecoration<'a> { const ENCODING_PREFIX: &'static str = "L"; + + fn encode(self, w: &mut impl fmt::Write) -> fmt::Result { + let Self { + file_name, + line, + col, + } = self; + write!(w, "{file_name}:{line}:{col}") + } + fn decode(s: &'a str) -> Self { + #[derive(Copy, Clone, Debug)] + struct InvalidSrcLoc<'a>(&'a str); + let err = InvalidSrcLoc(s); + + let (s, col) = s.rsplit_once(':').ok_or(err).unwrap(); + let (s, line) = s.rsplit_once(':').ok_or(err).unwrap(); + let file_name = s; + + Self { + file_name, + line: line.parse().unwrap(), + col: col.parse().unwrap(), + } + } } impl<'tcx> SrcLocDecoration<'tcx> { @@ -142,7 +173,7 @@ impl<'tcx> SrcLocDecoration<'tcx> { let (file, line, col) = builder.file_line_col_for_op_line(span); Some(Self { - file_name: file.file_name.into(), + file_name: file.file_name, line, col, }) @@ -155,11 +186,11 @@ pub struct SpanRegenerator<'a> { source_map: &'a SourceMap, module: &'a Module, - src_loc_decorations: Option>>>, + src_loc_decorations: Option>>>, // HACK(eddyb) this has no really good reason to belong here, but it's easier // to handle it together with `SrcLocDecoration`, than separately. - zombie_decorations: Option>>>, + zombie_decorations: Option>>>, // HACK(eddyb) this is mostly replicating SPIR-T's module-level debuginfo. spv_debug_files: Option>>, @@ -191,7 +222,7 @@ impl<'a> SpanRegenerator<'a> { self.src_loc_decorations .get_or_insert_with(|| SrcLocDecoration::decode_all(self.module).collect()) .get(&id) - .map(|src_loc| src_loc.deserialize()) + .map(|src_loc| src_loc.decode()) } // HACK(eddyb) this has no really good reason to belong here, but it's easier @@ -200,7 +231,7 @@ impl<'a> SpanRegenerator<'a> { self.zombie_decorations .get_or_insert_with(|| ZombieDecoration::decode_all(self.module).collect()) .get(&id) - .map(|zombie| zombie.deserialize()) + .map(|zombie| zombie.decode()) } fn regenerate_rustc_source_file(&mut self, file_name: &str) -> Option<&SourceFile> { @@ -292,12 +323,12 @@ impl<'a> SpanRegenerator<'a> { file.as_deref() } - pub fn src_loc_to_rustc(&mut self, src_loc: &SrcLocDecoration<'_>) -> Option { + pub fn src_loc_to_rustc(&mut self, src_loc: SrcLocDecoration<'_>) -> Option { let SrcLocDecoration { - ref file_name, + file_name, line, col, - } = *src_loc; + } = src_loc; let file = self.regenerate_rustc_source_file(file_name)?; diff --git a/crates/rustc_codegen_spirv/src/linker/zombies.rs b/crates/rustc_codegen_spirv/src/linker/zombies.rs index eb0dda7c47..d3febc561c 100644 --- a/crates/rustc_codegen_spirv/src/linker/zombies.rs +++ b/crates/rustc_codegen_spirv/src/linker/zombies.rs @@ -121,7 +121,7 @@ fn report_error_zombies( let reason = span_regen.zombie_for_id(zombie.leaf_id).unwrap().reason; let span = span_regen .src_loc_for_id(zombie.leaf_id) - .and_then(|src_loc| span_regen.src_loc_to_rustc(&src_loc)) + .and_then(|src_loc| span_regen.src_loc_to_rustc(src_loc)) .unwrap_or(DUMMY_SP); let names = names.get_or_insert_with(|| get_names(module)); let stack = zombie From 322ccb31f8f26db99ca39c85f3a4f31aecb87807 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Wed, 19 Apr 2023 22:08:00 +0300 Subject: [PATCH 08/13] tests: add two broken tests to track their error message progression. --- .../ui/lang/core/ref/member_ref_arg-broken.rs | 53 +++++++++++++++++++ .../core/ref/member_ref_arg-broken.stderr | 11 ++++ .../core/ref/zst_member_ref_arg-broken.rs | 34 ++++++++++++ .../core/ref/zst_member_ref_arg-broken.stderr | 29 ++++++++++ 4 files changed, 127 insertions(+) create mode 100644 tests/ui/lang/core/ref/member_ref_arg-broken.rs create mode 100644 tests/ui/lang/core/ref/member_ref_arg-broken.stderr create mode 100644 tests/ui/lang/core/ref/zst_member_ref_arg-broken.rs create mode 100644 tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr diff --git a/tests/ui/lang/core/ref/member_ref_arg-broken.rs b/tests/ui/lang/core/ref/member_ref_arg-broken.rs new file mode 100644 index 0000000000..d79e38e4b9 --- /dev/null +++ b/tests/ui/lang/core/ref/member_ref_arg-broken.rs @@ -0,0 +1,53 @@ +// FIXME(eddyb) this is like `member_ref_arg`, but testing the error messages +// in some broken cases - this test should eventually pass, but for now +// we care more that the error messages do not regress too much. + +// build-fail +// normalize-stderr-not_spirt "36\[%36\]" -> "$$ID[%$$ID]" +// normalize-stderr-spirt "38\[%38\]" -> "$$ID[%$$ID]" +// normalize-stderr-test "(note: module `.*)\.(not_spirt|spirt)`" -> "$1.{not_spirt,spirt}`" + +use spirv_std::spirv; + +struct S { + x: u32, + y: u32, +} + +// NOTE(eddyb) `#[inline(never)]` is for blocking inlining at the e.g. MIR level, +// whereas any Rust-GPU-specific legalization will intentially ignore it. + +#[inline(never)] +fn f(x: &u32) -> u32 { + *x +} + +#[inline(never)] +fn g(xy: (&u32, &u32)) -> (u32, u32) { + (*xy.0, *xy.1) +} + +#[inline(never)] +fn h(xyz: (&u32, &u32, &u32)) -> (u32, u32, u32) { + (*xyz.0, *xyz.1, *xyz.2) +} + +// FIXME(eddyb) the reason this doesn't work while the others do, is somewhat +// subtle - specifically, we currently have some passes (`destructure_composites` +// and `spirt_passes::reduce`) which can handle single-level `OpComposite*` +// instructions, but the extra nesting here stops them dead in their tracks +// (they're also not really the right solution for this problem, such composites +// should just never exist by-value, and `qptr` may very well get rid of them). +#[inline(never)] +fn h_newtyped(xyz: ((&u32, &u32, &u32),)) -> (u32, u32, u32) { + (*xyz.0 .0, *xyz.0 .1, *xyz.0 .2) +} + +#[spirv(fragment)] +pub fn main() { + let s = S { x: 2, y: 2 }; + f(&s.x); + g((&s.x, &s.y)); + h((&s.x, &s.y, &s.x)); + h_newtyped(((&s.x, &s.y, &s.x),)); +} diff --git a/tests/ui/lang/core/ref/member_ref_arg-broken.stderr b/tests/ui/lang/core/ref/member_ref_arg-broken.stderr new file mode 100644 index 0000000000..e2508b3e6d --- /dev/null +++ b/tests/ui/lang/core/ref/member_ref_arg-broken.stderr @@ -0,0 +1,11 @@ +warning: `#[inline(never)]` function `member_ref_arg_broken::h` needs to be inlined because it has illegal argument or return types + +warning: `#[inline(never)]` function `member_ref_arg_broken::h_newtyped` needs to be inlined because it has illegal argument or return types + +error: error:0:0 - OpLoad Pointer '$ID[%$ID]' is not a logical pointer. + | + = note: spirv-val failed + = note: module `$TEST_BUILD_DIR/lang/core/ref/member_ref_arg-broken.{not_spirt,spirt}` + +error: aborting due to previous error; 2 warnings emitted + diff --git a/tests/ui/lang/core/ref/zst_member_ref_arg-broken.rs b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.rs new file mode 100644 index 0000000000..cdb10e4fbd --- /dev/null +++ b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.rs @@ -0,0 +1,34 @@ +// FIXME(eddyb) this is like `zst_member_ref_arg`, but testing the error messages +// in some broken cases (see issue #1037) - this test should eventually pass, but +// for now we care more that the error messages do not regress too much. + +// build-fail + +use spirv_std::spirv; +struct A; +struct B; + +pub struct S { + x: A, + y: B, + + z: Z, + w: W, +} + +fn f(x: &B) {} + +#[spirv(fragment)] +pub fn main_scalar(#[spirv(push_constant)] s: &S) { + f(&s.y); +} + +#[spirv(fragment)] +pub fn main_scalar_pair(#[spirv(push_constant)] s: &S) { + f(&s.y); +} + +#[spirv(fragment)] +pub fn main_scalar_scalar_pair_nested(#[spirv(push_constant)] s: &S<(usize, usize)>) { + f(&s.y); +} diff --git a/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr new file mode 100644 index 0000000000..c95cdafa55 --- /dev/null +++ b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr @@ -0,0 +1,29 @@ +error: Cannot cast between pointer types + --> $DIR/zst_member_ref_arg-broken.rs:23:5 + | +23 | f(&s.y); + | ^^^^^^^ + | + = note: from: *u32 + = note: to: *struct B { } + +error: Cannot cast between pointer types + --> $DIR/zst_member_ref_arg-broken.rs:28:5 + | +28 | f(&s.y); + | ^^^^^^^ + | + = note: from: *struct S { u32, u32 } + = note: to: *struct B { } + +error: Cannot cast between pointer types + --> $DIR/zst_member_ref_arg-broken.rs:33:5 + | +33 | f(&s.y); + | ^^^^^^^ + | + = note: from: *struct (usize, usize) { u32, u32 } + = note: to: *struct B { } + +error: aborting due to 3 previous errors + From 4ad8888ac669a57220947cad782e0a0892d90fb3 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Wed, 19 Apr 2023 04:19:16 +0300 Subject: [PATCH 09/13] linker/zombies: report all reachable zombies, w/ `OpLine`-based stack traces. --- .../src/codegen_cx/declare.rs | 48 +- crates/rustc_codegen_spirv/src/decorations.rs | 113 ++-- .../rustc_codegen_spirv/src/linker/zombies.rs | 497 ++++++++++++------ ...n-writable-storage_buffer.not_spirt.stderr | 4 +- tests/ui/dis/ptr_copy.normal.stderr | 25 +- .../consts/nested-ref-in-composite.stderr | 26 +- .../core/ptr/allocate_const_scalar.stderr | 15 +- 7 files changed, 488 insertions(+), 240 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs b/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs index 7757eecace..0ea3cfe775 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs @@ -2,6 +2,7 @@ use super::CodegenCx; use crate::abi::ConvSpirvType; use crate::attr::AggregatedSpirvAttributes; use crate::builder_spirv::{SpirvConst, SpirvValue, SpirvValueExt}; +use crate::decorations::{CustomDecoration, SrcLocDecoration}; use crate::spirv_type::SpirvType; use rspirv::spirv::{FunctionControl, LinkageType, StorageClass, Word}; use rustc_attr::InlineAttr; @@ -74,20 +75,36 @@ impl<'tcx> CodegenCx<'tcx> { self.zombie_with_span(result.def_cx(self), span, "called blocklisted fn"); return result; } - let mut emit = self.emit_global(); - let fn_id = emit - .begin_function(return_type, None, control, function_type) - .unwrap(); - if linkage != Some(LinkageType::Import) { - let parameter_values = argument_types - .iter() - .map(|&ty| emit.function_parameter(ty).unwrap().with_type(ty)) - .collect::>(); - self.function_parameter_values - .borrow_mut() - .insert(fn_id, parameter_values); - } - emit.end_function().unwrap(); + let fn_id = { + let mut emit = self.emit_global(); + let fn_id = emit + .begin_function(return_type, None, control, function_type) + .unwrap(); + if linkage != Some(LinkageType::Import) { + let parameter_values = argument_types + .iter() + .map(|&ty| emit.function_parameter(ty).unwrap().with_type(ty)) + .collect::>(); + self.function_parameter_values + .borrow_mut() + .insert(fn_id, parameter_values); + } + emit.end_function().unwrap(); + fn_id + }; + + // HACK(eddyb) this is a temporary workaround due to our use of `rspirv`, + // which prevents us from attaching `OpLine`s to `OpFunction` definitions, + // but we can use our custom `SrcLocDecoration` instead. + let src_loc_inst = SrcLocDecoration::from_rustc_span( + self.tcx.def_ident_span(instance.def_id()).unwrap_or(span), + &self.builder, + ) + .map(|src_loc| src_loc.encode_to_inst(fn_id)); + self.emit_global() + .module_mut() + .annotations + .extend(src_loc_inst); // HACK(eddyb) this is a bit roundabout, but the easiest way to get a // fully absolute path that contains at least as much information as @@ -99,9 +116,8 @@ impl<'tcx> CodegenCx<'tcx> { // (as some sort of opt-in, or toggled based on the platform, etc.). let symbol_name = self.tcx.symbol_name(instance).name; let demangled_symbol_name = format!("{:#}", rustc_demangle::demangle(symbol_name)); - emit.name(fn_id, &demangled_symbol_name); + self.emit_global().name(fn_id, &demangled_symbol_name); - drop(emit); // set_linkage uses emit if let Some(linkage) = linkage { self.set_linkage(fn_id, symbol_name.to_owned(), linkage); } diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index 89f7b95364..d06b9e0942 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -5,7 +5,7 @@ use crate::builder_spirv::BuilderSpirv; use itertools::Itertools; use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Decoration, Op, Word}; -use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; +use rustc_data_structures::fx::FxIndexMap; use rustc_data_structures::sync::Lrc; use rustc_span::{source_map::SourceMap, Span}; use rustc_span::{FileName, SourceFile}; @@ -193,7 +193,53 @@ pub struct SpanRegenerator<'a> { zombie_decorations: Option>>>, // HACK(eddyb) this is mostly replicating SPIR-T's module-level debuginfo. - spv_debug_files: Option>>, + spv_debug_info: Option>, +} + +#[derive(Default)] +struct SpvDebugInfo<'a> { + id_to_op_string: FxIndexMap, + files: FxIndexMap<&'a str, SpvDebugFile<'a>>, +} + +impl<'a> SpvDebugInfo<'a> { + fn collect(module: &'a Module) -> Self { + let mut this = Self::default(); + let mut insts = module.debug_string_source.iter().peekable(); + while let Some(inst) = insts.next() { + match inst.class.opcode { + Op::String => { + this.id_to_op_string.insert( + inst.result_id.unwrap(), + inst.operands[0].unwrap_literal_string(), + ); + } + Op::Source if inst.operands.len() == 4 => { + let file_name_id = inst.operands[2].unwrap_id_ref(); + if let Some(&file_name) = this.id_to_op_string.get(&file_name_id) { + let mut file = SpvDebugFile::default(); + file.op_source_parts + .push(inst.operands[3].unwrap_literal_string()); + while let Some(&next_inst) = insts.peek() { + if next_inst.class.opcode != Op::SourceContinued { + break; + } + insts.next(); + + file.op_source_parts + .push(next_inst.operands[0].unwrap_literal_string()); + } + + // FIXME(eddyb) what if the file is already present, + // should it be considered ambiguous overall? + this.files.insert(file_name, file); + } + } + _ => {} + } + } + this + } } // HACK(eddyb) this is mostly replicating SPIR-T's module-level debuginfo. @@ -214,7 +260,7 @@ impl<'a> SpanRegenerator<'a> { src_loc_decorations: None, zombie_decorations: None, - spv_debug_files: None, + spv_debug_info: None, } } @@ -234,46 +280,29 @@ impl<'a> SpanRegenerator<'a> { .map(|zombie| zombie.decode()) } - fn regenerate_rustc_source_file(&mut self, file_name: &str) -> Option<&SourceFile> { - let spv_debug_files = self.spv_debug_files.get_or_insert_with(|| { - let mut op_string_by_id = FxHashMap::default(); - let mut spv_debug_files = FxIndexMap::default(); - let mut insts = self.module.debug_string_source.iter().peekable(); - while let Some(inst) = insts.next() { - match inst.class.opcode { - Op::String => { - op_string_by_id.insert( - inst.result_id.unwrap(), - inst.operands[0].unwrap_literal_string(), - ); - } - Op::Source if inst.operands.len() == 4 => { - let file_name_id = inst.operands[2].unwrap_id_ref(); - if let Some(&file_name) = op_string_by_id.get(&file_name_id) { - let mut file = SpvDebugFile::default(); - file.op_source_parts - .push(inst.operands[3].unwrap_literal_string()); - while let Some(&next_inst) = insts.peek() { - if next_inst.class.opcode != Op::SourceContinued { - break; - } - insts.next(); - - file.op_source_parts - .push(next_inst.operands[0].unwrap_literal_string()); - } + pub fn src_loc_from_op_line( + &mut self, + file_id: Word, + line: u32, + col: u32, + ) -> Option> { + self.spv_debug_info + .get_or_insert_with(|| SpvDebugInfo::collect(self.module)) + .id_to_op_string + .get(&file_id) + .map(|&file_name| SrcLocDecoration { + file_name, + line, + col, + }) + } - // FIXME(eddyb) what if the file is already present, - // should it be considered ambiguous overall? - spv_debug_files.insert(file_name, file); - } - } - _ => {} - } - } - spv_debug_files - }); - let spv_debug_file = spv_debug_files.get_mut(file_name)?; + fn regenerate_rustc_source_file(&mut self, file_name: &str) -> Option<&SourceFile> { + let spv_debug_file = self + .spv_debug_info + .get_or_insert_with(|| SpvDebugInfo::collect(self.module)) + .files + .get_mut(file_name)?; let file = &mut spv_debug_file.regenerated_rustc_source_file; if file.is_none() { diff --git a/crates/rustc_codegen_spirv/src/linker/zombies.rs b/crates/rustc_codegen_spirv/src/linker/zombies.rs index d3febc561c..5c99d8dce1 100644 --- a/crates/rustc_codegen_spirv/src/linker/zombies.rs +++ b/crates/rustc_codegen_spirv/src/linker/zombies.rs @@ -2,140 +2,310 @@ use super::{get_name, get_names}; use crate::decorations::{CustomDecoration, SpanRegenerator, ZombieDecoration}; -use rspirv::dr::{Instruction, Module}; +use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Op, Word}; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; +use rustc_errors::{DiagnosticBuilder, ErrorGuaranteed}; use rustc_session::Session; -use rustc_span::DUMMY_SP; -use std::iter::once; - -// FIXME(eddyb) change this to chain through IDs instead of wasting allocations. -#[derive(Clone)] -struct Zombie { - leaf_id: Word, - stack: Vec, +use rustc_span::{Span, DUMMY_SP}; +use smallvec::SmallVec; + +#[derive(Copy, Clone)] +struct Zombie<'a> { + id: Word, + kind: &'a ZombieKind, } -impl Zombie { - fn push_stack(&self, word: Word) -> Self { - Self { - leaf_id: self.leaf_id, - stack: self.stack.iter().cloned().chain(once(word)).collect(), - } - } +enum ZombieKind { + /// Definition annotated with `ZombieDecoration`. + Leaf, + + /// Transitively zombie'd by using other zombies, from an instruction. + Uses(Vec), } -fn contains_zombie<'h>( - inst: &Instruction, - zombies: &'h FxHashMap, -) -> Option<&'h Zombie> { - if let Some(result_type) = inst.result_type { - if let Some(reason) = zombies.get(&result_type) { - return Some(reason); - } - } - inst.operands - .iter() - .find_map(|op| op.id_ref_any().and_then(|w| zombies.get(&w))) +struct ZombieUse { + used_zombie_id: Word, + + /// Operands of the active `OpLine` at the time of the use, if any. + use_file_id_line_col: Option<(Word, u32, u32)>, + + origin: UseOrigin, } -fn is_zombie<'h>(inst: &Instruction, zombies: &'h FxHashMap) -> Option<&'h Zombie> { - if let Some(result_id) = inst.result_id { - zombies.get(&result_id) - } else { - contains_zombie(inst, zombies) - } +enum UseOrigin { + GlobalOperandOrResultType, + IntraFuncOperandOrResultType { parent_func_id: Word }, + CallCalleeOperand { caller_func_id: Word }, } -fn is_or_contains_zombie<'h>( - inst: &Instruction, - zombies: &'h FxHashMap, -) -> Option<&'h Zombie> { - let result_zombie = inst.result_id.and_then(|result_id| zombies.get(&result_id)); - result_zombie.or_else(|| contains_zombie(inst, zombies)) +struct Zombies { + id_to_zombie_kind: FxIndexMap, } -fn spread_zombie(module: &Module, zombies: &mut FxHashMap) -> bool { - let mut any = false; - // globals are easy - for inst in module.global_inst_iter() { - if let Some(result_id) = inst.result_id { - if let Some(reason) = contains_zombie(inst, zombies) { - if zombies.contains_key(&result_id) { - continue; +impl Zombies { + // FIXME(eddyb) rename all the other methods to say `_inst` explicitly. + fn get_zombie_by_id(&self, id: Word) -> Option> { + self.id_to_zombie_kind + .get(&id) + .map(|kind| Zombie { id, kind }) + } + + fn zombies_used_from_inst<'a>( + &'a self, + inst: &'a Instruction, + ) -> impl Iterator> + 'a { + inst.result_type + .into_iter() + .chain(inst.operands.iter().filter_map(|op| op.id_ref_any())) + .filter_map(move |id| self.get_zombie_by_id(id)) + } + + fn spread(&mut self, module: &Module) -> bool { + let mut any = false; + // globals are easy + { + let mut file_id_line_col = None; + for inst in module.global_inst_iter() { + match inst.class.opcode { + Op::Line => { + file_id_line_col = Some(( + inst.operands[0].unwrap_id_ref(), + inst.operands[1].unwrap_literal_int32(), + inst.operands[2].unwrap_literal_int32(), + )); + } + Op::NoLine => file_id_line_col = None, + _ => {} + } + + if let Some(result_id) = inst.result_id { + if self.id_to_zombie_kind.contains_key(&result_id) { + continue; + } + let zombie_uses: Vec<_> = self + .zombies_used_from_inst(inst) + .map(|zombie| ZombieUse { + used_zombie_id: zombie.id, + use_file_id_line_col: file_id_line_col, + origin: UseOrigin::GlobalOperandOrResultType, + }) + .collect(); + if !zombie_uses.is_empty() { + self.id_to_zombie_kind + .insert(result_id, ZombieKind::Uses(zombie_uses)); + any = true; + } } - let reason = reason.clone(); - zombies.insert(result_id, reason); - any = true; } } - } - // No need to zombie defs within a function: If any def within a function is zombied, then the - // whole function is zombied. But, we don't have to mark the defs within a function as zombie, - // because the defs can't escape the function. - // HACK(eddyb) one exception to this is function-local variables, which may - // be unused and as such cannot be allowed to always zombie the function. - for func in &module.functions { - let func_id = func.def_id().unwrap(); - // Can't use zombie.entry() here, due to using the map in contains_zombie - if zombies.contains_key(&func_id) { - // Func is already zombie, no need to scan it again. - continue; - } - for inst in func.all_inst_iter() { - if inst.class.opcode == Op::Variable { - let result_id = inst.result_id.unwrap(); - if let Some(reason) = contains_zombie(inst, zombies) { - if zombies.contains_key(&result_id) { + // No need to zombie defs within a function: If any def within a function is zombied, then the + // whole function is zombied. But, we don't have to mark the defs within a function as zombie, + // because the defs can't escape the function. + // HACK(eddyb) one exception to this is function-local variables, which may + // be unused and as such cannot be allowed to always zombie the function. + for func in &module.functions { + let func_id = func.def_id().unwrap(); + if self.id_to_zombie_kind.contains_key(&func_id) { + // Func is already zombie, no need to scan it again. + continue; + } + + let mut all_zombie_uses_in_func = vec![]; + let mut file_id_line_col = None; + for inst in func.all_inst_iter() { + match inst.class.opcode { + Op::Line => { + file_id_line_col = Some(( + inst.operands[0].unwrap_id_ref(), + inst.operands[1].unwrap_literal_int32(), + inst.operands[2].unwrap_literal_int32(), + )); + } + Op::NoLine => file_id_line_col = None, + _ => {} + } + + if inst.class.opcode == Op::Variable { + let result_id = inst.result_id.unwrap(); + if self.id_to_zombie_kind.contains_key(&result_id) { continue; } - let reason = reason.clone(); - zombies.insert(result_id, reason); - any = true; + let zombie_uses: Vec<_> = self + .zombies_used_from_inst(inst) + .map(|zombie| ZombieUse { + used_zombie_id: zombie.id, + use_file_id_line_col: file_id_line_col, + origin: UseOrigin::IntraFuncOperandOrResultType { + parent_func_id: func_id, + }, + }) + .collect(); + if !zombie_uses.is_empty() { + self.id_to_zombie_kind + .insert(result_id, ZombieKind::Uses(zombie_uses)); + any = true; + } + continue; } - } else if let Some(reason) = is_or_contains_zombie(inst, zombies) { - let pushed_reason = reason.push_stack(func_id); - zombies.insert(func_id, pushed_reason); + + all_zombie_uses_in_func.extend( + inst.result_id + .and_then(|result_id| self.get_zombie_by_id(result_id)) + .into_iter() + .chain(self.zombies_used_from_inst(inst)) + .map(|zombie| { + let origin = if inst.class.opcode == Op::FunctionCall + && inst.operands[0] == Operand::IdRef(zombie.id) + { + UseOrigin::CallCalleeOperand { + caller_func_id: func_id, + } + } else { + UseOrigin::IntraFuncOperandOrResultType { + parent_func_id: func_id, + } + }; + ZombieUse { + used_zombie_id: zombie.id, + use_file_id_line_col: file_id_line_col, + origin, + } + }), + ); + } + if !all_zombie_uses_in_func.is_empty() { + self.id_to_zombie_kind + .insert(func_id, ZombieKind::Uses(all_zombie_uses_in_func)); any = true; - break; } } + any } - any } -// If an entry point references a zombie'd value, then the entry point would normally get removed. -// That's an absolutely horrible experience to debug, though, so instead, create a nice error -// message containing the stack trace of how the entry point got to the zombie value. -fn report_error_zombies( - sess: &Session, - module: &Module, - zombies: &FxHashMap, -) -> super::Result<()> { - let mut span_regen = SpanRegenerator::new(sess.source_map(), module); - - let mut result = Ok(()); - let mut names = None; - for root in super::dce::collect_roots(module) { - if let Some(zombie) = zombies.get(&root) { - let reason = span_regen.zombie_for_id(zombie.leaf_id).unwrap().reason; - let span = span_regen - .src_loc_for_id(zombie.leaf_id) - .and_then(|src_loc| span_regen.src_loc_to_rustc(src_loc)) - .unwrap_or(DUMMY_SP); - let names = names.get_or_insert_with(|| get_names(module)); - let stack = zombie - .stack - .iter() - .map(|&s| get_name(names, s).into_owned()); - let stack_note = once("Stack:".to_string()) - .chain(stack) - .collect::>() - .join("\n"); - result = Err(sess.struct_span_err(span, reason).note(&stack_note).emit()); +struct ZombieReporter<'a> { + sess: &'a Session, + module: &'a Module, + zombies: &'a Zombies, + + id_to_name: Option>, + span_regen: SpanRegenerator<'a>, +} +impl<'a> ZombieReporter<'a> { + fn new(sess: &'a Session, module: &'a Module, zombies: &'a Zombies) -> Self { + Self { + sess, + module, + zombies, + + id_to_name: None, + span_regen: SpanRegenerator::new(sess.source_map(), module), } } - result + + // If an entry point references a zombie'd value, then the entry point would normally get removed. + // That's an absolutely horrible experience to debug, though, so instead, create a nice error + // message containing the stack trace of how the entry point got to the zombie value. + fn report_all(mut self) -> super::Result<()> { + let mut result = Ok(()); + // FIXME(eddyb) this loop means that every entry-point can potentially + // list out all the leaves, but that shouldn't be a huge issue. + for root_id in super::dce::collect_roots(self.module) { + if let Some(zombie) = self.zombies.get_zombie_by_id(root_id) { + for (_, mut err) in self.build_errors_keyed_by_leaf_id(zombie) { + result = Err(err.emit()); + } + } + } + result + } + + fn add_use_note_to_err( + &mut self, + err: &mut DiagnosticBuilder<'a, ErrorGuaranteed>, + span: Span, + zombie: Zombie<'_>, + zombie_use: &ZombieUse, + ) { + let mut id_to_name = |id, kind| { + self.id_to_name + .get_or_insert_with(|| get_names(self.module)) + .get(&id) + .map_or_else( + || format!("unnamed {kind} (%{id})"), + |&name| format!("`{name}`"), + ) + }; + let note = match zombie_use.origin { + UseOrigin::GlobalOperandOrResultType => { + format!("used by {}", id_to_name(zombie.id, "global")) + } + UseOrigin::IntraFuncOperandOrResultType { parent_func_id } => { + format!( + "used from within {}", + id_to_name(parent_func_id, "function") + ) + } + UseOrigin::CallCalleeOperand { caller_func_id } => { + format!("called by {}", id_to_name(caller_func_id, "function")) + } + }; + + let span = zombie_use + .use_file_id_line_col + .and_then(|(file_id, line, col)| { + self.span_regen.src_loc_from_op_line(file_id, line, col) + }) + .and_then(|src_loc| self.span_regen.src_loc_to_rustc(src_loc)) + .unwrap_or(span); + err.span_note(span, note); + } + + fn build_errors_keyed_by_leaf_id( + &mut self, + zombie: Zombie<'_>, + ) -> FxIndexMap> { + // FIXME(eddyb) this is a bit inefficient, compared to some kind of + // "small map", but this is the error path, and being correct is more + // important here - in particular, we don't want to ignore *any* leaves. + let mut errors_keyed_by_leaf_id = FxIndexMap::default(); + + let span = self + .span_regen + .src_loc_for_id(zombie.id) + .and_then(|src_loc| self.span_regen.src_loc_to_rustc(src_loc)) + .unwrap_or(DUMMY_SP); + match zombie.kind { + ZombieKind::Leaf => { + let reason = self.span_regen.zombie_for_id(zombie.id).unwrap().reason; + errors_keyed_by_leaf_id.insert(zombie.id, self.sess.struct_span_err(span, reason)); + } + ZombieKind::Uses(zombie_uses) => { + for zombie_use in zombie_uses { + let used_zombie = self + .zombies + .get_zombie_by_id(zombie_use.used_zombie_id) + .unwrap(); + for (leaf_id, err) in self.build_errors_keyed_by_leaf_id(used_zombie) { + use rustc_data_structures::fx::IndexEntry as Entry; + match errors_keyed_by_leaf_id.entry(leaf_id) { + Entry::Occupied(_) => err.cancel(), + Entry::Vacant(entry) => { + self.add_use_note_to_err( + entry.insert(err), + span, + zombie, + zombie_use, + ); + } + } + } + } + } + } + errors_keyed_by_leaf_id + } } pub fn remove_zombies( @@ -143,30 +313,35 @@ pub fn remove_zombies( opts: &super::Options, module: &mut Module, ) -> super::Result<()> { - let mut zombies = ZombieDecoration::decode_all(module) - .map(|(id, _)| { - ( - id, - Zombie { - leaf_id: id, - stack: vec![], - }, - ) - }) - .collect(); + let mut zombies = Zombies { + id_to_zombie_kind: ZombieDecoration::decode_all(module) + .map(|(id, _)| (id, ZombieKind::Leaf)) + .collect(), + }; // Note: This is O(n^2). - while spread_zombie(module, &mut zombies) {} + while zombies.spread(module) {} - let result = report_error_zombies(sess, module, &zombies); + let result = ZombieReporter::new(sess, module, &zombies).report_all(); // FIXME(eddyb) use `log`/`tracing` instead. if opts.print_all_zombie { let mut span_regen = SpanRegenerator::new(sess.source_map(), module); - for (&zombie_id, zombie) in &zombies { - let reason = span_regen.zombie_for_id(zombie.leaf_id).unwrap().reason; + for &zombie_id in zombies.id_to_zombie_kind.keys() { + let mut zombie_leaf_id = zombie_id; + let mut infection_chain = SmallVec::<[_; 4]>::new(); + loop { + zombie_leaf_id = match zombies.get_zombie_by_id(zombie_leaf_id).unwrap().kind { + ZombieKind::Leaf => break, + // FIXME(eddyb) this is all very lossy and should probably go away. + ZombieKind::Uses(zombie_uses) => zombie_uses[0].used_zombie_id, + }; + infection_chain.push(zombie_leaf_id); + } + + let reason = span_regen.zombie_for_id(zombie_leaf_id).unwrap().reason; eprint!("zombie'd %{zombie_id} because {reason}"); - if zombie_id != zombie.leaf_id { - eprint!(" (infected by %{})", zombie.leaf_id); + if !infection_chain.is_empty() { + eprint!(" (infected via {:?})", infection_chain); } eprintln!(); } @@ -176,54 +351,46 @@ pub fn remove_zombies( let mut span_regen = SpanRegenerator::new(sess.source_map(), module); let names = get_names(module); for f in &module.functions { - if let Some(zombie) = is_zombie(f.def.as_ref().unwrap(), &zombies) { + if let Some(zombie) = zombies.get_zombie_by_id(f.def_id().unwrap()) { + let mut zombie_leaf_id = zombie.id; + loop { + zombie_leaf_id = match zombies.get_zombie_by_id(zombie_leaf_id).unwrap().kind { + ZombieKind::Leaf => break, + // FIXME(eddyb) this is all very lossy and should probably go away. + ZombieKind::Uses(zombie_uses) => zombie_uses[0].used_zombie_id, + }; + } + let name = get_name(&names, f.def_id().unwrap()); - let reason = span_regen.zombie_for_id(zombie.leaf_id).unwrap().reason; + let reason = span_regen.zombie_for_id(zombie_leaf_id).unwrap().reason; eprintln!("function removed {name:?} because {reason:?}"); } } } - module - .capabilities - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .extensions - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .ext_inst_imports - .retain(|inst| is_zombie(inst, &zombies).is_none()); - if module - .memory_model - .as_ref() - .map_or(false, |inst| is_zombie(inst, &zombies).is_some()) + // FIXME(eddyb) this should be unnecessary, either something is unused, and + // it will get DCE'd *anyway*, or it caused an error. { - module.memory_model = None; + let keep = |inst: &Instruction| { + if let Some(result_id) = inst.result_id { + zombies.get_zombie_by_id(result_id).is_none() + } else { + zombies.zombies_used_from_inst(inst).next().is_none() + } + }; + module.capabilities.retain(keep); + module.extensions.retain(keep); + module.ext_inst_imports.retain(keep); + module.memory_model = module.memory_model.take().filter(keep); + module.entry_points.retain(keep); + module.execution_modes.retain(keep); + module.debug_string_source.retain(keep); + module.debug_names.retain(keep); + module.debug_module_processed.retain(keep); + module.annotations.retain(keep); + module.types_global_values.retain(keep); + module.functions.retain(|f| keep(f.def.as_ref().unwrap())); } - module - .entry_points - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .execution_modes - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .debug_string_source - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .debug_names - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .debug_module_processed - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .annotations - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .types_global_values - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .functions - .retain(|f| is_zombie(f.def.as_ref().unwrap(), &zombies).is_none()); result } diff --git a/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr b/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr index d04f4bcd31..4cee425ab0 100644 --- a/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr +++ b/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr @@ -8,8 +8,8 @@ OpExtension "SPV_KHR_shader_clock" OpMemoryModel Logical Simple OpEntryPoint Fragment %1 "main" OpExecutionMode %1 OriginUpperLeft -%2 = OpString "$OPSTRING_FILENAME/non-writable-storage_buffer.not_spirt.rs" -%3 = OpString "$OPSTRING_FILENAME/cell.rs" +%2 = OpString "$OPSTRING_FILENAME/cell.rs" +%3 = OpString "$OPSTRING_FILENAME/non-writable-storage_buffer.not_spirt.rs" OpName %4 "buf_imm" OpName %5 "buf_mut" OpName %6 "buf_interior_mut" diff --git a/tests/ui/dis/ptr_copy.normal.stderr b/tests/ui/dis/ptr_copy.normal.stderr index 2e27465411..1691090d61 100644 --- a/tests/ui/dis/ptr_copy.normal.stderr +++ b/tests/ui/dis/ptr_copy.normal.stderr @@ -4,11 +4,26 @@ error: Cannot memcpy dynamically sized data 2460 | copy(src, dst, count) | ^ | - = note: Stack: - core::intrinsics::copy:: - ptr_copy::copy_via_raw_ptr - ptr_copy::main - main +note: used from within `core::intrinsics::copy::` + --> $CORE_SRC/intrinsics.rs:2447:21 + | +2447 | pub const unsafe fn copy(src: *const T, dst: *mut T, count: usize) { + | ^ +note: called by `ptr_copy::copy_via_raw_ptr` + --> $DIR/ptr_copy.rs:30:18 + | +30 | unsafe { core::ptr::copy(src, dst, 1) } + | ^ +note: called by `ptr_copy::main` + --> $DIR/ptr_copy.rs:35:5 + | +35 | copy_via_raw_ptr(&i, o); + | ^ +note: called by `main` + --> $DIR/ptr_copy.rs:33:1 + | +33 | #[spirv(fragment)] + | ^ error: aborting due to previous error diff --git a/tests/ui/lang/consts/nested-ref-in-composite.stderr b/tests/ui/lang/consts/nested-ref-in-composite.stderr index 9f5c2a7aba..d6205a866a 100644 --- a/tests/ui/lang/consts/nested-ref-in-composite.stderr +++ b/tests/ui/lang/consts/nested-ref-in-composite.stderr @@ -4,9 +4,16 @@ error: constant arrays/structs cannot contain pointers to other constants 20 | *pair_out = pair_deep_load(&(&123, &3.14)); | ^ | - = note: Stack: - nested_ref_in_composite::main_pair - main_pair +note: used from within `nested_ref_in_composite::main_pair` + --> $DIR/nested-ref-in-composite.rs:20:17 + | +20 | *pair_out = pair_deep_load(&(&123, &3.14)); + | ^ +note: called by `main_pair` + --> $DIR/nested-ref-in-composite.rs:18:1 + | +18 | #[spirv(fragment)] + | ^ error: constant arrays/structs cannot contain pointers to other constants --> $DIR/nested-ref-in-composite.rs:25:19 @@ -14,9 +21,16 @@ error: constant arrays/structs cannot contain pointers to other constants 25 | *array3_out = array3_deep_load(&[&0, &1, &2]); | ^ | - = note: Stack: - nested_ref_in_composite::main_array3 - main_array3 +note: used from within `nested_ref_in_composite::main_array3` + --> $DIR/nested-ref-in-composite.rs:25:19 + | +25 | *array3_out = array3_deep_load(&[&0, &1, &2]); + | ^ +note: called by `main_array3` + --> $DIR/nested-ref-in-composite.rs:23:1 + | +23 | #[spirv(fragment)] + | ^ error: aborting due to 2 previous errors diff --git a/tests/ui/lang/core/ptr/allocate_const_scalar.stderr b/tests/ui/lang/core/ptr/allocate_const_scalar.stderr index 05d7b4690e..c6ca98ddc3 100644 --- a/tests/ui/lang/core/ptr/allocate_const_scalar.stderr +++ b/tests/ui/lang/core/ptr/allocate_const_scalar.stderr @@ -1,8 +1,15 @@ error: pointer has non-null integer address - | - = note: Stack: - allocate_const_scalar::main - main + | +note: used from within `allocate_const_scalar::main` + --> $DIR/allocate_const_scalar.rs:15:20 + | +15 | let _pointer = POINTER; + | ^ +note: called by `main` + --> $DIR/allocate_const_scalar.rs:13:1 + | +13 | #[spirv(fragment)] + | ^ error: aborting due to previous error From 4b646c06c2216c815df71c50d5213d6e9352d864 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Wed, 19 Apr 2023 04:55:51 +0300 Subject: [PATCH 10/13] Replace "system crate" vs "user code" distinction with zombies everywhere. --- crates/rustc_codegen_spirv/src/abi.rs | 2 +- .../src/builder/builder_methods.rs | 34 +++++----- .../src/builder/byte_addressable_buffer.rs | 4 +- crates/rustc_codegen_spirv/src/builder/mod.rs | 4 +- .../rustc_codegen_spirv/src/builder_spirv.rs | 61 ++++++------------ .../src/codegen_cx/constant.rs | 32 ++++------ .../src/codegen_cx/declare.rs | 3 +- .../src/codegen_cx/entry.rs | 2 +- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 63 ++++-------------- crates/rustc_codegen_spirv/src/spirv_type.rs | 64 +++++++++---------- tests/ui/dis/ptr_copy.normal.stderr | 2 +- .../core/ref/zst_member_ref_arg-broken.stderr | 62 +++++++++++++----- 12 files changed, 147 insertions(+), 186 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/abi.rs b/crates/rustc_codegen_spirv/src/abi.rs index 91c28476dc..d4e2004b58 100644 --- a/crates/rustc_codegen_spirv/src/abi.rs +++ b/crates/rustc_codegen_spirv/src/abi.rs @@ -210,7 +210,7 @@ impl<'tcx> RecursivePointeeCache<'tcx> { cx.zombie_with_span( new_id, span, - "Cannot create self-referential types, even through pointers", + "cannot create self-referential types, even through pointers", ); Some(new_id) } diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index d81a171dc8..f2cca482ef 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -171,7 +171,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { if invalid_seq_cst { self.zombie( semantics.def(self), - "Cannot use AtomicOrdering=SequentiallyConsistent on Vulkan memory model. Check if AcquireRelease fits your needs.", + "cannot use AtomicOrdering=SequentiallyConsistent on Vulkan memory model \ + (check if AcquireRelease fits your needs)", ); } semantics @@ -352,11 +353,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } fn zombie_convert_ptr_to_u(&self, def: Word) { - self.zombie(def, "Cannot convert pointers to integers"); + self.zombie(def, "cannot convert pointers to integers"); } fn zombie_convert_u_to_ptr(&self, def: Word) { - self.zombie(def, "Cannot convert integers to pointers"); + self.zombie(def, "cannot convert integers to pointers"); } fn zombie_ptr_equal(&self, def: Word, inst: &str) { @@ -1453,21 +1454,16 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { .with_type(dest_ty); if val_is_ptr || dest_is_ptr { - if self.is_system_crate(self.span()) { - self.zombie( - result.def(self), - &format!( - "Cannot cast between pointer and non-pointer types. From: {}. To: {}.", - self.debug_type(val.ty), - self.debug_type(dest_ty) - ), - ); - } else { - self.struct_err("Cannot cast between pointer and non-pointer types") - .note(&format!("from: {}", self.debug_type(val.ty))) - .note(&format!("to: {}", self.debug_type(dest_ty))) - .emit(); - } + self.zombie( + result.def(self), + &format!( + "cannot cast between pointer and non-pointer types\ + \nfrom `{}`\ + \n to `{}`", + self.debug_type(val.ty), + self.debug_type(dest_ty) + ), + ); } result @@ -1884,7 +1880,7 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { empty(), ) .unwrap(); - self.zombie(dst.def(self), "Cannot memcpy dynamically sized data"); + self.zombie(dst.def(self), "cannot memcpy dynamically sized data"); } } diff --git a/crates/rustc_codegen_spirv/src/builder/byte_addressable_buffer.rs b/crates/rustc_codegen_spirv/src/builder/byte_addressable_buffer.rs index ceb38c43a3..6128752ff9 100644 --- a/crates/rustc_codegen_spirv/src/builder/byte_addressable_buffer.rs +++ b/crates/rustc_codegen_spirv/src/builder/byte_addressable_buffer.rs @@ -11,7 +11,7 @@ use rustc_target::abi::{Align, Size}; impl<'a, 'tcx> Builder<'a, 'tcx> { fn load_err(&mut self, original_type: Word, invalid_type: Word) -> SpirvValue { let mut err = self.struct_err(&format!( - "Cannot load type {} in an untyped buffer load", + "cannot load type {} in an untyped buffer load", self.debug_type(original_type) )); if original_type != invalid_type { @@ -206,7 +206,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { fn store_err(&mut self, original_type: Word, value: SpirvValue) -> Result<(), ErrorGuaranteed> { let mut err = self.struct_err(&format!( - "Cannot store type {} in an untyped buffer store", + "cannot store type {} in an untyped buffer store", self.debug_type(original_type) )); if original_type != value.ty { diff --git a/crates/rustc_codegen_spirv/src/builder/mod.rs b/crates/rustc_codegen_spirv/src/builder/mod.rs index 16bc8ef489..4771ad2463 100644 --- a/crates/rustc_codegen_spirv/src/builder/mod.rs +++ b/crates/rustc_codegen_spirv/src/builder/mod.rs @@ -208,7 +208,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { .with_type(result_type); self.zombie( result.def(self), - "Cannot offset a pointer to an arbitrary element", + "cannot offset a pointer to an arbitrary element", ); result } @@ -219,7 +219,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let width = match self.lookup_type(shift.ty) { SpirvType::Integer(width, _) => width, other => self.fatal(&format!( - "Cannot rotate non-integer type: {}", + "cannot rotate non-integer type: {}", other.debug(shift.ty, self) )), }; diff --git a/crates/rustc_codegen_spirv/src/builder_spirv.rs b/crates/rustc_codegen_spirv/src/builder_spirv.rs index 86f1477888..3c003c6a34 100644 --- a/crates/rustc_codegen_spirv/src/builder_spirv.rs +++ b/crates/rustc_codegen_spirv/src/builder_spirv.rs @@ -140,9 +140,7 @@ impl SpirvValue { IllegalConst::Indirect(cause) => cause.message(), }; - // HACK(eddyb) we don't know whether this constant originated - // in a system crate, so it's better to always zombie. - cx.zombie_even_in_user_code(id, span, msg); + cx.zombie_with_span(id, span, msg); id } @@ -158,25 +156,15 @@ impl SpirvValue { } SpirvValueKind::FnAddr { .. } => { - if cx.is_system_crate(span) { - cx.builder - .const_to_id - .borrow() - .get(&WithType { - ty: self.ty, - val: SpirvConst::ZombieUndefForFnAddr, - }) - .expect("FnAddr didn't go through proper undef registration") - .val - } else { - cx.tcx.sess.span_err( - span, - "Cannot use this function pointer for anything other than calls", - ); - // Because we never get beyond compilation (into e.g. linking), - // emitting an invalid ID reference here is OK. - 0 - } + cx.builder + .const_to_id + .borrow() + .get(&WithType { + ty: self.ty, + val: SpirvConst::ZombieUndefForFnAddr, + }) + .expect("FnAddr didn't go through proper undef registration") + .val } SpirvValueKind::LogicalPtrCast { @@ -184,24 +172,17 @@ impl SpirvValue { original_pointee_ty, zombie_target_undef, } => { - if cx.is_system_crate(span) { - cx.zombie_with_span( - zombie_target_undef, - span, - &format!( - "Cannot cast between pointer types. From: {}. To: {}.", - cx.debug_type(original_pointee_ty), - cx.debug_type(self.ty) - ), - ); - } else { - cx.tcx - .sess - .struct_span_err(span, "Cannot cast between pointer types") - .note(&format!("from: *{}", cx.debug_type(original_pointee_ty))) - .note(&format!("to: {}", cx.debug_type(self.ty))) - .emit(); - } + cx.zombie_with_span( + zombie_target_undef, + span, + &format!( + "cannot cast between pointer types\ + \nfrom `*{}`\ + \n to `{}`", + cx.debug_type(original_pointee_ty), + cx.debug_type(self.ty) + ), + ); zombie_target_undef } diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs b/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs index 28b82a995d..dd75cc687c 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs @@ -255,11 +255,8 @@ impl<'tcx> ConstMethods<'tcx> for CodegenCx<'tcx> { self.constant_null(ty) } else { let result = self.undef(ty); - // HACK(eddyb) we don't know whether this constant originated - // in a system crate, so it's better to always zombie. - self.zombie_even_in_user_code( + self.zombie_no_span( result.def_cx(self), - DUMMY_SP, "pointer has non-null integer address", ); result @@ -408,7 +405,7 @@ impl<'tcx> CodegenCx<'tcx> { SpirvType::Void => self .tcx .sess - .fatal("Cannot create const alloc of type void"), + .fatal("cannot create const alloc of type void"), SpirvType::Bool | SpirvType::Integer(..) | SpirvType::Float(_) @@ -552,37 +549,36 @@ impl<'tcx> CodegenCx<'tcx> { *data = *c + asdf->y[*c]; } */ - // HACK(eddyb) we don't know whether this constant originated - // in a system crate, so it's better to always zombie. - self.zombie_even_in_user_code( - result.def_cx(self), - DUMMY_SP, - "constant runtime array value", - ); + // NOTE(eddyb) the above description is a bit outdated, it's now + // clear `OpTypeRuntimeArray` does not belong in user code, and + // is only for dynamically-sized SSBOs and descriptor indexing, + // and a general solution looks similar to `union` handling, but + // for the length of a fixed-length array. + self.zombie_no_span(result.def_cx(self), "constant `OpTypeRuntimeArray` value"); result } SpirvType::Function { .. } => self .tcx .sess .fatal("TODO: SpirvType::Function not supported yet in create_const_alloc"), - SpirvType::Image { .. } => self.tcx.sess.fatal("Cannot create a constant image value"), + SpirvType::Image { .. } => self.tcx.sess.fatal("cannot create a constant image value"), SpirvType::Sampler => self .tcx .sess - .fatal("Cannot create a constant sampler value"), + .fatal("cannot create a constant sampler value"), SpirvType::SampledImage { .. } => self .tcx .sess - .fatal("Cannot create a constant sampled image value"), + .fatal("cannot create a constant sampled image value"), SpirvType::InterfaceBlock { .. } => self .tcx .sess - .fatal("Cannot create a constant interface block value"), + .fatal("cannot create a constant interface block value"), SpirvType::AccelerationStructureKhr => self .tcx .sess - .fatal("Cannot create a constant acceleration structure"), - SpirvType::RayQueryKhr => self.tcx.sess.fatal("Cannot create a constant ray query"), + .fatal("cannot create a constant acceleration structure"), + SpirvType::RayQueryKhr => self.tcx.sess.fatal("cannot create a constant ray query"), } } } diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs b/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs index 0ea3cfe775..b155d535bc 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs @@ -217,7 +217,8 @@ impl<'tcx> CodegenCx<'tcx> { .variable(ptr_ty, None, StorageClass::Private, None) .with_type(ptr_ty); // TODO: These should be StorageClass::Private, so just zombie for now. - self.zombie_with_span(result.def_cx(self), span, "Globals are not supported yet"); + // FIXME(eddyb) why zombie? this looks like it should just work nowadays. + self.zombie_with_span(result.def_cx(self), span, "globals are not supported yet"); result } } diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs b/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs index 461f35fac0..3d3a62f876 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs @@ -70,7 +70,7 @@ impl<'tcx> CodegenCx<'tcx> { } else { self.tcx .sess - .span_err(span, format!("Cannot declare {name} as an entry point")); + .span_err(span, format!("cannot declare {name} as an entry point")); return; }; let body = self diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index d1a6297c70..baf4e6eeda 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -25,7 +25,7 @@ use rustc_middle::ty::layout::{HasParamEnv, HasTyCtxt}; use rustc_middle::ty::{Instance, ParamEnv, PolyExistentialTraitRef, Ty, TyCtxt}; use rustc_session::Session; use rustc_span::def_id::DefId; -use rustc_span::symbol::{sym, Symbol}; +use rustc_span::symbol::Symbol; use rustc_span::{SourceFile, Span, DUMMY_SP}; use rustc_target::abi::call::{FnAbi, PassMode}; use rustc_target::abi::{HasDataLayout, TargetDataLayout}; @@ -164,28 +164,16 @@ impl<'tcx> CodegenCx<'tcx> { } /// Zombie system: - /// When compiling libcore and other system libraries, if something unrepresentable is - /// encountered, we don't want to fail the compilation. Instead, we emit something bogus - /// (usually it's fairly faithful, though, e.g. u128 emits `OpTypeInt 128`), and then mark the - /// resulting ID as a "zombie". We continue compiling the rest of the crate, then, at the very - /// end, anything that transtively references a zombie value is stripped from the binary. /// - /// If an exported function is stripped, then we emit a special "zombie export" item, which is - /// consumed by the linker, which continues to infect other values that reference it. + /// If something unrepresentable is encountered, we don't want to fail + /// the compilation. Instead, we emit something bogus (usually it's fairly + /// faithful, though, e.g. `u128` emits `OpTypeInt 128 0`), and then mark the + /// resulting ID as a "zombie". We continue compiling the rest of the crate, + /// then, at the very end, anything that transitively references a zombie value + /// is stripped from the binary. /// - /// Finally, if *user* code is marked as zombie, then this means that the user tried to do - /// something that isn't supported, and should be an error. + /// Errors will only be emitted (by `linker::zombies`) for reachable zombies. pub fn zombie_with_span(&self, word: Word, span: Span, reason: &str) { - if self.is_system_crate(span) { - self.zombie_even_in_user_code(word, span, reason); - } else { - self.tcx.sess.span_err(span, reason); - } - } - pub fn zombie_no_span(&self, word: Word, reason: &str) { - self.zombie_with_span(word, DUMMY_SP, reason); - } - pub fn zombie_even_in_user_code(&self, word: Word, span: Span, reason: &str) { self.zombie_decorations.borrow_mut().insert( word, ( @@ -198,31 +186,8 @@ impl<'tcx> CodegenCx<'tcx> { ), ); } - - /// Returns `true` if the originating crate of `span` (which could very well - /// be a different crate, e.g. a generic/`#[inline]` function, or a macro), - /// is a "system crate", and therefore allowed to have some errors deferred - /// as "zombies" (see `zombie_with_span`'s docs above for more details). - pub fn is_system_crate(&self, span: Span) -> bool { - // HACK(eddyb) this ignores `.lo` vs `.hi` potentially resulting in - // different `SourceFile`s (which is likely a bug anyway). - let cnum = self - .tcx - .sess - .source_map() - .lookup_source_file(span.data().lo) - .cnum; - - self.tcx - .get_attr(cnum.as_def_id(), sym::compiler_builtins) - .is_some() - || [ - sym::core, - self.sym.spirv_std, - self.sym.libm, - self.sym.num_traits, - ] - .contains(&self.tcx.crate_name(cnum)) + pub fn zombie_no_span(&self, word: Word, reason: &str) { + self.zombie_with_span(word, DUMMY_SP, reason); } pub fn finalize_module(self) -> Module { @@ -796,11 +761,9 @@ impl<'tcx> MiscMethods<'tcx> for CodegenCx<'tcx> { } .def(span, self); - if self.is_system_crate(span) { - // Create these undefs up front instead of on demand in SpirvValue::def because - // SpirvValue::def can't use cx.emit() - self.def_constant(ty, SpirvConst::ZombieUndefForFnAddr); - } + // Create these `OpUndef`s up front, instead of on-demand in `SpirvValue::def`, + // because `SpirvValue::def` can't use `cx.emit()`. + self.def_constant(ty, SpirvConst::ZombieUndefForFnAddr); SpirvValue { kind: SpirvValueKind::FnAddr { diff --git a/crates/rustc_codegen_spirv/src/spirv_type.rs b/crates/rustc_codegen_spirv/src/spirv_type.rs index 5cce80991a..73cfdc662f 100644 --- a/crates/rustc_codegen_spirv/src/spirv_type.rs +++ b/crates/rustc_codegen_spirv/src/spirv_type.rs @@ -103,27 +103,28 @@ impl SpirvType<'_> { Self::Bool => cx.emit_global().type_bool_id(id), Self::Integer(width, signedness) => { let result = cx.emit_global().type_int_id(id, width, signedness as u32); + let u_or_i = if signedness { "i" } else { "u" }; match width { - 8 if !cx.builder.has_capability(Capability::Int8) => cx - .zombie_even_in_user_code(result, def_span, "u8 without OpCapability Int8"), - 16 if !cx.builder.has_capability(Capability::Int16) => cx - .zombie_even_in_user_code( - result, - def_span, - "u16 without OpCapability Int16", - ), - 64 if !cx.builder.has_capability(Capability::Int64) => cx - .zombie_even_in_user_code( - result, - def_span, - "u64 without OpCapability Int64", - ), - 8 | 16 | 32 | 64 => (), - 128 => cx.zombie_with_span(result, def_span, "u128"), - other => cx.zombie_with_span( + 8 if !cx.builder.has_capability(Capability::Int8) => cx.zombie_with_span( + result, + def_span, + &format!("`{u_or_i}8` without `OpCapability Int8`"), + ), + 16 if !cx.builder.has_capability(Capability::Int16) => cx.zombie_with_span( + result, + def_span, + &format!("`{u_or_i}16` without `OpCapability Int16`"), + ), + 64 if !cx.builder.has_capability(Capability::Int64) => cx.zombie_with_span( + result, + def_span, + &format!("`{u_or_i}64` without `OpCapability Int64`"), + ), + 8 | 16 | 32 | 64 => {} + w => cx.zombie_with_span( result, def_span, - &format!("Integer width {other} invalid for spir-v"), + &format!("`{u_or_i}{w}` unsupported in SPIR-V"), ), }; result @@ -131,17 +132,16 @@ impl SpirvType<'_> { Self::Float(width) => { let result = cx.emit_global().type_float_id(id, width); match width { - 64 if !cx.builder.has_capability(Capability::Float64) => cx - .zombie_even_in_user_code( - result, - def_span, - "f64 without OpCapability Float64", - ), + 64 if !cx.builder.has_capability(Capability::Float64) => cx.zombie_with_span( + result, + def_span, + "`f64` without `OpCapability Float64`", + ), 32 | 64 => (), other => cx.zombie_with_span( result, def_span, - &format!("Float width {other} invalid for spir-v"), + &format!("`f{other}` unsupported in SPIR-V"), ), }; result @@ -217,11 +217,8 @@ impl SpirvType<'_> { .type_pointer(id, StorageClass::Generic, pointee); // no pointers to functions if let SpirvType::Function { .. } = cx.lookup_type(pointee) { - cx.zombie_even_in_user_code( - result, - def_span, - "function pointer types are not allowed", - ); + // FIXME(eddyb) use the `SPV_INTEL_function_pointers` extension. + cx.zombie_with_span(result, def_span, "function pointer types are not allowed"); } result } @@ -293,11 +290,8 @@ impl SpirvType<'_> { .type_pointer(Some(id), StorageClass::Generic, pointee); // no pointers to functions if let SpirvType::Function { .. } = cx.lookup_type(pointee) { - cx.zombie_even_in_user_code( - result, - def_span, - "function pointer types are not allowed", - ); + // FIXME(eddyb) use the `SPV_INTEL_function_pointers` extension. + cx.zombie_with_span(result, def_span, "function pointer types are not allowed"); } result } diff --git a/tests/ui/dis/ptr_copy.normal.stderr b/tests/ui/dis/ptr_copy.normal.stderr index 1691090d61..c84157ee26 100644 --- a/tests/ui/dis/ptr_copy.normal.stderr +++ b/tests/ui/dis/ptr_copy.normal.stderr @@ -1,4 +1,4 @@ -error: Cannot memcpy dynamically sized data +error: cannot memcpy dynamically sized data --> $CORE_SRC/intrinsics.rs:2460:9 | 2460 | copy(src, dst, count) diff --git a/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr index c95cdafa55..bd341e6aa3 100644 --- a/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr +++ b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr @@ -1,29 +1,59 @@ -error: Cannot cast between pointer types - --> $DIR/zst_member_ref_arg-broken.rs:23:5 +error: cannot cast between pointer types + from `*struct (usize, usize) { u32, u32 }` + to `*struct B { }` + --> $DIR/zst_member_ref_arg-broken.rs:33:5 | -23 | f(&s.y); - | ^^^^^^^ +33 | f(&s.y); + | ^ + | +note: used from within `zst_member_ref_arg_broken::main_scalar_scalar_pair_nested` + --> $DIR/zst_member_ref_arg-broken.rs:33:5 + | +33 | f(&s.y); + | ^ +note: called by `main_scalar_scalar_pair_nested` + --> $DIR/zst_member_ref_arg-broken.rs:31:1 | - = note: from: *u32 - = note: to: *struct B { } +31 | #[spirv(fragment)] + | ^ -error: Cannot cast between pointer types - --> $DIR/zst_member_ref_arg-broken.rs:28:5 +error: cannot cast between pointer types + from `*struct (usize, usize) { u32, u32 }` + to `*struct B { }` + --> $DIR/zst_member_ref_arg-broken.rs:33:5 | -28 | f(&s.y); - | ^^^^^^^ +33 | f(&s.y); + | ^ + | +note: used from within `zst_member_ref_arg_broken::main_scalar` + --> $DIR/zst_member_ref_arg-broken.rs:23:5 + | +23 | f(&s.y); + | ^ +note: called by `main_scalar` + --> $DIR/zst_member_ref_arg-broken.rs:21:1 | - = note: from: *struct S { u32, u32 } - = note: to: *struct B { } +21 | #[spirv(fragment)] + | ^ -error: Cannot cast between pointer types +error: cannot cast between pointer types + from `*struct (usize, usize) { u32, u32 }` + to `*struct B { }` --> $DIR/zst_member_ref_arg-broken.rs:33:5 | 33 | f(&s.y); - | ^^^^^^^ + | ^ + | +note: used from within `zst_member_ref_arg_broken::main_scalar_pair` + --> $DIR/zst_member_ref_arg-broken.rs:28:5 + | +28 | f(&s.y); + | ^ +note: called by `main_scalar_pair` + --> $DIR/zst_member_ref_arg-broken.rs:26:1 | - = note: from: *struct (usize, usize) { u32, u32 } - = note: to: *struct B { } +26 | #[spirv(fragment)] + | ^ error: aborting due to 3 previous errors From 8cd3caa9e4b2166042360c5abbd28cb2bb5cbee6 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 21 Apr 2023 03:15:19 +0300 Subject: [PATCH 11/13] linker: add a SPIR-T-based zombie reporting alternative, behind `--no-early-report-zombies`. --- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 6 + crates/rustc_codegen_spirv/src/decorations.rs | 64 ++- crates/rustc_codegen_spirv/src/linker/mod.rs | 30 +- .../src/linker/spirt_passes/diagnostics.rs | 383 ++++++++++++++++++ .../src/linker/spirt_passes/mod.rs | 19 +- .../rustc_codegen_spirv/src/linker/zombies.rs | 2 +- docs/src/codegen-args.md | 6 + 7 files changed, 493 insertions(+), 17 deletions(-) create mode 100644 crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index baf4e6eeda..e900107ed6 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -330,6 +330,11 @@ impl CodegenArgs { "no-compact-ids", "disables compaction of SPIR-V IDs at the end of linking", ); + opts.optflag( + "", + "no-early-report-zombies", + "delays reporting zombies (to allow more legalization)", + ); opts.optflag("", "no-structurize", "disables CFG structurization"); opts.optflag( @@ -509,6 +514,7 @@ impl CodegenArgs { // FIXME(eddyb) clean up this `no-` "negation prefix" situation. dce: !matches.opt_present("no-dce"), compact_ids: !matches.opt_present("no-compact-ids"), + early_report_zombies: !matches.opt_present("no-early-report-zombies"), structurize: !matches.opt_present("no-structurize"), spirt: !matches.opt_present("no-spirt"), spirt_passes: matches diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index d06b9e0942..c3cbb843a0 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -2,6 +2,7 @@ //! the original codegen of a crate, and consumed by the `linker`. use crate::builder_spirv::BuilderSpirv; +use either::Either; use itertools::Itertools; use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Decoration, Op, Word}; @@ -14,7 +15,7 @@ use std::borrow::Cow; use std::marker::PhantomData; use std::ops::Range; use std::path::PathBuf; -use std::{fmt, iter, slice}; +use std::{fmt, iter, slice, str}; /// Decorations not native to SPIR-V require some form of encoding into existing /// SPIR-V constructs, for which we use `OpDecorateString` with decoration type @@ -127,11 +128,13 @@ impl<'a> CustomDecoration<'a> for ZombieDecoration<'a> { // // NOTE(eddyb) by keeping `line`+`col`, we mimick `OpLine` limitations // (which could be lifted in the future using custom SPIR-T debuginfo). +// NOTE(eddyb) `NonSemantic.Shader.DebugInfo`'s `DebugLine` has both start & end, +// might be good to invest in SPIR-T being able to use NonSemantic debuginfo. #[derive(Copy, Clone)] pub struct SrcLocDecoration<'a> { - file_name: &'a str, - line: u32, - col: u32, + pub file_name: &'a str, + pub line: u32, + pub col: u32, } impl<'a> CustomDecoration<'a> for SrcLocDecoration<'a> { @@ -184,7 +187,7 @@ impl<'tcx> SrcLocDecoration<'tcx> { /// back into an usable `Span`, until it's actually needed (i.e. for an error). pub struct SpanRegenerator<'a> { source_map: &'a SourceMap, - module: &'a Module, + module: Either<&'a Module, &'a spirt::Module>, src_loc_decorations: Option>>>, @@ -203,8 +206,35 @@ struct SpvDebugInfo<'a> { } impl<'a> SpvDebugInfo<'a> { - fn collect(module: &'a Module) -> Self { + fn collect(module: Either<&'a Module, &'a spirt::Module>) -> Self { let mut this = Self::default(); + + let module = match module { + Either::Left(module) => module, + + // HACK(eddyb) the SPIR-T codepath is simpler, and kind of silly, + // but we need the `SpvDebugFile`'s `regenerated_rustc_source_file` + // caching, so for now it reuses `SpvDebugInfo` overall. + Either::Right(module) => { + let cx = module.cx_ref(); + match &module.debug_info { + spirt::ModuleDebugInfo::Spv(debug_info) => { + for sources in debug_info.source_languages.values() { + for (&file_name, src) in &sources.file_contents { + // FIXME(eddyb) what if the file is already present, + // should it be considered ambiguous overall? + this.files + .entry(&cx[file_name]) + .or_default() + .op_source_parts = [&src[..]].into_iter().collect(); + } + } + } + } + return this; + } + }; + let mut insts = module.debug_string_source.iter().peekable(); while let Some(inst) = insts.next() { match inst.class.opcode { @@ -255,7 +285,19 @@ impl<'a> SpanRegenerator<'a> { pub fn new(source_map: &'a SourceMap, module: &'a Module) -> Self { Self { source_map, - module, + module: Either::Left(module), + + src_loc_decorations: None, + zombie_decorations: None, + + spv_debug_info: None, + } + } + + pub fn new_spirt(source_map: &'a SourceMap, module: &'a spirt::Module) -> Self { + Self { + source_map, + module: Either::Right(module), src_loc_decorations: None, zombie_decorations: None, @@ -266,7 +308,9 @@ impl<'a> SpanRegenerator<'a> { pub fn src_loc_for_id(&mut self, id: Word) -> Option> { self.src_loc_decorations - .get_or_insert_with(|| SrcLocDecoration::decode_all(self.module).collect()) + .get_or_insert_with(|| { + SrcLocDecoration::decode_all(self.module.left().unwrap()).collect() + }) .get(&id) .map(|src_loc| src_loc.decode()) } @@ -275,7 +319,9 @@ impl<'a> SpanRegenerator<'a> { // to handle it together with `SrcLocDecoration`, than separately. pub(crate) fn zombie_for_id(&mut self, id: Word) -> Option> { self.zombie_decorations - .get_or_insert_with(|| ZombieDecoration::decode_all(self.module).collect()) + .get_or_insert_with(|| { + ZombieDecoration::decode_all(self.module.left().unwrap()).collect() + }) .get(&id) .map(|zombie| zombie.decode()) } diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index 9611d26fbe..afe740920e 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -38,6 +38,7 @@ pub type Result = std::result::Result; pub struct Options { pub compact_ids: bool, pub dce: bool, + pub early_report_zombies: bool, pub structurize: bool, pub spirt: bool, pub spirt_passes: Vec, @@ -214,20 +215,27 @@ pub fn link( simple_passes::check_fragment_insts(sess, &output)?; } - // HACK(eddyb) this has to run before the `remove_zombies` pass, so that any - // zombies that are passed as call arguments, but eventually unused, won't - // be (incorrectly) considered used. + // HACK(eddyb) this has to run before the `report_and_remove_zombies` pass, + // so that any zombies that are passed as call arguments, but eventually unused, + // won't be (incorrectly) considered used. { let _timer = sess.timer("link_remove_unused_params"); output = param_weakening::remove_unused_params(output); } - { - let _timer = sess.timer("link_remove_zombies"); - zombies::remove_zombies(sess, opts, &mut output)?; + if opts.early_report_zombies { + let _timer = sess.timer("link_report_and_remove_zombies"); + zombies::report_and_remove_zombies(sess, opts, &mut output)?; } { + // HACK(eddyb) this is not the best approach, but storage class inference + // can still fail in entirely legitimate ways (i.e. mismatches in zombies). + if !opts.early_report_zombies { + let _timer = sess.timer("link_dce-before-specialize_generic_storage_class"); + dce::dce(&mut output); + } + let _timer = sess.timer("specialize_generic_storage_class"); // HACK(eddyb) `specializer` requires functions' blocks to be in RPO order // (i.e. `block_ordering_pass`) - this could be relaxed by using RPO visit @@ -411,6 +419,11 @@ pub fn link( ); } + let report_diagnostics_result = { + let _timer = sess.timer("spirt_passes::diagnostics::report_diagnostics"); + spirt_passes::diagnostics::report_diagnostics(sess, opts, &module) + }; + // 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 { @@ -446,6 +459,11 @@ pub fn link( .unwrap(); } + // NOTE(eddyb) this is late so that `--dump-spirt-passes` is processed, + // even/especially when error were reported, but lifting to SPIR-V is + // skipped (since it could very well fail due to reported errors). + report_diagnostics_result?; + let spv_words = { let _timer = sess.timer("spirt::Module::lift_to_spv_module_emitter"); module.lift_to_spv_module_emitter().unwrap().words diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs new file mode 100644 index 0000000000..94e1370cb7 --- /dev/null +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs @@ -0,0 +1,383 @@ +use crate::decorations::{CustomDecoration, SpanRegenerator, SrcLocDecoration, ZombieDecoration}; +use rustc_data_structures::fx::FxIndexSet; +use rustc_errors::{DiagnosticBuilder, ErrorGuaranteed}; +use rustc_session::Session; +use rustc_span::{Span, DUMMY_SP}; +use smallvec::SmallVec; +use spirt::visit::{InnerVisit, Visitor}; +use spirt::{ + spv, Attr, AttrSet, AttrSetDef, Const, Context, DataInstDef, DataInstKind, ExportKey, Exportee, + Func, GlobalVar, Module, Type, +}; +use std::marker::PhantomData; +use std::{mem, str}; + +pub(crate) fn report_diagnostics( + sess: &Session, + linker_options: &crate::linker::Options, + module: &Module, +) -> crate::linker::Result<()> { + let cx = &module.cx(); + + let mut reporter = DiagnosticReporter { + sess, + linker_options, + + cx, + module, + + seen_attrs: FxIndexSet::default(), + seen_types: FxIndexSet::default(), + seen_consts: FxIndexSet::default(), + seen_global_vars: FxIndexSet::default(), + seen_funcs: FxIndexSet::default(), + + use_stack: SmallVec::new(), + span_regen: SpanRegenerator::new_spirt(sess.source_map(), module), + overall_result: Ok(()), + }; + for (export_key, &exportee) in &module.exports { + assert_eq!(reporter.use_stack.len(), 0); + + if let Exportee::Func(func) = exportee { + let func_decl = &module.funcs[func]; + reporter.use_stack.push(UseOrigin::IntraFunc { + func_attrs: func_decl.attrs, + func_export_key: Some(export_key), + inst_attrs: AttrSet::default(), + origin: IntraFuncUseOrigin::Other, + }); + export_key.inner_visit_with(&mut reporter); + if reporter.seen_funcs.insert(func) { + reporter.visit_func_decl(func_decl); + } + reporter.use_stack.pop(); + } + export_key.inner_visit_with(&mut reporter); + exportee.inner_visit_with(&mut reporter); + } + reporter.overall_result +} + +// HACK(eddyb) version of `decorations::LazilyDecoded` that works for SPIR-T. +struct LazilyDecoded { + encoded: String, + _marker: PhantomData, +} + +impl LazilyDecoded { + fn decode<'a>(&'a self) -> D + where + D: CustomDecoration<'a>, + { + D::decode(&self.encoded) + } +} + +fn decode_spv_lit_str_with(imms: &[spv::Imm], f: impl FnOnce(&str) -> R) -> R { + let wk = &super::SpvSpecWithExtras::get().well_known; + + // FIXME(eddyb) deduplicate with `spirt::spv::extract_literal_string`. + let words = imms.iter().enumerate().map(|(i, &imm)| match (i, imm) { + (0, spirt::spv::Imm::Short(k, w) | spirt::spv::Imm::LongStart(k, w)) + | (1.., spirt::spv::Imm::LongCont(k, w)) => { + // FIXME(eddyb) use `assert_eq!` after updating to latest SPIR-T. + assert!(k == wk.LiteralString); + w + } + _ => unreachable!(), + }); + let bytes: SmallVec<[u8; 64]> = words + .flat_map(u32::to_le_bytes) + .take_while(|&byte| byte != 0) + .collect(); + + f(str::from_utf8(&bytes).expect("invalid UTF-8 in string literal")) +} + +fn try_decode_custom_decoration<'a, D: CustomDecoration<'a>>( + attrs_def: &AttrSetDef, +) -> Option> { + let wk = &super::SpvSpecWithExtras::get().well_known; + + attrs_def.attrs.iter().find_map(|attr| { + let spv_inst = match attr { + Attr::SpvAnnotation(spv_inst) if spv_inst.opcode == wk.OpDecorateString => spv_inst, + _ => return None, + }; + let str_imms = spv_inst + .imms + .strip_prefix(&[spv::Imm::Short(wk.Decoration, wk.UserTypeGOOGLE)])?; + + decode_spv_lit_str_with(str_imms, |prefixed_encoded| { + let encoded = prefixed_encoded.strip_prefix(D::ENCODING_PREFIX)?; + + Some(LazilyDecoded { + encoded: encoded.to_string(), + _marker: PhantomData, + }) + }) + }) +} + +// FIXME(eddyb) this looks a lot like `ReachableUseCollector`, maybe some +// automation should be built around "deep visitors" in general? +struct DiagnosticReporter<'a> { + sess: &'a Session, + linker_options: &'a crate::linker::Options, + + cx: &'a Context, + module: &'a Module, + + seen_attrs: FxIndexSet, + seen_types: FxIndexSet, + seen_consts: FxIndexSet, + seen_global_vars: FxIndexSet, + seen_funcs: FxIndexSet, + + use_stack: SmallVec<[UseOrigin<'a>; 8]>, + span_regen: SpanRegenerator<'a>, + overall_result: crate::linker::Result<()>, +} + +enum UseOrigin<'a> { + Global { + kind: &'static &'static str, + attrs: AttrSet, + }, + IntraFunc { + func_attrs: AttrSet, + func_export_key: Option<&'a ExportKey>, + + inst_attrs: AttrSet, + origin: IntraFuncUseOrigin, + }, +} + +enum IntraFuncUseOrigin { + CallCallee, + Other, +} + +impl UseOrigin<'_> { + fn to_rustc_span(&self, cx: &Context, span_regen: &mut SpanRegenerator<'_>) -> Option { + let mut from_attrs = |attrs: AttrSet| { + let attrs_def = &cx[attrs]; + attrs_def + .attrs + .iter() + .find_map(|attr| match attr { + &Attr::SpvDebugLine { + file_path, + line, + col, + } => span_regen.src_loc_to_rustc(SrcLocDecoration { + file_name: &cx[file_path.0], + line, + col, + }), + _ => None, + }) + .or_else(|| { + span_regen.src_loc_to_rustc( + try_decode_custom_decoration::>(attrs_def)?.decode(), + ) + }) + }; + match *self { + Self::Global { attrs, .. } => from_attrs(attrs), + Self::IntraFunc { + func_attrs, + inst_attrs, + .. + } => from_attrs(inst_attrs).or_else(|| from_attrs(func_attrs)), + } + } + + fn note( + &self, + cx: &Context, + span_regen: &mut SpanRegenerator<'_>, + err: &mut DiagnosticBuilder<'_, ErrorGuaranteed>, + ) { + let wk = &super::SpvSpecWithExtras::get().well_known; + + let name_from_attrs = |attrs: AttrSet, kind| { + cx[attrs] + .attrs + .iter() + .find_map(|attr| match attr { + Attr::SpvAnnotation(spv_inst) if spv_inst.opcode == wk.OpName => { + Some(decode_spv_lit_str_with(&spv_inst.imms, |name| { + format!("`{name}`") + })) + } + _ => None, + }) + .unwrap_or_else(|| format!("unnamed {kind}")) + }; + let note = match self { + &Self::Global { kind, attrs } => { + format!("used by {}", name_from_attrs(attrs, *kind)) + } + Self::IntraFunc { + func_attrs, + func_export_key, + inst_attrs: _, + origin, + } => { + let func_desc = func_export_key + .map(|export_key| match export_key { + &ExportKey::LinkName(name) => format!("function export `{}`", &cx[name]), + ExportKey::SpvEntryPoint { imms, .. } => match imms[..] { + [em @ spv::Imm::Short(em_kind, _), ref name_imms @ ..] => { + // FIXME(eddyb) use `assert_eq!` after updating to latest SPIR-T. + assert!(em_kind == wk.ExecutionModel); + let em = spv::print::operand_from_imms([em]).concat_to_plain_text(); + decode_spv_lit_str_with(name_imms, |name| { + format!( + "{} entry-point `{name}`", + em.strip_prefix("ExecutionModel.").unwrap() + ) + }) + } + _ => unreachable!(), + }, + }) + .unwrap_or_else(|| name_from_attrs(*func_attrs, "function")); + match origin { + IntraFuncUseOrigin::CallCallee => format!("called by {func_desc}"), + IntraFuncUseOrigin::Other => format!("used from within {func_desc}"), + } + } + }; + + let span = self.to_rustc_span(cx, span_regen).unwrap_or(DUMMY_SP); + err.span_note(span, note); + } +} + +impl DiagnosticReporter<'_> { + fn report_from_attrs(&mut self, attrs: AttrSet) { + if attrs == AttrSet::default() { + return; + } + + // Split off the last entry in `self.use_stack` if it's for the definition + // that `attrs` come from - this should almost always be the case, except + // for instructions inside a function body, or visitor bugs. + let (current_def, use_stack_for_def) = self + .use_stack + .split_last() + .filter( + |( + &UseOrigin::Global { + attrs: use_attrs, .. + } + | &UseOrigin::IntraFunc { + func_attrs: use_attrs, + .. + }, + _, + )| { use_attrs == attrs }, + ) + .map_or((None, &self.use_stack[..]), |(current, stack)| { + (Some(current), stack) + }); + + let attrs_def = &self.cx[attrs]; + if !self.linker_options.early_report_zombies { + if let Some(zombie) = try_decode_custom_decoration::>(attrs_def) { + let ZombieDecoration { reason } = zombie.decode(); + let def_span = current_def + .and_then(|def| def.to_rustc_span(self.cx, &mut self.span_regen)) + .unwrap_or(DUMMY_SP); + let mut err = self.sess.struct_span_err(def_span, reason); + for use_origin in use_stack_for_def.iter().rev() { + use_origin.note(self.cx, &mut self.span_regen, &mut err); + } + self.overall_result = Err(err.emit()); + } + } + } +} + +impl Visitor<'_> for DiagnosticReporter<'_> { + fn visit_attr_set_use(&mut self, attrs: AttrSet) { + // HACK(eddyb) this avoids reporting the same diagnostics more than once. + if self.seen_attrs.insert(attrs) { + self.report_from_attrs(attrs); + } + } + fn visit_type_use(&mut self, ty: Type) { + if self.seen_types.insert(ty) { + let ty_def = &self.cx[ty]; + self.use_stack.push(UseOrigin::Global { + kind: &"type", + attrs: ty_def.attrs, + }); + self.visit_type_def(ty_def); + self.use_stack.pop(); + } + } + fn visit_const_use(&mut self, ct: Const) { + if self.seen_consts.insert(ct) { + let ct_def = &self.cx[ct]; + self.use_stack.push(UseOrigin::Global { + kind: &"constant", + attrs: ct_def.attrs, + }); + self.visit_const_def(ct_def); + self.use_stack.pop(); + } + } + + fn visit_global_var_use(&mut self, gv: GlobalVar) { + if self.seen_global_vars.insert(gv) { + let gv_decl = &self.module.global_vars[gv]; + self.use_stack.push(UseOrigin::Global { + // FIXME(eddyb) may be a `&CONST`, or an interface variable, + // not necessarily an user variable, so this could be confusing. + kind: &"global variable", + attrs: gv_decl.attrs, + }); + self.visit_global_var_decl(gv_decl); + self.use_stack.pop(); + } + } + fn visit_func_use(&mut self, func: Func) { + if self.seen_funcs.insert(func) { + let func_decl = &self.module.funcs[func]; + self.use_stack.push(UseOrigin::IntraFunc { + func_attrs: func_decl.attrs, + func_export_key: None, + inst_attrs: AttrSet::default(), + origin: IntraFuncUseOrigin::Other, + }); + self.visit_func_decl(func_decl); + self.use_stack.pop(); + } + } + + fn visit_data_inst_def(&mut self, data_inst_def: &DataInstDef) { + match self.use_stack.last_mut() { + Some(UseOrigin::IntraFunc { inst_attrs, .. }) => *inst_attrs = data_inst_def.attrs, + _ => unreachable!(), + } + + if let DataInstKind::FuncCall(func) = data_inst_def.kind { + let replace_origin = |this: &mut Self, new_origin| match this.use_stack.last_mut() { + Some(UseOrigin::IntraFunc { origin, .. }) => mem::replace(origin, new_origin), + _ => unreachable!(), + }; + + // HACK(eddyb) visit `func` early, to control its `use_stack`, with + // the later visit from `inner_visit_with` ignored as a duplicate. + let old_origin = replace_origin(self, IntraFuncUseOrigin::CallCallee); + self.visit_func_use(func); + replace_origin(self, old_origin); + } + + data_inst_def.inner_visit_with(self); + } +} diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs index 335202c137..1772234e3b 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs @@ -1,5 +1,6 @@ //! SPIR-T pass infrastructure and supporting utilities. +pub(crate) mod diagnostics; mod fuse_selects; mod reduce; @@ -59,8 +60,17 @@ macro_rules! def_spv_spec_with_extra_well_known { } let spv_spec = spv::spec::Spec::get(); + let wk = &spv_spec.well_known; + + let decorations = match &spv_spec.operand_kinds[wk.Decoration] { + spv::spec::OperandKindDef::ValueEnum { variants } => variants, + _ => unreachable!(), + }; + let lookup_fns = PerWellKnownGroup { opcode: |name| spv_spec.instructions.lookup(name).unwrap(), + operand_kind: |name| spv_spec.operand_kinds.lookup(name).unwrap(), + decoration: |name| decorations.lookup(name).unwrap().into(), }; SpvSpecWithExtras { @@ -87,6 +97,12 @@ def_spv_spec_with_extra_well_known! { OpCompositeInsert, OpCompositeExtract, ], + operand_kind: spv::spec::OperandKind = [ + ExecutionModel, + ], + decoration: u32 = [ + UserTypeGOOGLE, + ], } /// Run intra-function passes on all `Func` definitions in the `Module`. @@ -112,7 +128,8 @@ pub(super) fn run_func_passes

( seen_global_vars: FxIndexSet::default(), seen_funcs: FxIndexSet::default(), }; - for &exportee in module.exports.values() { + for (export_key, &exportee) in &module.exports { + export_key.inner_visit_with(&mut collector); exportee.inner_visit_with(&mut collector); } collector.seen_funcs diff --git a/crates/rustc_codegen_spirv/src/linker/zombies.rs b/crates/rustc_codegen_spirv/src/linker/zombies.rs index 5c99d8dce1..1e54e0213e 100644 --- a/crates/rustc_codegen_spirv/src/linker/zombies.rs +++ b/crates/rustc_codegen_spirv/src/linker/zombies.rs @@ -308,7 +308,7 @@ impl<'a> ZombieReporter<'a> { } } -pub fn remove_zombies( +pub fn report_and_remove_zombies( sess: &Session, opts: &super::Options, module: &mut Module, diff --git a/docs/src/codegen-args.md b/docs/src/codegen-args.md index 704c733679..99c956e989 100644 --- a/docs/src/codegen-args.md +++ b/docs/src/codegen-args.md @@ -135,6 +135,12 @@ Disables compaction of SPIR-V IDs at the end of linking. Causes absolutely ginor emitted. Useful if you're println debugging IDs in the linker (although spirv-opt will compact them anyway, be careful). +### `--no-early-report-zombies` + +Delays reporting zombies (aka "deferred errors") even further, to allow more legalization. +Currently this also replaces the zombie reporting with a SPIR-T-based version +(which may become the default in the future). + ### `--no-structurize` Disables CFG structurization. Probably results in invalid modules. From 173d1be7565486ff5761c86783f64f6eea0c9d30 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 21 Apr 2023 03:15:44 +0300 Subject: [PATCH 12/13] linker/specializer: fix some latent bugs that were hidden by zombies. --- .../src/linker/specializer.rs | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/linker/specializer.rs b/crates/rustc_codegen_spirv/src/linker/specializer.rs index 50e0dcb52b..582daf1da9 100644 --- a/crates/rustc_codegen_spirv/src/linker/specializer.rs +++ b/crates/rustc_codegen_spirv/src/linker/specializer.rs @@ -576,24 +576,26 @@ impl Specializer { let mut forward_declared_pointers = FxHashSet::default(); for inst in types_global_values_and_functions { - let result_id = inst.result_id.unwrap_or_else(|| { - unreachable!( - "Op{:?} is in `types_global_values` but not have a result ID", - inst.class.opcode - ); - }); - - if inst.class.opcode == Op::TypeForwardPointer { + let result_id = if inst.class.opcode == Op::TypeForwardPointer { forward_declared_pointers.insert(inst.operands[0].unwrap_id_ref()); - } - if forward_declared_pointers.remove(&result_id) { - // HACK(eddyb) this is a forward-declared pointer, pretend - // it's not "generic" at all to avoid breaking the rest of - // the logic - see module-level docs for how this should be - // handled in the future to support recursive data types. - assert_eq!(inst.class.opcode, Op::TypePointer); - continue; - } + inst.operands[0].unwrap_id_ref() + } else { + let result_id = inst.result_id.unwrap_or_else(|| { + unreachable!( + "Op{:?} is in `types_global_values` but not have a result ID", + inst.class.opcode + ); + }); + if forward_declared_pointers.remove(&result_id) { + // HACK(eddyb) this is a forward-declared pointer, pretend + // it's not "generic" at all to avoid breaking the rest of + // the logic - see module-level docs for how this should be + // handled in the future to support recursive data types. + assert_eq!(inst.class.opcode, Op::TypePointer); + continue; + } + result_id + }; // Record all integer `OpConstant`s (used for `IndexComposite`). if inst.class.opcode == Op::Constant { @@ -1326,7 +1328,13 @@ impl<'a, S: Specialization> InferCx<'a, S> { } } TyPat::SampledImage(pat) => simple(Op::TypeSampledImage, pat), - TyPat::Struct(fields_pat) => self.match_ty_list_pat(fields_pat, ty_operands), + TyPat::Struct(fields_pat) => { + if generic.def.class.opcode == Op::TypeStruct { + self.match_ty_list_pat(fields_pat, ty_operands) + } else { + Err(Unapplicable) + } + } TyPat::Function(ret_pat, params_pat) => { let (ret_ty, params_ty_list) = ty_operands.split_first(self).unwrap(); Ok(self From 2e71d033abe54b2af1b40172d5032215da075665 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 21 Apr 2023 04:02:48 +0300 Subject: [PATCH 13/13] Update CHANGELOG. --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc37d7c34e..bd43801491 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [PR#1031](https://github.com/EmbarkStudios/rust-gpu/pull/1031) added `Components` generic parameter to `Image` type, allowing images to return lower dimensional vectors and even scalars from the sampling API ### Changed 🛠 +- [PR#1040](https://github.com/EmbarkStudios/rust-gpu/pull/1040) refactored "zombie" (delayed error) reporting to use SPIR-V `OpSource`, be more helpful, and added `--no-early-report-zombies` to delay it even further + (see also [the `--no-early-report-zombies` codegen args docs](docs/src/codegen-args.md#--no-early-report-zombies)) - [PR#1035](https://github.com/EmbarkStudios/rust-gpu/pull/1035) reduced the number of CGUs ("codegen units") used by `spirv-builder` to just `1` - [PR#1011](https://github.com/EmbarkStudios/rust-gpu/pull/1011) made `NonWritable` all read-only storage buffers (i.e. those typed `&T`, where `T` doesn't have interior mutability) - [PR#1029](https://github.com/EmbarkStudios/rust-gpu/pull/1029) fixed SampledImage::sample() fns being unnecessarily marked as unsafe