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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
