[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D104285#3044103 , @NoQ wrote:

> Hey, I brought you some regressions!
>
>> ! In D104285#3044804 , @martong 
>> wrote:
>
>   const VarDecl *VD = VR->getDecl()->getCanonicalDecl();

Wow, nice! I've been recently working on some improvements in this scope. I'll 
take this into account and present a fix soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Good catch Artem, thanks for the report!

Maybe a single line change could solve this?

  const VarDecl *VD = VR->getDecl()->getCanonicalDecl();




Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1663
 // We can trust a const value or a value of a global initializer in main().
 const VarDecl *VD = VR->getDecl();
 if (VD->getType().isConstQualified() ||

const VarDecl *VD = VR->getDecl()->getCanonicalDecl();


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hey, I brought you some regressions!

  const int arr[];
  
  const int arr[3] = {1, 2, 3};
  
  void foo() {
if (arr[0] < 3) {
}
  }



  test.c:6:14: warning: The left operand of '<' is a garbage value 
[core.UndefinedBinaryOperatorResult]
if (arr[0] < 3) {
~~ ^
  1 warning generated.

According to the `-ast-dump` these are redeclarations of the same variable:

  |-VarDecl 0x7fd1ed8844e0  col:7 used arr 'const int []'
  |-VarDecl 0x7fd1ed884670 prev 0x7fd1ed8844e0  col:7 used 
arr 'const int [3]' cinit
  | `-InitListExpr 0x7fd1ed8847a0  'const int [3]'
  |   |-IntegerLiteral 0x7fd1ed8846d8  'int' 1
  |   |-IntegerLiteral 0x7fd1ed8846f8  'int' 2
  |   `-IntegerLiteral 0x7fd1ed884718  'int' 3

So I suspect that you need to pick the redeclaration with the initializer 
before invoking the new machinery.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG98a95d4844ca: [analyzer] Retrieve a value from list 
initialization of constant array… (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/initialization.c
  clang/test/Analysis/initialization.cpp

Index: clang/test/Analysis/initialization.cpp
===
--- clang/test/Analysis/initialization.cpp
+++ clang/test/Analysis/initialization.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++14 -triple i386-apple-darwin10 -analyze -analyzer-checker=core.builtin,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -std=c++14 -triple i386-apple-darwin10 -analyze -analyzer-config eagerly-assume=false -analyzer-checker=core.uninitialized.Assign,core.builtin,debug.ExprInspection,core.uninitialized.UndefReturn -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -18,3 +18,113 @@
   // FIXME: Should recognize that it is 0.
   clang_analyzer_eval(arr[i][0]); // expected-warning{{UNKNOWN}}
 }
+
+int const glob_arr1[3] = {};
+void glob_array_index1() {
+  clang_analyzer_eval(glob_arr1[0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr1[1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr1[2] == 0); // expected-warning{{TRUE}}
+}
+
+void glob_invalid_index1() {
+  const int *ptr = glob_arr1;
+  int idx = -42;
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
+}
+
+int const glob_arr2[4] = {1, 2};
+void glob_ptr_index1() {
+  int const *ptr = glob_arr2;
+  clang_analyzer_eval(ptr[0] == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[1] == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[2] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNDEFINED}}
+}
+
+void glob_invalid_index2() {
+  const int *ptr = glob_arr2;
+  int idx = 42;
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
+}
+
+const float glob_arr3[] = {
+0., 0.0235, 0.0470, 0.0706, 0.0941, 0.1176};
+float no_warn_garbage_value() {
+  return glob_arr3[0]; // no-warning (garbage or undefined)
+}
+
+// TODO: Support multidimensional array.
+int const glob_arr4[4][2] = {};
+void glob_array_index2() {
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr4[1][0] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr4[1][1] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index3() {
+  int idx = -42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = glob_arr4[1][idx]; // no-warning
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index4() {
+  const int *ptr = glob_arr4[1];
+  int idx = -42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = ptr[idx]; // no-warning
+}
+
+// TODO: Support multidimensional array.
+int const glob_arr5[4][2] = {{1}, 3, 4, 5};
+void glob_array_index3() {
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[0][0] == 1); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[0][1] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[1][0] == 3); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[1][1] == 4); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[2][0] == 5); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[2][1] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[3][0] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[3][1] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_ptr_index2() {
+  int const *ptr = glob_arr5[1];
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(ptr[0] == 3); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(ptr[1] == 4); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[2] == 5); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index5() {
+  int idx = -42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = glob_arr5[1][idx]; // no-warning
+}
+
+// TODO: Support multidimensional 

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D104285#3018257 , @ASDenysPetrov 
wrote:

> @martong
> BTW, this patch is the first one in the stack. There are also D107339 
>  and D108032 
> . You could also express your opinion there.

Okay, I am going to have a look, in the meanwhile let's land this first and see 
if the build bots are happy.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@martong
BTW, this patch is the first one in the stack. There are also D107339 
 and D108032 
. You could also express your opinion there.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@martong
Thank you for your time!


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696-1697
+  const auto I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.
+  const uint64_t Extent = CAT->getSize().getZExtValue();
+  // Check for `Idx < 0`, NOT for `I < 0`, because `Idx` CAN be

martong wrote:
> Do you think it would make sense to `assert(CAT->getSize().isSigned())`?
`getSize` return `APInt` which is //signless// and has no `isSigned` method. 
But we know that an array extent shall be of type `std​::​size_­t` 
(http://eel.is/c++draft/dcl.array#1) which is //unsigned// 
(http://eel.is/c++draft/support.types.layout#3). So we can confidently get the 
size with `getZExtValue`.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM!




Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696-1697
+  const auto I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.
+  const uint64_t Extent = CAT->getSize().getZExtValue();
+  // Check for `Idx < 0`, NOT for `I < 0`, because `Idx` CAN be

Do you think it would make sense to `assert(CAT->getSize().isSigned())`?



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696
+  const llvm::APSInt  = CI->getValue();
+  const uint64_t I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.

ASDenysPetrov wrote:
> martong wrote:
> > ASDenysPetrov wrote:
> > > ASDenysPetrov wrote:
> > > > martong wrote:
> > > > > aaron.ballman wrote:
> > > > > > 
> > > > > This `static_cast` seems to be dangerous to me, it might overflow. 
> > > > > Can't we compare `Idx` directly to `Extent`? I see that `Idx` is an 
> > > > > `APSint` and `Extent` is an `APInt`, but  I think we should be able 
> > > > > to handle the comparison on the APInt level b/c that takes care of 
> > > > > the signedness. And the overflow situation should be handled as well 
> > > > > properly with `APInt`, given from it's name "arbitrary precision 
> > > > > int". In this sense I don't see why do we need `I` at all.
> > > > We can't get rid of `I` because we use it below anyway in `I >= 
> > > > InitList->getNumInits()` and `InitList->getInit(I)`.
> > > > I couldn't find any appropriate function to compare without additional 
> > > > checking for signedness or bit-width adjusting.
> > > > I'll try to improve this snippet.
> > > This is not dangerous because we check for negatives separately in `Idx < 
> > > 0`, so we can be sure that `I` is positive while `I >= Extent`. 
> > > Unfortunately, I didn't find any suitable way to compare `APSint` //of 
> > > unknown sign and bitwidth// with //signless// `APInt` without adding 
> > > another checks for sign and bitwidth conversions. In this way I prefer 
> > > the currect condition `I >= Extent`.
> > I think it would be possible to use `bool llvm::APInt::uge` that does an 
> > Unsigned greater or equal comparison. Or you could use `sge` for the signed 
> > comparison. Also, both have overload that take another APInt as parameter.
> I considered them. First of all choosing between `uge` and `sge` we 
> additionally need to check for signedness. Moreover, these functions require 
> bitwidth to be equal. Thus we need additional checks and transformations. I 
> found this approach too verbose. Mine one seems to me simpler and works under 
> natural rules of comparison.
Okay, thanks for thinking it through and answering my concerns!


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696
+  const llvm::APSInt  = CI->getValue();
+  const uint64_t I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.

martong wrote:
> ASDenysPetrov wrote:
> > ASDenysPetrov wrote:
> > > martong wrote:
> > > > aaron.ballman wrote:
> > > > > 
> > > > This `static_cast` seems to be dangerous to me, it might overflow. 
> > > > Can't we compare `Idx` directly to `Extent`? I see that `Idx` is an 
> > > > `APSint` and `Extent` is an `APInt`, but  I think we should be able to 
> > > > handle the comparison on the APInt level b/c that takes care of the 
> > > > signedness. And the overflow situation should be handled as well 
> > > > properly with `APInt`, given from it's name "arbitrary precision int". 
> > > > In this sense I don't see why do we need `I` at all.
> > > We can't get rid of `I` because we use it below anyway in `I >= 
> > > InitList->getNumInits()` and `InitList->getInit(I)`.
> > > I couldn't find any appropriate function to compare without additional 
> > > checking for signedness or bit-width adjusting.
> > > I'll try to improve this snippet.
> > This is not dangerous because we check for negatives separately in `Idx < 
> > 0`, so we can be sure that `I` is positive while `I >= Extent`. 
> > Unfortunately, I didn't find any suitable way to compare `APSint` //of 
> > unknown sign and bitwidth// with //signless// `APInt` without adding 
> > another checks for sign and bitwidth conversions. In this way I prefer the 
> > currect condition `I >= Extent`.
> I think it would be possible to use `bool llvm::APInt::uge` that does an 
> Unsigned greater or equal comparison. Or you could use `sge` for the signed 
> comparison. Also, both have overload that take another APInt as parameter.
I considered them. First of all choosing between `uge` and `sge` we 
additionally need to check for signedness. Moreover, these functions require 
bitwidth to be equal. Thus we need additional checks and transformations. I 
found this approach too verbose. Mine one seems to me simpler and works under 
natural rules of comparison.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696
+  const llvm::APSInt  = CI->getValue();
+  const uint64_t I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.

ASDenysPetrov wrote:
> ASDenysPetrov wrote:
> > martong wrote:
> > > aaron.ballman wrote:
> > > > 
> > > This `static_cast` seems to be dangerous to me, it might overflow. Can't 
> > > we compare `Idx` directly to `Extent`? I see that `Idx` is an `APSint` 
> > > and `Extent` is an `APInt`, but  I think we should be able to handle the 
> > > comparison on the APInt level b/c that takes care of the signedness. And 
> > > the overflow situation should be handled as well properly with `APInt`, 
> > > given from it's name "arbitrary precision int". In this sense I don't see 
> > > why do we need `I` at all.
> > We can't get rid of `I` because we use it below anyway in `I >= 
> > InitList->getNumInits()` and `InitList->getInit(I)`.
> > I couldn't find any appropriate function to compare without additional 
> > checking for signedness or bit-width adjusting.
> > I'll try to improve this snippet.
> This is not dangerous because we check for negatives separately in `Idx < 0`, 
> so we can be sure that `I` is positive while `I >= Extent`. Unfortunately, I 
> didn't find any suitable way to compare `APSint` //of unknown sign and 
> bitwidth// with //signless// `APInt` without adding another checks for sign 
> and bitwidth conversions. In this way I prefer the currect condition `I >= 
> Extent`.
I think it would be possible to use `bool llvm::APInt::uge` that does an 
Unsigned greater or equal comparison. Or you could use `sge` for the signed 
comparison. Also, both have overload that take another APInt as parameter.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 373895.
ASDenysPetrov added a comment.

Fixed nits.


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

https://reviews.llvm.org/D104285

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/initialization.c
  clang/test/Analysis/initialization.cpp

Index: clang/test/Analysis/initialization.cpp
===
--- clang/test/Analysis/initialization.cpp
+++ clang/test/Analysis/initialization.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++14 -triple i386-apple-darwin10 -analyze -analyzer-checker=core.builtin,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -std=c++14 -triple i386-apple-darwin10 -analyze -analyzer-config eagerly-assume=false -analyzer-checker=core.uninitialized.Assign,core.builtin,debug.ExprInspection,core.uninitialized.UndefReturn -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -18,3 +18,113 @@
   // FIXME: Should recognize that it is 0.
   clang_analyzer_eval(arr[i][0]); // expected-warning{{UNKNOWN}}
 }
+
+int const glob_arr1[3] = {};
+void glob_array_index1() {
+  clang_analyzer_eval(glob_arr1[0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr1[1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr1[2] == 0); // expected-warning{{TRUE}}
+}
+
+void glob_invalid_index1() {
+  const int *ptr = glob_arr1;
+  int idx = -42;
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
+}
+
+int const glob_arr2[4] = {1, 2};
+void glob_ptr_index1() {
+  int const *ptr = glob_arr2;
+  clang_analyzer_eval(ptr[0] == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[1] == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[2] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNDEFINED}}
+}
+
+void glob_invalid_index2() {
+  const int *ptr = glob_arr2;
+  int idx = 42;
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
+}
+
+const float glob_arr3[] = {
+0., 0.0235, 0.0470, 0.0706, 0.0941, 0.1176};
+float no_warn_garbage_value() {
+  return glob_arr3[0]; // no-warning (garbage or undefined)
+}
+
+// TODO: Support multidimensional array.
+int const glob_arr4[4][2] = {};
+void glob_array_index2() {
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr4[1][0] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr4[1][1] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index3() {
+  int idx = -42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = glob_arr4[1][idx]; // no-warning
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index4() {
+  const int *ptr = glob_arr4[1];
+  int idx = -42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = ptr[idx]; // no-warning
+}
+
+// TODO: Support multidimensional array.
+int const glob_arr5[4][2] = {{1}, 3, 4, 5};
+void glob_array_index3() {
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[0][0] == 1); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[0][1] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[1][0] == 3); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[1][1] == 4); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[2][0] == 5); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[2][1] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[3][0] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[3][1] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_ptr_index2() {
+  int const *ptr = glob_arr5[1];
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(ptr[0] == 3); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(ptr[1] == 4); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[2] == 5); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index5() {
+  int idx = -42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = glob_arr5[1][idx]; // no-warning
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index6() {
+  int const *ptr = _arr5[1][0];
+  int idx = 42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = ptr[idx]; // // no-warning
+}
Index: 

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1692-1694
+const bool IsOneDimensionalArray =
+!isa(CAT->getElementType());
+if (IsOneDimensionalArray) {

ASDenysPetrov wrote:
> aaron.ballman wrote:
> > ASDenysPetrov wrote:
> > > martong wrote:
> > > > aaron.ballman wrote:
> > > > > 
> > > > +1 for Aaron's suggestion, but then it would be really helpful to have 
> > > > an explanatory comment. E.g.:
> > > > ```
> > > > if (!isa(CAT->getElementType())) { // This is a one 
> > > > dimensional array.
> > > > ```
> > > I think that self-descriptive code is better than comments nearby. And it 
> > > does not affect anything in terms of performance.
> > FWIW, I found the condensed form more readable (I don't have to wonder 
> > what's going to care about that variable later in the function with lots of 
> > nesting).
> I see what you mean. That's fair in terms of variables caring. I think it's 
> just the other hand of the approach. I don't have strong preferences here, 
> it's just my personal flavor because it doesn't need to introspect the 
> expression to undersand what it means. But I'll make an update using your 
> proposition.
FWIW, my preference is weak as well -- the code is readable either way. :-)


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1692-1694
+const bool IsOneDimensionalArray =
+!isa(CAT->getElementType());
+if (IsOneDimensionalArray) {

aaron.ballman wrote:
> ASDenysPetrov wrote:
> > martong wrote:
> > > aaron.ballman wrote:
> > > > 
> > > +1 for Aaron's suggestion, but then it would be really helpful to have an 
> > > explanatory comment. E.g.:
> > > ```
> > > if (!isa(CAT->getElementType())) { // This is a one 
> > > dimensional array.
> > > ```
> > I think that self-descriptive code is better than comments nearby. And it 
> > does not affect anything in terms of performance.
> FWIW, I found the condensed form more readable (I don't have to wonder what's 
> going to care about that variable later in the function with lots of nesting).
I see what you mean. That's fair in terms of variables caring. I think it's 
just the other hand of the approach. I don't have strong preferences here, it's 
just my personal flavor because it doesn't need to introspect the expression to 
undersand what it means. But I'll make an update using your proposition.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1692-1694
+const bool IsOneDimensionalArray =
+!isa(CAT->getElementType());
+if (IsOneDimensionalArray) {

ASDenysPetrov wrote:
> martong wrote:
> > aaron.ballman wrote:
> > > 
> > +1 for Aaron's suggestion, but then it would be really helpful to have an 
> > explanatory comment. E.g.:
> > ```
> > if (!isa(CAT->getElementType())) { // This is a one 
> > dimensional array.
> > ```
> I think that self-descriptive code is better than comments nearby. And it 
> does not affect anything in terms of performance.
FWIW, I found the condensed form more readable (I don't have to wonder what's 
going to care about that variable later in the function with lots of nesting).


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696
+  const llvm::APSInt  = CI->getValue();
+  const uint64_t I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.

ASDenysPetrov wrote:
> martong wrote:
> > aaron.ballman wrote:
> > > 
> > This `static_cast` seems to be dangerous to me, it might overflow. Can't we 
> > compare `Idx` directly to `Extent`? I see that `Idx` is an `APSint` and 
> > `Extent` is an `APInt`, but  I think we should be able to handle the 
> > comparison on the APInt level b/c that takes care of the signedness. And 
> > the overflow situation should be handled as well properly with `APInt`, 
> > given from it's name "arbitrary precision int". In this sense I don't see 
> > why do we need `I` at all.
> We can't get rid of `I` because we use it below anyway in `I >= 
> InitList->getNumInits()` and `InitList->getInit(I)`.
> I couldn't find any appropriate function to compare without additional 
> checking for signedness or bit-width adjusting.
> I'll try to improve this snippet.
This is not dangerous because we check for negatives separately in `Idx < 0`, 
so we can be sure that `I` is positive while `I >= Extent`. Unfortunately, I 
didn't find any suitable way to compare `APSint` //of unknown sign and 
bitwidth// with //signless// `APInt` without adding another checks for sign and 
bitwidth conversions. In this way I prefer the currect condition `I >= Extent`.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1692-1694
+const bool IsOneDimensionalArray =
+!isa(CAT->getElementType());
+if (IsOneDimensionalArray) {

martong wrote:
> aaron.ballman wrote:
> > 
> +1 for Aaron's suggestion, but then it would be really helpful to have an 
> explanatory comment. E.g.:
> ```
> if (!isa(CAT->getElementType())) { // This is a one 
> dimensional array.
> ```
I think that self-descriptive code is better than comments nearby. And it does 
not affect anything in terms of performance.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696
+  const llvm::APSInt  = CI->getValue();
+  const uint64_t I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.

martong wrote:
> aaron.ballman wrote:
> > 
> This `static_cast` seems to be dangerous to me, it might overflow. Can't we 
> compare `Idx` directly to `Extent`? I see that `Idx` is an `APSint` and 
> `Extent` is an `APInt`, but  I think we should be able to handle the 
> comparison on the APInt level b/c that takes care of the signedness. And the 
> overflow situation should be handled as well properly with `APInt`, given 
> from it's name "arbitrary precision int". In this sense I don't see why do we 
> need `I` at all.
We can't get rid of `I` because we use it below anyway in `I >= 
InitList->getNumInits()` and `InitList->getInit(I)`.
I couldn't find any appropriate function to compare without additional checking 
for signedness or bit-width adjusting.
I'll try to improve this snippet.



Comment at: clang/test/Analysis/initialization.c:1
-// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze 
-analyzer-checker=core.builtin,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -analyzer-config 
eagerly-assume=false  
-analyzer-checker=core.uninitialized.Assign,debug.ExprInspection -verify %s
 

martong wrote:
> I don't see how this change is related.  How could the tests work before 
> having the `uninitialized.Assign` enabled before?
I've added `glob_invalid_index1` and `glob_invalid_index2` functions which were 
not there before.
 `core.uninitialized.Assign` produces warnings for them.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1692-1694
+const bool IsOneDimensionalArray =
+!isa(CAT->getElementType());
+if (IsOneDimensionalArray) {

aaron.ballman wrote:
> 
+1 for Aaron's suggestion, but then it would be really helpful to have an 
explanatory comment. E.g.:
```
if (!isa(CAT->getElementType())) { // This is a one 
dimensional array.
```



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696
+  const llvm::APSInt  = CI->getValue();
+  const uint64_t I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.

aaron.ballman wrote:
> 
This `static_cast` seems to be dangerous to me, it might overflow. Can't we 
compare `Idx` directly to `Extent`? I see that `Idx` is an `APSint` and 
`Extent` is an `APInt`, but  I think we should be able to handle the comparison 
on the APInt level b/c that takes care of the signedness. And the overflow 
situation should be handled as well properly with `APInt`, given from it's name 
"arbitrary precision int". In this sense I don't see why do we need `I` at all.



Comment at: clang/test/Analysis/initialization.c:1
-// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze 
-analyzer-checker=core.builtin,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -analyze -analyzer-config 
eagerly-assume=false  
-analyzer-checker=core.uninitialized.Assign,debug.ExprInspection -verify %s
 

I don't see how this change is related.  How could the tests work before having 
the `uninitialized.Assign` enabled before?


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Some minor drive-by nits, but this looks sensible to me.




Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1692-1694
+const bool IsOneDimensionalArray =
+!isa(CAT->getElementType());
+if (IsOneDimensionalArray) {





Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696-1698
+  const uint64_t I = static_cast(Idx.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.
+  const uint64_t Extent = CAT->getSize().getZExtValue();




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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Now this patch supports only one-dimensional arrays. Previously there were a 
bug when didn't take into account array's dimension.


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

https://reviews.llvm.org/D104285

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov updated this revision to Diff 370284.
ASDenysPetrov retitled this revision from "[analyzer][AST] Retrieve value by 
direct index from list initialization of constant array declaration." to 
"[analyzer] Retrieve a value from list initialization of constant array 
declaration in a global scope.".
ASDenysPetrov edited the summary of this revision.
ASDenysPetrov added a comment.

Changed the Title. Changed the Summary.


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

https://reviews.llvm.org/D104285

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/initialization.c
  clang/test/Analysis/initialization.cpp

Index: clang/test/Analysis/initialization.cpp
===
--- clang/test/Analysis/initialization.cpp
+++ clang/test/Analysis/initialization.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++14 -triple i386-apple-darwin10 -analyze -analyzer-checker=core.builtin,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -std=c++14 -triple i386-apple-darwin10 -analyze -analyzer-config eagerly-assume=false -analyzer-checker=core.uninitialized.Assign,core.builtin,debug.ExprInspection,core.uninitialized.UndefReturn -verify %s
 
 void clang_analyzer_eval(int);
 
@@ -18,3 +18,113 @@
   // FIXME: Should recognize that it is 0.
   clang_analyzer_eval(arr[i][0]); // expected-warning{{UNKNOWN}}
 }
+
+int const glob_arr1[3] = {};
+void glob_array_index1() {
+  clang_analyzer_eval(glob_arr1[0] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr1[1] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr1[2] == 0); // expected-warning{{TRUE}}
+}
+
+void glob_invalid_index1() {
+  const int *ptr = glob_arr1;
+  int idx = -42;
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
+}
+
+int const glob_arr2[4] = {1, 2};
+void glob_ptr_index1() {
+  int const *ptr = glob_arr2;
+  clang_analyzer_eval(ptr[0] == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[1] == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[2] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNDEFINED}}
+}
+
+void glob_invalid_index2() {
+  const int *ptr = glob_arr2;
+  int idx = 42;
+  auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
+}
+
+const float glob_arr3[] = {
+0., 0.0235, 0.0470, 0.0706, 0.0941, 0.1176};
+float no_warn_garbage_value() {
+  return glob_arr3[0]; // no-warning (garbage or undefined)
+}
+
+// TODO: Support multidimensional array.
+int const glob_arr4[4][2] = {};
+void glob_array_index2() {
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr4[1][0] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr4[1][1] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index3() {
+  int idx = -42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = glob_arr4[1][idx]; // no-warning
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index4() {
+  const int *ptr = glob_arr4[1];
+  int idx = -42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = ptr[idx]; // no-warning
+}
+
+// TODO: Support multidimensional array.
+int const glob_arr5[4][2] = {{1}, 3, 4, 5};
+void glob_array_index3() {
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[0][0] == 1); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[0][1] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[1][0] == 3); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[1][1] == 4); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[2][0] == 5); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[2][1] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[3][0] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(glob_arr5[3][1] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_ptr_index2() {
+  int const *ptr = glob_arr5[1];
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(ptr[0] == 3); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be TRUE.
+  clang_analyzer_eval(ptr[1] == 4); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[2] == 5); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[3] == 0); // expected-warning{{UNKNOWN}}
+  // FIXME: Should be UNDEFINED.
+  clang_analyzer_eval(ptr[4] == 0); // expected-warning{{UNKNOWN}}
+}
+
+// TODO: Support multidimensional array.
+void glob_invalid_index5() {
+  int idx = -42;
+  //