Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions crates/assembly-syntax/src/ast/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,15 @@ pub struct Module {
pub(crate) items: Vec<Export>,
/// 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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reflecting @bitwalker 's review on #3007 :

There are two issues with this:

  1. Programmatically-constructed modules can absolutely have a backing source file - it just means that the source language was not Miden Assembly (or that the AST was literally constructed by hand for a test, but that's not the important case).
  2. Source spans within a Module do not have to come from the same source file, and in fact that is the common case when compiling from Rust source code currently.

///
/// 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<Arc<SourceFile>>,
}

/// Constants
Expand All @@ -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<Path>) -> Self {
pub fn new(
kind: ModuleKind,
path: impl AsRef<Path>,
source_file: Option<Arc<SourceFile>>,
) -> Self {
let path = path.as_ref().to_absolute().into_owned();
Self {
span: Default::default(),
Expand All @@ -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
Expand Down Expand Up @@ -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<SourceFile>> {
self.source_file.as_ref()
}

/// Get an iterator over the constants defined in this module.
pub fn constants(&self) -> impl Iterator<Item = &Constant> + '_ {
self.items.iter().filter_map(|item| match item {
Expand Down
3 changes: 2 additions & 1 deletion crates/assembly-syntax/src/sema/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
11 changes: 7 additions & 4 deletions crates/assembly/src/assembler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source-manager check lands too late here.

compile_procedure() already uses self.source_manager for instruction spans and diagnostics before we reach register_procedure_debug_info(), so a mismatched module can still take the wrong source path first.

I reproduced that with a module parsed in one manager and assembled in another using loc_loadw_be.2: formatting the compile error panicked in SourceFile::location. We should reject the module before lowering starts.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the source-manager check to linking time so it covers this.

713e931

)?;

// Cache the compiled procedure
drop(proc);
Expand Down
89 changes: 56 additions & 33 deletions crates/assembly/src/assembler/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ use crate::{
TypeExpr,
types::{StructType, Type},
},
debuginfo::SourceManager,
diagnostics::Report,
debuginfo::{SourceFile, SourceManager},
diagnostics::{Report, report},
};

// DEBUG INFO SECTIONS
Expand Down Expand Up @@ -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::<str>::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::<str>::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(())
}

Expand Down
7 changes: 7 additions & 0 deletions crates/assembly/src/linker/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,13 @@ pub enum LinkerError {
prev_values: Vec<Felt>,
new_values: Vec<Felt>,
},
#[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<Path> },
#[error("undefined type alias")]
#[diagnostic()]
UndefinedType {
Expand Down
10 changes: 10 additions & 0 deletions crates/assembly/src/linker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,15 @@ impl Linker {
pub fn link_module(&mut self, module: &mut Module) -> Result<ModuleIndex, LinkerError> {
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() });
Expand Down Expand Up @@ -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);
Expand Down
22 changes: 21 additions & 1 deletion crates/assembly/src/linker/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -50,6 +50,12 @@ pub struct LinkModule {
///
/// This is only relevant for modules parsed from MASM sources.
advice_map: Option<AdviceMap>,
/// 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<Arc<SourceFile>>,
}

impl LinkModule {
Expand All @@ -69,6 +75,7 @@ impl LinkModule {
path,
symbols: Vec::default(),
advice_map: None,
source_file: None,
}
}

Expand All @@ -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<Arc<SourceFile>>) -> 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 {
Expand Down Expand Up @@ -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<SourceFile>> {
self.source_file.as_ref()
}

/// Get an iterator over the symbols in this module.
#[inline]
pub fn symbols(&self) -> core::slice::Iter<'_, Symbol> {
Expand Down
58 changes: 53 additions & 5 deletions crates/assembly/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 = {
Expand All @@ -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()
};
Expand All @@ -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
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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<dyn SourceManager> = 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<dyn SourceManager> = 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(())
}
Loading
Loading