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 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 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; + 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 --- 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) != + ComparisonResult::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); +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(); 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,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(Val1.getProperty("is_set")); + auto *IsSet2 = cast_or_null(Val2.getProperty("is_set")); if (IsSet1 == nullptr) - return true; + return IsSet2 == nullptr ? ComparisonResult::Same + : ComparisonResult::Different; - auto *IsSet2 = cast_or_null(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; } };