ASDenysPetrov added a comment.
I'm gonna add docs to `getSValFromStringLiteral` and update the patch. And of
course will fix your nits.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760
+ // FIXME: Nevertheless, we can't do the same for cases, like:
+ // const char *str = "123"; // literal length is 4
+ // char c = str[41]; // offset is 41
+ // It should be properly handled before reaching this point.
----------------
steakhal wrote:
> martong wrote:
> > ASDenysPetrov wrote:
> > > ASDenysPetrov wrote:
> > > > martong wrote:
> > > > > Thanks for updating the patch! However, this `FIXME` makes me
> > > > > worried. Do you think you could pass the `Decl` itself to
> > > > > `getSValFromInitListExpr` in order to be able to check whether the
> > > > > type is a pointer or an array?
> > > > This worried me as well. Currently I can't find a way to get the `Decl`
> > > > for `SL`.
> > > >
> > > We can load this as is for now with mentioning the known issue.
> > This might cause some itchy false positives. Perhaps, we could address this
> > in a follow-up patch and then commit them together?
> > Currently I can't find a way to get the Decl for SL.
> Why do you need a Decl? The SVal's gotta be an `Element{"123",41 S64b,char}`
> for the example in the comment (*).
>
> (*) With a minor modification: https://godbolt.org/z/7zhGMnf7P
> ```lang=C++
> template <class T> void clang_analyzer_dump(T);
> const char * const str = "123";
> const char * str2 = "123";
> void top() {
> clang_analyzer_dump(&str[41]); // &Element{"123",41 S64b,char}
> clang_analyzer_dump(&str2[41]); // &Element{SymRegion{reg_$0<const char *
> str2>},41 S64b,char}
> }
> ```
Because only `Decl` can tell us whether it is a `const char str[42]` or `const
char * const str`. We can't say anything just looking at `SVal`.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760
+ // FIXME: Nevertheless, we can't do the same for cases, like:
+ // const char *str = "123"; // literal length is 4
+ // char c = str[41]; // offset is 41
+ // It should be properly handled before reaching this point.
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > martong wrote:
> > > ASDenysPetrov wrote:
> > > > ASDenysPetrov wrote:
> > > > > martong wrote:
> > > > > > Thanks for updating the patch! However, this `FIXME` makes me
> > > > > > worried. Do you think you could pass the `Decl` itself to
> > > > > > `getSValFromInitListExpr` in order to be able to check whether the
> > > > > > type is a pointer or an array?
> > > > > This worried me as well. Currently I can't find a way to get the
> > > > > `Decl` for `SL`.
> > > > >
> > > > We can load this as is for now with mentioning the known issue.
> > > This might cause some itchy false positives. Perhaps, we could address
> > > this in a follow-up patch and then commit them together?
> > > Currently I can't find a way to get the Decl for SL.
> > Why do you need a Decl? The SVal's gotta be an `Element{"123",41
> > S64b,char}` for the example in the comment (*).
> >
> > (*) With a minor modification: https://godbolt.org/z/7zhGMnf7P
> > ```lang=C++
> > template <class T> void clang_analyzer_dump(T);
> > const char * const str = "123";
> > const char * str2 = "123";
> > void top() {
> > clang_analyzer_dump(&str[41]); // &Element{"123",41 S64b,char}
> > clang_analyzer_dump(&str2[41]); // &Element{SymRegion{reg_$0<const char *
> > str2>},41 S64b,char}
> > }
> > ```
> Because only `Decl` can tell us whether it is a `const char str[42]` or
> `const char * const str`. We can't say anything just looking at `SVal`.
> This might cause some itchy false positives. Perhaps, we could address this
> in a follow-up patch and then commit them together?
This will produce not more FP as before, even less. I didn't change the
behavior specifically here. I just found the issue point to it.
================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1784-1786
+ const auto Offset = static_cast<uint64_t>(Idx.getExtValue());
+ if (Idx < 0)
return UndefinedVal();
----------------
steakhal wrote:
> I think it would be better to have the check before we calculate the
> `Offset`. At least that way I find it easier to follow.
Yes, it's just my omission.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107339/new/
https://reviews.llvm.org/D107339
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
