[PATCH] D104285: [analyzer] Retrieve a value from list initialization of constant array declaration in a global scope.

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696-1697
+              const auto I = static_cast<uint64_t>(Idx.getExtValue());
+              // Use `getZExtValue` because array extent can not be negative.
+              const uint64_t Extent = CAT->getSize().getZExtValue();
+              // Check for `Idx < 0`, NOT for `I < 0`, because `Idx` CAN be
----------------
Do you think it would make sense to `assert(CAT->getSize().isSigned())`?


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1696
+              const llvm::APSInt &Idx = CI->getValue();
+              const uint64_t I = static_cast<uint64_t>(Idx.getExtValue());
+              // Use `getZExtValue` because array extent can not be negative.
----------------
ASDenysPetrov wrote:
> martong wrote:
> > ASDenysPetrov wrote:
> > > ASDenysPetrov wrote:
> > > > martong wrote:
> > > > > aaron.ballman wrote:
> > > > > > 
> > > > > This `static_cast` seems to be dangerous to me, it might overflow. Can't we compare `Idx` directly to `Extent`? I see that `Idx` is an `APSint` and `Extent` is an `APInt`, but  I think we should be able to handle the comparison on the APInt level b/c that takes care of the signedness. And the overflow situation should be handled as well properly with `APInt`, given from it's name "arbitrary precision int". In this sense I don't see why do we need `I` at all.
> > > > We can't get rid of `I` because we use it below anyway in `I >= InitList->getNumInits()` and `InitList->getInit(I)`.
> > > > I couldn't find any appropriate function to compare without additional checking for signedness or bit-width adjusting.
> > > > I'll try to improve this snippet.
> > > This is not dangerous because we check for negatives separately in `Idx < 0`, so we can be sure that `I` is positive while `I >= Extent`. Unfortunately, I didn't find any suitable way to compare `APSint` //of unknown sign and bitwidth// with //signless// `APInt` without adding another checks for sign and bitwidth conversions. In this way I prefer the currect condition `I >= Extent`.
> > I think it would be possible to use `bool llvm::APInt::uge` that does an Unsigned greater or equal comparison. Or you could use `sge` for the signed comparison. Also, both have overload that take another APInt as parameter.
> I considered them. First of all choosing between `uge` and `sge` we additionally need to check for signedness. Moreover, these functions require bitwidth to be equal. Thus we need additional checks and transformations. I found this approach too verbose. Mine one seems to me simpler and works under natural rules of comparison.
Okay, thanks for thinking it through and answering my concerns!


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

https://reviews.llvm.org/D104285