Skip to content

Commit

Permalink
fix: prevent no_predicates from removing predicates in calling func…
Browse files Browse the repository at this point in the history
…tion (#5452)

# Description

## Problem\*

Resolves #5435 

## Summary\*

This PR caches the current `side_effects_enabled` value on the
`PerFunctionContext` and restores it after inlining another function.
This ensures that any `EnableSideEffects` instructions from the inlined
function do not leak out into the calling function.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
TomAFrench authored Jul 9, 2024
1 parent fa9b444 commit 66244b6
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 0 deletions.
18 changes: 18 additions & 0 deletions compiler/noirc_evaluator/src/ssa/opt/inlining.rs
Original file line number Diff line number Diff line change
Expand Up @@ -475,19 +475,37 @@ impl<'function> PerFunctionContext<'function> {
/// Inline each instruction in the given block into the function being inlined into.
/// This may recurse if it finds another function to inline if a call instruction is within this block.
fn inline_block_instructions(&mut self, ssa: &Ssa, block_id: BasicBlockId) {
let mut side_effects_enabled: Option<ValueId> = None;

let block = &self.source_function.dfg[block_id];
for id in block.instructions() {
match &self.source_function.dfg[*id] {
Instruction::Call { func, arguments } => match self.get_function(*func) {
Some(func_id) => {
if self.should_inline_call(ssa, func_id) {
self.inline_function(ssa, *id, func_id, arguments);

// This is only relevant during handling functions with `InlineType::NoPredicates` as these
// can pollute the function they're being inlined into with `Instruction::EnabledSideEffects`,
// resulting in predicates not being applied properly.
//
// Note that this doesn't cover the case in which there exists an `Instruction::EnabledSideEffects`
// within the function being inlined whilst the source function has not encountered one yet.
// In practice this isn't an issue as the last `Instruction::EnabledSideEffects` in the
// function being inlined will be to turn off predicates rather than to create one.
if let Some(condition) = side_effects_enabled {
self.context.builder.insert_enable_side_effects_if(condition);
}
} else {
self.push_instruction(*id);
}
}
None => self.push_instruction(*id),
},
Instruction::EnableSideEffects { condition } => {
side_effects_enabled = Some(self.translate_value(*condition));
self.push_instruction(*id);
}
_ => self.push_instruction(*id),
}
}
Expand Down
7 changes: 7 additions & 0 deletions test_programs/execution_success/regression_5435/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_5435"
type = "bin"
authors = [""]
compiler_version = ">=0.31.0"

[dependencies]
2 changes: 2 additions & 0 deletions test_programs/execution_success/regression_5435/Prover.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
input = "0"
enable = false
18 changes: 18 additions & 0 deletions test_programs/execution_success/regression_5435/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
fn main(input: Field, enable: bool) {
if enable {
let hash = no_predicate_function(input);
// `EnableSideEffects` instruction from above instruction leaks out and removes the predicate from this call,
// resulting in execution failure.
fail(hash);
}
}

#[no_predicates]
fn no_predicate_function(enable: Field) -> Field {
// if-statement ensures that an `EnableSideEffects` instruction is emitted.
if enable == 0 { 1 } else { 0 }
}

unconstrained fn fail(_: Field) {
assert(false);
}

0 comments on commit 66244b6

Please sign in to comment.