diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index 2a9f8dce74c0a..e8c27d6c12038 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -727,20 +727,9 @@ RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME, std::vector getFieldsForInitListExpr(const RecordDecl *RD); /// Associates a new `RecordValue` with `Loc` and returns the new value. -/// It is not defined whether the field values remain the same or not. -/// -/// This function is primarily intended for use by checks that set custom -/// properties on `RecordValue`s to model the state of these values. Such checks -/// should avoid modifying the properties of an existing `RecordValue` because -/// these changes would be visible to other `Environment`s that share the same -/// `RecordValue`. Instead, call `refreshRecordValue()`, then set the properties -/// on the new `RecordValue` that it returns. Typical usage: -/// -/// refreshRecordValue(Loc, Env).setProperty("my_prop", MyPropValue); RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env); /// Associates a new `RecordValue` with `Expr` and returns the new value. -/// See also documentation for the overload above. RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env); } // namespace dataflow diff --git a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h index 7b87840d626b4..783e53e980aa2 100644 --- a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h +++ b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h @@ -22,19 +22,13 @@ namespace dataflow { /// Copies a record (struct, class, or union) from `Src` to `Dst`. /// /// This performs a deep copy, i.e. it copies every field (including synthetic -/// fields) and recurses on fields of record type. It also copies properties -/// from the `RecordValue` associated with `Src` to the `RecordValue` associated -/// with `Dst` (if these `RecordValue`s exist). +/// fields) and recurses on fields of record type. /// /// If there is a `RecordValue` associated with `Dst` in the environment, this /// function creates a new `RecordValue` and associates it with `Dst`; clients /// need to be aware of this and must not assume that the `RecordValue` /// associated with `Dst` remains the same after the call. /// -/// We create a new `RecordValue` rather than modifying properties on the old -/// `RecordValue` because the old `RecordValue` 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. @@ -49,9 +43,7 @@ void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst, /// /// This performs a deep comparison, i.e. it compares every field (including /// synthetic fields) and recurses on fields of record type. Fields of reference -/// type compare equal if they refer to the same storage location. If -/// `RecordValue`s are associated with `Loc1` and Loc2`, it also compares the -/// properties on those `RecordValue`s. +/// type compare equal if they refer to the same storage location. /// /// Note on how to interpret the result: /// - If this returns true, the records are guaranteed to be equal at runtime. diff --git a/clang/include/clang/Analysis/FlowSensitive/Value.h b/clang/include/clang/Analysis/FlowSensitive/Value.h index e6c68e5b4e93e..be1bf9324c87b 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Value.h +++ b/clang/include/clang/Analysis/FlowSensitive/Value.h @@ -63,7 +63,11 @@ class Value { /// Assigns `Val` as the value of the synthetic property with the given /// `Name`. + /// + /// Properties may not be set on `RecordValue`s; use synthetic fields instead + /// (for details, see documentation for `RecordStorageLocation`). void setProperty(llvm::StringRef Name, Value &Val) { + assert(getKind() != Kind::Record); Properties.insert_or_assign(Name, &Val); } @@ -184,33 +188,23 @@ class PointerValue final : public Value { /// In C++, prvalues of class type serve only a limited purpose: They can only /// be used to initialize a result object. It is not possible to access member /// variables or call member functions on a prvalue of class type. -/// Correspondingly, `RecordValue` also serves only two limited purposes: -/// - It conveys a prvalue of class type from the place where the object is -/// constructed to the result object that it initializes. +/// Correspondingly, `RecordValue` also serves only a limited purpose: It +/// conveys a prvalue of class type from the place where the object is +/// constructed to the result object that it initializes. /// -/// When creating a prvalue of class type, we already need a storage location -/// for `this`, even though prvalues are otherwise not associated with storage -/// locations. `RecordValue` is therefore essentially a wrapper for a storage -/// location, which is then used to set the storage location for the result -/// object when we process the AST node for that result object. +/// When creating a prvalue of class type, we already need a storage location +/// for `this`, even though prvalues are otherwise not associated with storage +/// locations. `RecordValue` is therefore essentially a wrapper for a storage +/// location, which is then used to set the storage location for the result +/// object when we process the AST node for that result object. /// -/// For example: -/// MyStruct S = MyStruct(3); +/// For example: +/// MyStruct S = MyStruct(3); /// -/// In this example, `MyStruct(3) is a prvalue, which is modeled as a -/// `RecordValue` that wraps a `RecordStorageLocation`. This -// `RecordStorageLocation` is then used as the storage location for `S`. +/// In this example, `MyStruct(3) is a prvalue, which is modeled as a +/// `RecordValue` that wraps a `RecordStorageLocation`. This +/// `RecordStorageLocation` is then used as the storage location for `S`. /// -/// - It allows properties to be associated with an object of class type. -/// Note that when doing so, you should avoid mutating the properties of an -/// existing `RecordValue` in place, as these changes would be visible to -/// other `Environment`s that share the same `RecordValue`. Instead, associate -/// a new `RecordValue` with the `RecordStorageLocation` and set the -/// properties on this new `RecordValue`. (See also `refreshRecordValue()` in -/// DataflowEnvironment.h, which makes this easy.) -/// Note also that this implies that it is common for the same -/// `RecordStorageLocation` to be associated with different `RecordValue`s -/// in different environments. /// Over time, we may eliminate `RecordValue` entirely. See also the discussion /// here: https://reviews.llvm.org/D155204#inline-1503204 class RecordValue final : public Value { diff --git a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp index a326826db2394..da4dd6dc07851 100644 --- a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp +++ b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp @@ -66,19 +66,8 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src, } } - RecordValue *SrcVal = Env.get(Src); - RecordValue *DstVal = Env.get(Dst); - - DstVal = &Env.create(Dst); + RecordValue *DstVal = &Env.create(Dst); Env.setValue(Dst, *DstVal); - - if (SrcVal == nullptr) - return; - - for (const auto &[Name, Value] : SrcVal->properties()) { - if (Value != nullptr) - DstVal->setProperty(Name, *Value); - } } bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1, @@ -125,25 +114,5 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1, } } - llvm::StringMap Props1, Props2; - - if (RecordValue *Val1 = Env1.get(Loc1)) - for (const auto &[Name, Value] : Val1->properties()) - Props1[Name] = Value; - if (RecordValue *Val2 = Env2.get(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; } diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp index 84fe675c32c2d..cd6a37d370e85 100644 --- a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp @@ -89,8 +89,6 @@ TEST(RecordOpsTest, CopyRecord) { auto *S2Val = cast(Env.getValue(S2)); EXPECT_NE(S1Val, S2Val); - S1Val->setProperty("prop", Env.getBoolLiteralValue(true)); - copyRecord(S1, S2, Env); EXPECT_EQ(getFieldValue(&S1, *OuterIntDecl, Env), @@ -104,8 +102,6 @@ TEST(RecordOpsTest, CopyRecord) { S1Val = cast(Env.getValue(S1)); S2Val = cast(Env.getValue(S2)); EXPECT_NE(S1Val, S2Val); - - EXPECT_EQ(S2Val->getProperty("prop"), &Env.getBoolLiteralValue(true)); }); } @@ -150,9 +146,6 @@ TEST(RecordOpsTest, RecordsEqual) { Env.setValue(S1.getSyntheticField("synth_int"), Env.create()); - cast(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. @@ -202,36 +195,6 @@ TEST(RecordOpsTest, RecordsEqual) { 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(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(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(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(Env.getValue(S1)) - ->setProperty("prop1", Env.getBoolLiteralValue(false)); - cast(Env.getValue(S2)) - ->setProperty("prop2", Env.getBoolLiteralValue(false)); - EXPECT_FALSE(recordsEqual(S1, S2, Env)); }); } diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index 4c3cb322eacfb..8d481788af208 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -623,11 +623,11 @@ TEST_F(JoinFlowConditionsTest, JoinDistinctButProvablyEquivalentValues) { }); } -class OptionalIntAnalysis final - : public DataflowAnalysis { +class NullPointerAnalysis final + : public DataflowAnalysis { public: - explicit OptionalIntAnalysis(ASTContext &Context) - : DataflowAnalysis(Context) {} + explicit NullPointerAnalysis(ASTContext &Context) + : DataflowAnalysis(Context) {} static NoopLattice initialElement() { return {}; } @@ -636,40 +636,37 @@ class OptionalIntAnalysis final if (!CS) return; const Stmt *S = CS->getStmt(); - auto OptionalIntRecordDecl = recordDecl(hasName("OptionalInt")); - auto HasOptionalIntType = hasType(OptionalIntRecordDecl); - - SmallVector Matches = match( - stmt(anyOf(cxxConstructExpr(HasOptionalIntType).bind("construct"), - cxxOperatorCallExpr( - callee(cxxMethodDecl(ofClass(OptionalIntRecordDecl)))) - .bind("operator"))), - *S, getASTContext()); - if (const auto *E = selectFirst( - "construct", Matches)) { - cast(Env.getValue(*E)) - ->setProperty("has_value", Env.getBoolLiteralValue(false)); - } else if (const auto *E = - selectFirst("operator", Matches)) { - assert(E->getNumArgs() > 0); - auto *Object = E->getArg(0); - assert(Object != nullptr); - - refreshRecordValue(*Object, Env) - .setProperty("has_value", Env.getBoolLiteralValue(true)); + const Expr *E = dyn_cast(S); + if (!E) + return; + + if (!E->getType()->isPointerType()) + return; + + // Make sure we have a `PointerValue` for `E`. + auto *PtrVal = cast_or_null(Env.getValue(*E)); + if (PtrVal == nullptr) { + PtrVal = cast(Env.createValue(E->getType())); + Env.setValue(*E, *PtrVal); } + + if (auto *Cast = dyn_cast(E); + Cast && Cast->getCastKind() == CK_NullToPointer) + PtrVal->setProperty("is_null", Env.getBoolLiteralValue(true)); + else if (auto *Op = dyn_cast(E); + Op && Op->getOpcode() == UO_AddrOf) + PtrVal->setProperty("is_null", Env.getBoolLiteralValue(false)); } ComparisonResult compare(QualType Type, const Value &Val1, const Environment &Env1, const Value &Val2, const Environment &Env2) override { - // Nothing to say about a value that does not model an `OptionalInt`. - if (!Type->isRecordType() || - Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt") + // Nothing to say about a value that is not a pointer. + if (!Type->isPointerType()) return ComparisonResult::Unknown; - auto *Prop1 = Val1.getProperty("has_value"); - auto *Prop2 = Val2.getProperty("has_value"); + auto *Prop1 = Val1.getProperty("is_null"); + auto *Prop2 = Val2.getProperty("is_null"); assert(Prop1 != nullptr && Prop2 != nullptr); return areEquivalentValues(*Prop1, *Prop2) ? ComparisonResult::Same : ComparisonResult::Different; @@ -678,23 +675,22 @@ class OptionalIntAnalysis final bool merge(QualType Type, const Value &Val1, const Environment &Env1, const Value &Val2, const Environment &Env2, Value &MergedVal, Environment &MergedEnv) override { - // Nothing to say about a value that does not model an `OptionalInt`. - if (!Type->isRecordType() || - Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt") + // Nothing to say about a value that is not a pointer. + if (!Type->isPointerType()) return false; - auto *HasValue1 = cast_or_null(Val1.getProperty("has_value")); - if (HasValue1 == nullptr) + auto *IsNull1 = cast_or_null(Val1.getProperty("is_null")); + if (IsNull1 == nullptr) return false; - auto *HasValue2 = cast_or_null(Val2.getProperty("has_value")); - if (HasValue2 == nullptr) + auto *IsNull2 = cast_or_null(Val2.getProperty("is_null")); + if (IsNull2 == nullptr) return false; - if (HasValue1 == HasValue2) - MergedVal.setProperty("has_value", *HasValue1); + if (IsNull1 == IsNull2) + MergedVal.setProperty("is_null", *IsNull1); else - MergedVal.setProperty("has_value", MergedEnv.makeTopBoolValue()); + MergedVal.setProperty("is_null", MergedEnv.makeTopBoolValue()); return true; } }; @@ -703,23 +699,14 @@ class WideningTest : public Test { protected: template void runDataflow(llvm::StringRef Code, Matcher Match) { - tooling::FileContentMappings FilesContents; - FilesContents.push_back( - std::make_pair("widening_test_defs.h", R"( - struct OptionalInt { - OptionalInt() = default; - OptionalInt& operator=(int); - }; - )")); ASSERT_THAT_ERROR( - checkDataflow( - AnalysisInputs( + checkDataflow( + AnalysisInputs( Code, ast_matchers::hasName("target"), [](ASTContext &Context, Environment &Env) { - return OptionalIntAnalysis(Context); + return NullPointerAnalysis(Context); }) - .withASTBuildArgs({"-fsyntax-only", "-std=c++17"}) - .withASTBuildVirtualMappedFiles(std::move(FilesContents)), + .withASTBuildArgs({"-fsyntax-only", "-std=c++17"}), /*VerifyResults=*/[&Match](const llvm::StringMap< DataflowAnalysisState> &Results, @@ -731,13 +718,12 @@ class WideningTest : public Test { TEST_F(WideningTest, JoinDistinctValuesWithDistinctProperties) { std::string Code = R"( - #include "widening_test_defs.h" - void target(bool Cond) { - OptionalInt Foo; + int *Foo = nullptr; + int i = 0; /*[[p1]]*/ if (Cond) { - Foo = 1; + Foo = &i; /*[[p2]]*/ } (void)0; @@ -760,27 +746,27 @@ TEST_F(WideningTest, JoinDistinctValuesWithDistinctProperties) { return Env.getValue(*FooDecl); }; - EXPECT_EQ(GetFooValue(Env1)->getProperty("has_value"), - &Env1.getBoolLiteralValue(false)); - EXPECT_EQ(GetFooValue(Env2)->getProperty("has_value"), - &Env2.getBoolLiteralValue(true)); + EXPECT_EQ(GetFooValue(Env1)->getProperty("is_null"), + &Env1.getBoolLiteralValue(true)); + EXPECT_EQ(GetFooValue(Env2)->getProperty("is_null"), + &Env2.getBoolLiteralValue(false)); EXPECT_TRUE( - isa(GetFooValue(Env3)->getProperty("has_value"))); + isa(GetFooValue(Env3)->getProperty("is_null"))); }); } TEST_F(WideningTest, JoinDistinctValuesWithSameProperties) { std::string Code = R"( - #include "widening_test_defs.h" - void target(bool Cond) { - OptionalInt Foo; + int *Foo = nullptr; + int i1 = 0; + int i2 = 0; /*[[p1]]*/ if (Cond) { - Foo = 1; + Foo = &i1; /*[[p2]]*/ } else { - Foo = 2; + Foo = &i2; /*[[p3]]*/ } (void)0; @@ -805,14 +791,14 @@ TEST_F(WideningTest, JoinDistinctValuesWithSameProperties) { return Env.getValue(*FooDecl); }; - EXPECT_EQ(GetFooValue(Env1)->getProperty("has_value"), - &Env1.getBoolLiteralValue(false)); - EXPECT_EQ(GetFooValue(Env2)->getProperty("has_value"), - &Env2.getBoolLiteralValue(true)); - EXPECT_EQ(GetFooValue(Env3)->getProperty("has_value"), - &Env3.getBoolLiteralValue(true)); - EXPECT_EQ(GetFooValue(Env4)->getProperty("has_value"), - &Env4.getBoolLiteralValue(true)); + EXPECT_EQ(GetFooValue(Env1)->getProperty("is_null"), + &Env1.getBoolLiteralValue(true)); + EXPECT_EQ(GetFooValue(Env2)->getProperty("is_null"), + &Env2.getBoolLiteralValue(false)); + EXPECT_EQ(GetFooValue(Env3)->getProperty("is_null"), + &Env3.getBoolLiteralValue(false)); + EXPECT_EQ(GetFooValue(Env4)->getProperty("is_null"), + &Env4.getBoolLiteralValue(false)); }); } @@ -849,13 +835,13 @@ TEST_F(WideningTest, DistinctPointersToTheSameLocationAreEquivalent) { TEST_F(WideningTest, DistinctValuesWithSamePropertiesAreEquivalent) { std::string Code = R"( - #include "widening_test_defs.h" - void target(bool Cond) { - OptionalInt Foo; - Foo = 1; + int *Foo; + int i1 = 0; + int i2 = 0; + Foo = &i1; while (Cond) { - Foo = 2; + Foo = &i2; } (void)0; /*[[p]]*/ @@ -872,8 +858,8 @@ TEST_F(WideningTest, DistinctValuesWithSamePropertiesAreEquivalent) { ASSERT_THAT(FooDecl, NotNull()); const auto *FooVal = Env.getValue(*FooDecl); - EXPECT_EQ(FooVal->getProperty("has_value"), - &Env.getBoolLiteralValue(true)); + EXPECT_EQ(FooVal->getProperty("is_null"), + &Env.getBoolLiteralValue(false)); }); }