[PATCH] D46823: [analyzer] const init: handle non-explicit cases more accurately

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Looks good!

Do you have commit access? I think you should get commit access.



================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1650
+
+            // If there is a list, but no init, it must be zero.
+            if (i >= InitList->getNumInits())
----------------
r.stahl wrote:
> r.stahl wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > Would this work correctly if the element is not of an integral or enumeration type? I think this needs an explicit check.
> > > What if we have an out-of-bounds access to a variable-length array? I don't think it'd yield zero.
> > I'm getting "variable-sized object may not be initialized", so this case should not be possible.
> I'm having a hard time reproducing this either.
> 
> 
> ```
> struct S {
>   int a = 3;
> };
> S const sarr[2] = {};
> void definit() {
>   int i = 1;
>   clang_analyzer_dump(sarr[i].a); // expected-warning{{test}}
> }
> ```
> 
> results in a symbolic value, because makeZeroVal returns an empty SVal list for arrays, records, vectors and complex types. Otherwise it just returns UnknownVal (for example for not-yet-implemented floats).
> 
> Can you think of a case where this would be an issue?
Yup, sounds reasonable.


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1650-1652
+            // If there is a list, but no init, it must be zero.
+            if (i >= InitList->getNumInits())
+              return svalBuilder.makeZeroVal(R->getElementType());
----------------
NoQ wrote:
> r.stahl wrote:
> > r.stahl wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > Would this work correctly if the element is not of an integral or enumeration type? I think this needs an explicit check.
> > > > What if we have an out-of-bounds access to a variable-length array? I don't think it'd yield zero.
> > > I'm getting "variable-sized object may not be initialized", so this case should not be possible.
> > I'm having a hard time reproducing this either.
> > 
> > 
> > ```
> > struct S {
> >   int a = 3;
> > };
> > S const sarr[2] = {};
> > void definit() {
> >   int i = 1;
> >   clang_analyzer_dump(sarr[i].a); // expected-warning{{test}}
> > }
> > ```
> > 
> > results in a symbolic value, because makeZeroVal returns an empty SVal list for arrays, records, vectors and complex types. Otherwise it just returns UnknownVal (for example for not-yet-implemented floats).
> > 
> > Can you think of a case where this would be an issue?
> Yup, sounds reasonable.
Had a look. This indeed looks fine, but for a completely different reason. In fact structs don't appear in `getBindingForElement()` because they all go through `getBindingForStruct()` instead. Hopefully this does take care of other cornercases.

So your test case doesn't even trigger your code to be executed, neither would `S s = sarr[i]; clang_analyzer_dump(s.a);`.

Still, as far as i understand, `sarr` here should be zero-initialized, so i think the symbolic value can be improved upon, so we might as well add this as a FIXME test.


https://reviews.llvm.org/D46823