[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.
Artem Dergachev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 15 17:30:36 PDT 2019
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet, mgorny.
Herald added a project: clang.
D44934 <https://reviews.llvm.org/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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 5678 bytes
Desc: not available
More information about the cfe-commits