[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I'm still worried about regressions.
Please split the patch into two by separating the tests into an NFC patch, on which you would apply the behavioral change.
That way it would be clear what and why changed. It would also help us to see what previously had defects you plan to preserve and annotate with FIXMEs.



================
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:
> 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.
I probably still miss the point. https://godbolt.org/z/EMhbq3745
Since the case you mention is actually represented by a `NonParamVarRegion` which holds a pointer to its decl.
```lang=C++
const char str3[43] = "123";
void top() {
  clang_analyzer_dump(&str3[41]); // &Element{str3,41 S64b,char}
  //                    NonParamVarRegion  ---^^^^ (it can print the name of the *decl*)
}
```

---

What I wanted to highlight is, that it's a non-issue. In your example you had a **non-const** global variable, thus we could not infer any meaningful initial value for it, and that is actually the expected behavior. As soon as I marked the `str` pointer //const// (along with its //pointee//), suddenly the analyzer can infer its initial value.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107339/new/

https://reviews.llvm.org/D107339