diff --git a/crates/assembly-syntax/src/ast/module.rs b/crates/assembly-syntax/src/ast/module.rs index a7fd99004b..a441af7b68 100644 --- a/crates/assembly-syntax/src/ast/module.rs +++ b/crates/assembly-syntax/src/ast/module.rs @@ -126,6 +126,15 @@ pub struct Module { pub(crate) items: Vec, /// AdviceMap that this module expects to be loaded in the host before executing. pub(crate) advice_map: AdviceMap, + /// The source file from which this module was parsed, if any. + /// + /// This is `None` for programmatically-constructed modules (e.g. `new_kernel()`, + /// `new_executable()`) that have no backing source text. + /// + /// When present, it is used to verify that the source manager used during assembly is the + /// same one that was used to parse this module, via pointer identity + /// ([`SourceManager::is_manager_of`]). + source_file: Option>, } /// Constants @@ -144,7 +153,11 @@ impl Module { impl Module { /// Creates a new [Module] with the specified `kind` and fully-qualified path, e.g. /// `std::math::u64`. - pub fn new(kind: ModuleKind, path: impl AsRef) -> Self { + pub fn new( + kind: ModuleKind, + path: impl AsRef, + source_file: Option>, + ) -> Self { let path = path.as_ref().to_absolute().into_owned(); Self { span: Default::default(), @@ -153,17 +166,18 @@ impl Module { kind, items: Default::default(), advice_map: Default::default(), + source_file, } } /// An alias for creating the default, but empty, `#kernel` [Module]. pub fn new_kernel() -> Self { - Self::new(ModuleKind::Kernel, Path::kernel_path()) + Self::new(ModuleKind::Kernel, Path::kernel_path(), None) } /// An alias for creating the default, but empty, `$exec` [Module]. pub fn new_executable() -> Self { - Self::new(ModuleKind::Executable, Path::exec_path()) + Self::new(ModuleKind::Executable, Path::exec_path(), None) } /// Specifies the source span in the source file in which this module was defined, that covers @@ -440,6 +454,13 @@ impl Module { &self.advice_map } + /// Returns the source file from which this module was parsed, if any. + /// + /// Returns `None` for programmatically-constructed modules that have no backing source text. + pub fn source_file(&self) -> Option<&Arc> { + self.source_file.as_ref() + } + /// Get an iterator over the constants defined in this module. pub fn constants(&self) -> impl Iterator + '_ { self.items.iter().filter_map(|item| match item { diff --git a/crates/assembly-syntax/src/sema/mod.rs b/crates/assembly-syntax/src/sema/mod.rs index 832a88c82d..56355de1c8 100644 --- a/crates/assembly-syntax/src/sema/mod.rs +++ b/crates/assembly-syntax/src/sema/mod.rs @@ -44,7 +44,8 @@ pub fn analyze( let mut analyzer = AnalysisContext::new(source.clone(), source_manager); analyzer.set_warnings_as_errors(warnings_as_errors); - let mut module = Box::new(Module::new(kind, path).with_span(source.source_span())); + let mut module = + Box::new(Module::new(kind, path, Some(source.clone())).with_span(source.source_span())); let mut forms = VecDeque::from(forms); let mut enums = SmallVec::<[EnumType; 1]>::new_const(); diff --git a/crates/assembly/src/assembler.rs b/crates/assembly/src/assembler.rs index 9bb4e7b5ee..8e6c656bd5 100644 --- a/crates/assembly/src/assembler.rs +++ b/crates/assembly/src/assembler.rs @@ -920,9 +920,9 @@ impl Assembler { continue; } // Fetch procedure metadata from the graph - let (module_kind, module_path) = { + let (module_kind, module_path, module_source_file) = { let module = &self.linker[procedure_gid.module]; - (module.kind(), module.path().clone()) + (module.kind(), module.path().clone(), module.source_file().cloned()) }; match self.linker[procedure_gid].item() { SymbolItem::Procedure(proc) => { @@ -954,8 +954,11 @@ impl Assembler { // be added to the forest. // Record the debug info for this procedure - self.debug_info - .register_procedure_debug_info(&procedure, self.source_manager.as_ref())?; + self.debug_info.register_procedure_debug_info( + &procedure, + self.source_manager.as_ref(), + module_source_file.as_deref(), + )?; // Cache the compiled procedure drop(proc); diff --git a/crates/assembly/src/assembler/debuginfo.rs b/crates/assembly/src/assembler/debuginfo.rs index 5f64e1b52e..1295c906f2 100644 --- a/crates/assembly/src/assembler/debuginfo.rs +++ b/crates/assembly/src/assembler/debuginfo.rs @@ -18,8 +18,8 @@ use crate::{ TypeExpr, types::{StructType, Type}, }, - debuginfo::SourceManager, - diagnostics::Report, + debuginfo::{SourceFile, SourceManager}, + diagnostics::{Report, report}, }; // DEBUG INFO SECTIONS @@ -50,40 +50,63 @@ impl DebugInfoSections { &mut self, procedure: &Procedure, source_manager: &dyn SourceManager, + module_source_file: Option<&SourceFile>, ) -> Result<(), Report> { - if let Ok(file_line_col) = source_manager.file_line_col(*procedure.span()) { - let path_id = - self.debug_sources_section.add_string(Arc::from(file_line_col.uri.path())); - let file_id = self - .debug_sources_section - .add_file(miden_mast_package::debug_info::DebugFileInfo::new(path_id)); - let name = Arc::::from(procedure.path().as_str()); - let name_id = self.debug_functions_section.add_string(name.clone()); - let type_index = if let Some(signature) = procedure.signature() { - Some(register_debug_type( - &mut self.debug_types_section, - Some(name), - None, - &Type::Function(signature), - )?) - } else { - None - }; - let func_info = miden_mast_package::debug_info::DebugFunctionInfo::new( - name_id, - file_id, - file_line_col.line, - file_line_col.column, - ) - .with_mast_root(procedure.mast_root()); - let func_info = if let Some(type_index) = type_index { - func_info.with_type(type_index) - } else { - func_info - }; - self.debug_functions_section.add_function(func_info); + let span = *procedure.span(); + + // If no source file is available, skip debug info registration + // (the module is synthetic or was deserialized without source info). + let Some(source_file) = module_source_file else { + return Ok(()); + }; + + // Verify the source file belongs to this source manager. + if !source_manager.is_manager_of(source_file) { + return Err(report!( + "source manager mismatch for procedure '{}': the module's source file \ + is not owned by the assembler's source manager", + procedure.path(), + )); } + let file_line_col = + source_manager.file_line_col(span).map_err(|err| { + report!( + "failed to resolve source location for procedure '{}': {err}", + procedure.path(), + ) + })?; + + let path_id = self.debug_sources_section.add_string(Arc::from(file_line_col.uri.path())); + let file_id = self + .debug_sources_section + .add_file(miden_mast_package::debug_info::DebugFileInfo::new(path_id)); + let name = Arc::::from(procedure.path().as_str()); + let name_id = self.debug_functions_section.add_string(name.clone()); + let type_index = if let Some(signature) = procedure.signature() { + Some(register_debug_type( + &mut self.debug_types_section, + Some(name), + None, + &Type::Function(signature), + )?) + } else { + None + }; + let func_info = miden_mast_package::debug_info::DebugFunctionInfo::new( + name_id, + file_id, + file_line_col.line, + file_line_col.column, + ) + .with_mast_root(procedure.mast_root()); + let func_info = if let Some(type_index) = type_index { + func_info.with_type(type_index) + } else { + func_info + }; + self.debug_functions_section.add_function(func_info); + Ok(()) } diff --git a/crates/assembly/src/linker/errors.rs b/crates/assembly/src/linker/errors.rs index c87d87a75c..3c7b600423 100644 --- a/crates/assembly/src/linker/errors.rs +++ b/crates/assembly/src/linker/errors.rs @@ -91,6 +91,13 @@ pub enum LinkerError { prev_values: Vec, new_values: Vec, }, + #[error( + "source manager mismatch: module '{path}' was parsed with a different source manager than the one used by the assembler" + )] + #[diagnostic(help( + "ensure the module is parsed with the same source manager that is passed to the assembler" + ))] + SourceManagerMismatch { path: Arc }, #[error("undefined type alias")] #[diagnostic()] UndefinedType { diff --git a/crates/assembly/src/linker/mod.rs b/crates/assembly/src/linker/mod.rs index 8962ebe95c..e8d44b860b 100644 --- a/crates/assembly/src/linker/mod.rs +++ b/crates/assembly/src/linker/mod.rs @@ -277,6 +277,15 @@ impl Linker { pub fn link_module(&mut self, module: &mut Module) -> Result { log::debug!(target: "linker", "adding unprocessed module {}", module.path()); + // Reject the module early if it was parsed with a different source manager. + // Compiling a mismatched module would use the wrong source manager for span + // resolution, which can produce garbled diagnostics or panics. + if let Some(source_file) = module.source_file() + && !self.source_manager.is_manager_of(source_file) + { + return Err(LinkerError::SourceManagerMismatch { path: module.path().into() }); + } + let is_duplicate = self.find_module_index(module.path()).is_some(); if is_duplicate { return Err(LinkerError::DuplicateModule { path: module.path().into() }); @@ -316,6 +325,7 @@ impl Linker { module.path().into(), ) .with_advice_map(module.advice_map().clone()) + .with_source_file(module.source_file().cloned()) .with_symbols(symbols); self.modules.push(link_module); diff --git a/crates/assembly/src/linker/module.rs b/crates/assembly/src/linker/module.rs index 75842b89c3..045fb99271 100644 --- a/crates/assembly/src/linker/module.rs +++ b/crates/assembly/src/linker/module.rs @@ -7,7 +7,7 @@ use miden_assembly_syntax::{ self, AliasTarget, ItemIndex, LocalSymbol, LocalSymbolResolver, ModuleIndex, ModuleKind, SymbolResolution, SymbolResolutionError, SymbolTable, }, - debuginfo::{SourceManager, Span, Spanned}, + debuginfo::{SourceFile, SourceManager, Span, Spanned}, }; use super::{AdviceMap, LinkStatus, Symbol, SymbolItem, SymbolResolver}; @@ -50,6 +50,12 @@ pub struct LinkModule { /// /// This is only relevant for modules parsed from MASM sources. advice_map: Option, + /// The source file from which this module was parsed (if available). + /// + /// Used to verify source manager identity via [`SourceManager::is_manager_of`] during + /// assembly, preventing silent attachment of wrong debug info when a module was parsed + /// with a different source manager than the assembler's. + source_file: Option>, } impl LinkModule { @@ -69,6 +75,7 @@ impl LinkModule { path, symbols: Vec::default(), advice_map: None, + source_file: None, } } @@ -91,6 +98,13 @@ impl LinkModule { self } + /// Specify the source file from which this module was parsed. + #[inline] + pub fn with_source_file(mut self, source_file: Option>) -> Self { + self.source_file = source_file; + self + } + /// Get the unique identifier/index of this module in the containing linker. #[inline(always)] pub fn id(&self) -> ModuleIndex { @@ -157,6 +171,12 @@ impl LinkModule { self.advice_map.as_ref() } + /// Get the source file from which this module was parsed, if available. + #[inline] + pub fn source_file(&self) -> Option<&Arc> { + self.source_file.as_ref() + } + /// Get an iterator over the symbols in this module. #[inline] pub fn symbols(&self) -> core::slice::Iter<'_, Symbol> { diff --git a/crates/assembly/src/tests.rs b/crates/assembly/src/tests.rs index 2c443bf035..7563f2dee2 100644 --- a/crates/assembly/src/tests.rs +++ b/crates/assembly/src/tests.rs @@ -10,7 +10,11 @@ use std::{ }; use miden_assembly_syntax::{ - MAX_REPEAT_COUNT, ast::Path, diagnostics::WrapErr, library::LibraryExport, + MAX_REPEAT_COUNT, + ast::{Block, Instruction, Op, Path, Procedure, Visibility}, + debuginfo::{DefaultSourceManager, SourceManager, Span}, + diagnostics::WrapErr, + library::LibraryExport, }; use miden_core::{ Felt, Word, assert_matches, @@ -4115,7 +4119,7 @@ fn vendoring() -> TestResult { let mod1 = mod_parser .parse(PathBuf::new("test::mod1").unwrap(), source, context.source_manager()) .unwrap(); - Assembler::default().assemble_library([mod1]).unwrap() + Assembler::new(context.source_manager()).assemble_library([mod1]).unwrap() }; let lib = { @@ -4124,7 +4128,7 @@ fn vendoring() -> TestResult { .parse(PathBuf::new("test::mod2").unwrap(), source, context.source_manager()) .unwrap(); - let mut assembler = Assembler::default(); + let mut assembler = Assembler::new(context.source_manager()); assembler.link_static_library(vendor_lib)?; assembler.assemble_library([mod2]).unwrap() }; @@ -4140,7 +4144,7 @@ fn vendoring() -> TestResult { let expected_lib = { let source = source_file!(&context, "pub proc foo push.1 end"); let mod2 = mod_parser.parse("test::expected", source, context.source_manager()).unwrap(); - Assembler::default().assemble_library([mod2]).unwrap() + Assembler::new(context.source_manager()).assemble_library([mod2]).unwrap() }; // 3. Verify that the expected library (which has push.1) has AssemblyOps @@ -5097,7 +5101,7 @@ fn test_assembler_debug_info_present() { let module = parse_module!(&context, "test::foo", source); // Test: With debug mode always enabled (issue #1821), debug info should always be present - let assembler = Assembler::default(); + let assembler = Assembler::new(context.source_manager()); let library = assembler.assemble_library([module]).unwrap(); let mast_forest = library.mast_forest(); @@ -5789,3 +5793,47 @@ fn test_linking_recursive_expansion_via_renamed_aliases() -> TestResult { Ok(()) } + +#[test] +fn mismatched_source_manager_caught_before_lowering() { + // Source manager A (assembler's): pre-load a short file so it occupies SourceId(0). + let sm_a: Arc = Arc::new(DefaultSourceManager::default()); + let _dummy = Module::parser(ModuleKind::Library) + .parse_str("lib::dummy", "pub proc dummy\n nop\nend", sm_a.clone()) + .unwrap(); + + let assembler = Assembler::new(sm_a); + + // Source manager B (external): parse a module whose code will trigger a compile error + // during lowering. `loc_loadw_be.2` fails because 2 is not word-aligned (must be a + // multiple of 4). The error formatting tries to resolve the instruction's source span + // via the assembler's source manager, which returns the wrong file. + let sm_b: Arc = Arc::new(DefaultSourceManager::default()); + let module = Module::parser(ModuleKind::Library) + .parse_str("lib::external", "@locals(4) pub proc bar\n loc_loadw_be.2\nend", sm_b) + .unwrap(); + + let result = assembler.assemble_library([module]); + let err = result.expect_err("should fail with source manager mismatch"); + let msg = err.to_string(); + assert!(msg.contains("source manager mismatch"), "unexpected error: {msg}"); +} + +#[test] +fn programmatic_module_without_source_file_assembles_ok() -> TestResult { + let context = TestContext::default(); + + let mut module = Module::new(ModuleKind::Library, "lib::programmatic", None); + let nop_body = Block::new(Default::default(), vec![Op::Inst(Span::unknown(Instruction::Nop))]); + let procedure = Procedure::new( + Default::default(), + Visibility::Public, + "nop_proc".parse().unwrap(), + 0, + nop_body, + ); + module.define_procedure(procedure, context.source_manager()).unwrap(); + + context.assemble_library([alloc::boxed::Box::new(module)])?; + Ok(()) +} diff --git a/crates/debug-types/src/source_file.rs b/crates/debug-types/src/source_file.rs index c8b36a1261..809bf543a3 100644 --- a/crates/debug-types/src/source_file.rs +++ b/crates/debug-types/src/source_file.rs @@ -192,12 +192,15 @@ impl SourceFile { } /// Get a [FileLineCol] equivalent to the start of the given [SourceSpan] - pub fn location(&self, span: SourceSpan) -> FileLineCol { - assert_eq!(span.source_id(), self.id, "mismatched source ids"); + /// + /// Returns `None` if the span's source id does not match this file, or if the + /// starting byte offset is out of bounds. + pub fn location(&self, span: SourceSpan) -> Option { + if span.source_id() != self.id { + return None; + } - self.content - .location(ByteIndex(span.into_range().start)) - .expect("invalid source span: starting byte is out of bounds") + self.content.location(ByteIndex(span.into_range().start)) } } @@ -355,7 +358,7 @@ impl<'a> miette::SpanContents<'a> for ScopedSourceFileRef<'a> { let start = self.span.offset() as u32; let end = start + self.span.len() as u32; let span = SourceSpan::new(self.file.id(), start..end); - let loc = self.file.location(span); + let loc = self.file.location(span).expect("span was constructed from this file"); loc.column.to_index().to_usize() } diff --git a/crates/debug-types/src/source_manager.rs b/crates/debug-types/src/source_manager.rs index a798e52ac6..b65da264d1 100644 --- a/crates/debug-types/src/source_manager.rs +++ b/crates/debug-types/src/source_manager.rs @@ -368,7 +368,7 @@ impl DefaultSourceManagerImpl { self.files .get(span.source_id()) .ok_or(SourceManagerError::InvalidSourceId) - .map(|file| file.location(span)) + .and_then(|file| file.location(span).ok_or(SourceManagerError::InvalidBounds)) } fn location_to_span(&self, loc: Location) -> Option {