Skip to content
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

analyze: support rewriting field projections on nullable pointers #1096

Merged
merged 12 commits into from
Jul 22, 2024

Conversation

spernsteiner
Copy link
Collaborator

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.

@spernsteiner spernsteiner force-pushed the analyze-rewrite-non-null branch 2 times, most recently from 8aa58bf to 73652be Compare June 25, 2024 19:10
@spernsteiner spernsteiner changed the base branch from analyze-rewrite-non-null to analyze-rewrite-non-null-base June 25, 2024 19:12
@spernsteiner spernsteiner force-pushed the analyze-rewrite-nullable-projections branch from 7a068d3 to eb78836 Compare June 26, 2024 23:42
@spernsteiner spernsteiner force-pushed the analyze-rewrite-non-null-base branch from ff20263 to e526781 Compare June 26, 2024 23:42
@spernsteiner spernsteiner changed the base branch from analyze-rewrite-non-null-base to master June 26, 2024 23:42
@spernsteiner spernsteiner force-pushed the analyze-rewrite-nullable-projections branch from eb78836 to 75fc5ac Compare June 26, 2024 23:50
@spernsteiner spernsteiner requested a review from ahomescu June 26, 2024 23:58
let mut ptrs = Vec::new();
let ty_str = context::print_ty_with_pointer_labels(acx.local_tys[local], |ptr| {
let ty_str = context::print_ty_with_pointer_labels(lty, |ptr| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny coincidence: this is where the analyzer was crashing before #1105

@spernsteiner spernsteiner force-pushed the analyze-rewrite-nullable-projections branch from 75fc5ac to 8434b34 Compare July 22, 2024 18:13
@spernsteiner spernsteiner merged commit 7bf22e0 into master Jul 22, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants