martong added inline comments.
================
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:
> 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.
But @steakhal, we have this case in the test file with a `FIXME` that is not
aligned with your observation.
```
char const *const glob_ptr8 = "123";
void glob_ptr_index4() {
clang_analyzer_eval(glob_ptr8[0] == '1'); // expected-warning{{TRUE}}
clang_analyzer_eval(glob_ptr8[1] == '2'); // expected-warning{{TRUE}}
clang_analyzer_eval(glob_ptr8[2] == '3'); // expected-warning{{TRUE}}
clang_analyzer_eval(glob_ptr8[3] == '\0'); // expected-warning{{TRUE}}
// FIXME: Should be UNDEFINED.
// We should take into account a declaration in which the literal is used.
clang_analyzer_eval(glob_ptr8[4] == '\0'); // expected-warning{{TRUE}}
}
```
================
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.
----------------
martong wrote:
> steakhal wrote:
> > 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.
> But @steakhal, we have this case in the test file with a `FIXME` that is not
> aligned with your observation.
> ```
> char const *const glob_ptr8 = "123";
> void glob_ptr_index4() {
> clang_analyzer_eval(glob_ptr8[0] == '1'); // expected-warning{{TRUE}}
> clang_analyzer_eval(glob_ptr8[1] == '2'); // expected-warning{{TRUE}}
> clang_analyzer_eval(glob_ptr8[2] == '3'); // expected-warning{{TRUE}}
> clang_analyzer_eval(glob_ptr8[3] == '\0'); // expected-warning{{TRUE}}
> // FIXME: Should be UNDEFINED.
> // We should take into account a declaration in which the literal is used.
> clang_analyzer_eval(glob_ptr8[4] == '\0'); // expected-warning{{TRUE}}
> }
> ```
> > 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.
Would it be a FP introduced by this patch? Or we would report the "bug" in the
baseline too?
```
char const *const glob_ptr8 = "123";
int glob_ptr_index4(int x) {
return x/glob_ptr8[4]; // Div by zero
}
```
Actually, we should report "accessing garbage or undefined" instead of div zero.
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
