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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
