mboehme updated this revision to Diff 532604. mboehme marked an inline comment as done. mboehme added a comment.
Use `cast_or_null` instead of `cast` in `VisitCXXOperatorCallExpr`. Added a test that crashes without this change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153006/new/ https://reviews.llvm.org/D153006 Files: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h clang/include/clang/Analysis/FlowSensitive/RecordOps.h clang/include/clang/Analysis/FlowSensitive/StorageLocation.h clang/include/clang/Analysis/FlowSensitive/Value.h clang/lib/Analysis/FlowSensitive/CMakeLists.txt clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp clang/lib/Analysis/FlowSensitive/RecordOps.cpp clang/lib/Analysis/FlowSensitive/Transfer.cpp clang/unittests/Analysis/FlowSensitive/CMakeLists.txt clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp clang/unittests/Analysis/FlowSensitive/TestingSupport.h clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp =================================================================== --- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -12,7 +12,7 @@ #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" -#include "clang/Analysis/FlowSensitive/NoopAnalysis.h" +#include "clang/Analysis/FlowSensitive/RecordOps.h" #include "clang/Analysis/FlowSensitive/StorageLocation.h" #include "clang/Analysis/FlowSensitive/Value.h" #include "clang/Basic/LangStandard.h" @@ -38,59 +38,22 @@ using ::testing::NotNull; using ::testing::UnorderedElementsAre; -using BuiltinOptions = DataflowAnalysisContext::Options; - -template <typename Matcher> -llvm::Error -runDataflowReturnError(llvm::StringRef Code, Matcher Match, - DataflowAnalysisOptions Options, - LangStandard::Kind Std = LangStandard::lang_cxx17, - llvm::StringRef TargetFun = "target") { - using ast_matchers::hasName; - 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 - // platforms. - "-fsyntax-only", "-fno-delayed-template-parsing", - "-std=" + - std::string(LangStandard::getLangStandardForKind(Std).getName())}; - AnalysisInputs<NoopAnalysis> AI( - Code, hasName(TargetFun), - [UseBuiltinModel = Options.BuiltinOpts.has_value()](ASTContext &C, - Environment &Env) { - return NoopAnalysis( - C, - DataflowAnalysisOptions{ - UseBuiltinModel ? Env.getDataflowAnalysisContext().getOptions() - : std::optional<BuiltinOptions>()}); - }); - AI.ASTBuildArgs = ASTBuildArgs; - if (Options.BuiltinOpts) - AI.BuiltinOptions = *Options.BuiltinOpts; - return checkDataflow<NoopAnalysis>( - std::move(AI), - /*VerifyResults=*/ - [&Match]( - const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, - const AnalysisOutputs &AO) { Match(Results, AO.ASTCtx); }); -} - -template <typename Matcher> -void runDataflow(llvm::StringRef Code, Matcher Match, +template <typename VerifyResultsT> +void runDataflow(llvm::StringRef Code, VerifyResultsT VerifyResults, DataflowAnalysisOptions Options, LangStandard::Kind Std = LangStandard::lang_cxx17, llvm::StringRef TargetFun = "target") { ASSERT_THAT_ERROR( - runDataflowReturnError(Code, Match, Options, Std, TargetFun), + runDataflowReturnError(Code, VerifyResults, Options, Std, TargetFun), llvm::Succeeded()); } -template <typename Matcher> -void runDataflow(llvm::StringRef Code, Matcher Match, +template <typename VerifyResultsT> +void runDataflow(llvm::StringRef Code, VerifyResultsT VerifyResults, LangStandard::Kind Std = LangStandard::lang_cxx17, bool ApplyBuiltinTransfer = true, llvm::StringRef TargetFun = "target") { - runDataflow(Code, std::move(Match), + runDataflow(Code, std::move(VerifyResults), {ApplyBuiltinTransfer ? BuiltinOptions{} : std::optional<BuiltinOptions>()}, Std, TargetFun); @@ -1987,22 +1950,20 @@ }; void target() { - A Foo; - A Bar; - (void)Foo.Baz; + A Foo = { 1 }; + A Bar = { 2 }; // [[p1]] Foo = Bar; // [[p2]] + int val = 3; + Foo.Baz = val; + // [[p3]] } )"; runDataflow( Code, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, ASTContext &ASTCtx) { - ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1", "p2")); - const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1"); - const Environment &Env2 = getEnvironmentAtAnnotation(Results, "p2"); - const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); ASSERT_THAT(FooDecl, NotNull()); @@ -2012,35 +1973,81 @@ const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz"); ASSERT_THAT(BazDecl, NotNull()); - const auto *FooLoc1 = - cast<AggregateStorageLocation>(Env1.getStorageLocation(*FooDecl)); - const auto *BarLoc1 = - cast<AggregateStorageLocation>(Env1.getStorageLocation(*BarDecl)); + // Before copy assignment. + { + const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1"); + + const auto *FooLoc1 = + cast<AggregateStorageLocation>(Env1.getStorageLocation(*FooDecl)); + const auto *BarLoc1 = + cast<AggregateStorageLocation>(Env1.getStorageLocation(*BarDecl)); + EXPECT_FALSE(recordsEqual(*FooLoc1, *BarLoc1, Env1)); + + const auto *FooBazVal1 = + cast<IntegerValue>(Env1.getValue(FooLoc1->getChild(*BazDecl))); + const auto *BarBazVal1 = + cast<IntegerValue>(Env1.getValue(BarLoc1->getChild(*BazDecl))); + EXPECT_NE(FooBazVal1, BarBazVal1); + } - const auto *FooVal1 = cast<StructValue>(Env1.getValue(*FooLoc1)); - const auto *BarVal1 = cast<StructValue>(Env1.getValue(*BarLoc1)); - EXPECT_NE(FooVal1, BarVal1); + // After copy assignment. + { + const Environment &Env2 = getEnvironmentAtAnnotation(Results, "p2"); - const auto *FooBazVal1 = - cast<IntegerValue>(Env1.getValue(FooLoc1->getChild(*BazDecl))); - const auto *BarBazVal1 = - cast<IntegerValue>(Env1.getValue(BarLoc1->getChild(*BazDecl))); - EXPECT_NE(FooBazVal1, BarBazVal1); + const auto *FooLoc2 = + cast<AggregateStorageLocation>(Env2.getStorageLocation(*FooDecl)); + const auto *BarLoc2 = + cast<AggregateStorageLocation>(Env2.getStorageLocation(*BarDecl)); - const auto *FooLoc2 = - cast<AggregateStorageLocation>(Env2.getStorageLocation(*FooDecl)); - const auto *BarLoc2 = - cast<AggregateStorageLocation>(Env2.getStorageLocation(*BarDecl)); + const auto *FooVal2 = cast<StructValue>(Env2.getValue(*FooLoc2)); + const auto *BarVal2 = cast<StructValue>(Env2.getValue(*BarLoc2)); + EXPECT_NE(FooVal2, BarVal2); - const auto *FooVal2 = cast<StructValue>(Env2.getValue(*FooLoc2)); - const auto *BarVal2 = cast<StructValue>(Env2.getValue(*BarLoc2)); - EXPECT_EQ(FooVal2, BarVal2); + EXPECT_TRUE(recordsEqual(*FooLoc2, *BarLoc2, Env2)); - const auto *FooBazVal2 = - cast<IntegerValue>(Env2.getValue(FooLoc1->getChild(*BazDecl))); - const auto *BarBazVal2 = - cast<IntegerValue>(Env2.getValue(BarLoc1->getChild(*BazDecl))); - EXPECT_EQ(FooBazVal2, BarBazVal2); + const auto *FooBazVal2 = + cast<IntegerValue>(Env2.getValue(FooLoc2->getChild(*BazDecl))); + const auto *BarBazVal2 = + cast<IntegerValue>(Env2.getValue(BarLoc2->getChild(*BazDecl))); + EXPECT_EQ(FooBazVal2, BarBazVal2); + } + + // After value update. + { + const Environment &Env3 = getEnvironmentAtAnnotation(Results, "p3"); + + const auto *FooLoc3 = + cast<AggregateStorageLocation>(Env3.getStorageLocation(*FooDecl)); + const auto *BarLoc3 = + cast<AggregateStorageLocation>(Env3.getStorageLocation(*BarDecl)); + EXPECT_FALSE(recordsEqual(*FooLoc3, *BarLoc3, Env3)); + + const auto *FooBazVal3 = + cast<IntegerValue>(Env3.getValue(FooLoc3->getChild(*BazDecl))); + const auto *BarBazVal3 = + cast<IntegerValue>(Env3.getValue(BarLoc3->getChild(*BazDecl))); + EXPECT_NE(FooBazVal3, BarBazVal3); + } + }); +} + +TEST(TransferTest, AssignmentOperatorFromCallResult) { + std::string Code = R"( + struct A {}; + A ReturnA(); + + void target() { + A MyA; + MyA = ReturnA(); + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + // As of this writing, we don't produce a `Value` for the call + // `ReturnA()`. The only condition we're testing for is that the + // analysis should not crash in this case. }); } @@ -2051,19 +2058,20 @@ }; void target() { - A Foo; - (void)Foo.Baz; + A Foo = { 1 }; A Bar = Foo; - // [[p]] + // [[after_copy]] + // FIXME: Integer literals don't yet produce `Value`s, so we need to use + // a variable to assign from here. + int val = 2; + Foo.Baz = val; + // [[after_update]] } )"; runDataflow( 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()); @@ -2073,20 +2081,50 @@ const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz"); ASSERT_THAT(BazDecl, NotNull()); - const auto *FooLoc = - cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl)); - const auto *BarLoc = - cast<AggregateStorageLocation>(Env.getStorageLocation(*BarDecl)); + // after_copy + { + const Environment &Env = + getEnvironmentAtAnnotation(Results, "after_copy"); + + const auto *FooLoc = + cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl)); + const auto *BarLoc = + cast<AggregateStorageLocation>(Env.getStorageLocation(*BarDecl)); + + // `Foo` and `Bar` have different `StructValue`s associated with them. + const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc)); + const auto *BarVal = cast<StructValue>(Env.getValue(*BarLoc)); + EXPECT_NE(FooVal, BarVal); + + // But the records compare equal. + EXPECT_TRUE(recordsEqual(*FooLoc, *BarLoc, Env)); + + // In particular, the value of `Baz` in both records is the same. + const auto *FooBazVal = + cast<IntegerValue>(Env.getValue(FooLoc->getChild(*BazDecl))); + const auto *BarBazVal = + cast<IntegerValue>(Env.getValue(BarLoc->getChild(*BazDecl))); + EXPECT_EQ(FooBazVal, BarBazVal); + } - const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc)); - const auto *BarVal = cast<StructValue>(Env.getValue(*BarLoc)); - EXPECT_EQ(FooVal, BarVal); + // after_update + { + const Environment &Env = + getEnvironmentAtAnnotation(Results, "after_update"); - const auto *FooBazVal = - cast<IntegerValue>(Env.getValue(FooLoc->getChild(*BazDecl))); - const auto *BarBazVal = - cast<IntegerValue>(Env.getValue(BarLoc->getChild(*BazDecl))); - EXPECT_EQ(FooBazVal, BarBazVal); + const auto *FooLoc = + cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl)); + const auto *BarLoc = + cast<AggregateStorageLocation>(Env.getStorageLocation(*BarDecl)); + + EXPECT_FALSE(recordsEqual(*FooLoc, *BarLoc, Env)); + + const auto *FooBazVal = + cast<IntegerValue>(Env.getValue(FooLoc->getChild(*BazDecl))); + const auto *BarBazVal = + cast<IntegerValue>(Env.getValue(BarLoc->getChild(*BazDecl))); + EXPECT_NE(FooBazVal, BarBazVal); + } }); } @@ -2125,10 +2163,7 @@ cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl)); const auto *BarLoc = cast<AggregateStorageLocation>(Env.getStorageLocation(*BarDecl)); - - const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc)); - const auto *BarVal = cast<StructValue>(Env.getValue(*BarLoc)); - EXPECT_EQ(FooVal, BarVal); + EXPECT_TRUE(recordsEqual(*FooLoc, *BarLoc, Env)); const auto *FooBazVal = cast<IntegerValue>(Env.getValue(FooLoc->getChild(*BazDecl))); @@ -2171,10 +2206,7 @@ cast<AggregateStorageLocation>(Env.getStorageLocation(*FooDecl)); const auto *BarLoc = cast<AggregateStorageLocation>(Env.getStorageLocation(*BarDecl)); - - const auto *FooVal = cast<StructValue>(Env.getValue(*FooLoc)); - const auto *BarVal = cast<StructValue>(Env.getValue(*BarLoc)); - EXPECT_EQ(FooVal, BarVal); + EXPECT_TRUE(recordsEqual(*FooLoc, *BarLoc, Env)); const auto *FooBazVal = cast<IntegerValue>(Env.getValue(FooLoc->getChild(*BazDecl))); @@ -2239,6 +2271,8 @@ const auto *BarVal1 = cast<StructValue>(Env1.getValue(*BarLoc1)); EXPECT_NE(FooVal1, BarVal1); + EXPECT_FALSE(recordsEqual(*FooLoc1, *BarLoc1, Env1)); + const auto *FooBazVal1 = cast<IntegerValue>(Env1.getValue(FooLoc1->getChild(*BazDecl))); const auto *BarBazVal1 = @@ -2248,7 +2282,8 @@ const auto *FooLoc2 = cast<AggregateStorageLocation>(Env2.getStorageLocation(*FooDecl)); const auto *FooVal2 = cast<StructValue>(Env2.getValue(*FooLoc2)); - EXPECT_EQ(FooVal2, BarVal1); + EXPECT_NE(FooVal2, BarVal1); + EXPECT_TRUE(recordsEqual(*FooLoc2, Env2, *BarLoc1, Env1)); const auto *FooBazVal2 = cast<IntegerValue>(Env2.getValue(FooLoc1->getChild(*BazDecl))); Index: clang/unittests/Analysis/FlowSensitive/TestingSupport.h =================================================================== --- clang/unittests/Analysis/FlowSensitive/TestingSupport.h +++ clang/unittests/Analysis/FlowSensitive/TestingSupport.h @@ -33,6 +33,7 @@ #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" #include "clang/Analysis/FlowSensitive/MatchSwitch.h" +#include "clang/Analysis/FlowSensitive/NoopAnalysis.h" #include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h" #include "clang/Basic/LLVM.h" #include "clang/Serialization/PCHContainerOperations.h" @@ -381,6 +382,45 @@ }); } +using BuiltinOptions = DataflowAnalysisContext::Options; + +/// Runs dataflow on `Code` with a `NoopAnalysis` and calls `VerifyResults` to +/// verify the results. +template <typename VerifyResultsT> +llvm::Error +runDataflowReturnError(llvm::StringRef Code, VerifyResultsT VerifyResults, + DataflowAnalysisOptions Options, + LangStandard::Kind Std = LangStandard::lang_cxx17, + llvm::StringRef TargetFun = "target") { + using ast_matchers::hasName; + 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 + // platforms. + "-fsyntax-only", "-fno-delayed-template-parsing", + "-std=" + + std::string(LangStandard::getLangStandardForKind(Std).getName())}; + AnalysisInputs<NoopAnalysis> AI( + Code, hasName(TargetFun), + [UseBuiltinModel = Options.BuiltinOpts.has_value()](ASTContext &C, + Environment &Env) { + return NoopAnalysis( + C, + DataflowAnalysisOptions{ + UseBuiltinModel ? Env.getDataflowAnalysisContext().getOptions() + : std::optional<BuiltinOptions>()}); + }); + AI.ASTBuildArgs = ASTBuildArgs; + if (Options.BuiltinOpts) + AI.BuiltinOptions = *Options.BuiltinOpts; + return checkDataflow<NoopAnalysis>( + std::move(AI), + /*VerifyResults=*/ + [&VerifyResults]( + const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + const AnalysisOutputs &AO) { VerifyResults(Results, AO.ASTCtx); }); +} + /// Returns the `ValueDecl` for the given identifier. /// /// Requirements: @@ -388,6 +428,21 @@ /// `Name` must be unique in `ASTCtx`. const ValueDecl *findValueDecl(ASTContext &ASTCtx, llvm::StringRef Name); +/// Returns the storage location (of type `LocT`) for the given identifier. +/// `LocT` must be a subclass of `StorageLocation` and must be of the +/// appropriate type. +/// +/// Requirements: +/// +/// `Name` must be unique in `ASTCtx`. +template <class LocT> +LocT &getLocForDecl(ASTContext &ASTCtx, const Environment &Env, + llvm::StringRef Name) { + const ValueDecl *VD = findValueDecl(ASTCtx, Name); + assert(VD != nullptr); + return *cast<LocT>(Env.getStorageLocation(*VD)); +} + /// Returns the value (of type `ValueT`) for the given identifier. /// `ValueT` must be a subclass of `Value` and must be of the appropriate type. /// Index: clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp =================================================================== --- /dev/null +++ clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp @@ -0,0 +1,205 @@ +//===- unittests/Analysis/FlowSensitive/RecordOpsTest.cpp -----------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/FlowSensitive/RecordOps.h" +#include "TestingSupport.h" +#include "llvm/Testing/Support/Error.h" +#include "gtest/gtest.h" + +namespace clang { +namespace dataflow { +namespace test { +namespace { + +template <typename VerifyResultsT> +void runDataflow(llvm::StringRef Code, VerifyResultsT VerifyResults, + LangStandard::Kind Std = LangStandard::lang_cxx17, + llvm::StringRef TargetFun = "target") { + ASSERT_THAT_ERROR( + runDataflowReturnError(Code, VerifyResults, + DataflowAnalysisOptions{BuiltinOptions{}}, Std, + TargetFun), + llvm::Succeeded()); +} + +TEST(RecordOpsTest, CopyRecord) { + std::string Code = R"( + struct S { + int outer_int; + int &ref; + struct { + int inner_int; + } inner; + }; + void target(S s1, S s2) { + (void)s1.outer_int; + (void)s1.ref; + (void)s1.inner.inner_int; + // [[p]] + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + Environment Env = getEnvironmentAtAnnotation(Results, "p"); + + const ValueDecl *OuterIntDecl = findValueDecl(ASTCtx, "outer_int"); + const ValueDecl *RefDecl = findValueDecl(ASTCtx, "ref"); + const ValueDecl *InnerDecl = findValueDecl(ASTCtx, "inner"); + const ValueDecl *InnerIntDecl = findValueDecl(ASTCtx, "inner_int"); + + auto &S1 = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s1"); + auto &S2 = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s2"); + auto &Inner1 = cast<AggregateStorageLocation>(S1.getChild(*InnerDecl)); + auto &Inner2 = cast<AggregateStorageLocation>(S2.getChild(*InnerDecl)); + + EXPECT_NE(Env.getValue(S1.getChild(*OuterIntDecl)), + Env.getValue(S2.getChild(*OuterIntDecl))); + EXPECT_NE(Env.getValue(S1.getChild(*RefDecl)), + Env.getValue(S2.getChild(*RefDecl))); + EXPECT_NE(Env.getValue(Inner1.getChild(*InnerIntDecl)), + Env.getValue(Inner2.getChild(*InnerIntDecl))); + + auto *S1Val = cast<StructValue>(Env.getValue(S1)); + auto *S2Val = cast<StructValue>(Env.getValue(S2)); + EXPECT_NE(S1Val, S2Val); + + S1Val->setProperty("prop", Env.getBoolLiteralValue(true)); + + copyRecord(S1, S2, Env); + + EXPECT_EQ(Env.getValue(S1.getChild(*OuterIntDecl)), + Env.getValue(S2.getChild(*OuterIntDecl))); + EXPECT_EQ(Env.getValue(S1.getChild(*RefDecl)), + Env.getValue(S2.getChild(*RefDecl))); + EXPECT_EQ(Env.getValue(Inner1.getChild(*InnerIntDecl)), + Env.getValue(Inner2.getChild(*InnerIntDecl))); + + S1Val = cast<StructValue>(Env.getValue(S1)); + S2Val = cast<StructValue>(Env.getValue(S2)); + EXPECT_NE(S1Val, S2Val); + + EXPECT_EQ(S2Val->getProperty("prop"), &Env.getBoolLiteralValue(true)); + }); +} + +TEST(RecordOpsTest, RecordsEqual) { + std::string Code = R"( + struct S { + int outer_int; + int &ref; + struct { + int inner_int; + } inner; + }; + void target(S s1, S s2) { + (void)s1.outer_int; + (void)s1.ref; + (void)s1.inner.inner_int; + // [[p]] + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + Environment Env = getEnvironmentAtAnnotation(Results, "p"); + + const ValueDecl *OuterIntDecl = findValueDecl(ASTCtx, "outer_int"); + const ValueDecl *RefDecl = findValueDecl(ASTCtx, "ref"); + const ValueDecl *InnerDecl = findValueDecl(ASTCtx, "inner"); + const ValueDecl *InnerIntDecl = findValueDecl(ASTCtx, "inner_int"); + + auto &S1 = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s1"); + auto &S2 = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "s2"); + auto &Inner2 = cast<AggregateStorageLocation>(S2.getChild(*InnerDecl)); + + cast<StructValue>(Env.getValue(S1)) + ->setProperty("prop", Env.getBoolLiteralValue(true)); + + // Strategy: Create two equal records, then verify each of the various + // ways in which records can differ causes recordsEqual to return false. + // changes we can make to the record. + + // This test reuses the same objects for multiple checks, which isn't + // great, but seems better than duplicating the setup code for every + // check. + + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S2 has a different outer_int. + Env.setValue(S2.getChild(*OuterIntDecl), Env.create<IntegerValue>()); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S2 doesn't have outer_int at all. + Env.clearValue(S2.getChild(*OuterIntDecl)); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S2 has a different ref. + Env.setValue(S2.getChild(*RefDecl), + Env.create<ReferenceValue>(Env.createStorageLocation( + RefDecl->getType().getNonReferenceType()))); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S2 doesn't have ref at all. + Env.clearValue(S2.getChild(*RefDecl)); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S2 as a different inner_int. + Env.setValue(Inner2.getChild(*InnerIntDecl), + Env.create<IntegerValue>()); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S1 and S2 have the same property with different values. + cast<StructValue>(Env.getValue(S2)) + ->setProperty("prop", Env.getBoolLiteralValue(false)); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S1 has a property that S2 doesn't have. + cast<StructValue>(Env.getValue(S1)) + ->setProperty("other_prop", Env.getBoolLiteralValue(false)); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + // We modified S1 this time, so need to copy back the other way. + copyRecord(S2, S1, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S2 has a property that S1 doesn't have. + cast<StructValue>(Env.getValue(S2)) + ->setProperty("other_prop", Env.getBoolLiteralValue(false)); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S1 and S2 have the same number of properties, but with different + // names. + cast<StructValue>(Env.getValue(S1)) + ->setProperty("prop1", Env.getBoolLiteralValue(false)); + cast<StructValue>(Env.getValue(S2)) + ->setProperty("prop2", Env.getBoolLiteralValue(false)); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + }); +} + +} // namespace +} // namespace test +} // namespace dataflow +} // namespace clang Index: clang/unittests/Analysis/FlowSensitive/CMakeLists.txt =================================================================== --- clang/unittests/Analysis/FlowSensitive/CMakeLists.txt +++ clang/unittests/Analysis/FlowSensitive/CMakeLists.txt @@ -14,6 +14,7 @@ MapLatticeTest.cpp MatchSwitchTest.cpp MultiVarConstantPropagationTest.cpp + RecordOpsTest.cpp SignAnalysisTest.cpp SingleVarConstantPropagationTest.cpp SolverTest.cpp Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp =================================================================== --- clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -23,6 +23,7 @@ #include "clang/Analysis/FlowSensitive/ControlFlowContext.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" #include "clang/Analysis/FlowSensitive/NoopAnalysis.h" +#include "clang/Analysis/FlowSensitive/RecordOps.h" #include "clang/Analysis/FlowSensitive/Value.h" #include "clang/Basic/Builtins.h" #include "clang/Basic/OperatorKinds.h" @@ -591,16 +592,21 @@ const Expr *Arg = S->getArg(0); assert(Arg != nullptr); - if (S->isElidable()) { - auto *ArgLoc = Env.getStorageLocation(*Arg, SkipPast::Reference); - if (ArgLoc == nullptr) - return; + auto *ArgLoc = cast<AggregateStorageLocation>( + Env.getStorageLocation(*Arg, SkipPast::Reference)); + if (ArgLoc == nullptr) + return; + if (S->isElidable()) { Env.setStorageLocation(*S, *ArgLoc); - } else if (auto *ArgVal = Env.getValue(*Arg, SkipPast::Reference)) { - auto &Loc = Env.createStorageLocation(*S); + } else { + auto &Loc = + cast<AggregateStorageLocation>(Env.createStorageLocation(*S)); Env.setStorageLocation(*S, Loc); - Env.setValue(Loc, *ArgVal); + if (Value *Val = Env.createValue(S->getType())) { + Env.setValue(Loc, *Val); + copyRecord(*ArgLoc, Loc, Env); + } } return; } @@ -632,16 +638,17 @@ !Method->isMoveAssignmentOperator()) return; - auto *ObjectLoc = Env.getStorageLocation(*Arg0, SkipPast::Reference); + auto *ObjectLoc = cast_or_null<AggregateStorageLocation>( + Env.getStorageLocation(*Arg0, SkipPast::Reference)); if (ObjectLoc == nullptr) return; - auto *Val = Env.getValue(*Arg1, SkipPast::Reference); - if (Val == nullptr) + auto *ArgLoc = cast_or_null<AggregateStorageLocation>( + Env.getStorageLocation(*Arg1, SkipPast::Reference)); + if (ArgLoc == nullptr) return; - // Assign a value to the storage location of the object. - Env.setValue(*ObjectLoc, *Val); + copyRecord(*ArgLoc, *ObjectLoc, Env); // FIXME: Add a test for the value of the whole expression. // Assign a storage location for the whole expression. Index: clang/lib/Analysis/FlowSensitive/RecordOps.cpp =================================================================== --- /dev/null +++ clang/lib/Analysis/FlowSensitive/RecordOps.cpp @@ -0,0 +1,133 @@ +//===-- RecordOps.cpp -------------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// Operations on records (structs, classes, and unions). +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/FlowSensitive/RecordOps.h" + +#define DEBUG_TYPE "dataflow" + +namespace clang { +namespace dataflow { + +void copyRecord(AggregateStorageLocation &Src, AggregateStorageLocation &Dst, + Environment &Env) { + QualType Type = Src.getType(); + + LLVM_DEBUG({ + if (Dst.getType().getCanonicalType().getUnqualifiedType() != + Type.getCanonicalType().getUnqualifiedType()) { + llvm::dbgs() << "Source type " << Type << "\n"; + llvm::dbgs() << "Destination type " << Dst.getType() << "\n"; + } + }); + assert(Dst.getType().getCanonicalType().getUnqualifiedType() == + Type.getCanonicalType().getUnqualifiedType()); + + for (auto [Field, SrcFieldLoc] : Src.children()) { + assert(SrcFieldLoc != nullptr); + + StorageLocation &DstFieldLoc = Dst.getChild(*Field); + + if (Field->getType()->isRecordType()) { + copyRecord(cast<AggregateStorageLocation>(*SrcFieldLoc), + cast<AggregateStorageLocation>(DstFieldLoc), Env); + } else { + if (Value *Val = Env.getValue(*SrcFieldLoc)) + Env.setValue(DstFieldLoc, *Val); + else + Env.clearValue(DstFieldLoc); + } + } + + StructValue *SrcVal = cast_or_null<StructValue>(Env.getValue(Src)); + StructValue *DstVal = cast_or_null<StructValue>(Env.getValue(Dst)); + + if (SrcVal == nullptr || DstVal == nullptr) + return; + + auto DstChildren = DstVal->children(); + DstVal = &Env.create<StructValue>(llvm::DenseMap<const ValueDecl *, Value *>( + DstChildren.begin(), DstChildren.end())); + Env.setValue(Dst, *DstVal); + + for (const auto &[Name, Value] : SrcVal->properties()) { + if (Value != nullptr) + DstVal->setProperty(Name, *Value); + } +} + +bool recordsEqual(const AggregateStorageLocation &Loc1, const Environment &Env1, + const AggregateStorageLocation &Loc2, + const Environment &Env2) { + QualType Type = Loc1.getType(); + + LLVM_DEBUG({ + if (Loc2.getType().getCanonicalType().getUnqualifiedType() != + Type.getCanonicalType().getUnqualifiedType()) { + llvm::dbgs() << "Loc1 type " << Type << "\n"; + llvm::dbgs() << "Loc2 type " << Loc2.getType() << "\n"; + } + }); + assert(Loc2.getType().getCanonicalType().getUnqualifiedType() == + Type.getCanonicalType().getUnqualifiedType()); + + for (auto [Field, FieldLoc1] : Loc1.children()) { + assert(FieldLoc1 != nullptr); + + StorageLocation &FieldLoc2 = Loc2.getChild(*Field); + + if (Field->getType()->isRecordType()) { + if (!recordsEqual(cast<AggregateStorageLocation>(*FieldLoc1), Env1, + cast<AggregateStorageLocation>(FieldLoc2), Env2)) + return false; + } else if (Field->getType()->isReferenceType()) { + auto *RefVal1 = cast_or_null<ReferenceValue>(Env1.getValue(*FieldLoc1)); + auto *RefVal2 = cast_or_null<ReferenceValue>(Env1.getValue(FieldLoc2)); + if (RefVal1 && RefVal2) { + if (&RefVal1->getReferentLoc() != &RefVal2->getReferentLoc()) + return false; + } else { + // If either of `RefVal1` and `RefVal2` is null, we only consider them + // equal if they're both null. + if (RefVal1 || RefVal2) + return false; + } + } else { + if (Env1.getValue(*FieldLoc1) != Env2.getValue(FieldLoc2)) + return false; + } + } + + llvm::StringMap<Value *> Props1, Props2; + + if (StructValue *Val1 = cast_or_null<StructValue>(Env1.getValue(Loc1))) + for (const auto &[Name, Value] : Val1->properties()) + Props1[Name] = Value; + if (StructValue *Val2 = cast_or_null<StructValue>(Env2.getValue(Loc2))) + for (const auto &[Name, Value] : Val2->properties()) + Props2[Name] = Value; + + if (Props1.size() != Props2.size()) + return false; + + for (const auto &[Name, Value] : Props1) { + auto It = Props2.find(Name); + if (It == Props2.end()) + return false; + if (Value != It->second) + return false; + } + + return true; +} + +} // namespace dataflow +} // namespace clang Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp =================================================================== --- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -748,6 +748,23 @@ } } +void Environment::clearValue(const StorageLocation &Loc) { + LocToVal.erase(&Loc); + + if (auto It = MemberLocToStruct.find(&Loc); It != MemberLocToStruct.end()) { + // `Loc` is the location of a struct member so we need to also clear the + // member in the corresponding `StructValue`. + + assert(It->second.first != nullptr); + StructValue &StructVal = *It->second.first; + + assert(It->second.second != nullptr); + const ValueDecl &Member = *It->second.second; + + StructVal.clearChild(Member); + } +} + void Environment::setValueStrict(const Expr &E, Value &Val) { assert(E.isPRValue()); assert(!isa<ReferenceValue>(Val)); Index: clang/lib/Analysis/FlowSensitive/CMakeLists.txt =================================================================== --- clang/lib/Analysis/FlowSensitive/CMakeLists.txt +++ clang/lib/Analysis/FlowSensitive/CMakeLists.txt @@ -5,6 +5,7 @@ DataflowEnvironment.cpp HTMLLogger.cpp Logger.cpp + RecordOps.cpp Transfer.cpp TypeErasedDataflowAnalysis.cpp Value.cpp Index: clang/include/clang/Analysis/FlowSensitive/Value.h =================================================================== --- clang/include/clang/Analysis/FlowSensitive/Value.h +++ clang/include/clang/Analysis/FlowSensitive/Value.h @@ -306,6 +306,9 @@ /// Assigns `Val` as the child value for `D`. void setChild(const ValueDecl &D, Value &Val) { Children[&D] = &Val; } + /// Clears any value assigned as the child value for `D`. + void clearChild(const ValueDecl &D) { Children.erase(&D); } + llvm::iterator_range< llvm::DenseMap<const ValueDecl *, Value *>::const_iterator> children() const { Index: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h =================================================================== --- clang/include/clang/Analysis/FlowSensitive/StorageLocation.h +++ clang/include/clang/Analysis/FlowSensitive/StorageLocation.h @@ -70,13 +70,12 @@ /// that all of the members of a given union have the same storage location. class AggregateStorageLocation final : public StorageLocation { public: + using FieldToLoc = llvm::DenseMap<const ValueDecl *, StorageLocation *>; + explicit AggregateStorageLocation(QualType Type) - : AggregateStorageLocation( - Type, llvm::DenseMap<const ValueDecl *, StorageLocation *>()) {} + : AggregateStorageLocation(Type, FieldToLoc()) {} - AggregateStorageLocation( - QualType Type, - llvm::DenseMap<const ValueDecl *, StorageLocation *> Children) + AggregateStorageLocation(QualType Type, FieldToLoc Children) : StorageLocation(Kind::Aggregate, Type), Children(std::move(Children)) {} static bool classof(const StorageLocation *Loc) { @@ -90,8 +89,12 @@ return *It->second; } + llvm::iterator_range<FieldToLoc::const_iterator> children() const { + return {Children.begin(), Children.end()}; + } + private: - llvm::DenseMap<const ValueDecl *, StorageLocation *> Children; + FieldToLoc Children; }; } // namespace dataflow Index: clang/include/clang/Analysis/FlowSensitive/RecordOps.h =================================================================== --- /dev/null +++ clang/include/clang/Analysis/FlowSensitive/RecordOps.h @@ -0,0 +1,71 @@ +//===-- RecordOps.h ---------------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// Operations on records (structs, classes, and unions). +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_RECORDOPS_H +#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_RECORDOPS_H + +#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" +#include "clang/Analysis/FlowSensitive/StorageLocation.h" + +namespace clang { +namespace dataflow { + +/// Copies a record (struct, class, or union) from `Src` to `Dst`. +/// +/// This performs a deep copy, i.e. it copies every field and recurses on +/// fields of record type. It also copies properties from the `StructValue` +/// associated with `Dst` to the `StructValue` associated with `Src` (if these +/// `StructValue`s exist). +/// +/// If there is a `StructValue` associated with `Dst` in the environment, this +/// function creates a new `StructValue` and associates it with `Dst`; clients +/// need to be aware of this and must not assume that the `StructValue` +/// associated with `Dst` remains the same after the call. +/// +/// We create a new `StructValue` rather than modifying properties on the old +/// `StructValue` because the old `StructValue` may be shared with other +/// `Environment`s, and we don't want changes to properties to be visible there. +/// +/// Requirements: +/// +/// `Src` and `Dst` must have the same canonical unqualified type. +void copyRecord(AggregateStorageLocation &Src, AggregateStorageLocation &Dst, + Environment &Env); + +/// Returns whether the records `Loc1` and `Loc2` are equal. +/// +/// Values for `Loc1` are retrieved from `Env1`, and values for `Loc2` are +/// retrieved from `Env2`. A convenience overload retrieves values for `Loc1` +/// and `Loc2` from the same environment. +/// +/// This performs a deep comparison, i.e. it compares every field and recurses +/// on fields of record type. Fields of reference type compare equal if they +/// refer to the same storage location. If `StructValue`s are associated with +/// `Loc1` and `Loc2`, it also compares the properties on those `StructValue`s. +/// +/// Requirements: +/// +/// `Src` and `Dst` must have the same canonical unqualified type. +bool recordsEqual(const AggregateStorageLocation &Loc1, const Environment &Env1, + const AggregateStorageLocation &Loc2, + const Environment &Env2); + +inline bool recordsEqual(const AggregateStorageLocation &Loc1, + const AggregateStorageLocation &Loc2, + const Environment &Env) { + return recordsEqual(Loc1, Env, Loc2, Env); +} + +} // namespace dataflow +} // namespace clang + +#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_RECORDOPS_H Index: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h =================================================================== --- clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -387,6 +387,9 @@ /// Assigns `Val` as the value of `Loc` in the environment. void setValue(const StorageLocation &Loc, Value &Val); + /// Clears any association between `Loc` and a value in the environment. + void clearValue(const StorageLocation &Loc); + /// Assigns `Val` as the value of the prvalue `E` in the environment. /// /// If `E` is not yet associated with a storage location, associates it with
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits