[PATCH] D111654: [analyzer] Retrieve a value from list initialization of multi-dimensional array declaration.

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

steakhal added a comment.

Please, try to focus on marking inline comments //done// if you accomplished them.
It takes some time to reevaluate them one-by-one on each update.
Aside from that, I'd like to apply these and play with them but I'm frequently having conflicts applying your patches.

Regarding the patch, I think it's in a pretty good shape, given that you fix the regression I pointed in an inline comment.



================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1737-1738
+  //  arr[-2][0]; // UB
+  SmallVector<uint64_t, 2> ConcreteOffsets;
+  ConcreteOffsets.resize(SValOffsets.size());
+  auto ExtentIt = Extents.begin();
----------------
Can this be empty? If so, the subsequent `.front()` is UB.
Put here an assertion if you are sure that it cannot be empty.


================
Comment at: clang/test/Analysis/initialization.cpp:17-23
-int const arr[2][2] = {};
-void arr2init() {
-  int i = 1;
-  // FIXME: Should recognize that it is 0.
-  clang_analyzer_eval(arr[i][0]); // expected-warning{{UNKNOWN}}
-}
-
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > Sorry If I was misunderstood in the previous patches.
> > I think, for this instance, the key is that `arr` is `const`, so this TU is supposed to provide this linker symbol, thus any other definitions would violate the ODR.
> > So, the `FIXME` is actually accurate, and we should report `0` here.
> Right. FALSE expected. And it is fixed with the patch. But this case duplicates some more detailed cases I've added below. So I decide to remove it.
Awesome, thanks!


================
Comment at: clang/test/Analysis/initialization.cpp:56
 
-// TODO: Support multidimensional array.
 int const glob_arr4[4][2] = {};
 void glob_array_index2() {
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > Does this work if the ` = {}` is not present?
> The compiler (AST) doesn't pass you through without an initializer by emitting a warning. But still there is a case without initializer at the end of the file. Yes, it does work.
Okay, thanks!


================
Comment at: clang/test/Analysis/initialization.cpp:89
+  // FIXME: Should be TRUE
+  clang_analyzer_eval(ptr[0] == 3); // expected-warning{{UNDEFINED}}
+  // FIXME: Should be TRUE
----------------
ASDenysPetrov wrote:
> steakhal wrote:
> > Uh, this would be a regression.
> > Accessing `Unknown` is not a bug, unlike accessing `Undefined` - which is a clear indication that we must have had UB in the calculation previously, to get this. So, this is slightly similar to llvm poison on that sense.
> This is a big problem with casts (in `SValBuilder::evalCast`). I've investigated it. Relates to D89055. IMO the solution will take a separate patch stack. I'm going to fix this next.
I think until you do that we should stick to the previous behavior.
Please address this regression, so report `UNKNOWN` for these cases or the correct answer.


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

https://reviews.llvm.org/D111654