search

immunant / c2rust

Migrate C code to Rust
https://c2rust.com/
Other
3.91k stars 229 forks source link

analyze: introduce FlagSet::FIXED #920

Closed spernsteiner closed 1 year ago

spernsteiner commented 1 year ago

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.

aneksteind commented 1 year ago

could we add some filecheck tests for this?

kkysen commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I've reformulated the rules for applying FIXED: a63fca4a0fa285e443806c02d96e6182e98fdd9c. 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.

spernsteiner commented 1 year ago

Rebased onto master

spernsteiner commented 1 year ago

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

spernsteiner commented 1 year ago

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.