shuaiwang updated this revision to Diff 166068. shuaiwang marked 7 inline comments as done. shuaiwang added a comment.
[WIP] Addressed some of review comments. Repository: rC Clang https://reviews.llvm.org/D52219 Files: include/clang/Analysis/Analyses/ExprMutationAnalyzer.h lib/Analysis/ExprMutationAnalyzer.cpp unittests/Analysis/ExprMutationAnalyzerTest.cpp
Index: unittests/Analysis/ExprMutationAnalyzerTest.cpp =================================================================== --- unittests/Analysis/ExprMutationAnalyzerTest.cpp +++ unittests/Analysis/ExprMutationAnalyzerTest.cpp @@ -52,16 +52,34 @@ bool isMutated(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) { const auto *const S = selectFirst<Stmt>("stmt", Results); const auto *const E = selectFirst<Expr>("expr", Results); + assert(S && E); return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E); } +bool isPointeeMutated(const SmallVectorImpl<BoundNodes> &Results, + ASTUnit *AST) { + const auto *const S = selectFirst<Stmt>("stmt", Results); + const auto *const E = selectFirst<Expr>("expr", Results); + assert(S && E); + return ExprMutationAnalyzer(*S, AST->getASTContext()).isPointeeMutated(E); +} + SmallVector<std::string, 1> mutatedBy(const SmallVectorImpl<BoundNodes> &Results, ASTUnit *AST) { + const Stmt *(ExprMutationAnalyzer::*MutationFinder)(const Expr *) = + &ExprMutationAnalyzer::findMutation; + const Stmt *(ExprMutationAnalyzer::*PointeeMutationFinder)(const Expr *) = + &ExprMutationAnalyzer::findPointeeMutation; const auto *const S = selectFirst<Stmt>("stmt", Results); SmallVector<std::string, 1> Chain; ExprMutationAnalyzer Analyzer(*S, AST->getASTContext()); for (const auto *E = selectFirst<Expr>("expr", Results); E != nullptr;) { - const Stmt *By = Analyzer.findMutation(E); + auto Finder = MutationFinder; + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) { + if (DRE->getNameInfo().getAsString()[0] == 'p') + Finder = PointeeMutationFinder; + } + const Stmt *By = (Analyzer.*Finder)(E); std::string buffer; llvm::raw_string_ostream stream(buffer); By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy()); @@ -100,10 +118,14 @@ } // namespace TEST(ExprMutationAnalyzerTest, Trivial) { - const auto AST = buildASTFromCode("void f() { int x; x; }"); - const auto Results = + auto AST = buildASTFromCode("void f() { int x; x; }"); + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_FALSE(isMutated(Results, AST.get())); + + AST = buildASTFromCode("void f() { const int x = 0; x; }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); } class AssignmentTest : public ::testing::TestWithParam<std::string> {}; @@ -134,41 +156,104 @@ Values("++x", "--x", "x++", "x--"), ); TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) { - const auto AST = buildASTFromCode( + auto AST = buildASTFromCode( "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }"); - const auto Results = + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_TRUE(isMutated(Results, AST.get())); + EXPECT_FALSE(isPointeeMutated(Results, AST.get())); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()")); + + AST = buildASTFromCode( + "void f() { struct Foo { void mf(); }; Foo *p; p->mf(); }"); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_TRUE(isPointeeMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()")); } TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) { auto AST = buildASTFromCodeWithArgs( "struct X { template <class T> void mf(); };" - "template <class T> void f() { X x; x.mf<T>(); }", + "template <class T> void f() { X x; x.mf<T>(); }" + "template <class T> void g() { X *p; p->mf<T>(); }", {"-fno-delayed-template-parsing"}); auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf<T>()")); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf<T>()")); - AST = buildASTFromCodeWithArgs("template <class T> void f() { T x; x.mf(); }", - {"-fno-delayed-template-parsing"}); + AST = + buildASTFromCodeWithArgs("template <class T> void f() { T x; x.mf(); }" + "template <class T> void g() { T *p; p->mf(); }", + {"-fno-delayed-template-parsing"}); Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()")); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()")); AST = buildASTFromCodeWithArgs( "template <class T> struct X;" - "template <class T> void f() { X<T> x; x.mf(); }", + "template <class T> void f() { X<T> x; x.mf(); }" + "template <class T> void g() { X<T> *p; p->mf(); }", {"-fno-delayed-template-parsing"}); Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()")); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()")); + + AST = buildASTFromCodeWithArgs( + "struct X { template <class T> void mf() const; };" + "template <class T> void f() { const X x; x.mf<T>(); }" + "template <class T> void g() { const X *p; p->mf<T>(); }", + {"-fno-delayed-template-parsing"}); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isPointeeMutated(Results, AST.get())); + + AST = buildASTFromCodeWithArgs( + "template <class T> void f() { const T x; x.mf(); }" + "template <class T> void g() { const T *p; p->mf(); }", + {"-fno-delayed-template-parsing"}); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isPointeeMutated(Results, AST.get())); + + AST = buildASTFromCodeWithArgs( + "template <class T> struct X;" + "template <class T> void f() { const X<T> x; x.mf(); }" + "template <class T> void g() { const X<T> *p; p->mf(); }", + {"-fno-delayed-template-parsing"}); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isPointeeMutated(Results, AST.get())); } TEST(ExprMutationAnalyzerTest, ConstMemberFunc) { - const auto AST = buildASTFromCode( + auto AST = buildASTFromCode( "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }"); - const auto Results = + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_FALSE(isMutated(Results, AST.get())); + + AST = buildASTFromCode( + "void f() { struct Foo { void mf() const; }; const Foo x; x.mf(); }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); + + AST = buildASTFromCode( + "void f() { struct Foo { void mf() const; }; Foo *p; p->mf(); }"); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isPointeeMutated(Results, AST.get())); + + AST = buildASTFromCode( + "void f() { struct Foo { void mf() const; }; const Foo *p; p->mf(); }"); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isPointeeMutated(Results, AST.get())); } TEST(ExprMutationAnalyzerTest, NonConstOperator) { @@ -193,9 +278,11 @@ match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_FALSE(isMutated(Results, AST.get())); - AST = buildASTFromCode("void g(int*); void f() { int* x; g(x); }"); - Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + AST = buildASTFromCode("void g(int*); void f() { int* p; g(p); }"); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_TRUE(isPointeeMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(p)")); AST = buildASTFromCode("typedef int* IntPtr;" "void g(IntPtr); void f() { int* x; g(x); }"); @@ -559,18 +646,27 @@ } TEST(ExprMutationAnalyzerTest, TakeAddress) { - const auto AST = buildASTFromCode("void g(int*); void f() { int x; g(&x); }"); - const auto Results = + auto AST = buildASTFromCode("void g(int*); void f() { int x; g(&x); }"); + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("&x")); + + AST = buildASTFromCode( + "void g(const int*); void f() { const int x = 10; g(&x); }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); } TEST(ExprMutationAnalyzerTest, ArrayToPointerDecay) { - const auto AST = - buildASTFromCode("void g(int*); void f() { int x[2]; g(x); }"); - const auto Results = + auto AST = buildASTFromCode("void g(int*); void f() { int x[2]; g(x); }"); + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x")); + + AST = buildASTFromCode( + "void g(const int*); void f() { const int x[2] = {}; g(x); }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); } TEST(ExprMutationAnalyzerTest, TemplateWithArrayToPointerDecay) { @@ -850,33 +946,55 @@ } TEST(ExprMutationAnalyzerTest, LambdaDefaultCaptureByValue) { - const auto AST = buildASTFromCode("void f() { int x; [=]() { x; }; }"); - const auto Results = + auto AST = buildASTFromCode("void f() { int x; [=]() { x; }; }"); + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_FALSE(isMutated(Results, AST.get())); + + AST = buildASTFromCode("void f() { int *p; [=]() { *p = 10; }; }"); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_TRUE(isPointeeMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre(ResultOf(removeSpace, "[=](){*p=10;}"))); } TEST(ExprMutationAnalyzerTest, LambdaExplicitCaptureByValue) { - const auto AST = buildASTFromCode("void f() { int x; [x]() { x; }; }"); - const auto Results = + auto AST = buildASTFromCode("void f() { int x; [x]() { x; }; }"); + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_FALSE(isMutated(Results, AST.get())); + + AST = buildASTFromCode("void f() { int *p; [p]() { *p = 10; }; }"); + Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); + EXPECT_TRUE(isPointeeMutated(Results, AST.get())); + EXPECT_THAT(mutatedBy(Results, AST.get()), + ElementsAre(ResultOf(removeSpace, "[p](){*p=10;}"))); } TEST(ExprMutationAnalyzerTest, LambdaDefaultCaptureByRef) { - const auto AST = buildASTFromCode("void f() { int x; [&]() { x = 10; }; }"); - const auto Results = + auto AST = buildASTFromCode("void f() { int x; [&]() { x = 10; }; }"); + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ResultOf(removeSpace, "[&](){x=10;}"))); + + AST = buildASTFromCode("void f() { const int x = 10; [&]() { x; }; }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); } TEST(ExprMutationAnalyzerTest, LambdaExplicitCaptureByRef) { - const auto AST = buildASTFromCode("void f() { int x; [&x]() { x = 10; }; }"); - const auto Results = + auto AST = buildASTFromCode("void f() { int x; [&x]() { x = 10; }; }"); + auto Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ResultOf(removeSpace, "[&x](){x=10;}"))); + + AST = buildASTFromCode("void f() { const int x = 10; [&x]() { x; }; }"); + Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()); + EXPECT_FALSE(isMutated(Results, AST.get())); } TEST(ExprMutationAnalyzerTest, RangeForArrayByRefModified) { Index: lib/Analysis/ExprMutationAnalyzer.cpp =================================================================== --- lib/Analysis/ExprMutationAnalyzer.cpp +++ lib/Analysis/ExprMutationAnalyzer.cpp @@ -81,6 +81,34 @@ return nullptr; } +// Pointee of `Exp` can be *directly* mutated if the type of `Exp` is: +// - Pointer to non-const +// - Pointer-like type with `operator*` returning non-const reference +bool isPointeeMutable(const Expr *Exp, const ASTContext &Context) { + if (const auto *PT = Exp->getType()->getAs<PointerType>()) { + return !PT->getPointeeType().isConstant(Context); + } + if (const auto *RT = Exp->getType()->getAs<RecordType>()) { + if (const CXXRecordDecl *RecordDecl = RT->getAsCXXRecordDecl()) { + bool ExpIsConst = Exp->getType().isConstant(Context); + return llvm::any_of( + RecordDecl->methods(), + [ExpIsConst, &Context](const CXXMethodDecl *MethodDecl) { + // Look for `operator*` with the same constness as `Exp`. + if (MethodDecl->getOverloadedOperator() != OO_Star || + MethodDecl->isConst() != ExpIsConst) + return false; + // Check whether returning non-const reference. + if (const auto *RT = + MethodDecl->getReturnType()->getAs<ReferenceType>()) + return !RT->getPointeeType().isConstant(Context); + return false; + }); + } + } + return false; +} + } // namespace const Stmt *ExprMutationAnalyzer::findMutation(const Expr *Exp) { @@ -100,7 +128,10 @@ } const Stmt *ExprMutationAnalyzer::findPointeeMutation(const Expr *Exp) { - return findMutationMemoized(Exp, {/*TODO*/}, PointeeResults); + return findMutationMemoized(Exp, + {&ExprMutationAnalyzer::findPointeeDirectMutation, + &ExprMutationAnalyzer::findPointeeCastMutation}, + PointeeResults); } const Stmt *ExprMutationAnalyzer::findPointeeMutation(const Decl *Dec) { @@ -192,6 +223,12 @@ } const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) { + // `Exp` can be *directly* mutated if the type of `Exp` is not const. + // Bail out early otherwise. + if (Exp->getType().isConstant(Context)) + return nullptr; + bool ExpIsPointer = Exp->getType()->getAs<PointerType>() != nullptr; + // LHS of any assignment operators. const auto AsAssignmentLhs = binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp))); @@ -204,14 +241,17 @@ // 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))), - cxxOperatorCallExpr(callee(NonConstMethod), - hasArgument(0, equalsNode(Exp))), - callExpr(callee(expr(anyOf( - unresolvedMemberExpr(hasObjectExpression(equalsNode(Exp))), - cxxDependentScopeMemberExpr( - hasObjectExpression(equalsNode(Exp))))))))); + const auto AsNonConstNonPointerThis = + ExpIsPointer + ? expr(unless(anything())) + : expr(anyOf( + cxxMemberCallExpr(callee(NonConstMethod), on(equalsNode(Exp))), + cxxOperatorCallExpr(callee(NonConstMethod), + hasArgument(0, equalsNode(Exp))), + callExpr(callee(expr(anyOf( + unresolvedMemberExpr(hasObjectExpression(equalsNode(Exp))), + cxxDependentScopeMemberExpr( + hasObjectExpression(equalsNode(Exp))))))))); // Taking address of 'Exp'. // We're assuming 'Exp' is mutated as soon as its address is taken, though in @@ -224,15 +264,19 @@ hasUnaryOperand(equalsNode(Exp))); const auto AsPointerFromArrayDecay = castExpr(hasCastKind(CK_ArrayToPointerDecay), + // A NoOp implicit cast is adding const. + unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))), unless(hasParent(arraySubscriptExpr())), has(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. const auto AsOperatorArrowThis = - cxxOperatorCallExpr(hasOverloadedOperatorName("->"), - callee(cxxMethodDecl(ofClass(isMoveOnly()), - returns(nonConstPointerType()))), - argumentCountIs(1), hasArgument(0, equalsNode(Exp))); + ExpIsPointer ? cxxOperatorCallExpr(unless(anything())) + : cxxOperatorCallExpr( + hasOverloadedOperatorName("->"), + callee(cxxMethodDecl(ofClass(isMoveOnly()), + returns(nonConstPointerType()))), + argumentCountIs(1), hasArgument(0, 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. @@ -265,10 +309,11 @@ const auto AsNonConstRefReturn = returnStmt(hasReturnValue(equalsNode(Exp))); const auto Matches = - match(findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, AsNonConstThis, - AsAmpersandOperand, AsPointerFromArrayDecay, - AsOperatorArrowThis, AsNonConstRefArg, - AsLambdaRefCaptureInit, AsNonConstRefReturn)) + match(findAll(stmt(anyOf(AsAssignmentLhs, AsIncDecOperand, + AsNonConstNonPointerThis, AsAmpersandOperand, + AsPointerFromArrayDecay, AsOperatorArrowThis, + AsNonConstRefArg, AsLambdaRefCaptureInit, + AsNonConstRefReturn)) .bind("stmt")), Stm, Context); return selectFirst<Stmt>("stmt", Matches); @@ -412,6 +457,62 @@ return nullptr; } +const Stmt *ExprMutationAnalyzer::findPointeeDirectMutation(const Expr *Exp) { + // Bail out early if pointee can't be *directly* mutated. + if (!isPointeeMutable(Exp, Context)) + return nullptr; + bool ExpIsPointer = Exp->getType()->getAs<PointerType>() != nullptr; + + // Invoking non-const member function. + // A member function is assumed to be non-const when it is unresolved. + // We don't handle pointer-like types here, because they need to be + // dereferenced first before any non-const member function of the pointee can + // be invoked, and once dereferenced they become real pointers. + const auto NonConstMethod = cxxMethodDecl(unless(isConst())); + const auto AsNonConstPointerThis = + ExpIsPointer + ? expr(anyOf( + cxxMemberCallExpr(callee(NonConstMethod), on(equalsNode(Exp))), + cxxOperatorCallExpr(callee(NonConstMethod), + hasArgument(0, equalsNode(Exp))), + callExpr(callee(expr(anyOf( + unresolvedMemberExpr(hasObjectExpression(equalsNode(Exp))), + cxxDependentScopeMemberExpr( + hasObjectExpression(equalsNode(Exp))))))))) + : expr(unless(anything())); + + // Used as an argument when calling a function. + const auto AsArg = + anyOf(callExpr(hasAnyArgument(equalsNode(Exp))), + cxxConstructExpr(hasAnyArgument(equalsNode(Exp))), + cxxUnresolvedConstructExpr(hasAnyArgument(equalsNode(Exp)))); + + // Captured by a lambda. + const auto AsLambdaCaptureInit = lambdaExpr(hasCaptureInit(Exp)); + + // Returned from the function. + const auto AsReturnValue = returnStmt(hasReturnValue(equalsNode(Exp))); + + const auto Matches = + match(findAll(stmt(anyOf(AsNonConstPointerThis, AsArg, + AsLambdaCaptureInit, AsReturnValue)) + .bind("stmt")), + Stm, Context); + return selectFirst<Stmt>("stmt", Matches); +} + +const Stmt *ExprMutationAnalyzer::findPointeeCastMutation(const Expr *Exp) { + // Only handling LValueToRValue for now for easier unit testing during + // implementation. + // TODO: properly handle all casts scenarios. + const auto Casts = + match(findAll(implicitCastExpr(hasSourceExpression(equalsNode(Exp)), + hasCastKind(CK_LValueToRValue)) + .bind(NodeID<Expr>::value)), + Stm, Context); + return findExprPointeeMutation(Casts); +} + FunctionParmMutationAnalyzer::FunctionParmMutationAnalyzer( const FunctionDecl &Func, ASTContext &Context) : BodyAnalyzer(*Func.getBody(), Context) { Index: include/clang/Analysis/Analyses/ExprMutationAnalyzer.h =================================================================== --- include/clang/Analysis/Analyses/ExprMutationAnalyzer.h +++ include/clang/Analysis/Analyses/ExprMutationAnalyzer.h @@ -66,6 +66,9 @@ const Stmt *findReferenceMutation(const Expr *Exp); const Stmt *findFunctionArgMutation(const Expr *Exp); + const Stmt *findPointeeDirectMutation(const Expr *Exp); + const Stmt *findPointeeCastMutation(const Expr *Exp); + const Stmt &Stm; ASTContext &Context; llvm::DenseMap<const FunctionDecl *,
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits