ymandel updated this revision to Diff 472978.
ymandel added a comment.
address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137334/new/
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,27 @@
}
}
- 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 @@
}
}
- 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 @@
}
}
- 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;
}
};
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);
+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 @@
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) !=
+ ComparisonResult::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;
+ 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,
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 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 @@
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 @@
///
/// `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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
