[PATCH] D80903: [analyzer] Ignore calculated indices of <= 0 in VLASizeChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:114-115
     // Convert the array length to size_t.
     NonLoc IndexLength =
         SVB.evalCast(SizeD, SizeTy, SizeE->getType()).castAs<NonLoc>();
     // Multiply the array length by the element size.
----------------
balazske wrote:
> vabridgers wrote:
> > NoQ wrote:
> > > Do i understand correctly that this cast is the only difference between the value that has been checked and the value on which the assertion is asserted?
> > Yes, looks that way to me. Let's see if Balasz, Gabor, Adam or Kristof responds in the next day or two? Thanks Artem!
> Yes the cast is the difference. Even if this problem is fixed (cast included in `checkVLAIndexSize`) the same problem happens.
> The following code (in `checkVLAIndexSize`) prints `(reg_$0<int a>) + 1 0` before the crash. So there is some difference between `assume` and `getKnownValue`. It can be better to include check of `getKnownValue` in `checkVLAIndexSize` (at least for zero value).
> ```lang=c++
>   // Convert the array length to size_t.
>   NonLoc SizeNL =
>       SVB.evalCast(SizeD, SizeTy, SizeE->getType()).castAs<NonLoc>();
> 
>   // Check if the size is zero.
>   ProgramStateRef StateNotZero, StateZero;
>   std::tie(StateNotZero, StateZero) = State->assume(SizeNL);
> 
>   if (StateZero && !StateNotZero) {
>     reportBug(VLA_Zero, SizeE, StateZero, C);
>     return nullptr;
>   }
> 
>   // From this point on, assume that the size is not zero.
>   State = StateNotZero;
> 
>   if (const llvm::APSInt *IndexLVal = SVB.getKnownValue(State, SizeNL)) {
>     uint64_t L = IndexLVal->getZExtValue();
>     llvm::errs() << SizeNL << " " << L << "\n";
>     assert(L != 0);
>   }
> 
> ```
It might be that `simplifySVal` isn't applied consistently. Like, `assume()` fails to apply it so it fails to see that the value is in fact concrete zero.

Let's remove the assertion for now and add a FIXME to investigate why can't we add it back. Like, there's nothing preventing us from investigating it now but it will take some time whereas crashes are already there and we should fix them to unblock other people.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80903