-
Notifications
You must be signed in to change notification settings - Fork 198
Fix_temp_segment_chain_bug #2195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: starkware-development
Are you sure you want to change the base?
Fix_temp_segment_chain_bug #2195
Conversation
|
Benchmark Results for unmodified programs 🚀
|
f8ad17b
to
602c809
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2195 +/- ##
=========================================================
+ Coverage 96.63% 96.65% +0.02%
=========================================================
Files 103 103
Lines 43867 44180 +313
=========================================================
+ Hits 42391 42704 +313
Misses 1476 1476 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Will add tests next week for codecov |
Hi, @YairVaknin-starkware! It would be great if we have a description of this PR just to keep track of the development in the base branch more easily. |
sure, done. PTAL. |
602c809
to
f65d023
Compare
f65d023
to
d23f7a6
Compare
Done. |
Also, please note that this is a quick and simple fix, since we assume this table won't grow too much (and each separate chain won't be long). I can also impl it in a way that we won't traverse intermediate chain entries once we already set the value for the key that we started the chain on, but as I said, this doesn't seem worth it, and would need to record the visited entries in each chain. |
Description looks good. Also I like the detailed tests.
|
d23f7a6
to
8ab7764
Compare
added. PTAL @FrancoGiachetta @gabrielbosio @Yael-Starkware.
It's a bug that could occur in any cairo0 code, but I only know of a (future) use-case that's needed for Stwo's backend (so aligned with the changes in starkware-development currently). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @FrancoGiachetta and @YairVaknin-starkware)
vm/src/vm/vm_memory/memory.rs
line 290 at r2 (raw file):
} #[cfg(not(feature = "extensive_hints"))] fn flatten_relocation_rules(&mut self) -> Result<(), MemoryError> {
this function and the next one has a lot of common logic, I'd make them one and separate with the cfg decorator only to the minimal extent.
Code quote:
fn flatten_relocation_rules(&mut self) -> Result<(), MemoryError> {
vm/src/vm/vm_memory/memory.rs
line 330 at r2 (raw file):
loop { match dst { MaybeRelocatable::RelocatableValue(r) if r.segment_index < 0 => {
Suggestion:
relocatable
vm/src/vm/vm_memory/memory.rs
line 344 at r2 (raw file):
match next { MaybeRelocatable::RelocatableValue(nr) => {
Suggestion:
next_relocatable
vm/src/vm/vm_memory/memory.rs
line 381 at r2 (raw file):
for segment in self.data.iter_mut().chain(self.temp_data.iter_mut()) { for cell in segment.iter_mut() { let value = cell.get_value();
how does a value from the segment turn into a relocatable?
Code quote:
let value = cell.get_value();
vm/src/vm/vm_memory/memory.rs
line 387 at r2 (raw file):
addr, &self.relocation_rules, )?);
isn't that duplicate of what happens in flatten_relocation_rules?
Code quote:
Some(MaybeRelocatable::RelocatableValue(addr)) if addr.segment_index < 0 => {
let mut new_cell = MemoryCell::new(Memory::relocate_address(
addr,
&self.relocation_rules,
)?);
TITLE
[BUGFIX] Fix_temp_segment_chain_bug
Description
Fixes relocation chaining: when a temp segment pointed to another temp segment (multi temp segment hop), we weren’t resolving it all the way to its final destination.
We now flatten relocation rules so each temp maps directly to a concrete address (or int under
extensive_hints
).Includes a cycle guard and rejects non-zero offsets when chains end at an int.
Factor out relocation-rule flattening into flatten_relocation_rules() with cfg variants:
Keep shared relocation flow in relocate_memory(); only preprocessing differs.
Added unit tests:
Without
extensive_hints
flatten_relocation_rules_chain_happy
— temp→temp→real chain flattens correctly (offsets composed).flatten_relocation_rules_cycle_err
— detects cycle and returnsMemoryError::Relocation
.flatten_relocation_rules_missing_next_err
— dangling chain (no rule for next temp) returnsMemoryError::UnallocatedSegment((next_key, temp_len))
.With
extensive_hints
flatten_relocation_rules_chain_happy_extensive_reloc_and_int
— mixed chain:flatten_relocation_rules_int_with_non_zero_offset_err
— multi-hop ending inInt
with non-zero offset returnsMemoryError::NonZeroOffset
.flatten_relocation_rules_cycle_err_extensive
— detects cycle and returnsMemoryError::Relocation
.flatten_relocation_rules_missing_next_err_extensive
— dangling chain (no rule for next temp) returnsMemoryError::UnallocatedSegment((next_key, temp_len))
.Integration (
relocate_memory
)relocate_memory_temp_chain_to_reloc_multi_hop
— exercises a multi-hop temp→temp→real chain end-to-end: references into temp memory are updated to real addresses, offsets are composed correctly, and the temp segment data is moved into the target real segment with consistency checks.relocate_memory_temp_chain_to_int_multi_hop
(withextensive_hints
) — verifies the “collapse to Int” semantics: a chain like temp→temp→…→Int(99)
causes all references to that temp to becomeInt(99)
, and the involved temp segments are dropped (their raw cells are not copied) by design.Checklist