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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.


================
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.
----------------
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.


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

https://reviews.llvm.org/D104285