[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.

This is an important step towards better handling of global initializer expressions.
I'm looking forward to it. Although, I have concerns to address.



================
Comment at: clang/lib/AST/Type.cpp:141-143
+/// Return an array with extents of the declared array type.
+///
+/// E.g. for `const int x[1][2][3];` returns {1,2,3}.
----------------
These comments should be in the header file instead.


================
Comment at: clang/lib/AST/Type.cpp:149
+    Extents.push_back(CAT->getSize().getZExtValue());
+  } while ((CAT = dyn_cast<ConstantArrayType>(CAT->getElementType())));
+  return Extents;
----------------
I suspect this not gonna look through typedefs.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1643-1654
+  const VarRegion *VR = dyn_cast<VarRegion>(R->getSuperRegion());
+  SmallVector<SVal, 2> SValOffsets;
+  SValOffsets.push_back(R->getIndex());
+  if (const ElementRegion *ER = dyn_cast<ElementRegion>(R->getSuperRegion())) {
+    const ElementRegion *LastER = nullptr;
+    do {
+      SValOffsets.push_back(ER->getIndex());
----------------
I dislike this part of code. It has side effects all over the place.
Please use immediately called lambdas to describe your intentions in a more //pure// manner.

Besides that, when I see `ElementRegions`, I'm always scared. They sometimes represent reinterpret casts.
Since you have dealt with so many casts and region store stuff in the past, I'm assuming you have thought about this here.
Although, I'm still scared slightly, and I'd like to have evidence of this thought process. Likely some assertions or at least some comments? Could you @ASDenysPetrov elaborate on this?


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1728
+  // Reverse `SValOffsets` to make it consistent with `Extents`.
+  for (SVal &V : llvm::reverse(SValOffsets)) {
+    if (auto CI = V.getAs<nonloc::ConcreteInt>()) {
----------------
I would rather take it by value. They are designed so.
A mutable reference is always a bad sign anyway.
BTW for a brief moment I thought `reverse()` returns a temporary, but it doesn't, so we don't dangle at least.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1734
+      // be negative, but `I` can NOT (because it's an uint64_t).
+      if (Offset < 0 || I >= *(ExtentIt++))
+        return UndefinedVal();
----------------
Please use the `isNegative()`. And don't have sideffects in conditions. advance the iterator at the end of the `if`.
I also think you can eliminate the `I` variable.
That way the `getExtValue()` would be guarded by the negativity check. I know that your code would behave the same, but I would find that phrasing slightly easier to follow.
This way you could even spare the 2 comment lines.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1738
+      *(OffsetIt++) = I;
+    } else
+      // Symbolic index presented. Return Unknown value.
----------------
This is probably a rare case when a `continue` is preferred instead of the `else` statement.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1777
 
-  // C++20 [dcl.init.string] 9.4.2.1:
-  //   An array of ordinary character type [...] can be initialized by [...]
-  //   an appropriately-typed string-literal enclosed in braces.
-  // Example:
-  //   const char arr[] = { "abc" };
-  if (ILE->isStringLiteralInit())
-    if (const auto *SL = dyn_cast<StringLiteral>(ILE->getInit(0)))
-      return getSValFromStringLiteral(SL, Offset, ElemT);
-
-  // C++20 [expr.add] 9.4.17.5 (excerpt):
-  //   i-th array element is value-initialized for each k < i ≤ n,
-  //   where k is an expression-list size and n is an array extent.
-  if (Offset >= ILE->getNumInits())
-    return svalBuilder.makeZeroVal(ElemT);
-
-  // Return a constant value, if it is presented.
-  // FIXME: Support other SVals.
-  const Expr *E = ILE->getInit(Offset);
-  return svalBuilder.getConstantVal(E);
+  for (auto Offset : ConcreteOffsets) {
+    // C++20 [dcl.init.string] 9.4.2.1:
----------------
Just spell out `uint64_t` here. We don't win much by using `auto`.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1795-1800
+    if (const auto *IL = dyn_cast<InitListExpr>(E))
+      ILE = IL;
+    else
+      // Return a constant value, if it is presented.
+      // FIXME: Support other SVals.
+      return svalBuilder.getConstantVal(E);
----------------
Please, restructure this. Probably negating the condition and returning on that path would do the job.


================
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}}
-}
-
----------------
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.


================
Comment at: clang/test/Analysis/initialization.cpp:56
 
-// TODO: Support multidimensional array.
 int const glob_arr4[4][2] = {};
 void glob_array_index2() {
----------------
Does this work if the ` = {}` is not present?


================
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
----------------
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.


================
Comment at: clang/test/Analysis/initialization.cpp:105
   int idx = 42;
-  // FIXME: Should warn {{garbage or undefined}}.
-  auto x = ptr[idx]; // // no-warning
+  auto x = ptr[idx]; // // expected-warning{{garbage or undefined}}
 }
----------------
typo


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

https://reviews.llvm.org/D111654