From 931a426f3341d5cdbb5f8c92f7d56f7647a013f8 Mon Sep 17 00:00:00 2001 From: Petar Jovanovic Date: Thu, 7 Mar 2019 15:50:52 +0000 Subject: [PATCH] [analyzer] handle modification of vars inside an expr with comma operator We should track mutation of a variable within a comma operator expression. Current code in ExprMutationAnalyzer does not handle it. This will handle cases like: (a, b) ++ < == b is modified (a, b) = c < == b is modifed Patch by Djordje Todorovic. Differential Revision: https://reviews.llvm.org/D58894 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@355605 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Expr.h | 3 + lib/Analysis/ExprMutationAnalyzer.cpp | 46 ++++-- .../Analysis/ExprMutationAnalyzerTest.cpp | 131 ++++++++++++++++++ 3 files changed, 167 insertions(+), 13 deletions(-) diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h index e0b20f74dd..a43c61935e 100644 --- a/include/clang/AST/Expr.h +++ b/include/clang/AST/Expr.h @@ -3403,6 +3403,9 @@ public: static bool isComparisonOp(Opcode Opc) { return Opc >= BO_Cmp && Opc<=BO_NE; } bool isComparisonOp() const { return isComparisonOp(getOpcode()); } + static bool isCommaOp(Opcode Opc) { return Opc == BO_Comma; } + bool isCommaOp() const { return isCommaOp(getOpcode()); } + static Opcode negateComparisonOp(Opcode Opc) { switch (Opc) { default: diff --git a/lib/Analysis/ExprMutationAnalyzer.cpp b/lib/Analysis/ExprMutationAnalyzer.cpp index f8d75f4778..fb5a139e82 100644 --- a/lib/Analysis/ExprMutationAnalyzer.cpp +++ b/lib/Analysis/ExprMutationAnalyzer.cpp @@ -24,6 +24,18 @@ AST_MATCHER_P(CXXForRangeStmt, hasRangeStmt, return InnerMatcher.matches(*Range, Finder, Builder); } +AST_MATCHER_P(Expr, maybeEvalCommaExpr, + ast_matchers::internal::Matcher, InnerMatcher) { + const Expr* Result = &Node; + while (const auto *BOComma = + dyn_cast_or_null(Result->IgnoreParens())) { + if (!BOComma->isCommaOp()) + break; + Result = BOComma->getRHS(); + } + return InnerMatcher.matches(*Result, Finder, Builder); +} + const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxTypeidExpr; @@ -193,24 +205,28 @@ const Stmt *ExprMutationAnalyzer::findDeclPointeeMutation( const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) { // LHS of any assignment operators. const auto AsAssignmentLhs = - binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp))); + binaryOperator(isAssignmentOperator(), + hasLHS(maybeEvalCommaExpr(equalsNode(Exp)))); // Operand of increment/decrement operators. const auto AsIncDecOperand = unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")), - hasUnaryOperand(equalsNode(Exp))); + hasUnaryOperand(maybeEvalCommaExpr(equalsNode(Exp)))); // Invoking non-const member function. // A member function is assumed to be non-const when it is unresolved. const auto NonConstMethod = cxxMethodDecl(unless(isConst())); const auto AsNonConstThis = - expr(anyOf(cxxMemberCallExpr(callee(NonConstMethod), on(equalsNode(Exp))), + expr(anyOf(cxxMemberCallExpr(callee(NonConstMethod), + on(maybeEvalCommaExpr(equalsNode(Exp)))), cxxOperatorCallExpr(callee(NonConstMethod), - hasArgument(0, equalsNode(Exp))), + hasArgument(0, + maybeEvalCommaExpr(equalsNode(Exp)))), callExpr(callee(expr(anyOf( - unresolvedMemberExpr(hasObjectExpression(equalsNode(Exp))), + unresolvedMemberExpr( + hasObjectExpression(maybeEvalCommaExpr(equalsNode(Exp)))), cxxDependentScopeMemberExpr( - hasObjectExpression(equalsNode(Exp))))))))); + hasObjectExpression(maybeEvalCommaExpr(equalsNode(Exp)))))))))); // Taking address of 'Exp'. // We're assuming 'Exp' is mutated as soon as its address is taken, though in @@ -220,10 +236,11 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) { unaryOperator(hasOperatorName("&"), // A NoOp implicit cast is adding const. unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))), - hasUnaryOperand(equalsNode(Exp))); + hasUnaryOperand(maybeEvalCommaExpr(equalsNode(Exp)))); const auto AsPointerFromArrayDecay = castExpr(hasCastKind(CK_ArrayToPointerDecay), - unless(hasParent(arraySubscriptExpr())), has(equalsNode(Exp))); + unless(hasParent(arraySubscriptExpr())), + has(maybeEvalCommaExpr(equalsNode(Exp)))); // Treat calling `operator->()` of move-only classes as taking address. // These are typically smart pointers with unique ownership so we treat // mutation of pointee as mutation of the smart pointer itself. @@ -231,7 +248,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) { cxxOperatorCallExpr(hasOverloadedOperatorName("->"), callee(cxxMethodDecl(ofClass(isMoveOnly()), returns(nonConstPointerType()))), - argumentCountIs(1), hasArgument(0, equalsNode(Exp))); + argumentCountIs(1), + hasArgument(0, maybeEvalCommaExpr(equalsNode(Exp)))); // Used as non-const-ref argument when calling a function. // An argument is assumed to be non-const-ref when the function is unresolved. @@ -239,7 +257,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) { // findFunctionArgMutation which has additional smarts for handling forwarding // references. const auto NonConstRefParam = forEachArgumentWithParam( - equalsNode(Exp), parmVarDecl(hasType(nonConstReferenceType()))); + maybeEvalCommaExpr(equalsNode(Exp)), + parmVarDecl(hasType(nonConstReferenceType()))); const auto NotInstantiated = unless(hasDeclaration(isInstantiated())); const auto AsNonConstRefArg = anyOf( callExpr(NonConstRefParam, NotInstantiated), @@ -247,8 +266,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) { callExpr(callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(), cxxDependentScopeMemberExpr(), hasType(templateTypeParmType())))), - hasAnyArgument(equalsNode(Exp))), - cxxUnresolvedConstructExpr(hasAnyArgument(equalsNode(Exp)))); + hasAnyArgument(maybeEvalCommaExpr(equalsNode(Exp)))), + cxxUnresolvedConstructExpr(hasAnyArgument(maybeEvalCommaExpr(equalsNode(Exp))))); // Captured by a lambda by reference. // If we're initializing a capture with 'Exp' directly then we're initializing @@ -261,7 +280,8 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) { // For returning by value there will be an ImplicitCastExpr . // For returning by const-ref there will be an ImplicitCastExpr (for // adding const.) - const auto AsNonConstRefReturn = returnStmt(hasReturnValue(equalsNode(Exp))); + const auto AsNonConstRefReturn = returnStmt(hasReturnValue( + maybeEvalCommaExpr(equalsNode(Exp)))); const auto Matches = match(findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, AsNonConstThis, diff --git a/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/unittests/Analysis/ExprMutationAnalyzerTest.cpp index 871c915149..c8ddc48fa7 100644 --- a/unittests/Analysis/ExprMutationAnalyzerTest.cpp +++ b/unittests/Analysis/ExprMutationAnalyzerTest.cpp @@ -881,6 +881,137 @@ TEST(ExprMutationAnalyzerTest, CastToConstRef) { EXPECT_FALSE(isMutated(Results, AST.get())); } +TEST(ExprMutationAnalyzerTest, CommaExprWithAnAssigment) { + const auto AST = + buildASTFromCodeWithArgs("void f() { int x; int y; (x, y) = 5; }", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("y")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprWithDecOp) { + const auto AST = + buildASTFromCodeWithArgs("void f() { int x; int y; (x, y)++; }", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("y")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprWithNonConstMemberCall) { + const auto AST = + buildASTFromCodeWithArgs("class A { public: int mem; void f() { mem ++; } };" + "void fn() { A o1, o2; (o1, o2).f(); }", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("o2")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprWithConstMemberCall) { + const auto AST = + buildASTFromCodeWithArgs("class A { public: int mem; void f() const { } };" + "void fn() { A o1, o2; (o1, o2).f(); }", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("o2")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprWithCallExpr) { + const auto AST = + buildASTFromCodeWithArgs("class A { public: int mem; void f(A &O1) {} };" + "void fn() { A o1, o2; o2.f((o2, o1)); }", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("o1")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprWithCallUnresolved) { + auto AST = buildASTFromCodeWithArgs( + "template struct S;" + "template void f() { S s; int x, y; s.mf((y, x)); }", + {"-fno-delayed-template-parsing -Wno-unused-value"}); + auto Results = + match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); + + AST = buildASTFromCodeWithArgs( + "template void f(T t) { int x, y; g(t, (y, x)); }", + {"-fno-delayed-template-parsing -Wno-unused-value"}); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprParmRef) { + const auto AST = + buildASTFromCodeWithArgs("class A { public: int mem;};" + "extern void fn(A &o1);" + "void fn2 () { A o1, o2; fn((o2, o1)); } ", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("o1")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprWithAmpersandOp) { + const auto AST = + buildASTFromCodeWithArgs("class A { public: int mem;};" + "void fn () { A o1, o2;" + "void *addr = &(o2, o1); } ", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("o1")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprAsReturnAsValue) { + auto AST = buildASTFromCodeWithArgs("int f() { int x, y; return (x, y); }", + {"-Wno-unused-value"}); + auto Results = + match(withEnclosingCompound(declRefTo("y")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaEpxrAsReturnAsNonConstRef) { + const auto AST = + buildASTFromCodeWithArgs("int& f() { int x, y; return (y, x); }", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprAsArrayToPointerDecay) { + const auto AST = + buildASTFromCodeWithArgs("void g(int*); " + "void f() { int x[2], y[2]; g((y, x)); }", + {"-Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + +TEST(ExprMutationAnalyzerTest, CommaExprAsUniquePtr) { + const std::string UniquePtrDef = + "template struct UniquePtr {" + " UniquePtr();" + " UniquePtr(const UniquePtr&) = delete;" + " T& operator*() const;" + " T* operator->() const;" + "};"; + const auto AST = buildASTFromCodeWithArgs( + UniquePtrDef + "template void f() " + "{ UniquePtr x; UniquePtr y;" + " (y, x)->mf(); }", + {"-fno-delayed-template-parsing -Wno-unused-value"}); + const auto Results = + match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); +} + TEST(ExprMutationAnalyzerTest, LambdaDefaultCaptureByValue) { const auto AST = buildASTFromCode("void f() { int x; [=]() { x; }; }"); const auto Results = -- 2.40.0