llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-temporal-safety @llvm/pr-subscribers-clang Author: PushkarSingh (iitianpushkar) <details> <summary>Changes</summary> `AnyCall::arguments()` now returns a normalized argument list. For instance member calls, the implicit object argument is included as argument 0. For static `operator()` calls, the syntactic object operand is excluded so the result matches the actual callable parameters. LifetimeSafety now uses this normalized `AnyCall` argument list directly, removing the separate `FunctionCallInfo`/`getFunctionCallInfo` path. Tests were added for: - ordinary member calls with implicit object arguments - non-static `operator()` calls where the object is already present - static `operator()` calls where the object operand should be excluded --- Full diff: https://github.com/llvm/llvm-project/pull/217838.diff 6 Files Affected: - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h (+1-3) - (modified) clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h (-10) - (modified) clang/include/clang/Analysis/AnyCall.h (+28-16) - (modified) clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp (+14-17) - (modified) clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp (+10-34) - (modified) clang/unittests/Analysis/AnyCallTest.cpp (+84-8) ``````````diff diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h index 9821078ec1d1e..ec5027965002e 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/FactsGenerator.h @@ -113,9 +113,7 @@ class FactsGenerator : public ConstStmtVisitor<FactsGenerator> { /// reference parameter, creating an IssueFact if it does. /// \param IsGslConstruction True if this is a GSL construction where all /// argument origins should flow to the returned origin. - void handleFunctionCall(const Expr *Call, const FunctionDecl *FD, - ArrayRef<const Expr *> Args, - bool IsGslConstruction = false); + void handleFunctionCall(const Expr *Call, bool IsGslConstruction = false); // Detect methods that invalidate iterators/references/pointees. // For instance methods, Args[0] is the implicit 'this' pointer. diff --git a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h index 6f1259fa867dc..60573a15432dd 100644 --- a/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h +++ b/clang/include/clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h @@ -14,7 +14,6 @@ #include "clang/AST/DeclCXX.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/PointerUnion.h" -#include "llvm/ADT/SmallVector.h" #include <optional> namespace clang ::lifetimes { @@ -63,15 +62,6 @@ bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD); using LifetimeBoundParamInfo = llvm::PointerUnion<const ParmVarDecl *, const CXXMethodDecl *>; -struct FunctionCallInfo { - const FunctionDecl *FD = nullptr; - llvm::SmallVector<const Expr *, 4> Args; -}; - -/// Returns the callee and arguments corresponding to Call. For instance member -/// calls, Args includes the implicit object argument as argument 0. -FunctionCallInfo getFunctionCallInfo(const Expr *Call); - /// Returns the parameter corresponding to argument I when the argument should /// be tracked for lifetime safety. std::optional<LifetimeBoundParamInfo> diff --git a/clang/include/clang/Analysis/AnyCall.h b/clang/include/clang/Analysis/AnyCall.h index 65975de78d94a..00129ebbe9469 100644 --- a/clang/include/clang/Analysis/AnyCall.h +++ b/clang/include/clang/Analysis/AnyCall.h @@ -16,6 +16,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/ExprObjC.h" +#include "llvm/ADT/SmallVector.h" #include <optional> namespace clang { @@ -162,43 +163,54 @@ class AnyCall { size_t param_size() const { return parameters().size(); } bool param_empty() const { return parameters().empty(); } - /// \returns actual arguments for expression-backed calls, or an empty range + /// \returns actual arguments for expression-backed calls, or an empty list /// for declaration-backed calls and call kinds with implicit or synthesized - /// argument lists, such as allocators and destructors. - ArrayRef<const Expr *> arguments() const { + /// argument lists, such as allocators and destructors. For instance member + /// calls, the implicit object argument is included as argument 0. + llvm::SmallVector<const Expr *, 4> arguments() const { + llvm::SmallVector<const Expr *, 4> Args; if (!E) - return {}; + return Args; switch (K) { - case Function: + case Function: { + const auto *CE = cast<CallExpr>(E); + if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(CE)) + Args.push_back(MCE->getImplicitObjectArgument()); + Args.append(CE->arg_begin(), CE->arg_end()); + + if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(CE)) + // For `static operator()`, the first argument is the object argument, + // remove it from the argument list to avoid off-by-one errors. + if (const auto *FD = dyn_cast_or_null<FunctionDecl>(D); + FD && FD->isStatic() && OCE->getOperator() == OO_Call) + Args.erase(Args.begin()); + return Args; + } case Block: { const auto *CE = cast<CallExpr>(E); - return {CE->getArgs(), CE->getNumArgs()}; + Args.append(CE->arg_begin(), CE->arg_end()); + return Args; } case ObjCMethod: { const auto *ME = cast<ObjCMessageExpr>(E); - return {ME->getArgs(), ME->getNumArgs()}; + Args.append(ME->arg_begin(), ME->arg_end()); + return Args; } case Constructor: { const auto *CE = cast<CXXConstructExpr>(E); - return {CE->getArgs(), CE->getNumArgs()}; + Args.append(CE->arg_begin(), CE->arg_end()); + return Args; } case Destructor: case InheritedConstructor: case Allocator: case Deallocator: - return {}; + return Args; } llvm_unreachable("Unknown AnyCall::Kind"); } - using arg_const_iterator = ArrayRef<const Expr *>::const_iterator; - arg_const_iterator arg_begin() const { return arguments().begin(); } - arg_const_iterator arg_end() const { return arguments().end(); } - size_t arg_size() const { return arguments().size(); } - bool arg_empty() const { return arguments().empty(); } - const Expr *getArg(unsigned I) const { return arguments()[I]; } - QualType getReturnType(ASTContext &Ctx) const { switch (K) { case Function: diff --git a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp index ce3039528b283..41d330f48520e 100644 --- a/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp +++ b/clang/lib/Analysis/LifetimeSafety/FactsGenerator.cpp @@ -19,6 +19,7 @@ #include "clang/Analysis/Analyses/LifetimeSafety/LifetimeAnnotations.h" #include "clang/Analysis/Analyses/LifetimeSafety/Origins.h" #include "clang/Analysis/Analyses/PostOrderCFGView.h" +#include "clang/Analysis/AnyCall.h" #include "clang/Analysis/CFG.h" #include "clang/Basic/OperatorKinds.h" #include "llvm/ADT/ArrayRef.h" @@ -233,9 +234,7 @@ void FactsGenerator::VisitCXXConstructExpr(const CXXConstructExpr *CCE) { return; } } - auto [FD, Args] = getFunctionCallInfo(CCE); - handleFunctionCall(CCE, FD, Args, - /*IsGslConstruction=*/false); + handleFunctionCall(CCE, /*IsGslConstruction=*/false); } void FactsGenerator::VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *DIE) { @@ -256,13 +255,10 @@ void FactsGenerator::VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) { if (isGslPointerType(MCE->getType()) && isa_and_present<CXXConversionDecl>(MCE->getCalleeDecl()) && isGslOwnerType(MCE->getImplicitObjectArgument()->getType())) { - auto [FD, Args] = getFunctionCallInfo(MCE); - handleFunctionCall(MCE, FD, Args, - /*IsGslConstruction=*/true); + handleFunctionCall(MCE, /*IsGslConstruction=*/true); return; } - auto [FD, Args] = getFunctionCallInfo(MCE); - handleFunctionCall(MCE, FD, Args, /*IsGslConstruction=*/false); + handleFunctionCall(MCE, /*IsGslConstruction=*/false); } void FactsGenerator::VisitMemberExpr(const MemberExpr *ME) { @@ -282,8 +278,7 @@ void FactsGenerator::VisitMemberExpr(const MemberExpr *ME) { } void FactsGenerator::VisitCallExpr(const CallExpr *CE) { - auto [FD, Args] = getFunctionCallInfo(CE); - handleFunctionCall(CE, FD, Args); + handleFunctionCall(CE); } void FactsGenerator::VisitCXXNullPtrLiteralExpr( @@ -630,8 +625,7 @@ void FactsGenerator::VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *OCE) { } } - auto [FD, Args] = getFunctionCallInfo(OCE); - handleFunctionCall(OCE, FD, Args); + handleFunctionCall(OCE); } void FactsGenerator::VisitCXXFunctionalCastExpr( @@ -904,9 +898,7 @@ void FactsGenerator::handleGSLPointerConstruction(const CXXConstructExpr *CCE) { } else { // This could be a new borrow. // TODO: Add code example here. - auto [FD, Args] = getFunctionCallInfo(CCE); - handleFunctionCall(CCE, FD, Args, - /*IsGslConstruction=*/true); + handleFunctionCall(CCE, /*IsGslConstruction=*/true); } } @@ -1090,9 +1082,14 @@ void FactsGenerator::handleLifetimeCaptureBy(const FunctionDecl *FD, } void FactsGenerator::handleFunctionCall(const Expr *Call, - const FunctionDecl *FD, - ArrayRef<const Expr *> Args, bool IsGslConstruction) { + std::optional<AnyCall> AC = AnyCall::forExpr(Call->IgnoreParenImpCasts()); + if (!AC) + return; + const auto *FD = dyn_cast_or_null<FunctionDecl>(AC->getDecl()); + if (!FD) + return; + llvm::SmallVector<const Expr *, 4> Args = AC->arguments(); OriginList *CallList = getOriginsList(*Call); // Ignore functions returning values with no origin. FD = getDeclWithMergedLifetimeBoundAttrs(FD); diff --git a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp index 154657193cabc..37746de0a0dc4 100644 --- a/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp +++ b/clang/lib/Analysis/LifetimeSafety/LifetimeAnnotations.cpp @@ -108,34 +108,6 @@ bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) { return isNormalAssignmentOperator(FD); } -FunctionCallInfo getFunctionCallInfo(const Expr *Call) { - FunctionCallInfo Info; - if (!Call) - return Info; - - Call = Call->IgnoreParenImpCasts(); - std::optional<AnyCall> AC = AnyCall::forExpr(Call); - if (!AC) - return Info; - - Info.FD = dyn_cast_or_null<FunctionDecl>(AC->getDecl()); - if (!Info.FD) - return Info; - - if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) - Info.Args.push_back(MCE->getImplicitObjectArgument()); - - Info.Args.append(AC->arg_begin(), AC->arg_end()); - - if (const auto *OCE = dyn_cast<CXXOperatorCallExpr>(Call)) - // For `static operator()`, the first argument is the object argument, - // remove it from the argument list to avoid off-by-one errors. - if (OCE->getOperator() == OO_Call && Info.FD->isStatic()) - Info.Args.erase(Info.Args.begin()); - - return Info; -} - std::optional<LifetimeBoundParamInfo> getTrackedArgInfo(const FunctionDecl *FD, llvm::ArrayRef<const Expr *> Args, unsigned I) { @@ -178,15 +150,19 @@ getTrackingInfoForCallArg(const Expr *Call, const Expr *Source) { if (!Call || !Source) return std::nullopt; - FunctionCallInfo CallInfo = getFunctionCallInfo(Call); - if (!CallInfo.FD) + std::optional<AnyCall> AC = AnyCall::forExpr(Call->IgnoreParenImpCasts()); + if (!AC) + return std::nullopt; + + const auto *FD = dyn_cast_or_null<FunctionDecl>(AC->getDecl()); + if (!FD) return std::nullopt; - for (unsigned I = 0; I < CallInfo.Args.size(); ++I) - if (CallInfo.Args[I]->IgnoreParenImpCasts() == - Source->IgnoreParenImpCasts()) + llvm::SmallVector<const Expr *, 4> Args = AC->arguments(); + for (unsigned I = 0; I < Args.size(); ++I) + if (Args[I]->IgnoreParenImpCasts() == Source->IgnoreParenImpCasts()) if (std::optional<LifetimeBoundParamInfo> ParamInfo = - getTrackedArgInfo(CallInfo.FD, CallInfo.Args, I)) + getTrackedArgInfo(FD, Args, I)) return ParamInfo; return std::nullopt; diff --git a/clang/unittests/Analysis/AnyCallTest.cpp b/clang/unittests/Analysis/AnyCallTest.cpp index 425e10a3bd54c..a3137cd48f07e 100644 --- a/clang/unittests/Analysis/AnyCallTest.cpp +++ b/clang/unittests/Analysis/AnyCallTest.cpp @@ -37,14 +37,13 @@ const IntegerLiteral *asIntegerLiteral(const Expr *E) { void expectIntegerArguments(const AnyCall &Call, std::initializer_list<int> Expected) { - ASSERT_EQ(Call.arg_size(), Expected.size()); - EXPECT_FALSE(Call.arg_empty()); - EXPECT_EQ(Call.arguments()[0], Call.getArg(0)); - EXPECT_EQ(*Call.arg_begin(), Call.getArg(0)); + llvm::SmallVector<const Expr *, 4> Args = Call.arguments(); + ASSERT_EQ(Args.size(), Expected.size()); + EXPECT_FALSE(Args.empty()); unsigned Index = 0; for (int ExpectedValue : Expected) { - const auto *Arg = asIntegerLiteral(Call.getArg(Index)); + const auto *Arg = asIntegerLiteral(Args[Index]); ASSERT_NE(Arg, nullptr); EXPECT_EQ(Arg->getValue(), ExpectedValue); ++Index; @@ -52,9 +51,7 @@ void expectIntegerArguments(const AnyCall &Call, } void expectNoArguments(const AnyCall &Call) { - EXPECT_TRUE(Call.arg_empty()); - EXPECT_EQ(Call.arg_size(), 0u); - EXPECT_EQ(Call.arg_begin(), Call.arg_end()); + EXPECT_TRUE(Call.arguments().empty()); } TEST(AnyCallTest, ExposesFunctionParametersAndArguments) { @@ -81,6 +78,85 @@ TEST(AnyCallTest, ExposesFunctionParametersAndArguments) { expectIntegerArguments(Call, {1, 2}); } +TEST(AnyCallTest, IncludesImplicitObjectInMemberCallArguments) { + auto AST = buildAST(R"cpp( + struct Widget { + void method(int value); + }; + void target(Widget &widget) { widget.method(3); } + )cpp"); + ASTContext &Ctx = AST->getASTContext(); + ASSERT_EQ(Ctx.getDiagnostics().getClient()->getNumErrors(), 0U); + + const auto *CE = selectFirst<CXXMemberCallExpr>( + "call", match(cxxMemberCallExpr(callee(cxxMethodDecl(hasName("method")))) + .bind("call"), + Ctx)); + ASSERT_NE(CE, nullptr); + + AnyCall Call(CE); + llvm::SmallVector<const Expr *, 4> Args = Call.arguments(); + ASSERT_EQ(Args.size(), 2u); + + const auto *Object = dyn_cast<DeclRefExpr>(Args[0]->IgnoreParenImpCasts()); + ASSERT_NE(Object, nullptr); + EXPECT_EQ(Object->getDecl()->getName(), "widget"); + + const auto *Argument = asIntegerLiteral(Args[1]); + ASSERT_NE(Argument, nullptr); + EXPECT_EQ(Argument->getValue(), 3); +} + +TEST(AnyCallTest, OperatorCallArgumentsAlreadyIncludeImplicitObject) { + auto AST = buildAST(R"cpp( + struct Callable { + void operator()(int value); + }; + void target(Callable &callable) { callable(4); } + )cpp"); + ASTContext &Ctx = AST->getASTContext(); + ASSERT_EQ(Ctx.getDiagnostics().getClient()->getNumErrors(), 0U); + + const auto *CE = selectFirst<CXXOperatorCallExpr>( + "call", + match(cxxOperatorCallExpr(hasOverloadedOperatorName("()")).bind("call"), + Ctx)); + ASSERT_NE(CE, nullptr); + + AnyCall Call(CE); + llvm::SmallVector<const Expr *, 4> Args = Call.arguments(); + ASSERT_EQ(Args.size(), 2u); + + const auto *Object = dyn_cast<DeclRefExpr>(Args[0]->IgnoreParenImpCasts()); + ASSERT_NE(Object, nullptr); + EXPECT_EQ(Object->getDecl()->getName(), "callable"); + + const auto *Argument = asIntegerLiteral(Args[1]); + ASSERT_NE(Argument, nullptr); + EXPECT_EQ(Argument->getValue(), 4); +} + +TEST(AnyCallTest, StaticOperatorCallExcludesObjectArgument) { + auto AST = buildAST(R"cpp( + struct Callable { + static void operator()(int value); + }; + void target(Callable &callable) { callable(5); } + )cpp", + {"-fsyntax-only", "-std=c++23"}); + ASTContext &Ctx = AST->getASTContext(); + ASSERT_EQ(Ctx.getDiagnostics().getClient()->getNumErrors(), 0U); + + const auto *CE = selectFirst<CXXOperatorCallExpr>( + "call", + match(cxxOperatorCallExpr(hasOverloadedOperatorName("()")).bind("call"), + Ctx)); + ASSERT_NE(CE, nullptr); + + AnyCall Call(CE); + expectIntegerArguments(Call, {5}); +} + TEST(AnyCallTest, ExposesBlockCallArguments) { auto AST = buildAST(R"cpp( void target() { `````````` </details> https://github.com/llvm/llvm-project/pull/217838 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
