[clang] c072586 - [clang][dataflow] Generalize custom comparison to return tri-value result.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 3 16:31:46 PDT 2022
Author: Yitzhak Mandelbaum
Date: 2022-11-03T23:31:20Z
New Revision: c0725865b188f71f904ecd4dac56ef37268b30d2
URL: https://github.com/llvm/llvm-project/commit/c0725865b188f71f904ecd4dac56ef37268b30d2
DIFF: https://github.com/llvm/llvm-project/commit/c0725865b188f71f904ecd4dac56ef37268b30d2.diff
LOG: [clang][dataflow] Generalize custom comparison to return tri-value result.
Currently, the API for a model's custom value comparison returns a
boolean. Therefore, models cannot distinguish between situations where the
values are recognized by the model and different and those where the values are
just not recognized. This patch changes the return value to a tri-valued enum,
allowing models to express "don't know".
This patch is essentially a NFC -- no practical differences result from this
change in this patch. But, it prepares for future patches (particularly,
upcoming patches for widening) which will take advantage of the new flexibility.
Differential Revision: https://reviews.llvm.org/D137334
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index efea46b4a0c5b..e362d79263ff2 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -48,6 +48,13 @@ enum class SkipPast {
ReferenceThenPointer,
};
+/// Indicates the result of a tentative comparison.
+enum class ComparisonResult {
+ Same,
+ Different,
+ Unknown,
+};
+
/// Holds the state of the program (store and heap) at a given program point.
///
/// WARNING: Symbolic values that are created by the environment for static
@@ -62,7 +69,11 @@ class Environment {
public:
virtual ~ValueModel() = default;
- /// Returns true if and only if `Val1` is equivalent to `Val2`.
+ /// Returns:
+ /// `Same`: `Val1` is equivalent to `Val2`, according to the model.
+ /// `Different`: `Val1` is distinct from `Val2`, according to the model.
+ /// `Unknown`: The model can't determine a relationship between `Val1` and
+ /// `Val2`.
///
/// Requirements:
///
@@ -72,16 +83,16 @@ class Environment {
///
/// `Val1` and `Val2` must be assigned to the same storage location in
/// `Env1` and `Env2` respectively.
- virtual bool compareEquivalent(QualType Type, const Value &Val1,
- const Environment &Env1, const Value &Val2,
- const Environment &Env2) {
+ virtual ComparisonResult compare(QualType Type, const Value &Val1,
+ const Environment &Env1, const Value &Val2,
+ const Environment &Env2) {
// FIXME: Consider adding QualType to StructValue and removing the Type
// argument here.
//
- // FIXME: default to a sound comparison and/or expand the comparison logic
- // built into the framework to support broader forms of equivalence than
- // strict pointer equality.
- return true;
+ // FIXME: default to a sound comparison (`Unknown`) and/or expand the
+ // comparison logic built into the framework to support broader forms of
+ // equivalence than strict pointer equality.
+ return ComparisonResult::Same;
}
/// Modifies `MergedVal` to approximate both `Val1` and `Val2`. This could
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 66aabb531a213..b053a10327c3f 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -54,9 +54,9 @@ class UncheckedOptionalAccessModel
void transfer(const CFGElement *Elt, NoopLattice &L, Environment &Env);
- bool compareEquivalent(QualType Type, const Value &Val1,
- const Environment &Env1, const Value &Val2,
- const Environment &Env2) override;
+ ComparisonResult compare(QualType Type, const Value &Val1,
+ const Environment &Env1, const Value &Val2,
+ const Environment &Env2) override;
bool merge(QualType Type, const Value &Val1, const Environment &Env1,
const Value &Val2, const Environment &Env2, Value &MergedVal,
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 0b098c43ba3d1..ab1241d95eea0 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -295,8 +295,8 @@ bool Environment::equivalentTo(const Environment &Other,
assert(It->second != nullptr);
if (!areEquivalentValues(*Val, *It->second) &&
- !Model.compareEquivalent(Loc->getType(), *Val, *this, *It->second,
- Other))
+ Model.compare(Loc->getType(), *Val, *this, *It->second, Other) !=
+ ComparisonResult::Same)
return false;
}
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 1ffd88697f3a7..1a41cfaa5fa13 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -208,7 +208,7 @@ QualType stripReference(QualType Type) {
}
/// Returns true if and only if `Type` is an optional type.
-bool IsOptionalType(QualType Type) {
+bool isOptionalType(QualType Type) {
if (!Type->isRecordType())
return false;
// FIXME: Optimize this by avoiding the `getQualifiedNameAsString` call.
@@ -222,7 +222,7 @@ bool IsOptionalType(QualType Type) {
/// For example, if `Type` is `optional<optional<int>>`, the result of this
/// function will be 2.
int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) {
- if (!IsOptionalType(Type))
+ if (!isOptionalType(Type))
return 0;
return 1 + countOptionalWrappers(
ASTCtx,
@@ -720,12 +720,14 @@ void UncheckedOptionalAccessModel::transfer(const CFGElement *Elt,
TransferMatchSwitch(*Elt, getASTContext(), State);
}
-bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type,
- const Value &Val1,
- const Environment &Env1,
- const Value &Val2,
- const Environment &Env2) {
- return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2);
+ComparisonResult UncheckedOptionalAccessModel::compare(
+ QualType Type, const Value &Val1, const Environment &Env1,
+ const Value &Val2, const Environment &Env2) {
+ if (!isOptionalType(Type))
+ return ComparisonResult::Unknown;
+ return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2)
+ ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
@@ -734,7 +736,7 @@ bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
const Environment &Env2,
Value &MergedVal,
Environment &MergedEnv) {
- if (!IsOptionalType(Type))
+ if (!isOptionalType(Type))
return true;
auto &HasValueVal = MergedEnv.makeAtomicBoolValue();
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index e36f207389e4c..8e0e27efae9e8 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -351,24 +351,27 @@ class SpecialBoolAnalysis final
}
}
- bool compareEquivalent(QualType Type, const Value &Val1,
- const Environment &Env1, const Value &Val2,
- const Environment &Env2) override {
+ ComparisonResult compare(QualType Type, const Value &Val1,
+ const Environment &Env1, const Value &Val2,
+ const Environment &Env2) override {
const auto *Decl = Type->getAsCXXRecordDecl();
if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
Decl->getName() != "SpecialBool")
- return false;
+ return ComparisonResult::Unknown;
auto *IsSet1 = cast_or_null<BoolValue>(Val1.getProperty("is_set"));
+ auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
if (IsSet1 == nullptr)
- return true;
+ return IsSet2 == nullptr ? ComparisonResult::Same
+ : ComparisonResult::Different;
- auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
if (IsSet2 == nullptr)
- return false;
+ return ComparisonResult::Different;
return Env1.flowConditionImplies(*IsSet1) ==
- Env2.flowConditionImplies(*IsSet2);
+ Env2.flowConditionImplies(*IsSet2)
+ ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
// Always returns `true` to accept the `MergedVal`.
@@ -509,18 +512,19 @@ class OptionalIntAnalysis final
}
}
- bool compareEquivalent(QualType Type, const Value &Val1,
- const Environment &Env1, const Value &Val2,
- const Environment &Env2) override {
+ 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")
- return false;
+ return ComparisonResult::Unknown;
auto *Prop1 = Val1.getProperty("has_value");
auto *Prop2 = Val2.getProperty("has_value");
assert(Prop1 != nullptr && Prop2 != nullptr);
- return areEquivalentValues(*Prop1, *Prop2);
+ return areEquivalentValues(*Prop1, *Prop2) ? ComparisonResult::Same
+ : ComparisonResult::Different;
}
bool merge(QualType Type, const Value &Val1, const Environment &Env1,
@@ -1182,12 +1186,12 @@ class TopAnalysis final : public DataflowAnalysis<TopAnalysis, NoopLattice> {
}
}
- bool compareEquivalent(QualType Type, const Value &Val1,
- const Environment &Env1, const Value &Val2,
- const Environment &Env2) override {
+ ComparisonResult compare(QualType Type, const Value &Val1,
+ const Environment &Env1, const Value &Val2,
+ const Environment &Env2) override {
// Changes to a sound approximation, which allows us to test whether we can
// (soundly) converge for some loops.
- return false;
+ return ComparisonResult::Unknown;
}
};
More information about the cfe-commits
mailing list