https://github.com/martinboehme created https://github.com/llvm/llvm-project/pull/65319:
Now that prvalue expressions map directly to values (see https://reviews.llvm.org/D158977), it's no longer guaranteed that `RecordValue`s associated with the same expression will always have the same storage location. In other words, D158977 invalidated the assertion in `mergeDistinctValues()`. The newly added test causes this assertion to fail without the other changes in the patch. This patch fixes the issue. However, the real fix will be to eliminate the `StorageLocation` from `RecordValue` entirely. >From 5c5723da55fb26204c7ca9ed170c4703d3f3927b Mon Sep 17 00:00:00 2001 From: Martin Braenne <mboe...@google.com> Date: Tue, 5 Sep 2023 12:32:05 +0000 Subject: [PATCH] [clang][dataflow] Merge `RecordValue`s with different locations correctly. Now that prvalue expressions map directly to values (see https://reviews.llvm.org/D158977), it's no longer guaranteed that `RecordValue`s associated with the same expression will always have the same storage location. In other words, D158977 invalidated the assertion in `mergeDistinctValues()`. The newly added test causes this assertion to fail without the other changes in the patch. This patch fixes the issue. However, the real fix will be to eliminate the `StorageLocation` from `RecordValue` entirely. --- .../FlowSensitive/DataflowEnvironment.cpp | 24 +++---- .../FlowSensitive/DataflowEnvironmentTest.cpp | 69 +++++++++++++++++++ 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index b40fbbc991c8f8e..7ce7ee7e579eec9 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -121,18 +121,18 @@ static Value *mergeDistinctValues(QualType Type, Value &Val1, Value *MergedVal = nullptr; if (auto *RecordVal1 = dyn_cast<RecordValue>(&Val1)) { - [[maybe_unused]] auto *RecordVal2 = cast<RecordValue>(&Val2); - - // Values to be merged are always associated with the same location in - // `LocToVal`. The location stored in `RecordVal` should therefore also - // be the same. - assert(&RecordVal1->getLoc() == &RecordVal2->getLoc()); - - // `RecordVal1` and `RecordVal2` may have different properties associated - // with them. Create a new `RecordValue` without any properties so that we - // soundly approximate both values. If a particular analysis needs to merge - // properties, it should do so in `DataflowAnalysis::merge()`. - MergedVal = &MergedEnv.create<RecordValue>(RecordVal1->getLoc()); + auto *RecordVal2 = cast<RecordValue>(&Val2); + + if (&RecordVal1->getLoc() == &RecordVal2->getLoc()) + // `RecordVal1` and `RecordVal2` may have different properties associated + // with them. Create a new `RecordValue` without any properties so that we + // soundly approximate both values. If a particular analysis needs to + // merge properties, it should do so in `DataflowAnalysis::merge()`. + MergedVal = &MergedEnv.create<RecordValue>(RecordVal1->getLoc()); + else + // If the locations for the two records are different, need to create a + // completely new value. + MergedVal = MergedEnv.createValue(Type); } else { MergedVal = MergedEnv.createValue(Type); } diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp index a318d9ab7391aa1..6e37011a052c5e4 100644 --- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp @@ -96,6 +96,75 @@ TEST_F(EnvironmentTest, CreateValueRecursiveType) { EXPECT_THAT(PV, NotNull()); } +TEST_F(EnvironmentTest, JoinRecords) { + using namespace ast_matchers; + + std::string Code = R"cc( + struct S {}; + // Need to use the type somewhere so that the `QualType` gets created; + S s; + )cc"; + + auto Unit = + tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"}); + auto &Context = Unit->getASTContext(); + + ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U); + + auto Results = + match(qualType(hasDeclaration(recordDecl(hasName("S")))).bind("SType"), + Context); + const QualType *TyPtr = selectFirst<QualType>("SType", Results); + ASSERT_THAT(TyPtr, NotNull()); + QualType Ty = *TyPtr; + ASSERT_FALSE(Ty.isNull()); + + auto *ConstructExpr = CXXConstructExpr::CreateEmpty(Context, 0); + ConstructExpr->setType(Ty); + ConstructExpr->setValueKind(VK_PRValue); + + // Two different `RecordValue`s with the same location are joined into a + // third `RecordValue` with that same location. + { + Environment Env1(DAContext); + auto &Val1 = *cast<RecordValue>(Env1.createValue(Ty)); + RecordStorageLocation &Loc = Val1.getLoc(); + Env1.setValue(*ConstructExpr, Val1); + + Environment Env2(DAContext); + auto &Val2 = Env2.create<RecordValue>(Loc); + Env2.setValue(Loc, Val2); + Env2.setValue(*ConstructExpr, Val2); + + Environment::ValueModel Model; + Environment EnvJoined = Environment::join(Env1, Env2, Model); + auto *JoinedVal = cast<RecordValue>(EnvJoined.getValue(*ConstructExpr)); + EXPECT_NE(JoinedVal, &Val1); + EXPECT_NE(JoinedVal, &Val2); + EXPECT_EQ(&JoinedVal->getLoc(), &Loc); + } + + // Two different `RecordValue`s with different locations are joined into a + // third `RecordValue` with a location different from the other two. + { + Environment Env1(DAContext); + auto &Val1 = *cast<RecordValue>(Env1.createValue(Ty)); + Env1.setValue(*ConstructExpr, Val1); + + Environment Env2(DAContext); + auto &Val2 = *cast<RecordValue>(Env2.createValue(Ty)); + Env2.setValue(*ConstructExpr, Val2); + + Environment::ValueModel Model; + Environment EnvJoined = Environment::join(Env1, Env2, Model); + auto *JoinedVal = cast<RecordValue>(EnvJoined.getValue(*ConstructExpr)); + EXPECT_NE(JoinedVal, &Val1); + EXPECT_NE(JoinedVal, &Val2); + EXPECT_NE(&JoinedVal->getLoc(), &Val1.getLoc()); + EXPECT_NE(&JoinedVal->getLoc(), &Val2.getLoc()); + } +} + TEST_F(EnvironmentTest, InitGlobalVarsFun) { using namespace ast_matchers; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
