ymandel created this revision.
ymandel added reviewers: xazax.hun, sgatev, gribozavr2.
Herald added subscribers: martong, rnkovacs.
Herald added a reviewer: NoQ.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D137334
Files:
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
Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -351,24 +351,26 @@
}
}
- bool compareEquivalent(QualType Type, const Value &Val1,
- const Environment &Env1, const Value &Val2,
- const Environment &Env2) override {
+ ComparisonStatus 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 ComparisonStatus::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 ? ComparisonStatus::Same : ComparisonStatus::Different;
- auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
if (IsSet2 == nullptr)
- return false;
+ return ComparisonStatus::Different;
return Env1.flowConditionImplies(*IsSet1) ==
- Env2.flowConditionImplies(*IsSet2);
+ Env2.flowConditionImplies(*IsSet2)
+ ? ComparisonStatus::Same
+ : ComparisonStatus::Different;
}
// Always returns `true` to accept the `MergedVal`.
@@ -509,18 +511,19 @@
}
}
- bool compareEquivalent(QualType Type, const Value &Val1,
- const Environment &Env1, const Value &Val2,
- const Environment &Env2) override {
+ ComparisonStatus 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 ComparisonStatus::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) ? ComparisonStatus::Same
+ : ComparisonStatus::Different;
}
bool merge(QualType Type, const Value &Val1, const Environment &Env1,
@@ -1181,14 +1184,6 @@
Env.setStorageLocation(*E, Loc);
}
}
-
- bool compareEquivalent(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;
- }
};
class TopTest : public Test {
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -208,7 +208,7 @@
}
/// 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 @@
/// 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 @@
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);
+ComparisonStatus UncheckedOptionalAccessModel::compare(
+ QualType Type, const Value &Val1, const Environment &Env1,
+ const Value &Val2, const Environment &Env2) {
+ if (!isOptionalType(Type))
+ return ComparisonStatus::Unknown;
+ return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2)
+ ? ComparisonStatus::Same
+ : ComparisonStatus::Different;
}
bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
@@ -734,7 +736,7 @@
const Environment &Env2,
Value &MergedVal,
Environment &MergedEnv) {
- if (!IsOptionalType(Type))
+ if (!isOptionalType(Type))
return true;
auto &HasValueVal = MergedEnv.makeAtomicBoolValue();
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -295,8 +295,8 @@
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) !=
+ ComparisonStatus::Same)
return false;
}
Index: clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -54,9 +54,9 @@
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;
+ ComparisonStatus 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,
Index: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -48,6 +48,13 @@
ReferenceThenPointer,
};
+/// Indicates the result of a tentative comparison.
+enum class ComparisonStatus {
+ 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 @@
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 does not determine a relationship between `Val1`
+ /// and `Val2`.
///
/// Requirements:
///
@@ -72,16 +83,16 @@
///
/// `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 ComparisonStatus 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 ComparisonStatus::Same;
}
/// Modifies `MergedVal` to approximate both `Val1` and `Val2`. This could
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
