This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Let TK_PreserveContents span across the whole base region.
ClosedPublic

Authored by NoQ on Apr 13 2016, 7:16 AM.

Details

Summary

Essentially, if s is a structure, and foo(const void *) is evaluated conservatively, then foo(&s) does not invalidate s, but foo(&(s.x)) invalidates the whole s, because the store only looks at traits of base regions (inside binding keys), and s.x is a field region.

This patch represents the idea that only whole base regions should carry the TK_PreserveContents trait. This also makes a bit of sense, because no matter what pointer arithmetic we do with a const pointer, it's still a const pointer. There's an extra complication with mutable fields in C++ classes, which i neither added nor fixed here.

In CallEvent.cpp below the changed code there's a FIXME comment, but i'm not sure what it means; if anybody thinks it means exactly what this patch is about, then i'd have to update it :)

What i don't like about the approach this patch implements, is that it makes the core rely on an implementation detail of RegionStoreManager ("only base regions are relevant" is such implementation detail). Instead, i also tried to add a few extra virtual methods into the StoreManager to avoid this problem, but it made the patch much heavier. I can post that, unless anybody else thinks that it's a natural thing (rather than implementation detail) to propagate this trait to base regions.

Instead, it should be possible to auto-replace the region with a base region inside setTrait() and hasTrait() methods.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ updated this revision to Diff 53551.Apr 13 2016, 7:16 AM
NoQ retitled this revision from to [analyzer] Let TK_PreserveContents span across the whole base region..
NoQ updated this object.
NoQ added reviewers: zaks.anna, dcoughlin.
NoQ added a subscriber: cfe-commits.
zaks.anna accepted this revision.Apr 23 2016, 4:35 PM
zaks.anna edited edge metadata.

LGTM!

One thing to be aware here is that a const pointer could be deleted, so we should be able to delete a parent object without a warning. (I think that should work with this patch since you don't change the trait, but you might want to add a test.)

What i don't like about the approach this patch implements, is that it makes the core rely on an
implementation detail of RegionStoreManager ("only base regions are relevant" is such implementation
detail). Instead, i also tried to add a few extra virtual methods into the StoreManager to avoid this
problem, but it made the patch much heavier. I can post that, unless anybody else thinks that it's a
natural thing (rather than implementation detail) to propagate this trait to base regions.

I'd commit this patch. If you want, feel free to follow up with another to remove the leaking of the implementation detail. I do not fully understand what the alternate solution is... I do not think it would make sense to automatically apply the TK_PreserveContents trait to the base region when one calls ETraits.setTrait(); not sure if you are suggesting that.

lib/StaticAnalyzer/Core/CallEvent.cpp
182 ↗(On Diff #53551)

Not sure, but this could mean to deal with cases where you pass a non const pointer to a class that has const fields..

This revision is now accepted and ready to land.Apr 23 2016, 4:35 PM
This revision was automatically updated to reflect the committed changes.