fix: map databus after unrolling a loop iteration #11399
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 4dedee8 | Previous: c695737 | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
sha512-100-bytes |
0.085 s |
0.056 s |
1.52 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
|
||
| // Since `self`, and its `inserter`, are moved into this method, | ||
| // it's the last chance we have to update the data bus values. | ||
| self.inserter.map_data_bus_in_place(); |
There was a problem hiding this comment.
It's not entirely clear to me when we call map_data_bus_in_place in general. Sometimes it is at the end of an entire pass, other times it's seemingly more granular, like in flatten_single_conditional.
In any case, maybe we could add some protection against this being forgotten by adding a flag to the FunctionInserter which is (re)set to false in map_values and set to true in map_data_bus_in_place, and then implement Drop for FunctionInserter where we can panic if we haven't called it when it goes out of scope, or just all it there if need be.
There was a problem hiding this comment.
On a different note, for values in the databus, which are either call arguments or return values, wouldn't it be surprising if any of them changed during the unrolling of a single loop iteration? I thought we don't return from the middle of loops, so for example the return value would refer to something in the last block, which must get the data through a reference or a block parameter.
I could be wrong, but it would be nice to see an example where this is a problem. OTOH we now clone the entire databus in each loop iteration.
There was a problem hiding this comment.
Right... I worked on this mainly because it was a security advisory, but there was no actual bug linked to this. Maybe we can close this until we find a bug.
There was a problem hiding this comment.
Another thing that seems intuitive to me since that if we have to call map_terminator_in_place then we should call map_data_bus_in_place as well, and vice versa.
There was a problem hiding this comment.
Ah no, that's not right, only if the terminator is a return one.
Description
Problem
Resolves https://github.com/noir-lang/noir/security/advisories/GHSA-4462-q25g-3fmc
Summary
Additional Context
User Documentation
Check one:
PR Checklist
cargo fmton default settings.