donat.nagy accepted this revision. donat.nagy added a comment. This revision is now accepted and ready to land.
Thanks for the updates! I didn't spot any important issue, so I'm accepting this commit, but let's wait for the opinion of @steakhal before merging this. In D158707#4613955 <https://reviews.llvm.org/D158707#4613955>, @danix800 wrote: > In D158707#4613743 <https://reviews.llvm.org/D158707#4613743>, @donat.nagy > wrote: > >> [...] Does this commit fix the "root" of the issue by eliminating some >> misuse of correctly implemented (but perhaps surprising) `SVal` / `APSInt` >> arithmetic; or is there an underlying bug in the `SVal` / `APSInt` >> arithmetic which is just avoided (and not eliminated) by this commit? In the >> latter case, what obstacles prevent us from fixing the root cause? > > For the observed signed compared to unsigned unexpectation from ArrayBoundV2, > the constraint manager does give the CORRECT result. > > It's a misuse of the constraint API, mainly caused by unexpected unsigned > extent > set by memory modeling. Actually `ArrayBoundV2::getSimplifiedOffsets()` has > clear > comment that this `signed <-> unsigned` comparison is problematic. > > // TODO: once the constraint manager is smart enough to handle non > simplified > // symbolic expressions remove this function. Note that this can not be > used in > // the constraint manager as is, since this does not handle overflows. It is > // safe to assume, however, that memory offsets will not overflow. > // NOTE: callers of this function need to be aware of the effects of > overflows > // and signed<->unsigned conversions! > static std::pair<NonLoc, nonloc::ConcreteInt> > getSimplifiedOffsets(NonLoc offset, nonloc::ConcreteInt extent, > SValBuilder &svalBuilder) { In fact, the "NOTE:" comment was added by my patch D135375 <https://reviews.llvm.org/D135375> (which eliminated lots of false positives caused by a situation when the argument `offset` was unsigned), but I was still confused by this new bug. (Where the other argument `extent` was unsigned and this led to incorrect conclusions like "`len` cannot be larger than `3u`, so `len` cannot be `-1`, because `-1` is larger than `3u`" 🙃 .) Thanks for troubleshooting this issue! ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:34 + if (auto SSize = + SVB.convertToArrayIndex(*Size).getAs<DefinedOrUnknownSVal>()) + return *SSize; ---------------- I think it's a good convention if `getDynamicExtent()` will always return concrete values as `ArrayIndexTy`. (If I didn't miss some very obscure case, then this will be true when this commit is merged.) ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:49 + return UnknownVal(); + MR = MR->StripCasts(); + ---------------- Note that `StripCasts()` is overzealous and strips array indexing when the index is zero. For example if you have a rectangular matrix `int m[6][8];` then this function would (correctly) say that the element count of `m[1]` is 8, but it would claim that the element count of `m[0]` is 6 (because it strips the 'cast' and calculates the element count of `m`). This is caused by the hacky "solution" that casts are represented by `ElementRegion{original region, 0, new type}` which cannot be distinguished from a real element access where the index happens to be 0. (This is just a public service announcement, this already affects e.g. `getDynamicExtent` and I don't think that you can find a local solution. For more information, see also https://github.com/llvm/llvm-project/issues/42709 which plans to refactor the memory model.) ================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:72-75 + auto ElementCount = + SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType()) + .getAs<DefinedOrUnknownSVal>(); + return ElementCount ? *ElementCount : UnknownVal(); ---------------- Perhaps use the method `value_or` of std::optional? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158707/new/ https://reviews.llvm.org/D158707 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
