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