[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG44e803ef6d41: [analyzer][NFCI] Move a block from 
`getBindingForElement` to separate functions (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106681

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,5 +1,7 @@
 // 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
 
+template 
+void clang_analyzer_dump(T x);
 void clang_analyzer_eval(int);
 
 struct S {
@@ -32,6 +34,10 @@
   auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
 }
 
+void glob_symbolic_index1(int idx) {
+  clang_analyzer_dump(glob_arr1[idx]); // expected-warning{{Unknown}}
+}
+
 int const glob_arr2[4] = {1, 2};
 void glob_ptr_index1() {
   int const *ptr = glob_arr2;
@@ -128,3 +134,15 @@
   // FIXME: Should warn {{garbage or undefined}}.
   auto x = ptr[idx]; // // no-warning
 }
+
+extern const int glob_arr_no_init[10];
+void glob_array_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
+
+struct S2 {
+  static const int arr_no_init[10];
+};
+void struct_arr_index1() {
+  clang_analyzer_eval(S2::arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
Index: clang/test/Analysis/initialization.c
===
--- clang/test/Analysis/initialization.c
+++ clang/test/Analysis/initialization.c
@@ -97,3 +97,9 @@
   // FIXME: Should warn {{garbage or undefined}}.
   int res = glob_arr2[x][y]; // no-warning
 }
+
+const int glob_arr_no_init[10];
+void glob_arr_index4() {
+  // FIXME: Should warn {{FALSE}}, since the array has a static storage.
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -437,6 +437,10 @@
 
   RegionBindingsRef removeSubRegionBindings(RegionBindingsConstRef B,
 const SubRegion *R);
+  Optional getConstantValFromConstArrayInitializer(
+  RegionBindingsConstRef B, const VarRegion *VR, const ElementRegion *R);
+  Optional getSValFromInitListExpr(const InitListExpr *ILE,
+ uint64_t Offset, QualType ElemT);
 
 public: // Part of public interface to class.
 
@@ -1625,6 +1629,95 @@
   return Result;
 }
 
+Optional RegionStoreManager::getConstantValFromConstArrayInitializer(
+RegionBindingsConstRef B, const VarRegion *VR, const ElementRegion *R) {
+  assert(R && VR && "Regions should not be null");
+
+  // Check if the containing array has an initialized value that we can trust.
+  // We can trust a const value or a value of a global initializer in main().
+  const VarDecl *VD = VR->getDecl();
+  if (!VD->getType().isConstQualified() &&
+  !R->getElementType().isConstQualified() &&
+  (!B.isMainAnalysis() || !VD->hasGlobalStorage()))
+return None;
+
+  // Array's declaration should have an initializer.
+  const Expr *Init = VD->getAnyInitializer();
+  if (!Init)
+return None;
+
+  // Array's declaration should have ConstantArrayType type, because only this
+  // type contains an array extent.
+  const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(VD->getType());
+  if (!CAT)
+return None;
+
+  // Array should be one-dimensional.
+  // TODO: Support multidimensional array.
+  if (isa(CAT->getElementType())) // is multidimensional
+return None;
+
+  // Array's offset should be a concrete value.
+  // Return Unknown value if symbolic index presented.
+  // FIXME: We also need to take ElementRegions with symbolic
+  // indexes into account.
+  const auto OffsetVal = R->getIndex().getAs();
+  if (!OffsetVal.hasValue())
+return UnknownVal();
+
+  // Check offset for being out of bounds.
+  // C++20 [expr.add] 7.6.6.4 (excerpt):
+  //   If P points to an array element i of an array object x with n
+  //   elements, where i < 0 or i > n, the behavior is undefined.
+  //   Dereferencing is not allowed on the "one past the last
+  //   element", when i == n.
+  // Example:
+  //   const int arr[4] = {1, 2};
+  //   const int *ptr = arr;
+  //   int x0 = ptr[0];  // 1
+  //   int x1 = ptr[1];  // 2
+  //   int x2 = ptr[2];  // 0
+  //   int x3 = ptr[3];  // 0
+  //   int x4 = ptr[4];  // UB
+  //   int x5 = ptr[-1]; // 

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D106681#3083409 , @steakhal wrote:

> Sorry for blocking the review of this one for so long.

Thank you for the review!


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

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

steakhal accepted this revision.
steakhal added a comment.

Sorry for blocking the review of this one for so long.




Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

ASDenysPetrov wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > ASDenysPetrov wrote:
> > > > ASDenysPetrov wrote:
> > > > > steakhal wrote:
> > > > > > martong wrote:
> > > > > > > ASDenysPetrov wrote:
> > > > > > > > steakhal wrote:
> > > > > > > > > I'm pretty sure we should not get `Unknown` for simply 
> > > > > > > > > loading from a global variable.
> > > > > > > > > That would imply that loading two times subsequently we could 
> > > > > > > > > not prove that the value remained the same.
> > > > > > > > > But that should remain the same, thus compare equal unless 
> > > > > > > > > some other code modifier that memory region.
> > > > > > > > > 
> > > > > > > > > That could happen for two reasons:
> > > > > > > > > 1) There is a racecondition, and another thread modified that 
> > > > > > > > > location. But that would be UB, so that could not happen.
> > > > > > > > > 2) The global variable is //volatile//, so the hardware might 
> > > > > > > > > changed its content- but this global is not volatile so this 
> > > > > > > > > does not apply.
> > > > > > > > > 
> > > > > > > > > That being said, this load should have resulted in a 
> > > > > > > > > //fresh// conjured symbolic value instead of //unknown//.
> > > > > > > > > Could you please check if it did result in //unknown// before 
> > > > > > > > > your patch, or you did introduce this behavior?
> > > > > > > > I'm not sure I caught your thoughts.
> > > > > > > > But I think the things is much simplier:
> > > > > > > > `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or 
> > > > > > > > `FALSE`. If we know the constraint of `glob_arr_no_init[2]` we 
> > > > > > > > return `TRUE` or `FALSE`, and `UNKNOWN` otherwise.
> > > > > > > > But in fact I should use `clang_analyzer_dump` here instead of 
> > > > > > > > `clang_analyzer_eval`. This is actually my fault. I'll update 
> > > > > > > > this.
> > > > > > > > Could you please check if it did result in unknown before your 
> > > > > > > > patch, or you did introduce this behavior?
> > > > > > > 
> > > > > > > I've just checked it, it was `Unknown` before this patch as well. 
> > > > > > > And actually, that is wrong because the array has static storage 
> > > > > > > duration and as such, we know that it is initialized with zeros 
> > > > > > > according to the C standard. But, that should be addressed in a 
> > > > > > > follow-up patch (if someone has the capacity).
> > > > > > > https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288
> > > > > > Oh true. I was actually tricked by the `initialization.cpp:38`, 
> > > > > > where I actually caught this. And in that case, you use `dump()` 
> > > > > > yet you get `Unknown` as a result. But the issue remains the same.
> > > > > C++ also states about zero-initialization for static storage lifetime 
> > > > > duration: http://eel.is/c++draft/basic.start.static#2
> > > > > I think it will be among my next patches.
> > > > > And in that case, you use dump() yet you get Unknown as a result. But 
> > > > > the issue remains the same.
> > > > I just realized that I was confused as well :) The `dump` returns a 
> > > > symbolic value like `reg_$0`, 
> > > > **not** `Unknown`. So my intention of using `eval` was deliberate. 
> > > > Anyway we should improve this to produce `FALSE` instead of `UNKNOWN`, 
> > > > since it has a //static storage//.
> > > It's a really complex topic. I would highly recommend taking baby steps 
> > > to improve this area.
> > > 
> > > In C, you might have a chance to accomplish something, but in C++ static 
> > > globals might be initialized by running a constructor, which means 
> > > arbitrary user-defined code. This is actually why we disabled similar 
> > > logic in the `RegionStore::getInitialStore()`. I highly recommend taking 
> > > a look.
> > > 
> > > Consider this code:
> > > ```lang=C++
> > > // TU 1:
> > > #include 
> > > static int a;  // zero-initialized initially
> > > int *p2a =  // escapes the address of 'a'
> > > 
> > > int main() {
> > >   printf("%d\n", a); // reports 42
> > > }
> > > 
> > > // TU 2:
> > > extern int *p2a;
> > > static bool sentinel = (*p2a = 42, false);
> > > ```
> > Yes, I see what you mean. I'm going to tough the part of C++ in the near 
> > future.
> > Yes, I see what you mean. I'm going to tough the part of C++ in the near 
> > future.
> *Yes, I see what you mean. I'm **not** going to tough the part of C++ in the 
> near future.
Okay, I see now.


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

https://reviews.llvm.org/D106681

___
cfe-commits mailing 

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

ASDenysPetrov wrote:
> steakhal wrote:
> > ASDenysPetrov wrote:
> > > ASDenysPetrov wrote:
> > > > steakhal wrote:
> > > > > martong wrote:
> > > > > > ASDenysPetrov wrote:
> > > > > > > steakhal wrote:
> > > > > > > > I'm pretty sure we should not get `Unknown` for simply loading 
> > > > > > > > from a global variable.
> > > > > > > > That would imply that loading two times subsequently we could 
> > > > > > > > not prove that the value remained the same.
> > > > > > > > But that should remain the same, thus compare equal unless some 
> > > > > > > > other code modifier that memory region.
> > > > > > > > 
> > > > > > > > That could happen for two reasons:
> > > > > > > > 1) There is a racecondition, and another thread modified that 
> > > > > > > > location. But that would be UB, so that could not happen.
> > > > > > > > 2) The global variable is //volatile//, so the hardware might 
> > > > > > > > changed its content- but this global is not volatile so this 
> > > > > > > > does not apply.
> > > > > > > > 
> > > > > > > > That being said, this load should have resulted in a //fresh// 
> > > > > > > > conjured symbolic value instead of //unknown//.
> > > > > > > > Could you please check if it did result in //unknown// before 
> > > > > > > > your patch, or you did introduce this behavior?
> > > > > > > I'm not sure I caught your thoughts.
> > > > > > > But I think the things is much simplier:
> > > > > > > `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or 
> > > > > > > `FALSE`. If we know the constraint of `glob_arr_no_init[2]` we 
> > > > > > > return `TRUE` or `FALSE`, and `UNKNOWN` otherwise.
> > > > > > > But in fact I should use `clang_analyzer_dump` here instead of 
> > > > > > > `clang_analyzer_eval`. This is actually my fault. I'll update 
> > > > > > > this.
> > > > > > > Could you please check if it did result in unknown before your 
> > > > > > > patch, or you did introduce this behavior?
> > > > > > 
> > > > > > I've just checked it, it was `Unknown` before this patch as well. 
> > > > > > And actually, that is wrong because the array has static storage 
> > > > > > duration and as such, we know that it is initialized with zeros 
> > > > > > according to the C standard. But, that should be addressed in a 
> > > > > > follow-up patch (if someone has the capacity).
> > > > > > https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288
> > > > > Oh true. I was actually tricked by the `initialization.cpp:38`, where 
> > > > > I actually caught this. And in that case, you use `dump()` yet you 
> > > > > get `Unknown` as a result. But the issue remains the same.
> > > > C++ also states about zero-initialization for static storage lifetime 
> > > > duration: http://eel.is/c++draft/basic.start.static#2
> > > > I think it will be among my next patches.
> > > > And in that case, you use dump() yet you get Unknown as a result. But 
> > > > the issue remains the same.
> > > I just realized that I was confused as well :) The `dump` returns a 
> > > symbolic value like `reg_$0`, 
> > > **not** `Unknown`. So my intention of using `eval` was deliberate. Anyway 
> > > we should improve this to produce `FALSE` instead of `UNKNOWN`, since it 
> > > has a //static storage//.
> > It's a really complex topic. I would highly recommend taking baby steps to 
> > improve this area.
> > 
> > In C, you might have a chance to accomplish something, but in C++ static 
> > globals might be initialized by running a constructor, which means 
> > arbitrary user-defined code. This is actually why we disabled similar logic 
> > in the `RegionStore::getInitialStore()`. I highly recommend taking a look.
> > 
> > Consider this code:
> > ```lang=C++
> > // TU 1:
> > #include 
> > static int a;  // zero-initialized initially
> > int *p2a =  // escapes the address of 'a'
> > 
> > int main() {
> >   printf("%d\n", a); // reports 42
> > }
> > 
> > // TU 2:
> > extern int *p2a;
> > static bool sentinel = (*p2a = 42, false);
> > ```
> Yes, I see what you mean. I'm going to tough the part of C++ in the near 
> future.
> Yes, I see what you mean. I'm going to tough the part of C++ in the near 
> future.
*Yes, I see what you mean. I'm **not** going to tough the part of C++ in the 
near future.


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

steakhal wrote:
> ASDenysPetrov wrote:
> > ASDenysPetrov wrote:
> > > steakhal wrote:
> > > > martong wrote:
> > > > > ASDenysPetrov wrote:
> > > > > > steakhal wrote:
> > > > > > > I'm pretty sure we should not get `Unknown` for simply loading 
> > > > > > > from a global variable.
> > > > > > > That would imply that loading two times subsequently we could not 
> > > > > > > prove that the value remained the same.
> > > > > > > But that should remain the same, thus compare equal unless some 
> > > > > > > other code modifier that memory region.
> > > > > > > 
> > > > > > > That could happen for two reasons:
> > > > > > > 1) There is a racecondition, and another thread modified that 
> > > > > > > location. But that would be UB, so that could not happen.
> > > > > > > 2) The global variable is //volatile//, so the hardware might 
> > > > > > > changed its content- but this global is not volatile so this does 
> > > > > > > not apply.
> > > > > > > 
> > > > > > > That being said, this load should have resulted in a //fresh// 
> > > > > > > conjured symbolic value instead of //unknown//.
> > > > > > > Could you please check if it did result in //unknown// before 
> > > > > > > your patch, or you did introduce this behavior?
> > > > > > I'm not sure I caught your thoughts.
> > > > > > But I think the things is much simplier:
> > > > > > `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or 
> > > > > > `FALSE`. If we know the constraint of `glob_arr_no_init[2]` we 
> > > > > > return `TRUE` or `FALSE`, and `UNKNOWN` otherwise.
> > > > > > But in fact I should use `clang_analyzer_dump` here instead of 
> > > > > > `clang_analyzer_eval`. This is actually my fault. I'll update this.
> > > > > > Could you please check if it did result in unknown before your 
> > > > > > patch, or you did introduce this behavior?
> > > > > 
> > > > > I've just checked it, it was `Unknown` before this patch as well. 
> > > > > And actually, that is wrong because the array has static storage 
> > > > > duration and as such, we know that it is initialized with zeros 
> > > > > according to the C standard. But, that should be addressed in a 
> > > > > follow-up patch (if someone has the capacity).
> > > > > https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288
> > > > Oh true. I was actually tricked by the `initialization.cpp:38`, where I 
> > > > actually caught this. And in that case, you use `dump()` yet you get 
> > > > `Unknown` as a result. But the issue remains the same.
> > > C++ also states about zero-initialization for static storage lifetime 
> > > duration: http://eel.is/c++draft/basic.start.static#2
> > > I think it will be among my next patches.
> > > And in that case, you use dump() yet you get Unknown as a result. But the 
> > > issue remains the same.
> > I just realized that I was confused as well :) The `dump` returns a 
> > symbolic value like `reg_$0`, 
> > **not** `Unknown`. So my intention of using `eval` was deliberate. Anyway 
> > we should improve this to produce `FALSE` instead of `UNKNOWN`, since it 
> > has a //static storage//.
> It's a really complex topic. I would highly recommend taking baby steps to 
> improve this area.
> 
> In C, you might have a chance to accomplish something, but in C++ static 
> globals might be initialized by running a constructor, which means arbitrary 
> user-defined code. This is actually why we disabled similar logic in the 
> `RegionStore::getInitialStore()`. I highly recommend taking a look.
> 
> Consider this code:
> ```lang=C++
> // TU 1:
> #include 
> static int a;  // zero-initialized initially
> int *p2a =  // escapes the address of 'a'
> 
> int main() {
>   printf("%d\n", a); // reports 42
> }
> 
> // TU 2:
> extern int *p2a;
> static bool sentinel = (*p2a = 42, false);
> ```
Yes, I see what you mean. I'm going to tough the part of C++ in the near future.


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

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

Added a comment to //initialization.c//, that we can reason about values of 
constant array which doesn't have an initializer. This is not such simple for 
C++, since there are much more ways to initialize the array.


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

https://reviews.llvm.org/D106681

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,5 +1,7 @@
 // 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
 
+template 
+void clang_analyzer_dump(T x);
 void clang_analyzer_eval(int);
 
 struct S {
@@ -32,6 +34,10 @@
   auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
 }
 
+void glob_symbolic_index1(int idx) {
+  clang_analyzer_dump(glob_arr1[idx]); // expected-warning{{Unknown}}
+}
+
 int const glob_arr2[4] = {1, 2};
 void glob_ptr_index1() {
   int const *ptr = glob_arr2;
@@ -128,3 +134,15 @@
   // FIXME: Should warn {{garbage or undefined}}.
   auto x = ptr[idx]; // // no-warning
 }
+
+extern const int glob_arr_no_init[10];
+void glob_array_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
+
+struct S2 {
+  static const int arr_no_init[10];
+};
+void struct_arr_index1() {
+  clang_analyzer_eval(S2::arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
Index: clang/test/Analysis/initialization.c
===
--- clang/test/Analysis/initialization.c
+++ clang/test/Analysis/initialization.c
@@ -97,3 +97,9 @@
   // FIXME: Should warn {{garbage or undefined}}.
   int res = glob_arr2[x][y]; // no-warning
 }
+
+const int glob_arr_no_init[10];
+void glob_arr_index4() {
+  // FIXME: Should warn {{FALSE}}, since the array has a static storage.
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -437,6 +437,10 @@
 
   RegionBindingsRef removeSubRegionBindings(RegionBindingsConstRef B,
 const SubRegion *R);
+  Optional getConstantValFromConstArrayInitializer(
+  RegionBindingsConstRef B, const VarRegion *VR, const ElementRegion *R);
+  Optional getSValFromInitListExpr(const InitListExpr *ILE,
+ uint64_t Offset, QualType ElemT);
 
 public: // Part of public interface to class.
 
@@ -1625,6 +1629,95 @@
   return Result;
 }
 
+Optional RegionStoreManager::getConstantValFromConstArrayInitializer(
+RegionBindingsConstRef B, const VarRegion *VR, const ElementRegion *R) {
+  assert(R && VR && "Regions should not be null");
+
+  // Check if the containing array has an initialized value that we can trust.
+  // We can trust a const value or a value of a global initializer in main().
+  const VarDecl *VD = VR->getDecl();
+  if (!VD->getType().isConstQualified() &&
+  !R->getElementType().isConstQualified() &&
+  (!B.isMainAnalysis() || !VD->hasGlobalStorage()))
+return None;
+
+  // Array's declaration should have an initializer.
+  const Expr *Init = VD->getAnyInitializer();
+  if (!Init)
+return None;
+
+  // Array's declaration should have ConstantArrayType type, because only this
+  // type contains an array extent.
+  const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(VD->getType());
+  if (!CAT)
+return None;
+
+  // Array should be one-dimensional.
+  // TODO: Support multidimensional array.
+  if (isa(CAT->getElementType())) // is multidimensional
+return None;
+
+  // Array's offset should be a concrete value.
+  // Return Unknown value if symbolic index presented.
+  // FIXME: We also need to take ElementRegions with symbolic
+  // indexes into account.
+  const auto OffsetVal = R->getIndex().getAs();
+  if (!OffsetVal.hasValue())
+return UnknownVal();
+
+  // Check offset for being out of bounds.
+  // C++20 [expr.add] 7.6.6.4 (excerpt):
+  //   If P points to an array element i of an array object x with n
+  //   elements, where i < 0 or i > n, the behavior is undefined.
+  //   Dereferencing is not allowed on the "one past the last
+  //   element", when i == n.
+  // Example:
+  //   const int arr[4] = {1, 2};
+  //   const int *ptr = arr;
+  //   int x0 = ptr[0];  // 1
+  //   int x1 = ptr[1];  // 2
+  //   int x2 = ptr[2];  // 0
+  //   int x3 = ptr[3];  // 0
+  //   int x4 = 

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

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

steakhal added inline comments.



Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

ASDenysPetrov wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > martong wrote:
> > > > ASDenysPetrov wrote:
> > > > > steakhal wrote:
> > > > > > I'm pretty sure we should not get `Unknown` for simply loading from 
> > > > > > a global variable.
> > > > > > That would imply that loading two times subsequently we could not 
> > > > > > prove that the value remained the same.
> > > > > > But that should remain the same, thus compare equal unless some 
> > > > > > other code modifier that memory region.
> > > > > > 
> > > > > > That could happen for two reasons:
> > > > > > 1) There is a racecondition, and another thread modified that 
> > > > > > location. But that would be UB, so that could not happen.
> > > > > > 2) The global variable is //volatile//, so the hardware might 
> > > > > > changed its content- but this global is not volatile so this does 
> > > > > > not apply.
> > > > > > 
> > > > > > That being said, this load should have resulted in a //fresh// 
> > > > > > conjured symbolic value instead of //unknown//.
> > > > > > Could you please check if it did result in //unknown// before your 
> > > > > > patch, or you did introduce this behavior?
> > > > > I'm not sure I caught your thoughts.
> > > > > But I think the things is much simplier:
> > > > > `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or 
> > > > > `FALSE`. If we know the constraint of `glob_arr_no_init[2]` we return 
> > > > > `TRUE` or `FALSE`, and `UNKNOWN` otherwise.
> > > > > But in fact I should use `clang_analyzer_dump` here instead of 
> > > > > `clang_analyzer_eval`. This is actually my fault. I'll update this.
> > > > > Could you please check if it did result in unknown before your patch, 
> > > > > or you did introduce this behavior?
> > > > 
> > > > I've just checked it, it was `Unknown` before this patch as well. 
> > > > And actually, that is wrong because the array has static storage 
> > > > duration and as such, we know that it is initialized with zeros 
> > > > according to the C standard. But, that should be addressed in a 
> > > > follow-up patch (if someone has the capacity).
> > > > https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288
> > > Oh true. I was actually tricked by the `initialization.cpp:38`, where I 
> > > actually caught this. And in that case, you use `dump()` yet you get 
> > > `Unknown` as a result. But the issue remains the same.
> > C++ also states about zero-initialization for static storage lifetime 
> > duration: http://eel.is/c++draft/basic.start.static#2
> > I think it will be among my next patches.
> > And in that case, you use dump() yet you get Unknown as a result. But the 
> > issue remains the same.
> I just realized that I was confused as well :) The `dump` returns a symbolic 
> value like `reg_$0`, **not** 
> `Unknown`. So my intention of using `eval` was deliberate. Anyway we should 
> improve this to produce `FALSE` instead of `UNKNOWN`, since it has a //static 
> storage//.
It's a really complex topic. I would highly recommend taking baby steps to 
improve this area.

In C, you might have a chance to accomplish something, but in C++ static 
globals might be initialized by running a constructor, which means arbitrary 
user-defined code. This is actually why we disabled similar logic in the 
`RegionStore::getInitialStore()`. I highly recommend taking a look.

Consider this code:
```lang=C++
// TU 1:
#include 
static int a;  // zero-initialized initially
int *p2a =  // escapes the address of 'a'

int main() {
  printf("%d\n", a); // reports 42
}

// TU 2:
extern int *p2a;
static bool sentinel = (*p2a = 42, false);
```


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

ASDenysPetrov wrote:
> steakhal wrote:
> > martong wrote:
> > > ASDenysPetrov wrote:
> > > > steakhal wrote:
> > > > > I'm pretty sure we should not get `Unknown` for simply loading from a 
> > > > > global variable.
> > > > > That would imply that loading two times subsequently we could not 
> > > > > prove that the value remained the same.
> > > > > But that should remain the same, thus compare equal unless some other 
> > > > > code modifier that memory region.
> > > > > 
> > > > > That could happen for two reasons:
> > > > > 1) There is a racecondition, and another thread modified that 
> > > > > location. But that would be UB, so that could not happen.
> > > > > 2) The global variable is //volatile//, so the hardware might changed 
> > > > > its content- but this global is not volatile so this does not apply.
> > > > > 
> > > > > That being said, this load should have resulted in a //fresh// 
> > > > > conjured symbolic value instead of //unknown//.
> > > > > Could you please check if it did result in //unknown// before your 
> > > > > patch, or you did introduce this behavior?
> > > > I'm not sure I caught your thoughts.
> > > > But I think the things is much simplier:
> > > > `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or `FALSE`. 
> > > > If we know the constraint of `glob_arr_no_init[2]` we return `TRUE` or 
> > > > `FALSE`, and `UNKNOWN` otherwise.
> > > > But in fact I should use `clang_analyzer_dump` here instead of 
> > > > `clang_analyzer_eval`. This is actually my fault. I'll update this.
> > > > Could you please check if it did result in unknown before your patch, 
> > > > or you did introduce this behavior?
> > > 
> > > I've just checked it, it was `Unknown` before this patch as well. 
> > > And actually, that is wrong because the array has static storage duration 
> > > and as such, we know that it is initialized with zeros according to the C 
> > > standard. But, that should be addressed in a follow-up patch (if someone 
> > > has the capacity).
> > > https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288
> > Oh true. I was actually tricked by the `initialization.cpp:38`, where I 
> > actually caught this. And in that case, you use `dump()` yet you get 
> > `Unknown` as a result. But the issue remains the same.
> C++ also states about zero-initialization for static storage lifetime 
> duration: http://eel.is/c++draft/basic.start.static#2
> I think it will be among my next patches.
> And in that case, you use dump() yet you get Unknown as a result. But the 
> issue remains the same.
I just realized that I was confused as well :) The `dump` returns a symbolic 
value like `reg_$0`, **not** 
`Unknown`. So my intention of using `eval` was deliberate. Anyway we should 
improve this to produce `FALSE` instead of `UNKNOWN`, since it has a //static 
storage//.


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

steakhal wrote:
> martong wrote:
> > ASDenysPetrov wrote:
> > > steakhal wrote:
> > > > I'm pretty sure we should not get `Unknown` for simply loading from a 
> > > > global variable.
> > > > That would imply that loading two times subsequently we could not prove 
> > > > that the value remained the same.
> > > > But that should remain the same, thus compare equal unless some other 
> > > > code modifier that memory region.
> > > > 
> > > > That could happen for two reasons:
> > > > 1) There is a racecondition, and another thread modified that location. 
> > > > But that would be UB, so that could not happen.
> > > > 2) The global variable is //volatile//, so the hardware might changed 
> > > > its content- but this global is not volatile so this does not apply.
> > > > 
> > > > That being said, this load should have resulted in a //fresh// conjured 
> > > > symbolic value instead of //unknown//.
> > > > Could you please check if it did result in //unknown// before your 
> > > > patch, or you did introduce this behavior?
> > > I'm not sure I caught your thoughts.
> > > But I think the things is much simplier:
> > > `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or `FALSE`. If 
> > > we know the constraint of `glob_arr_no_init[2]` we return `TRUE` or 
> > > `FALSE`, and `UNKNOWN` otherwise.
> > > But in fact I should use `clang_analyzer_dump` here instead of 
> > > `clang_analyzer_eval`. This is actually my fault. I'll update this.
> > > Could you please check if it did result in unknown before your patch, or 
> > > you did introduce this behavior?
> > 
> > I've just checked it, it was `Unknown` before this patch as well. 
> > And actually, that is wrong because the array has static storage duration 
> > and as such, we know that it is initialized with zeros according to the C 
> > standard. But, that should be addressed in a follow-up patch (if someone 
> > has the capacity).
> > https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288
> Oh true. I was actually tricked by the `initialization.cpp:38`, where I 
> actually caught this. And in that case, you use `dump()` yet you get 
> `Unknown` as a result. But the issue remains the same.
C++ also states about zero-initialization for static storage lifetime duration: 
http://eel.is/c++draft/basic.start.static#2
I think it will be among my next patches.


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

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

steakhal added inline comments.



Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

martong wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > I'm pretty sure we should not get `Unknown` for simply loading from a 
> > > global variable.
> > > That would imply that loading two times subsequently we could not prove 
> > > that the value remained the same.
> > > But that should remain the same, thus compare equal unless some other 
> > > code modifier that memory region.
> > > 
> > > That could happen for two reasons:
> > > 1) There is a racecondition, and another thread modified that location. 
> > > But that would be UB, so that could not happen.
> > > 2) The global variable is //volatile//, so the hardware might changed its 
> > > content- but this global is not volatile so this does not apply.
> > > 
> > > That being said, this load should have resulted in a //fresh// conjured 
> > > symbolic value instead of //unknown//.
> > > Could you please check if it did result in //unknown// before your patch, 
> > > or you did introduce this behavior?
> > I'm not sure I caught your thoughts.
> > But I think the things is much simplier:
> > `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or `FALSE`. If 
> > we know the constraint of `glob_arr_no_init[2]` we return `TRUE` or 
> > `FALSE`, and `UNKNOWN` otherwise.
> > But in fact I should use `clang_analyzer_dump` here instead of 
> > `clang_analyzer_eval`. This is actually my fault. I'll update this.
> > Could you please check if it did result in unknown before your patch, or 
> > you did introduce this behavior?
> 
> I've just checked it, it was `Unknown` before this patch as well. 
> And actually, that is wrong because the array has static storage duration and 
> as such, we know that it is initialized with zeros according to the C 
> standard. But, that should be addressed in a follow-up patch (if someone has 
> the capacity).
> https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288
Oh true. I was actually tricked by the `initialization.cpp:38`, where I 
actually caught this. And in that case, you use `dump()` yet you get `Unknown` 
as a result. But the issue remains the same.


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Gabor Marton via Phabricator via cfe-commits]

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

Thanks, this looks good to me! But, please wait for an approve from @steakhal 
as well.




Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

ASDenysPetrov wrote:
> steakhal wrote:
> > I'm pretty sure we should not get `Unknown` for simply loading from a 
> > global variable.
> > That would imply that loading two times subsequently we could not prove 
> > that the value remained the same.
> > But that should remain the same, thus compare equal unless some other code 
> > modifier that memory region.
> > 
> > That could happen for two reasons:
> > 1) There is a racecondition, and another thread modified that location. But 
> > that would be UB, so that could not happen.
> > 2) The global variable is //volatile//, so the hardware might changed its 
> > content- but this global is not volatile so this does not apply.
> > 
> > That being said, this load should have resulted in a //fresh// conjured 
> > symbolic value instead of //unknown//.
> > Could you please check if it did result in //unknown// before your patch, 
> > or you did introduce this behavior?
> I'm not sure I caught your thoughts.
> But I think the things is much simplier:
> `clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or `FALSE`. If we 
> know the constraint of `glob_arr_no_init[2]` we return `TRUE` or `FALSE`, and 
> `UNKNOWN` otherwise.
> But in fact I should use `clang_analyzer_dump` here instead of 
> `clang_analyzer_eval`. This is actually my fault. I'll update this.
> Could you please check if it did result in unknown before your patch, or you 
> did introduce this behavior?

I've just checked it, it was `Unknown` before this patch as well. 
And actually, that is wrong because the array has static storage duration and 
as such, we know that it is initialized with zeros according to the C standard. 
But, that should be addressed in a follow-up patch (if someone has the 
capacity).
https://stackoverflow.com/questions/32708161/value-of-uninitialized-elements-in-array-of-c-language/32708288


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

In D106681#3075337 , @steakhal wrote:

> I just wanted you to think about these cases.
> BTW this should work for **L1646**:
>
>   extern const int arr[]; // Incomplete array type
>   void top() {
> (void)arr[42];
>   }

Yes this will work for **L1646**. But I've already added similar tests for this 
line: `glob_array_index4` and `struct_arr_index1` functions in 
//initialization.cpp//. I mean it doesn't matter whether it is an incomplete or 
complete type.




Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

steakhal wrote:
> I'm pretty sure we should not get `Unknown` for simply loading from a global 
> variable.
> That would imply that loading two times subsequently we could not prove that 
> the value remained the same.
> But that should remain the same, thus compare equal unless some other code 
> modifier that memory region.
> 
> That could happen for two reasons:
> 1) There is a racecondition, and another thread modified that location. But 
> that would be UB, so that could not happen.
> 2) The global variable is //volatile//, so the hardware might changed its 
> content- but this global is not volatile so this does not apply.
> 
> That being said, this load should have resulted in a //fresh// conjured 
> symbolic value instead of //unknown//.
> Could you please check if it did result in //unknown// before your patch, or 
> you did introduce this behavior?
I'm not sure I caught your thoughts.
But I think the things is much simplier:
`clang_analyzer_eval` can only produce `UNKNOWN` or `TRUE` or `FALSE`. If we 
know the constraint of `glob_arr_no_init[2]` we return `TRUE` or `FALSE`, and 
`UNKNOWN` otherwise.
But in fact I should use `clang_analyzer_dump` here instead of 
`clang_analyzer_eval`. This is actually my fault. I'll update this.


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

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

steakhal added a comment.

In D106681#3074779 , @ASDenysPetrov 
wrote:

> In D106681#3074678 , @steakhal 
> wrote:
>
>> I think it's fine, maybe `NFCi` is would be slightly more accurate, while 
>> stating the minor behavior change and the reason for doing so in the patch 
>> summary could further improve the visibility of this issue.
>>
>> That being said, since it actually changes some behavior, I'd like to 
>> request some tests covering the following (uncovered lines):
>> L1646, L1689, L1699
>
> For **L1646** currently I'm not sure about the exact test, since it is a 
> heritage of the old code, so it just jumped here from the past. If you could 
> give an example I would appreciate this.
> For **L1689** I'll add it.
> For **L1699** I've added //TODO// cases in D104285 
> .

I just wanted you to think about these cases.
BTW this should work for **L1646**:

  extern const int arr[]; // Incomplete array type
  void top() {
(void)arr[42];
  }

And I'm okay with the rest of the uncovered lines.




Comment at: clang/test/Analysis/initialization.c:103
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}

I'm pretty sure we should not get `Unknown` for simply loading from a global 
variable.
That would imply that loading two times subsequently we could not prove that 
the value remained the same.
But that should remain the same, thus compare equal unless some other code 
modifier that memory region.

That could happen for two reasons:
1) There is a racecondition, and another thread modified that location. But 
that would be UB, so that could not happen.
2) The global variable is //volatile//, so the hardware might changed its 
content- but this global is not volatile so this does not apply.

That being said, this load should have resulted in a //fresh// conjured 
symbolic value instead of //unknown//.
Could you please check if it did result in //unknown// before your patch, or 
you did introduce this behavior?


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

https://reviews.llvm.org/D106681

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

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

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

Fixed redefinition in test file //initialization.cpp//.


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

https://reviews.llvm.org/D106681

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,5 +1,7 @@
 // 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
 
+template 
+void clang_analyzer_dump(T x);
 void clang_analyzer_eval(int);
 
 struct S {
@@ -32,6 +34,10 @@
   auto x = ptr[idx]; // expected-warning{{garbage or undefined}}
 }
 
+void glob_symbolic_index1(int idx) {
+  clang_analyzer_dump(glob_arr1[idx]); // expected-warning{{Unknown}}
+}
+
 int const glob_arr2[4] = {1, 2};
 void glob_ptr_index1() {
   int const *ptr = glob_arr2;
@@ -128,3 +134,15 @@
   // FIXME: Should warn {{garbage or undefined}}.
   auto x = ptr[idx]; // // no-warning
 }
+
+extern const int glob_arr_no_init[10];
+void glob_array_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
+
+struct S2 {
+  static const int arr_no_init[10];
+};
+void struct_arr_index1() {
+  clang_analyzer_eval(S2::arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
Index: clang/test/Analysis/initialization.c
===
--- clang/test/Analysis/initialization.c
+++ clang/test/Analysis/initialization.c
@@ -97,3 +97,8 @@
   // FIXME: Should warn {{garbage or undefined}}.
   int res = glob_arr2[x][y]; // no-warning
 }
+
+const int glob_arr_no_init[10];
+void glob_arr_index4() {
+  clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -437,6 +437,10 @@
 
   RegionBindingsRef removeSubRegionBindings(RegionBindingsConstRef B,
 const SubRegion *R);
+  Optional getConstantValFromConstArrayInitializer(
+  RegionBindingsConstRef B, const VarRegion *VR, const ElementRegion *R);
+  Optional getSValFromInitListExpr(const InitListExpr *ILE,
+ uint64_t Offset, QualType ElemT);
 
 public: // Part of public interface to class.
 
@@ -1625,6 +1629,95 @@
   return Result;
 }
 
+Optional RegionStoreManager::getConstantValFromConstArrayInitializer(
+RegionBindingsConstRef B, const VarRegion *VR, const ElementRegion *R) {
+  assert(R && VR && "Regions should not be null");
+
+  // Check if the containing array has an initialized value that we can trust.
+  // We can trust a const value or a value of a global initializer in main().
+  const VarDecl *VD = VR->getDecl();
+  if (!VD->getType().isConstQualified() &&
+  !R->getElementType().isConstQualified() &&
+  (!B.isMainAnalysis() || !VD->hasGlobalStorage()))
+return None;
+
+  // Array's declaration should have an initializer.
+  const Expr *Init = VD->getAnyInitializer();
+  if (!Init)
+return None;
+
+  // Array's declaration should have ConstantArrayType type, because only this
+  // type contains an array extent.
+  const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(VD->getType());
+  if (!CAT)
+return None;
+
+  // Array should be one-dimensional.
+  // TODO: Support multidimensional array.
+  if (isa(CAT->getElementType())) // is multidimensional
+return None;
+
+  // Array's offset should be a concrete value.
+  // Return Unknown value if symbolic index presented.
+  // FIXME: We also need to take ElementRegions with symbolic
+  // indexes into account.
+  const auto OffsetVal = R->getIndex().getAs();
+  if (!OffsetVal.hasValue())
+return UnknownVal();
+
+  // Check offset for being out of bounds.
+  // C++20 [expr.add] 7.6.6.4 (excerpt):
+  //   If P points to an array element i of an array object x with n
+  //   elements, where i < 0 or i > n, the behavior is undefined.
+  //   Dereferencing is not allowed on the "one past the last
+  //   element", when i == n.
+  // Example:
+  //   const int arr[4] = {1, 2};
+  //   const int *ptr = arr;
+  //   int x0 = ptr[0];  // 1
+  //   int x1 = ptr[1];  // 2
+  //   int x2 = ptr[2];  // 0
+  //   int x3 = ptr[3];  // 0
+  //   int x4 = ptr[4];  // UB
+  //   int x5 = ptr[-1]; // UB
+  const llvm::APSInt  = OffsetVal->getValue();
+  const auto Offset = static_cast(OffsetInt.getExtValue());
+  // Use `getZExtValue` because array extent can not be negative.
+  const 

[PATCH] D106681: [analyzer][NFCI] Move a block from `getBindingForElement` to separate functions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1666
+  if (!OffsetVal.hasValue())
+return UnknownVal();
+

Item 2 from the Summary. This was borrowed from 
`RegionStoreManager::getBindingForFieldOrElementCommon` :
```
   if (hasSymbolicIndex)
  return UnknownVal();
```


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

https://reviews.llvm.org/D106681

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