[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

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

steakhal added a comment.

Looks great.



================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1636-1640
+  // Technically, only i == length is guaranteed to be null.
+  // However, such overflows should be caught before reaching this point;
+  // the only time such an access would be made is if a string literal was
+  // used to initialize a larger array.
+  // FIXME: Take array dimension into account to prevent exceeding its size.
----------------
This seems like a huge hack. Do we really need this? I think we should account for this case at the initialization of the mentioned array...
Leave it as-is right now, but eh, ugly.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1641
+  // FIXME: Take array dimension into account to prevent exceeding its size.
+  const int64_t I = Idx.getExtValue();
+  uint32_t Code =
----------------
You could use the `uint64_t` type here, and spare the subsequent explicit cast. This operation would be safe since `Idx` must be non-negative here.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1677-1682
+        if (const auto *SL = dyn_cast<StringLiteral>(Init)) {
+          if (auto CI = R->getIndex().getAs<nonloc::ConcreteInt>()) {
+            const llvm::APSInt &Idx = CI->getValue();
+            QualType ElemT = R->getElementType();
+            return getSValFromStringLiteralByIndex(SL, Idx, ElemT);
+          }
----------------
So, your patch is NFC except for this part, which applies the very same logic for global initializers.
Am'I right?


================
Comment at: clang/test/Analysis/initialization.cpp:132
+
+char const glob_arr6[5] = "123";
+void glob_array_index4() {
----------------
Ah, it's somewhat confusing.
At first, when I looked at it, I assumed that this array has `6` elements as its name suggests.
But it has actually 5 elements.


================
Comment at: clang/test/Analysis/initialization.cpp:156-157
+void glob_invalid_index7() {
+  int idx = -42;
+  auto x = glob_arr6[idx]; // expected-warning{{garbage or undefined}}
+}
----------------
You could inline the `-42` without changing any expected behavior.
It would make the test terser IMO. The same applies to the other case.


================
Comment at: clang/test/Analysis/initialization.cpp:160-166
+// TODO: Support multidimensional array.
+void glob_invalid_index8() {
+  const char *ptr = glob_arr6;
+  int idx = 42;
+  // FIXME: Should warn {{garbage or undefined}}.
+  auto x = ptr[idx]; // no-warning
+}
----------------
I'm not sure if I follow. The `TODO` implies to me that this case is about //multidimensional  array//s, but it's actually not.
`glob_arr6` is of type `const char[5]`
Could you clarify this?
BTW, at first glance, the gist of this case is the same as the `glob_invalid_index7`.
Why does this behave differently? I'm puzzled.


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

https://reviews.llvm.org/D107339