[clang] [clang][dataflow] Disallow setting properties on `RecordValue`s. (PR #76042)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 20 03:59:17 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
Author: None (martinboehme)
<details>
<summary>Changes</summary>
Instead, synthetic fields should now be used for the same purpose. These have a
number of advantages, as described in
https://github.com/llvm/llvm-project/pull/73860, and longer-term, we want to
eliminate `RecordValue` entirely.
As `RecordValue`s cannot have properties any more, I have replaced the
`OptionalIntAnalysis` with an equivalent analysis that tracks nullness of
pointers (instead of whether an optional has a value). This serves the same
purpose, namely to check whether the framework applies a custom `merge()`
operation to widen properties.
---
Patch is 21.32 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/76042.diff
6 Files Affected:
- (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (-11)
- (modified) clang/include/clang/Analysis/FlowSensitive/RecordOps.h (+2-10)
- (modified) clang/include/clang/Analysis/FlowSensitive/Value.h (+17-23)
- (modified) clang/lib/Analysis/FlowSensitive/RecordOps.cpp (+1-32)
- (modified) clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp (-37)
- (modified) clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp (+68-82)
``````````diff
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 5943af50b6ad8f..47064e1898142d 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -691,20 +691,9 @@ RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
std::vector<FieldDecl *> 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 7b87840d626b4b..783e53e980aa2c 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 e6c68e5b4e93e1..021acc14ca15a7 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 isx
+/// 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 caaf443382b02c..da4dd6dc078515 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 = cast_or_null<RecordValue>(Env.getValue(Src));
- RecordValue *DstVal = cast_or_null<RecordValue>(Env.getValue(Dst));
-
- DstVal = &Env.create<RecordValue>(Dst);
+ RecordValue *DstVal = &Env.create<RecordValue>(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<Value *> Props1, Props2;
-
- if (RecordValue *Val1 = cast_or_null<RecordValue>(Env1.getValue(Loc1)))
- for (const auto &[Name, Value] : Val1->properties())
- Props1[Name] = Value;
- if (RecordValue *Val2 = cast_or_null<RecordValue>(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;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp
index 84fe675c32c2d0..cd6a37d370e854 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<RecordValue>(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<RecordValue>(Env.getValue(S1));
S2Val = cast<RecordValue>(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<IntegerValue>());
- cast<RecordValue>(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<RecordValue>(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<RecordValue>(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<RecordValue>(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<RecordValue>(Env.getValue(S1))
- ->setProperty("prop1", Env.getBoolLiteralValue(false));
- cast<RecordValue>(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 4c3cb322eacfb3..8d481788af208a 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<OptionalIntAnalysis, NoopLattice> {
+class NullPointerAnalysis final
+ : public DataflowAnalysis<NullPointerAnalysis, NoopLattice> {
public:
- explicit OptionalIntAnalysis(ASTContext &Context)
- : DataflowAnalysis<OptionalIntAnalysis, NoopLattice>(Context) {}
+ explicit NullPointerAnalysis(ASTContext &Context)
+ : DataflowAnalysis<NullPointerAnalysis, NoopLattice>(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<BoundNodes, 1> Matches = match(
- stmt(anyOf(cxxConstructExpr(HasOptionalIntType).bind("construct"),
- cxxOperatorCallExpr(
- callee(cxxMethodDecl(ofClass(OptionalIntRecordDecl))))
- .bind("operator"))),
- *S, getASTContext());
- if (const auto *E = selectFirst<CXXConstructExpr>(
- "construct", Matches)) {
- cast<RecordValue>(Env.getValue(*E))
- ->setProperty("has_value", Env.getBoolLiteralValue(false));
- } else if (const auto *E =
- selectFirst<CXXOperatorCallExpr>("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<Expr>(S);
+ if (!E)
+ return;
+
+ if (!E->getType()->isPointerType())
+ return;
+
+ // Make sure we have a `PointerValue` for `E`.
+ auto *PtrVal = cast_or_null<PointerValue>(Env.getValue(*E));
+ if (PtrVal == nullptr) {
+ PtrVal = cast<PointerValue>(Env.createValue(E->getType()));
+ Env.setValue(*E, *PtrVal);
}
+
+ if (auto *Cast = dyn_cast<ImplicitCastExpr>(E);
+ Cast && Cast->getCastKind() == CK_NullToPointer)
+ PtrVal->setProperty("is_null", Env.getBoolLiteralValue(true));
+ else if (auto *Op = dyn_cast<UnaryOperator>(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<BoolValue>(Val1.getProperty("has_value"));
- if (HasValue1 == nullptr)
+ auto *IsNull1 = cast_or_null<BoolValue>(Val1.getProperty("is_null"));
+ if (IsNull1 == nullptr)
return false;
- auto *HasValue2 = cast_or_null<BoolValue>(Val2.getProperty("has_value"));
- if (HasValue2 == nullptr)
+ auto *IsNull2 = cast_or_null<BoolValue>(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 <typename Matcher>
void runDataflow(llvm::StringRef Code, Matcher Match) {
- tooling::FileContentMappings FilesContents;
- FilesContents.push_back(
- std::make_pair<std::string, std::string>("widening_test_defs.h", R"(
- struct OptionalInt {
- OptionalInt() = default;
- OptionalInt& operator=(int);
- };
- )"));
ASSERT_THAT_ERROR(
- checkDataflow<OptionalIntAnalysis>(
- AnalysisInputs<OptionalIntAnalysis>(
+ checkDataflow<NullPointerAnalysis>(
+ AnalysisInputs<NullPointerAnalysis>(
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<NoopLattice>>
&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<TopBoolValue>(GetFooValue(Env3)->getProperty("has_value")));
+ isa<TopBoolValue>(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(...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/76042
More information about the cfe-commits
mailing list