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