diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/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 diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/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, diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/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; } diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/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>`, 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(); diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/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(Val1.getProperty("is_set")); + auto *IsSet2 = cast_or_null(Val2.getProperty("is_set")); if (IsSet1 == nullptr) - return true; + return IsSet2 ? ComparisonStatus::Same : ComparisonStatus::Different; - auto *IsSet2 = cast_or_null(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 {