Avoid inst.body() duplicate call which reallocates AllocIds (#120)

dkcumming · web-flow · commit 9a7810914a33 · 2026-02-20T05:40:09.000Z
This PR corrects a bug in generating the Stable MIR JSON artefacts. Prior to this fix allocations were being collected twice (calling `Instance::body` twice), which resulted in certain allocs (slices) having multiple allocation ids. The fix is to refer to previously collected items so that the collector does not assign allocation ids again. ### Problem There has been a disconnection between `Ty` and `AllocId` in the `.smir.json` artefacts (for slices). A constant should be able to be traced to an allocation by the `Ty` that will yield an `AllocId`. However attempting to do this lead to a case where the `AllocId` from the `Ty` was not in the `Allocs` map, and allocations in the map had `AllocId`s unseen in the rest of the `.smir.json`. ### Cause The disconnection betwen the `Ty`s and `AllocId`s listed above came from there being a second collector pass calling `Instance::body` again that assigned new `AllocId`s for slices that were not connected to the `Ty`s (still referencing the originals). The code that initiates the second collector pass is from stable-mir-json, while the code that re-allocates fresh ids is in `rustc` itself. A rough trace of the pipeline goes like this: <details> 1. First collection initiated in `collect_smir` by [let items = collect_items(tcx);](https://github.com/runtimeverification/stable-mir-json/blob/06e5146e353259e13d3590798d7a03288c2bf7f2/src/printer.rs#L1377) 2. Each `MonoItem` is mapped to an `Item` by [mk_item(tcx, item.clone(), name)](https://github.com/runtimeverification/stable-mir-json/blob/06e5146e353259e13d3590798d7a03288c2bf7f2/src/printer.rs#L1125) 3. In `mk_item` if the `MonoItem` is `MonoItemFn(inst: Instance)` call `Instance::body(&self)` on `inst` by [inst.body()](https://github.com/runtimeverification/stable-mir-json/blob/06e5146e353259e13d3590798d7a03288c2bf7f2/src/printer.rs#L357) 4. [Instance::Body](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/stable_mir/src/mir/mono.rs#L52-L54) calls the `rustc_smir` [Context::instance_body](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/context.rs#L576-L582) (for `TablesWrapper` that `impl Context`) 5. `Context::instance_body` calls [BodyBuilder::new(tables.tcx, instance)](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/context.rs#L581) to create a new [BodyBuilder](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/builder.rs#L14-L18) which then calls the [BodyBuilder::build](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/builder.rs#L33-L54) method 6. `BodyBuilder::build` creates `mono_body` which is of type [rustc_middle::mir::Body](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_middle/src/mir/mod.rs#L248-L371) (_not_ `stable_mir` yet), which is then is converted to a [stable_mir::mir::Body](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/stable_mir/src/mir/body.rs#L13-L38) by [mono_body.stable(tables)](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/builder.rs#L52) 7. Converting to a `stable_mir::mir::Body` through [stable](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/convert/mir.rs#L16-L42) will convert all the `BasicBlocks`, `Statements`, `Terminators`, `LocalDecls`, and importantly [rustc_middle::mir::Const into a stable_mir::ty::{TyConst,MirConst}](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/convert/mir.rs#L732-L762) 8. If the `Const` to covert is a `rustc_middle::mir::Const::Val` then it is converted with a new [stable_mir::ty::Allocation](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/stable_mir/src/ty.rs#L1223-L1228) as argument to the [stable_mir::ty::ConstantKind::Allocated](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/stable_mir/src/ty.rs#L1301) by [ConstantKind::Allocated(alloc::new_allocation(ty, val, tables));](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/convert/mir.rs#L757) 9. The creation of the new `Allocation` will match the `ConstValue`, and if it is `ConstValue::Slice` then the `AllocId` produced will be from [let alloc_id = tables.tcx.reserve_and_set_memory_alloc(data);](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_smir/src/rustc_smir/alloc.rs#L62) which will call [TyCtxt::reserve_alloc_id](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_middle/src/mir/interpret/mod.rs#L499) and which will create [a _new_ allocation id](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_middle/src/mir/interpret/mod.rs#L436-L443) by calling [AllocMap::reserve](https://github.com/rust-lang/rust/blob/a2545fd6fc66b4323f555223a860c451885d1d2b/compiler/rustc_middle/src/mir/interpret/mod.rs#L424-L432) <- IMPORTANT 10. Given 9. explains how the `AllocId` was created from calling `inst.body()`, let's skip over the remaining details in executing `let items = collect_items(tcx);` to [collect_interned_values(tcx, items.iter().map(|i| &i.mono_item).collect::<Vec<_>>());](https://github.com/runtimeverification/stable-mir-json/blob/06e5146e353259e13d3590798d7a03288c2bf7f2/src/printer.rs#L1381) 11. `collect_interned_values` iterates through `items` and if it matches a `MonoItem::Fn(inst)` then [inst.body()](https://github.com/runtimeverification/stable-mir-json/blob/06e5146e353259e13d3590798d7a03288c2bf7f2/src/printer.rs#L967) is called again - PROBLEM. This repeats steps 4. - 9. again and at step 9. will allocate another `AllocId` for the `InternedValueCollector` that does not match what was originally allocated. </details> ### Solution Once the `Items` are collected we can just thread a slice through and use the `Body` that is already collected which means `inst.body()` does not need to be called again and will not reallocate ids.
diff --git a/src/printer.rs b/src/printer.rs
@@ -945,14 +945,14 @@ impl MirVisitor for InternedValueCollector<'_, '_> {
     }
 }
 
-fn collect_interned_values<'tcx>(tcx: TyCtxt<'tcx>, items: Vec<&MonoItem>) -> InternedValues<'tcx> {
+fn collect_interned_values<'tcx>(tcx: TyCtxt<'tcx>, items: &[Item]) -> InternedValues<'tcx> {
     let mut calls_map = HashMap::new();
     let mut visited_allocs = HashMap::new();
     let mut ty_visitor = TyCollector::new(tcx);
     let mut span_map = HashMap::new();
     if link_items_enabled() {
         for item in items.iter() {
-            if let MonoItem::Fn(inst) = item {
+            if let MonoItem::Fn(inst) = &item.mono_item {
                 update_link_map(
                     &mut calls_map,
                     fn_inst_sym(tcx, None, Some(inst)),
@@ -962,9 +962,12 @@ fn collect_interned_values<'tcx>(tcx: TyCtxt<'tcx>, items: Vec<&MonoItem>) -> In
         }
     }
     for item in items.iter() {
-        match &item {
+        match &item.mono_item {
             MonoItem::Fn(inst) => {
-                if let Some(body) = inst.body() {
+                if let MonoItemKind::MonoItemFn {
+                    body: Some(body), ..
+                } = &item.mono_item_kind
+                {
                     InternedValueCollector {
                         tcx,
                         _sym: inst.mangled_name(),
@@ -974,7 +977,7 @@ fn collect_interned_values<'tcx>(tcx: TyCtxt<'tcx>, items: Vec<&MonoItem>) -> In
                         ty_visitor: &mut ty_visitor,
                         spans: &mut span_map,
                     }
-                    .visit_body(&body)
+                    .visit_body(body)
                 } else {
                     eprintln!(
                         "Failed to retrive body for Instance of MonoItem::Fn {}",
@@ -1377,8 +1380,7 @@ pub fn collect_smir(tcx: TyCtxt<'_>) -> SmirJson {
     let items = collect_items(tcx);
     let items_clone = items.clone();
     let (unevaluated_consts, mut items) = collect_unevaluated_constant_items(tcx, items);
-    let (calls_map, visited_allocs, visited_tys, span_map) =
-        collect_interned_values(tcx, items.iter().map(|i| &i.mono_item).collect::<Vec<_>>());
+    let (calls_map, visited_allocs, visited_tys, span_map) = collect_interned_values(tcx, &items);
 
     // FIXME: We dump extra static items here --- this should be handled better
     for (_, alloc) in visited_allocs.iter() {
