immunant / c2rust

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

analyze: support rewriting field projections on nullable pointers #1096

Closed spernsteiner closed 2 months ago

spernsteiner commented 4 months ago

This adds support for rewriting field projections like &(*p).x when p is a nullable pointer. The result looks like Some(&(*p.unwrap()).x).

I initially tried to avoid a panic when p is null by implementing a rewrite using Option::map: p.map(|ptr| &ptr.x). However, implementing this correctly wound up being quite complex. It's undefined behavior in C to do &p->x when p == NULL, so it seems reasonable to introduce a panic in that case.

The mir_op changes for this are relatively straightforward, but unlower, distribute, and convert needed some work. In particular, unlower now has a new variant MirOriginDesc::LoadFromTempForAdjustment(i), which disambiguates cases like this:

// Rust:
f(&(*p).x)

// MIR:
// Evaluate the main expression:
_tmp1 = &(*_p).x;
// unlower_map entries for &(*p).x:
// * Expr
// * StoreIntoLocal

// Adjustments inserted to coerce `&T` to `*const T`:
_tmp2 = addr_of!(*_tmp1);
// * LoadFromTempForAdjustment(0) (load of _tmp1)
// * Adjustment(0) (deref)
// * Adjustment(1) (addr-of)
// * StoreIntoLocal

// The actual function call:
_result = f(_tmp2);
// * LoadFromTemp (load final result of &(*p).x from _tmp2)

Previously, the LoadFromTempForAdjustment would be recorded as LoadFromTemp, meaning there would be two LoadFromTemp entries in the unlower_map for the expression &(*p).x. Rewrites attached to the first LoadFromTemp (in this case, the use of _tmp1 in the second statement) would be wrongly applied at the site of the last LoadFromTemp. This caused unwrap() and Some(_) rewrites to be applied in the wrong order, causing type errors in the rewritten code.