[PATCH] D83295: [Analyzer] Hotfix for various crashes in iterator checkers
Kristóf Umann via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 9 07:36:35 PDT 2020
Szelethus accepted this revision.
Szelethus added a comment.
LGTM, but if we knowingly a subpar solution, we should make that clear in the surrounding code. I know the followup patch is around the corner, but just to be sure.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:275-276
} else if (isRandomIncrOrDecrOperator(OK)) {
+ if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+ return;
handlePtrIncrOrDecr(C, BO->getLHS(),
----------------
baloghadamsoftware wrote:
> gamesh411 wrote:
> > Szelethus wrote:
> > > This doesn't look symmetrical. How does this patch interact with D83190?
> > I see that patch D83190 will need not only to be rebased, but modified slightly to take this early checking into account. I will refer to this review over there.
> This //is// not symmetrical. Since this patch is a hotfix for three bugs, all of them cause crashes it is more urgent to land. The other patch should be rebased on it.
Please add a FIXME before commiting, if we knowingly add a hack.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:466-470
+ // If the value for which we just tried to set a new iterator position is
+ // an `SVal`for which no iterator position can be set then the setting was
+ // unsuccessful. We cannot handle the comparison in this case.
+ if (!LPos || !RPos)
+ return;
----------------
Is this always okay? Shouldn't we check what the reason was behind the failure to retrieve the position? Is a `TODO: ` at the very least appropriate?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorRangeChecker.cpp:172-173
SVal RVal = State->getSVal(BO->getRHS(), C.getLocationContext());
+ if (!BO->getRHS()->getType()->isIntegralOrEnumerationType())
+ return;
verifyRandomIncrOrDecr(C, BinaryOperator::getOverloadedOperator(OK), LVal,
----------------
FIXME:
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83295/new/
https://reviews.llvm.org/D83295
More information about the cfe-commits
mailing list