Page MenuHomePhabricator

[analyzer] RegionStore: Enable loading default bindings from variables.
ClosedPublic

Authored by NoQ on Apr 15 2019, 5:30 PM.

Details

Summary

D44934 added modeling for memset() that, for the case of setting memory to 0, boils down to default-binding a concrete 0 to the memory region.

Surprisingly, in some cases RegionStore fails to load default bindings from a region. That is, it assumes that the region either has a direct binding (i.e., initialized via assignment), or it is uninitialized. In particular, this applies to local variables regions of integer/pointer/float types.

It seems that a lot of code (eg., invalidateRegions()) carefully works around this problem by adding either a direct binding or a default binding, depending on what RegionStore expects. This essentially duplicates RegionStore's already-convoluted logic on this subject on the caller side.

Our options are:

  1. Duplicate this logic again in CStringChecker.
  2. Let RegionStore automatically do the direct binding for such plain integers.
  3. Add support for loading default bindings from variables.

Option 1 is a failure to improve checker API in an aspect in which it's already very bad, so i didn't go for it. In options 2 and 3 the difference is entirely within RegionStore. I believe that option 2 is slightly superior because it produces a more "normalized" data structure and, additionally, because stores are more rare than loads (unless they are "dead stores"^^), so it should be slightly more performant. But it is also harder to implement, as it requires gathering the logic of when exactly do we need a direct binding in one place, so i went for option 3 in order to address the regression and simplify that logic. I did not yet try to see how other callers of bindDefault***() can be simplified.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Apr 15 2019, 5:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2019, 5:30 PM
NoQ updated this revision to Diff 195279.Apr 15 2019, 5:32 PM

Fix test file name in the top comment.

NoQ updated this revision to Diff 195280.Apr 15 2019, 5:36 PM

Whoops, copy-paste error.

So, like, the reason why i added a unittest was that i wanted to test separation of concerns: that it's specifically our Store that works correctly, not only the system as a whole.

dcoughlin accepted this revision.Apr 15 2019, 5:53 PM

LGTM.

clang/unittests/StaticAnalyzer/StoreTest.cpp
20 ↗(On Diff #195280)

It is awesome to see unit tests for this!

This revision is now accepted and ready to land.Apr 15 2019, 5:53 PM
a_sidorin accepted this revision.Apr 16 2019, 2:51 AM

I like the test even more than the change itself!

clang/unittests/StaticAnalyzer/StoreTest.cpp
49 ↗(On Diff #195280)

This can fit one line.

82 ↗(On Diff #195280)

Do we need this dtor declaration?

93 ↗(On Diff #195280)

= default? (Or it seems like we can remove it at all)

NoQ marked an inline comment as done.Apr 16 2019, 6:06 PM
NoQ added inline comments.
clang/unittests/StaticAnalyzer/StoreTest.cpp
49 ↗(On Diff #195280)

Just wanted to make line breaks similar in all tests, for the sake of OCD-friendliness.

NoQ updated this revision to Diff 195500.Apr 16 2019, 6:10 PM

Fxd constructors and destructors.

This revision was automatically updated to reflect the committed changes.