[clang][dataflow] Disallow setting properties on RecordValues.
#76042
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (martinboehme) ChangesInstead, synthetic fields should now be used for the same purpose. These have a As 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:
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]
|
|
@llvm/pr-subscribers-clang-analysis Author: None (martinboehme) ChangesInstead, synthetic fields should now be used for the same purpose. These have a As 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:
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]
|
Xazax-hun
approved these changes
Dec 20, 2023
| /// - 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 |
There was a problem hiding this comment.
Oops -- thanks for catching! Fixed.
| return; | ||
|
|
||
| // Make sure we have a `PointerValue` for `E`. | ||
| auto *PtrVal = cast_or_null<PointerValue>(Env.getValue(*E)); |
There was a problem hiding this comment.
This is only a test, but this code snippet made me think. I wonder if it is a good idea to let checks create arbitrary Values. Specifically, I am concerned about a poorly written check triggering divergence.
There was a problem hiding this comment.
Agree that divergence is a concern -- but I do think we need to let checks create their own values when the framework doesn't. Here's an example of where we do this in Crubit's nullability check (i.e. in production code) to ensure that every expression of pointer type is associated with a value.
This is needed because the framework itself doesn't create Values for every expression. As just one example, the framework doesn't create values for CallExprs, but we definitely need values for these in the nullability check as a function returning a pointer can have nullability annotations on that pointer, and we then need to propagate this pointer value, along with its nullability information.
We could change this by making sure that the framework creates Values for every expression, but this would come at a cost: We would be creating a lot of values that any given analysis doesn't need, and this would negatively impact runtime, memory consumption, and potentially convergence.
AIUI from @gribozavr, the framework's philosophy so far has been to provide base functionality that is likely to be needed by most or all analyses, and it lets a specific analysis customize or extend the behavior where the existing behavior is not sufficient. But this does come with a tradeoff; as you point out, a badly written analysis can trigger divergence and other unwanted behavior. IOW, we're relying on the authors of the analysis to know what they're doing.
@gribozavr Do you have additional thoughts you want to add here?
(For the purposes of this patch, I don't think there's anything actionable here, so I will go ahead and submit.)
There was a problem hiding this comment.
There are a lot of things that a poorly-written check could do, so is this the one to spend our time protecting against? Maybe. I wish the framework helped more with convergence. Convergence issues are indeed difficult, and we ourselves have not yet fully solved the problem and haven't found a good tradeoff re: convergence for nullability analysis. As we deploy this check internally, I expect us to tweak things in this area, hopefully we will get a better understanding of convergence for our formalism, or we could even get ideas for framework-level features. There is also non-trivial work we already know needs to be done that is convergence-related, like a generic "Top" value.
So I think at this point I'd prefer to keep the extensibility with sharp edges, rather than reduce expressivity.
There was a problem hiding this comment.
I think the reason why it might be worth spending some time thinking about convergence because this is probably one of the bigger challenges for people writing checks. The other reason is, in case we want to run multiple checks in the same fixed-point iteration, we might not want arbitrary interactions between them.
That being said, there are many moving pieces that we have not figured yet out, so I 100% agree that it might be too early to try to solve this problem now.
Instead, synthetic fields should now be used for the same purpose. These have a number of advantages, as described in llvm#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.
martinboehme
force-pushed
the
piper_export_cl_592511520
branch
from
e33fc6a to
bdfae61
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead, synthetic fields should now be used for the same purpose. These have a
number of advantages, as described in
#73860, and longer-term, we want to
eliminate
RecordValueentirely.As
RecordValues cannot have properties any more, I have replaced theOptionalIntAnalysiswith an equivalent analysis that tracks nullness ofpointers (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.