Page MenuHomePhabricator

[analyzer] Do not conjure a symbol for return value of a conservatively evaluated function
AbandonedPublic

Authored by NoQ on Nov 29 2016, 4:46 AM.

Details

Summary

Instead, return a nonloc::LazyCompoundVal of a temporary region for the call expression, with a trivial store in which the temporary region is invalidated.

Returning a conjured record-type symbol is painful because the return value of the function may suddenly change during materialization of a temporary - it will be hotfixed into the same lazy compound value anyway in some cases, eg. in createTemporaryRegionIfNeeded when handling a MaterializeTemporaryExpr. After this patch, we should be receiving the same temporary region during materialization, instead of constructing a new region, and materialization of a conjured structure starts doing a lot less things.

I'm including a change in the yet-to-land iterator checker (D25660), which should or should not be committed depending on the order of patches landing. This change demonstrates how the problem explained above causes checkers to hack around.

In fact, there are other places in which we conjure structure-type symbols, eg. some cases of array invalidations. They aren't known to cause problems though.

Diff Detail

Event Timeline

NoQ updated this revision to Diff 79541.Nov 29 2016, 4:46 AM
NoQ retitled this revision from to [analyzer] Do not conjure a symbol for return value of a conservatively evaluated function.
NoQ updated this object.
NoQ added subscribers: cfe-commits, baloghadamsoftware.
NoQ updated this object.Nov 29 2016, 4:49 AM

Could you please remove the IteratorPastEndChecker file differences from this patch and make D25660 dependent on this one?

zaks.anna edited edge metadata.Dec 9 2016, 11:12 PM

Could you please remove the IteratorPastEndChecker file differences from this patch and make D25660
dependent on this one?

Since this has not been reviewed yet while the IteratorPastEndChecker has been, I do not think we should block the checker on this.

Any progress regarding this patch? Is D26837 necessarily a dependency, or we just need isCompoundType() function? This function could be moved to a separate patch so the dependency could be removed.

From what I recall, it is not clear that this patch is the step in the right direction. At least, it need more investigation.

NoQ planned changes to this revision.Jan 18 2017, 12:03 AM

Yep, because there are a lot more places in which the value of the expression suddenly changes, i think i'd want to either fix all of them, or (if i find some of those really reasonable) make a callback for checkers to subscribe to value replacements (which i'd prefer to treat as a trivial but definite step into alpha-renaming support).

NoQ abandoned this revision.Jul 11 2018, 6:10 PM

Outdated by D44131.