analyze: introduce FlagSet::FIXED #920

spernsteiner · 2023-05-09T23:28:54Z

Adds a new flag, FIXED, which indicates that the pointer type must not be changed during rewriting. This branch only adds the flag and sets it on some PointerIds where it's appropriate; nothing consumes this new flag (yet).

For now, we set the FIXED flag on all TyKind::Refs in the input program. This will eventually allow us to avoid rewriting code that's already safe. However, we don't set the FIXED flag for MIR temporaries introduced to hold the results of &x or &mut x expressions. This is because c2rust-transpile still uses &mut x as *mut T instead of addr_of_mut!(x) for the C address-of operator, and we sometimes need to rewrite these expressions in a way that changes their types. This also applies to cases like array.as_mut_ptr(), which implicitly works like (&mut array).as_mut_ptr().

Based on #919 for now; once this is approved, I'll rebase onto master.

c2rust-analyze/src/context.rs

c2rust-analyze/src/main.rs

aneksteind · 2023-05-10T16:38:22Z

c2rust-analyze/src/main.rs

+                _ => None,
+            };
+            if let Some(ref_pl) = ref_pl {
+                // For simplicity, we consider taking the address of a local to be a write.  We


why is this considered a write? what does that imply downstream?

Again, just being conservative. What we're looking for here is the code that's generated by &x/&mut x expressions in the source, which consists of a temporary _1 that's initialized with a statement _1 = &mut ...; and which otherwise isn't written in any way.

We might need to do something slightly smarter here in the future, if we eventually need to handle &mut &mut x.

I think some or all of your response is worth adding as a comment

aneksteind · 2023-05-10T16:40:15Z

could we add some filecheck tests for this?

c2rust-analyze/src/context.rs

c2rust-analyze/src/main.rs

kkysen · 2023-05-10T21:12:02Z

Would the temporary refs analysis still be necessary if the transpiler only used addr_of! and addr_of_mut! in these cases? This is also a bug in the transpiler (#301), and we've been meaning to unconditionally use addr_of{,_mut}!s for such temporaries.

spernsteiner · 2023-05-10T21:26:02Z

Would the temporary refs analysis still be necessary if the transpiler only used addr_of! and addr_of_mut! in these cases?

I think the analysis wouldn't be needed in that case, but I didn't want to block these changes on that transpiler bug being fixed.

kkysen · 2023-05-10T21:27:23Z

Would the temporary refs analysis still be necessary if the transpiler only used addr_of! and addr_of_mut! in these cases?

I think the analysis wouldn't be needed in that case, but I didn't want to block these changes on that transpiler bug being fixed.

Yeah, that makes sense. It's good to know, though, that fixing that bug would simplify these kinds of analyses as well as eliminate UB.

spernsteiner · 2023-05-10T23:54:15Z

I've reformulated the rules for applying FIXED: a63fca4. This should produce the same results as the previous version, but it should be easier to reason about and more consistent in its handling of complex cases.

Tomorrow I'll add a filecheck test, and then I think it will be ready to merge.

…l refs

Co-authored-by: Khyber Sen <[email protected]>

spernsteiner · 2023-05-11T16:27:36Z

Rebased onto master

…c order

spernsteiner · 2023-05-11T18:04:59Z

Added a test case (and fixed a small bug it uncovered in the new calculation of NOT_TEMPORARY_REF)

aneksteind · 2023-05-11T18:53:22Z

c2rust-analyze/src/main.rs

+
+    for local in mir.local_decls.indices() {
+        let is_temp_ref = mir.local_kind(local) == LocalKind::Temp
+            && write_count.get(&local).copied().unwrap_or(0) == 1


am i reading this right that you're only looking for cases where an address of a local is taken and stored into a temp, where the rhs is not a reborrow?

No, we don't care what Place the temporary reference points to. _1 = &_2, _1 = &_2.0, and _1 = &*_2 are all treated the same for our purposes. We only care whether the LHS variable looks like it's a temporary storing the result of a source-level &... expression.

aneksteind · 2023-05-11T18:54:50Z

c2rust-analyze/src/main.rs

@@ -547,6 +609,7 @@ fn run(tcx: TyCtxt) {
        assert!(acx.local_tys.is_empty());
        acx.local_tys = IndexVec::with_capacity(mir.local_decls.len());
        for (local, decl) in mir.local_decls.iter_enumerated() {
+            // TODO: set PointerInfo::ANNOTATED for the parts of the type with user annotations


can this even be gleaned from MIR? I thought type annotations were only visible at the HIR level

I think they're in Body::user_type_annotations (though I haven't looked into it in detail)

c2rust-analyze/src/main.rs

aneksteind

wasn't this going to fix rewriting bugs removing mut from &mut A in fields.rs in the function it has? Why is that no longer the case?

spernsteiner · 2023-05-11T19:56:33Z

wasn't this going to fix rewriting bugs removing mut from &mut A in fields.rs in the function it has?

This PR only adds code for computing the FIXED flag. The next PR in this series, #923, updates the rewriter to respect FIXED, and that one does fix the &mut A -> &A issue in fields.rs.

aneksteind self-requested a review May 10, 2023 02:45