Skip to content
Merged
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changelog

## 0.20.2 (TBD)
- Fix issue where decorator access was not bypassed properly in release mode ([#2529](https://github.com/0xMiden/miden-vm/pull/2529)).

## 0.20.1 (2025-12-14)

- Fix issue where calling procedures from statically linked libraries did not import their decorators ([#2459](https://github.com/0xMiden/miden-vm/pull/2459)).
Expand Down
32 changes: 28 additions & 4 deletions core/src/mast/debuginfo/decorator_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,36 @@ impl OpToDecoratorIds {
/// - Last value in `node_indptr_for_op_idx` must be <= `op_indptr_for_dec_ids.len() - 1`
/// - Both `op_indptr_for_dec_ids` and `node_indptr_for_op_idx` must be strictly monotonic (each
/// successive value must be >= the previous one)
#[cfg(test)]
pub fn from_components(
pub(crate) fn from_components(
decorator_ids: Vec<DecoratorId>,
op_indptr_for_dec_ids: Vec<usize>,
node_indptr_for_op_idx: IndexVec<MastNodeId, usize>,
) -> Result<Self, DecoratorIndexError> {
// Empty structures are valid (no nodes, no decorators)
if decorator_ids.is_empty()
&& op_indptr_for_dec_ids.is_empty()
&& node_indptr_for_op_idx.is_empty()
{
return Ok(Self {
decorator_ids,
op_indptr_for_dec_ids,
node_indptr_for_op_idx,
});
}

// Nodes but no decorators: empty decorator_ids/op_indptr, node_indptr with all zeros
if decorator_ids.is_empty() && op_indptr_for_dec_ids.is_empty() {
if node_indptr_for_op_idx.iter().all(|&ptr| ptr == 0) {
return Ok(Self {
decorator_ids,
op_indptr_for_dec_ids,
node_indptr_for_op_idx,
});
} else {
return Err(DecoratorIndexError::InternalStructure);
}
}

// Validate the structure
if op_indptr_for_dec_ids.is_empty() {
return Err(DecoratorIndexError::InternalStructure);
Expand Down Expand Up @@ -729,9 +753,9 @@ mod tests {

#[test]
fn test_from_components_invalid_structure() {
// Test with empty operation pointers
// Empty structures are now valid
let result = OpToDecoratorIds::from_components(vec![], vec![], IndexVec::new());
assert_eq!(result, Err(DecoratorIndexError::InternalStructure));
assert!(result.is_ok());

// Test with operation pointer exceeding decorator indices
let result = OpToDecoratorIds::from_components(
Expand Down
19 changes: 19 additions & 0 deletions core/src/mast/debuginfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,25 @@ impl DebugInfo {
}
}

/// Creates an empty [DebugInfo] with valid CSR structures for N nodes.
pub fn empty_for_nodes(num_nodes: usize) -> Self {
let mut node_indptr_for_op_idx = IndexVec::new();
for _ in 0..=num_nodes {
let _ = node_indptr_for_op_idx.push(0);
}
Comment on lines +114 to +117
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we initialize the vector with zeros directly, rather than pushing one by one?

Copy link
Contributor Author

@huitseeker huitseeker Jan 5, 2026

Choose a reason for hiding this comment

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

Not an API that's available on IndexVec. Should be fixed, just not on a PR to main. #2532


let op_decorator_storage =
OpToDecoratorIds::from_components(Vec::new(), Vec::new(), node_indptr_for_op_idx)
.expect("Empty CSR structure should be valid");

Self {
decorators: IndexVec::new(),
op_decorator_storage,
node_decorator_storage: NodeToDecoratorIds::new(),
error_codes: BTreeMap::new(),
}
}

// PUBLIC ACCESSORS
// --------------------------------------------------------------------------------------------

Expand Down
5 changes: 2 additions & 3 deletions core/src/mast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl MastForest {
///
/// This method modifies the forest in-place, removing all decorator information
/// including operation-indexed decorators, before-enter decorators, after-exit
/// decorators, and error codes.
/// decorators, and error codes while keeping the CSR structure valid.
///
/// # Examples
///
Expand All @@ -151,8 +151,7 @@ impl MastForest {
/// forest.strip_decorators(); // forest is now stripped
/// ```
pub fn strip_decorators(&mut self) {
// Clear all debug info (decorators and error codes)
self.debug_info.clear();
self.debug_info = DebugInfo::empty_for_nodes(self.nodes.len());
}

/// Compacts the forest by merging duplicate nodes.
Expand Down
69 changes: 45 additions & 24 deletions core/src/mast/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use crate::{
DebugOptions, Decorator, Felt, Kernel, Operation, ProgramInfo, Word,
chiplets::hasher,
mast::{
BasicBlockNodeBuilder, DynNode, DynNodeBuilder, MastForest, MastForestContributor,
MastNodeExt,
BasicBlockNodeBuilder, DynNode, DynNodeBuilder, JoinNodeBuilder, MastForest,
MastForestContributor, MastNodeExt, SplitNodeBuilder,
},
utils::{Deserializable, Serializable},
};
Expand Down Expand Up @@ -242,40 +242,61 @@ fn test_decorator_storage_consistency_with_multiple_blocks() {
fn test_decorator_storage_after_strip_decorators() {
let mut forest = MastForest::new();

// Create decorators
let deco1 = forest.add_decorator(Decorator::Trace(1)).unwrap();
let deco2 = forest.add_decorator(Decorator::Trace(2)).unwrap();

// Create operations and decorators
let operations = vec![Operation::Push(Felt::new(1)), Operation::Add];
let decorators = vec![(0, deco1), (1, deco2)];

// Add block to forest
let block_id = BasicBlockNodeBuilder::new(operations, decorators)
let block_id = BasicBlockNodeBuilder::new(operations, vec![(0, deco1), (1, deco2)])
.add_to_forest(&mut forest)
.unwrap();

// Verify decorators exist initially
assert!(!forest.debug_info.op_decorator_storage().is_empty());
assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 1);
assert_eq!(forest.debug_info.num_decorators(), 2);
assert_eq!(forest.debug_info.op_decorator_storage().num_decorator_ids(), 2);

// Strip decorators
forest.strip_decorators();

// Verify decorators are cleared from storage
assert!(forest.debug_info.op_decorator_storage().is_empty());
assert_eq!(forest.debug_info.num_decorators(), 0);
assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 1);
assert!(forest.decorator_links_for_node(block_id).unwrap().into_iter().next().is_none());
}

#[test]
fn test_strip_decorators_edge_cases() {
// Empty forest
let mut forest = MastForest::new();
forest.strip_decorators();
assert_eq!(forest.debug_info.num_decorators(), 0);
assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 0);
assert_eq!(forest.debug_info.op_decorator_storage().num_decorator_ids(), 0);

// Verify block also has no decorators after stripping
let block = if let crate::mast::MastNode::Block(block) = &forest[block_id] {
block
} else {
panic!("Expected a block node");
};
let block_decorators: Vec<_> = block.indexed_decorator_iter(&forest).collect();
assert_eq!(block_decorators, []);
// Idempotent: stripping twice should be safe
let operations = vec![Operation::Push(Felt::new(1)), Operation::Add];
let block_id = BasicBlockNodeBuilder::new(operations, vec![])
.add_to_forest(&mut forest)
.unwrap();
forest.strip_decorators();
forest.strip_decorators();
assert_eq!(forest.debug_info.num_decorators(), 0);
assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 1);
assert!(forest.decorator_links_for_node(block_id).unwrap().into_iter().next().is_none());
}

#[test]
fn test_strip_decorators_multiple_node_types() {
let mut forest = MastForest::new();
let deco = forest.add_decorator(Decorator::Trace(1)).unwrap();
let block_id = BasicBlockNodeBuilder::new(
vec![Operation::Push(Felt::new(1)), Operation::Add],
vec![(0, deco)],
)
.add_to_forest(&mut forest)
.unwrap();

JoinNodeBuilder::new([block_id, block_id]).add_to_forest(&mut forest).unwrap();
SplitNodeBuilder::new([block_id, block_id]).add_to_forest(&mut forest).unwrap();

forest.strip_decorators();

assert_eq!(forest.debug_info.op_decorator_storage().num_nodes(), 3);
assert!(forest.decorator_links_for_node(block_id).unwrap().into_iter().next().is_none());
}

#[test]
Expand Down
12 changes: 10 additions & 2 deletions crates/test-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,11 @@ impl Test {
let (program, mut host) = self.get_program_and_host();
let stack_inputs: Vec<Felt> = self.stack_inputs.clone().into_iter().rev().collect();
let advice_inputs = self.advice_inputs.clone();
let fast_process = FastProcessor::new_with_advice_inputs(&stack_inputs, advice_inputs);
let fast_process = if self.in_debug_mode {
FastProcessor::new_debug(&stack_inputs, advice_inputs)
} else {
FastProcessor::new_with_advice_inputs(&stack_inputs, advice_inputs)
};
let fast_stack_outputs = fast_process.execute_sync(&program, &mut host).unwrap();

assert_eq!(
Expand All @@ -593,7 +597,11 @@ impl Test {

let stack_inputs: Vec<Felt> = self.stack_inputs.clone().into_iter().rev().collect();
let advice_inputs: AdviceInputs = self.advice_inputs.clone();
let fast_process = FastProcessor::new_with_advice_inputs(&stack_inputs, advice_inputs);
let fast_process = if self.in_debug_mode {
FastProcessor::new_debug(&stack_inputs, advice_inputs)
} else {
FastProcessor::new_with_advice_inputs(&stack_inputs, advice_inputs)
};
let fast_result = fast_process.execute_sync(&program, &mut host);

match (fast_result, slow_result) {
Expand Down
32 changes: 24 additions & 8 deletions processor/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -520,11 +520,17 @@ pub enum AceError {
#[cfg(not(feature = "no_err_ctx"))]
#[macro_export]
macro_rules! err_ctx {
($mast_forest:expr, $node:expr, $host:expr) => {
$crate::errors::ErrorContextImpl::new($mast_forest, $node, $host)
($mast_forest:expr, $node:expr, $host:expr, $in_debug_mode:expr) => {
$crate::errors::ErrorContextImpl::new($mast_forest, $node, $host, $in_debug_mode)
};
($mast_forest:expr, $node:expr, $host:expr, $op_idx:expr) => {
$crate::errors::ErrorContextImpl::new_with_op_idx($mast_forest, $node, $host, $op_idx)
($mast_forest:expr, $node:expr, $host:expr, $in_debug_mode:expr, $op_idx:expr) => {
$crate::errors::ErrorContextImpl::new_with_op_idx(
$mast_forest,
$node,
$host,
$in_debug_mode,
$op_idx,
)
};
}

Expand All @@ -541,8 +547,8 @@ macro_rules! err_ctx {
#[cfg(feature = "no_err_ctx")]
#[macro_export]
macro_rules! err_ctx {
($mast_forest:expr, $node:expr, $host:expr) => {{ () }};
($mast_forest:expr, $node:expr, $host:expr, $op_idx:expr) => {{ () }};
($mast_forest:expr, $node:expr, $host:expr, $in_debug_mode:expr) => {{ () }};
($mast_forest:expr, $node:expr, $host:expr, $in_debug_mode:expr, $op_idx:expr) => {{ () }};
}

/// Trait defining the interface for error context providers.
Expand All @@ -567,21 +573,23 @@ impl ErrorContextImpl {
mast_forest: &MastForest,
node: &impl MastNodeErrorContext,
host: &impl BaseHost,
in_debug_mode: bool,
) -> Self {
let (label, source_file) =
Self::precalc_label_and_source_file(None, mast_forest, node, host);
Self::precalc_label_and_source_file(None, mast_forest, node, host, in_debug_mode);
Self { label, source_file }
}

pub fn new_with_op_idx(
mast_forest: &MastForest,
node: &impl MastNodeErrorContext,
host: &impl BaseHost,
in_debug_mode: bool,
op_idx: usize,
) -> Self {
let op_idx = op_idx.into();
let (label, source_file) =
Self::precalc_label_and_source_file(op_idx, mast_forest, node, host);
Self::precalc_label_and_source_file(op_idx, mast_forest, node, host, in_debug_mode);
Self { label, source_file }
}

Expand All @@ -590,7 +598,15 @@ impl ErrorContextImpl {
mast_forest: &MastForest,
node: &impl MastNodeErrorContext,
host: &impl BaseHost,
in_debug_mode: bool,
) -> (SourceSpan, Option<Arc<SourceFile>>) {
// When not in debug mode, skip the expensive decorator traversal entirely.
// Decorators (including AsmOp decorators used for error context) should only
// be accessed when debugging is enabled.
if !in_debug_mode {
return (SourceSpan::UNKNOWN, None);
}

node.get_assembly_op(mast_forest, op_idx)
.and_then(|assembly_op| assembly_op.location())
.map_or_else(
Expand Down
37 changes: 24 additions & 13 deletions processor/src/fast/basic_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,18 @@ impl FastProcessor {
// Corresponds to the row inserted for the END operation added to the trace.
self.increment_clk(tracer);

// execute any decorators which have not been executed during span ops execution; this can
// happen for decorators appearing after all operations in a block. these decorators are
// Execute any decorators which have not been executed during span ops execution; this can
// happen for decorators appearing after all operations in a block. These decorators are
// executed after BASIC BLOCK is closed to make sure the VM clock cycle advances beyond the
// last clock cycle of the BASIC BLOCK ops.
// For the linked case, check for decorators at an operation index beyond the last operation
let num_ops = basic_block_node.num_operations() as usize;
for decorator in current_forest.decorators_for_op(node_id, num_ops) {
self.execute_decorator(decorator, host)?;
if self.in_debug_mode {
#[cfg(test)]
self.record_decorator_retrieval();

let num_ops = basic_block_node.num_operations() as usize;
for decorator in current_forest.decorators_for_op(node_id, num_ops) {
self.execute_decorator(decorator, host)?;
}
}

self.execute_after_exit_decorators(node_id, current_forest, host)
Expand All @@ -138,16 +142,22 @@ impl FastProcessor {
let mut group_idx = 0;
let mut next_group_idx = 1;

// Get the node ID once since it doesn't change within the loop
let node_id = basic_block
.linked_id()
.expect("basic block node should be linked when executing operations");
Comment on lines +145 to +148
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but getting node ID from the node in this way feels a bit backwards to me. When we apply these changes to next, we could probably just pass in node_id instead of back_block into this function (and in general, we should think how to simplify the number of parameters passed into this function). This will require refactoring how we build error context - but it should be relatively easy (e.g., instead of node.get_assembly_op(mast_forest, target_op_idx) we should be able to do something like mast.get_assembly_op(node_id, target_op_idx).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, but a couple of nuances:

  • obviously, this is just moving a local variable definition out of a hot loop, not changing the definition (in a PR to main),
  • there's upcoming work on removing err_ctx as part of Investigate removing ErrorContext #1978 that may make this obsolete.


// execute operations in the batch one by one
for (op_idx_in_batch, op) in batch.ops().iter().enumerate() {
let op_idx_in_block = batch_offset_in_block + op_idx_in_batch;

// Use the forest's decorator storage to get decorators for this operation
let node_id = basic_block
.linked_id()
.expect("basic block node should be linked when executing operations");
for decorator in current_forest.decorators_for_op(node_id, op_idx_in_block) {
self.execute_decorator(decorator, host)?;
if self.in_debug_mode {
#[cfg(test)]
self.record_decorator_retrieval();

for decorator in current_forest.decorators_for_op(node_id, op_idx_in_block) {
self.execute_decorator(decorator, host)?;
}
}

// if in trace mode, check if we need to record a trace state before executing the
Expand All @@ -165,7 +175,8 @@ impl FastProcessor {
// whereas all the other operations are synchronous (resulting in a significant
// performance improvement).
{
let err_ctx = err_ctx!(program, basic_block, host, op_idx_in_block);
let err_ctx =
err_ctx!(program, basic_block, host, self.in_debug_mode, op_idx_in_block);
match op {
Operation::Emit => self.op_emit(host, &err_ctx).await?,
_ => {
Expand Down
Loading