diff --git a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp --- a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp @@ -109,7 +109,7 @@ bool Postfix) const; void handleRandomIncrOrDecr(CheckerContext &C, const Expr *CE, OverloadedOperatorKind Op, const SVal &RetVal, - const SVal &LHS, const SVal &RHS) const; + const SVal &Iterator, const SVal &Amount) const; void handlePtrIncrOrDecr(CheckerContext &C, const Expr *Iterator, OverloadedOperatorKind OK, SVal Offset) const; void handleAdvance(CheckerContext &C, const Expr *CE, SVal RetVal, SVal Iter, @@ -262,20 +262,30 @@ void IteratorModeling::checkPostStmt(const BinaryOperator *BO, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - BinaryOperatorKind OK = BO->getOpcode(); - SVal RVal = State->getSVal(BO->getRHS(), C.getLocationContext()); + const ProgramStateRef State = C.getState(); + const BinaryOperatorKind OK = BO->getOpcode(); + const Expr *const LHS = BO->getLHS(); + const Expr *const RHS = BO->getRHS(); + const SVal LVal = State->getSVal(LHS, C.getLocationContext()); + const SVal RVal = State->getSVal(RHS, C.getLocationContext()); if (isSimpleComparisonOperator(BO->getOpcode())) { - SVal LVal = State->getSVal(BO->getLHS(), C.getLocationContext()); SVal Result = State->getSVal(BO, C.getLocationContext()); handleComparison(C, BO, Result, LVal, RVal, BinaryOperator::getOverloadedOperator(OK)); } else if (isRandomIncrOrDecrOperator(OK)) { - if (!BO->getRHS()->getType()->isIntegralOrEnumerationType()) + // In case of operator+ the iterator can be either on the LHS (eg.: it + 1), + // or on the RHS (eg.: 1 + it). Both cases are modeled. + const bool IsIterOnLHS = BO->getLHS()->getType()->isPointerType(); + const Expr *const &IterExpr = IsIterOnLHS ? LHS : RHS; + const Expr *const &AmountExpr = IsIterOnLHS ? RHS : LHS; + + // The non-iterator side must have an integral or enumeration type. + if (!AmountExpr->getType()->isIntegralOrEnumerationType()) return; - handlePtrIncrOrDecr(C, BO->getLHS(), - BinaryOperator::getOverloadedOperator(OK), RVal); + const SVal &AmountVal = IsIterOnLHS ? RVal : LVal; + handlePtrIncrOrDecr(C, IterExpr, BinaryOperator::getOverloadedOperator(OK), + AmountVal); } } @@ -368,11 +378,24 @@ InstCall->getCXXThisVal(), Call.getArgSVal(0)); return; } - } else { - if (Call.getNumArgs() >= 2 && - Call.getArgExpr(1)->getType()->isIntegralOrEnumerationType()) { + } else if (Call.getNumArgs() >= 2) { + const Expr *FirstArg = Call.getArgExpr(0); + const Expr *SecondArg = Call.getArgExpr(1); + const QualType FirstType = FirstArg->getType(); + const QualType SecondType = SecondArg->getType(); + + if (FirstType->isIntegralOrEnumerationType() || + SecondType->isIntegralOrEnumerationType()) { + // In case of operator+ the iterator can be either on the LHS (eg.: + // it + 1), or on the RHS (eg.: 1 + it). Both cases are modeled. + const bool IsIterFirst = FirstType->isStructureOrClassType(); + const SVal FirstArg = Call.getArgSVal(0); + const SVal SecondArg = Call.getArgSVal(1); + const SVal &Iterator = IsIterFirst ? FirstArg : SecondArg; + const SVal &Amount = IsIterFirst ? SecondArg : FirstArg; + handleRandomIncrOrDecr(C, OrigExpr, Op, Call.getReturnValue(), - Call.getArgSVal(0), Call.getArgSVal(1)); + Iterator, Amount); return; } } @@ -564,35 +587,35 @@ C.addTransition(State); } -void IteratorModeling::handleRandomIncrOrDecr(CheckerContext &C, - const Expr *CE, +void IteratorModeling::handleRandomIncrOrDecr(CheckerContext &C, const Expr *CE, OverloadedOperatorKind Op, const SVal &RetVal, - const SVal &LHS, - const SVal &RHS) const { + const SVal &Iterator, + const SVal &Amount) const { // Increment or decrement the symbolic expressions which represents the // position of the iterator auto State = C.getState(); - const auto *Pos = getIteratorPosition(State, LHS); + const auto *Pos = getIteratorPosition(State, Iterator); if (!Pos) return; - const auto *value = &RHS; - SVal val; - if (auto loc = RHS.getAs()) { - val = State->getRawSVal(*loc); - value = &val; + const auto *Value = &Amount; + SVal Val; + if (auto LocAmount = Amount.getAs()) { + Val = State->getRawSVal(*LocAmount); + Value = &Val; } - auto &TgtVal = (Op == OO_PlusEqual || Op == OO_MinusEqual) ? LHS : RetVal; + const auto &TgtVal = + (Op == OO_PlusEqual || Op == OO_MinusEqual) ? Iterator : RetVal; // `AdvancedState` is a state where the position of `LHS` is advanced. We // only need this state to retrieve the new position, but we do not want // to change the position of `LHS` (in every case). - auto AdvancedState = advancePosition(State, LHS, Op, *value); + auto AdvancedState = advancePosition(State, Iterator, Op, *Value); if (AdvancedState) { - const auto *NewPos = getIteratorPosition(AdvancedState, LHS); + const auto *NewPos = getIteratorPosition(AdvancedState, Iterator); assert(NewPos && "Iterator should have position after successful advancement"); diff --git a/clang/test/Analysis/iterator-modeling.cpp b/clang/test/Analysis/iterator-modeling.cpp --- a/clang/test/Analysis/iterator-modeling.cpp +++ b/clang/test/Analysis/iterator-modeling.cpp @@ -149,7 +149,7 @@ clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning-re {{$v.end(){{$}}}} } -void plus(const std::vector &v) { +void plus_lhs(const std::vector &v) { auto i1 = v.begin(); clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()"); @@ -161,7 +161,19 @@ clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning-re{{$v.begin() + 2{{$}}}} } -void plus_negative(const std::vector &v) { +void plus_rhs(const std::vector &v) { + auto i1 = v.begin(); + + clang_analyzer_denote(clang_analyzer_container_begin(v), "$v.begin()"); + + auto i2 = 2 + i1; + + clang_analyzer_eval(clang_analyzer_iterator_container(i2) == &v); // expected-warning{{TRUE}} + clang_analyzer_express(clang_analyzer_iterator_position(i1)); // expected-warning-re{{$v.begin(){{$}}}} + clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning-re{{$v.begin() + 2{{$}}}} +} + +void plus_lhs_negative(const std::vector &v) { auto i1 = v.end(); clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()"); @@ -173,6 +185,18 @@ clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning-re {{$v.end() - 2{{$}}}} } +void plus_rhs_negative(const std::vector &v) { + auto i1 = v.end(); + + clang_analyzer_denote(clang_analyzer_container_end(v), "$v.end()"); + + auto i2 = (-2) + i1; + + clang_analyzer_eval(clang_analyzer_iterator_container(i2) == &v); // expected-warning{{TRUE}} + clang_analyzer_express(clang_analyzer_iterator_position(i1)); // expected-warning-re {{$v.end(){{$}}}} + clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning-re {{$v.end() - 2{{$}}}} +} + void minus(const std::vector &v) { auto i1 = v.end(); @@ -1955,7 +1979,7 @@ i -= n; // no-crash } -void plus_ptr_iterator(const cont_with_ptr_iterator &c) { +void plus_lhs_ptr_iterator(const cont_with_ptr_iterator &c) { auto i1 = c.begin(); clang_analyzer_denote(clang_analyzer_container_begin(c), "$c.begin()"); @@ -1967,6 +1991,18 @@ clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$c.begin() + 2}} } +void plus_rhs_ptr_iterator(const cont_with_ptr_iterator &c) { + auto i1 = c.begin(); + + clang_analyzer_denote(clang_analyzer_container_begin(c), "$c.begin()"); + + auto i2 = 2 + i1; + + clang_analyzer_eval(clang_analyzer_iterator_container(i2) == &c); // expected-warning{{TRUE}} + clang_analyzer_express(clang_analyzer_iterator_position(i1)); // expected-warning{{$c.begin()}} + clang_analyzer_express(clang_analyzer_iterator_position(i2)); // expected-warning{{$c.begin() + 2}} +} + void minus_ptr_iterator(const cont_with_ptr_iterator &c) { auto i1 = c.end();