https://github.com/sgatev updated https://github.com/llvm/llvm-project/pull/68558
>From 77d474bd7f7885cc454aeb0fdb5b25788d261e41 Mon Sep 17 00:00:00 2001 From: Stanislav Gatev <sga...@google.com> Date: Fri, 6 Oct 2023 04:32:06 +0000 Subject: [PATCH] [clang][dataflow] Add support for lambda captures This adds support for copy, ref, and this lambda captures to the core framework and also adds relevant test in UncheckedOptionalAccessTest. --- .../FlowSensitive/DataflowAnalysisContext.h | 2 +- .../FlowSensitive/DataflowEnvironment.h | 6 +- .../FlowSensitive/DataflowAnalysisContext.cpp | 2 +- .../FlowSensitive/DataflowEnvironment.cpp | 24 +- .../Analysis/FlowSensitive/TestingSupport.cpp | 15 +- .../Analysis/FlowSensitive/TestingSupport.h | 16 +- .../Analysis/FlowSensitive/TransferTest.cpp | 231 ++++++++++++++++++ .../UncheckedOptionalAccessModelTest.cpp | 117 +++++++++ 8 files changed, 398 insertions(+), 15 deletions(-) diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h index 4698f0616e66e82..c46109a02921e7f 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h @@ -98,7 +98,7 @@ class DataflowAnalysisContext { StorageLocation &createStorageLocation(QualType Type); /// Returns a stable storage location for `D`. - StorageLocation &getStableStorageLocation(const VarDecl &D); + StorageLocation &getStableStorageLocation(const ValueDecl &D); /// Returns a stable storage location for `E`. StorageLocation &getStableStorageLocation(const Expr &E); diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index 73f747ff88cf447..56d647f35b08430 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -242,7 +242,7 @@ class Environment { /// Creates a storage location for `D`. Does not assign the returned storage /// location to `D` in the environment. Does not assign a value to the /// returned storage location in the environment. - StorageLocation &createStorageLocation(const VarDecl &D); + StorageLocation &createStorageLocation(const ValueDecl &D); /// Creates a storage location for `E`. Does not assign the returned storage /// location to `E` in the environment. Does not assign a value to the @@ -408,7 +408,7 @@ class Environment { /// this value. Otherwise, initializes the object with a value created using /// `createValue()`. Uses the storage location returned by /// `DataflowAnalysisContext::getStableStorageLocation(D)`. - StorageLocation &createObject(const VarDecl &D, const Expr *InitExpr) { + StorageLocation &createObject(const ValueDecl &D, const Expr *InitExpr) { return createObjectInternal(&D, D.getType(), InitExpr); } @@ -614,7 +614,7 @@ class Environment { /// Shared implementation of `createObject()` overloads. /// `D` and `InitExpr` may be null. - StorageLocation &createObjectInternal(const VarDecl *D, QualType Ty, + StorageLocation &createObjectInternal(const ValueDecl *D, QualType Ty, const Expr *InitExpr); /// Shared implementation of `pushCall` overloads. Note that unlike diff --git a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp index e81048ce9233808..fa9b40fc49b3ae7 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp @@ -73,7 +73,7 @@ StorageLocation &DataflowAnalysisContext::createStorageLocation(QualType Type) { } StorageLocation & -DataflowAnalysisContext::getStableStorageLocation(const VarDecl &D) { +DataflowAnalysisContext::getStableStorageLocation(const ValueDecl &D) { if (auto *Loc = DeclToLoc.lookup(&D)) return *Loc; auto &Loc = createStorageLocation(D.getType().getNonReferenceType()); diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 66d98c995468595..01c6cc69e2b9fac 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -448,11 +448,23 @@ Environment::Environment(DataflowAnalysisContext &DACtx, if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) { auto *Parent = MethodDecl->getParent(); assert(Parent != nullptr); - if (Parent->isLambda()) - MethodDecl = dyn_cast<CXXMethodDecl>(Parent->getDeclContext()); - // FIXME: Initialize the ThisPointeeLoc of lambdas too. - if (MethodDecl && MethodDecl->isImplicitObjectMemberFunction()) { + if (Parent->isLambda()) { + for (auto Capture : Parent->captures()) { + if (Capture.capturesVariable()) { + const auto *VarDecl = Capture.getCapturedVar(); + assert(VarDecl != nullptr); + setStorageLocation(*VarDecl, createObject(*VarDecl, nullptr)); + } else if (Capture.capturesThis()) { + const auto *SurroundingMethodDecl = + cast<CXXMethodDecl>(DeclCtx.getNonClosureAncestor()); + QualType ThisPointeeType = + SurroundingMethodDecl->getFunctionObjectParameterType(); + ThisPointeeLoc = + &cast<RecordValue>(createValue(ThisPointeeType))->getLoc(); + } + } + } else if (MethodDecl->isImplicitObjectMemberFunction()) { QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType(); ThisPointeeLoc = &cast<RecordValue>(createValue(ThisPointeeType))->getLoc(); @@ -673,7 +685,7 @@ StorageLocation &Environment::createStorageLocation(QualType Type) { return DACtx->createStorageLocation(Type); } -StorageLocation &Environment::createStorageLocation(const VarDecl &D) { +StorageLocation &Environment::createStorageLocation(const ValueDecl &D) { // Evaluated declarations are always assigned the same storage locations to // ensure that the environment stabilizes across loop iterations. Storage // locations for evaluated declarations are stored in the analysis context. @@ -885,7 +897,7 @@ Environment::createLocAndMaybeValue(QualType Ty, return Loc; } -StorageLocation &Environment::createObjectInternal(const VarDecl *D, +StorageLocation &Environment::createObjectInternal(const ValueDecl *D, QualType Ty, const Expr *InitExpr) { if (Ty->isReferenceType()) { diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp index 72bdfee26fe7f3e..65c527ae63d2d71 100644 --- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp @@ -161,7 +161,18 @@ llvm::Error test::checkDataflowWithNoopAnalysis( VerifyResults, DataflowAnalysisOptions Options, LangStandard::Kind Std, llvm::StringRef TargetFun) { - using ast_matchers::hasName; + return checkDataflowWithNoopAnalysis(Code, ast_matchers::hasName(TargetFun), + VerifyResults, Options, Std); +} + +llvm::Error test::checkDataflowWithNoopAnalysis( + llvm::StringRef Code, + ast_matchers::internal::Matcher<FunctionDecl> TargetFuncMatcher, + std::function< + void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &, + ASTContext &)> + VerifyResults, + DataflowAnalysisOptions Options, LangStandard::Kind Std) { llvm::SmallVector<std::string, 3> ASTBuildArgs = { // -fnodelayed-template-parsing is the default everywhere but on Windows. // Set it explicitly so that tests behave the same on Windows as on other @@ -170,7 +181,7 @@ llvm::Error test::checkDataflowWithNoopAnalysis( "-std=" + std::string(LangStandard::getLangStandardForKind(Std).getName())}; AnalysisInputs<NoopAnalysis> AI( - Code, hasName(TargetFun), + Code, TargetFuncMatcher, [UseBuiltinModel = Options.BuiltinOpts.has_value()](ASTContext &C, Environment &Env) { return NoopAnalysis( diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h index 44d962d5da9a97b..de3046e22897c3e 100644 --- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h +++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h @@ -398,8 +398,8 @@ checkDataflow(AnalysisInputs<AnalysisT> AI, using BuiltinOptions = DataflowAnalysisContext::Options; -/// Runs dataflow on `Code` with a `NoopAnalysis` and calls `VerifyResults` to -/// verify the results. +/// Runs dataflow on function named `TargetFun` in `Code` with a `NoopAnalysis` +/// and calls `VerifyResults` to verify the results. llvm::Error checkDataflowWithNoopAnalysis( llvm::StringRef Code, std::function< @@ -410,6 +410,18 @@ llvm::Error checkDataflowWithNoopAnalysis( LangStandard::Kind Std = LangStandard::lang_cxx17, llvm::StringRef TargetFun = "target"); +/// Runs dataflow on function matched by `TargetFuncMatcher` in `Code` with a +/// `NoopAnalysis` and calls `VerifyResults` to verify the results. +llvm::Error checkDataflowWithNoopAnalysis( + llvm::StringRef Code, + ast_matchers::internal::Matcher<FunctionDecl> TargetFuncMatcher, + std::function< + void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &, + ASTContext &)> + VerifyResults = [](const auto &, auto &) {}, + DataflowAnalysisOptions Options = {BuiltinOptions()}, + LangStandard::Kind Std = LangStandard::lang_cxx17); + /// Returns the `ValueDecl` for the given identifier. /// /// Requirements: diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 2e8d38490fdb864..63541be0ea61877 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -66,6 +66,37 @@ void runDataflow( Std, TargetFun); } +void runDataflowOnLambda( + llvm::StringRef Code, + std::function< + void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &, + ASTContext &)> + VerifyResults, + DataflowAnalysisOptions Options, + LangStandard::Kind Std = LangStandard::lang_cxx17) { + ASSERT_THAT_ERROR( + checkDataflowWithNoopAnalysis( + Code, + ast_matchers::hasDeclContext( + ast_matchers::cxxRecordDecl(ast_matchers::isLambda())), + VerifyResults, Options, Std), + llvm::Succeeded()); +} + +void runDataflowOnLambda( + llvm::StringRef Code, + std::function< + void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &, + ASTContext &)> + VerifyResults, + LangStandard::Kind Std = LangStandard::lang_cxx17, + bool ApplyBuiltinTransfer = true) { + runDataflowOnLambda(Code, std::move(VerifyResults), + {ApplyBuiltinTransfer ? BuiltinOptions{} + : std::optional<BuiltinOptions>()}, + Std); +} + const Formula &getFormula(const ValueDecl &D, const Environment &Env) { return cast<BoolValue>(Env.getValue(D))->formula(); } @@ -5987,4 +6018,204 @@ TEST(TransferTest, EvaluateBlockWithUnreachablePreds) { ASTContext &ASTCtx) {}); } +TEST(TransferTest, LambdaCaptureByCopy) { + std::string Code = R"( + void target(int Foo, int Bar) { + [Foo]() { + (void)0; + // [[p]] + }(); + } + )"; + runDataflowOnLambda( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results.keys(), UnorderedElementsAre("p")); + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + const StorageLocation *FooLoc = Env.getStorageLocation(*FooDecl); + ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc)); + + const Value *FooVal = Env.getValue(*FooLoc); + EXPECT_TRUE(isa_and_nonnull<IntegerValue>(FooVal)); + + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); + + const StorageLocation *BarLoc = Env.getStorageLocation(*BarDecl); + EXPECT_THAT(BarLoc, IsNull()); + }); +} + +TEST(TransferTest, LambdaCaptureByReference) { + std::string Code = R"( + void target(int Foo, int Bar) { + [&Foo]() { + (void)0; + // [[p]] + }(); + } + )"; + runDataflowOnLambda( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results.keys(), UnorderedElementsAre("p")); + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + const StorageLocation *FooLoc = Env.getStorageLocation(*FooDecl); + ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc)); + + const Value *FooVal = Env.getValue(*FooLoc); + EXPECT_TRUE(isa_and_nonnull<IntegerValue>(FooVal)); + + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); + + const StorageLocation *BarLoc = Env.getStorageLocation(*BarDecl); + EXPECT_THAT(BarLoc, IsNull()); + }); +} + +TEST(TransferTest, LambdaCaptureWithInitializer) { + std::string Code = R"( + void target(int Bar) { + [Foo=Bar]() { + (void)0; + // [[p]] + }(); + } + )"; + runDataflowOnLambda( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results.keys(), UnorderedElementsAre("p")); + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + const StorageLocation *FooLoc = Env.getStorageLocation(*FooDecl); + ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc)); + + const Value *FooVal = Env.getValue(*FooLoc); + EXPECT_TRUE(isa_and_nonnull<IntegerValue>(FooVal)); + + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); + + const StorageLocation *BarLoc = Env.getStorageLocation(*BarDecl); + EXPECT_THAT(BarLoc, IsNull()); + }); +} + +TEST(TransferTest, LambdaCaptureByCopyImplicit) { + std::string Code = R"( + void target(int Foo, int Bar) { + [=]() { + Foo; + // [[p]] + }(); + } + )"; + runDataflowOnLambda( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results.keys(), UnorderedElementsAre("p")); + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + const StorageLocation *FooLoc = Env.getStorageLocation(*FooDecl); + ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc)); + + const Value *FooVal = Env.getValue(*FooLoc); + EXPECT_TRUE(isa_and_nonnull<IntegerValue>(FooVal)); + + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); + + const StorageLocation *BarLoc = Env.getStorageLocation(*BarDecl); + EXPECT_THAT(BarLoc, IsNull()); + }); +} + +TEST(TransferTest, LambdaCaptureByReferenceImplicit) { + std::string Code = R"( + void target(int Foo, int Bar) { + [&]() { + Foo; + // [[p]] + }(); + } + )"; + runDataflowOnLambda( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results.keys(), UnorderedElementsAre("p")); + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + const StorageLocation *FooLoc = Env.getStorageLocation(*FooDecl); + ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc)); + + const Value *FooVal = Env.getValue(*FooLoc); + EXPECT_TRUE(isa_and_nonnull<IntegerValue>(FooVal)); + + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); + + const StorageLocation *BarLoc = Env.getStorageLocation(*BarDecl); + EXPECT_THAT(BarLoc, IsNull()); + }); +} + +TEST(TransferTest, LambdaCaptureThis) { + std::string Code = R"( + struct Bar { + int Foo; + + void target() { + [this]() { + Foo; + // [[p]] + }(); + } + }; + )"; + runDataflowOnLambda( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results.keys(), UnorderedElementsAre("p")); + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + const RecordStorageLocation *ThisPointeeLoc = + Env.getThisPointeeStorageLocation(); + ASSERT_THAT(ThisPointeeLoc, NotNull()); + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + const StorageLocation *FooLoc = ThisPointeeLoc->getChild(*FooDecl); + ASSERT_TRUE(isa_and_nonnull<ScalarStorageLocation>(FooLoc)); + + const Value *FooVal = Env.getValue(*FooLoc); + EXPECT_TRUE(isa_and_nonnull<IntegerValue>(FooVal)); + }); +} + } // namespace diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 3f17557272d0344..dfbaebc9032235c 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -1280,6 +1280,12 @@ class UncheckedOptionalAccessTest ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target")); } + void ExpectDiagnosticsForLambda(std::string SourceCode) { + ExpectDiagnosticsFor( + SourceCode, ast_matchers::hasDeclContext( + ast_matchers::cxxRecordDecl(ast_matchers::isLambda()))); + } + template <typename FuncDeclMatcher> void ExpectDiagnosticsFor(std::string SourceCode, FuncDeclMatcher FuncMatcher) { @@ -3214,6 +3220,117 @@ TEST_P(UncheckedOptionalAccessTest, Bitfield) { } )"); } + +TEST_P(UncheckedOptionalAccessTest, Lambda) { + ExpectDiagnosticsForLambda(R"( + #include "unchecked_optional_access_test.h" + + void target() { + []($ns::$optional<int> opt) { + if (opt.has_value()) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + }(Make<$ns::$optional<int>>()); + } + )"); + ExpectDiagnosticsForLambda(R"( + #include "unchecked_optional_access_test.h" + + void target($ns::$optional<int> opt) { + [opt]() { + if (opt.has_value()) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + }(); + } + )"); + ExpectDiagnosticsForLambda(R"( + #include "unchecked_optional_access_test.h" + + void target($ns::$optional<int> opt) { + [&opt]() { + if (opt.has_value()) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + }(); + } + )"); + ExpectDiagnosticsForLambda(R"( + #include "unchecked_optional_access_test.h" + + void target($ns::$optional<int> opt) { + [opt2=opt]() { + if (opt2.has_value()) { + opt2.value(); + } else { + opt2.value(); // [[unsafe]] + } + }(); + } + )"); + ExpectDiagnosticsForLambda(R"( + #include "unchecked_optional_access_test.h" + + void target($ns::$optional<int> opt) { + [&]() { + if (opt.has_value()) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + }(); + } + )"); + ExpectDiagnosticsForLambda(R"( + #include "unchecked_optional_access_test.h" + + void target($ns::$optional<int> opt) { + [=]() { + if (opt.has_value()) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + }(); + } + )"); + ExpectDiagnosticsForLambda(R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional<int> opt; + + void target() { + [this]() { + if (opt.has_value()) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + }(); + } + }; + )"); + + // We can't propagate information from the surrounding context. + ExpectDiagnosticsForLambda(R"( + #include "unchecked_optional_access_test.h" + + void target($ns::$optional<int> opt) { + if (opt.has_value()) { + [&opt]() { + opt.value(); // [[unsafe]] + }(); + } + } + )"); +} // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits