Skip to content

Commit c072586

Browse files
ymandymand
committed
[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
1 parent 9ef3146 commit c072586

File tree

5 files changed

+56
-39
lines changed

5 files changed

+56
-39
lines changed

‎clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,13 @@ enum class SkipPast {
4848
ReferenceThenPointer,
4949
};
5050

51+
/// Indicates the result of a tentative comparison.
52+
enum class ComparisonResult {
53+
Same,
54+
Different,
55+
Unknown,
56+
};
57+
5158
/// Holds the state of the program (store and heap) at a given program point.
5259
///
5360
/// WARNING: Symbolic values that are created by the environment for static
@@ -62,7 +69,11 @@ class Environment {
6269
public:
6370
virtual ~ValueModel() = default;
6471

65-
/// Returns true if and only if `Val1` is equivalent to `Val2`.
72+
/// Returns:
73+
/// `Same`: `Val1` is equivalent to `Val2`, according to the model.
74+
/// `Different`: `Val1` is distinct from `Val2`, according to the model.
75+
/// `Unknown`: The model can't determine a relationship between `Val1` and
76+
/// `Val2`.
6677
///
6778
/// Requirements:
6879
///
@@ -72,16 +83,16 @@ class Environment {
7283
///
7384
/// `Val1` and `Val2` must be assigned to the same storage location in
7485
/// `Env1` and `Env2` respectively.
75-
virtual bool compareEquivalent(QualType Type, const Value &Val1,
76-
const Environment &Env1, const Value &Val2,
77-
const Environment &Env2) {
86+
virtual ComparisonResult compare(QualType Type, const Value &Val1,
87+
const Environment &Env1, const Value &Val2,
88+
const Environment &Env2) {
7889
// FIXME: Consider adding QualType to StructValue and removing the Type
7990
// argument here.
8091
//
81-
// FIXME: default to a sound comparison and/or expand the comparison logic
82-
// built into the framework to support broader forms of equivalence than
83-
// strict pointer equality.
84-
return true;
92+
// FIXME: default to a sound comparison (`Unknown`) and/or expand the
93+
// comparison logic built into the framework to support broader forms of
94+
// equivalence than strict pointer equality.
95+
return ComparisonResult::Same;
8596
}
8697

8798
/// Modifies `MergedVal` to approximate both `Val1` and `Val2`. This could

‎clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,9 @@ class UncheckedOptionalAccessModel
5454

5555
void transfer(const CFGElement *Elt, NoopLattice &L, Environment &Env);
5656

57-
bool compareEquivalent(QualType Type, const Value &Val1,
58-
const Environment &Env1, const Value &Val2,
59-
const Environment &Env2) override;
57+
ComparisonResult compare(QualType Type, const Value &Val1,
58+
const Environment &Env1, const Value &Val2,
59+
const Environment &Env2) override;
6060

6161
bool merge(QualType Type, const Value &Val1, const Environment &Env1,
6262
const Value &Val2, const Environment &Env2, Value &MergedVal,

‎clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,8 +295,8 @@ bool Environment::equivalentTo(const Environment &Other,
295295
assert(It->second != nullptr);
296296

297297
if (!areEquivalentValues(*Val, *It->second) &&
298-
!Model.compareEquivalent(Loc->getType(), *Val, *this, *It->second,
299-
Other))
298+
Model.compare(Loc->getType(), *Val, *this, *It->second, Other) !=
299+
ComparisonResult::Same)
300300
return false;
301301
}
302302

‎clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ QualType stripReference(QualType Type) {
208208
}
209209

210210
/// Returns true if and only if `Type` is an optional type.
211-
bool IsOptionalType(QualType Type) {
211+
bool isOptionalType(QualType Type) {
212212
if (!Type->isRecordType())
213213
return false;
214214
// FIXME: Optimize this by avoiding the `getQualifiedNameAsString` call.
@@ -222,7 +222,7 @@ bool IsOptionalType(QualType Type) {
222222
/// For example, if `Type` is `optional<optional<int>>`, the result of this
223223
/// function will be 2.
224224
int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) {
225-
if (!IsOptionalType(Type))
225+
if (!isOptionalType(Type))
226226
return 0;
227227
return 1 + countOptionalWrappers(
228228
ASTCtx,
@@ -720,12 +720,14 @@ void UncheckedOptionalAccessModel::transfer(const CFGElement *Elt,
720720
TransferMatchSwitch(*Elt, getASTContext(), State);
721721
}
722722

723-
bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type,
724-
const Value &Val1,
725-
const Environment &Env1,
726-
const Value &Val2,
727-
const Environment &Env2) {
728-
return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2);
723+
ComparisonResult UncheckedOptionalAccessModel::compare(
724+
QualType Type, const Value &Val1, const Environment &Env1,
725+
const Value &Val2, const Environment &Env2) {
726+
if (!isOptionalType(Type))
727+
return ComparisonResult::Unknown;
728+
return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2)
729+
? ComparisonResult::Same
730+
: ComparisonResult::Different;
729731
}
730732

731733
bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
@@ -734,7 +736,7 @@ bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
734736
const Environment &Env2,
735737
Value &MergedVal,
736738
Environment &MergedEnv) {
737-
if (!IsOptionalType(Type))
739+
if (!isOptionalType(Type))
738740
return true;
739741

740742
auto &HasValueVal = MergedEnv.makeAtomicBoolValue();

‎clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -351,24 +351,27 @@ class SpecialBoolAnalysis final
351351
}
352352
}
353353

354-
bool compareEquivalent(QualType Type, const Value &Val1,
355-
const Environment &Env1, const Value &Val2,
356-
const Environment &Env2) override {
354+
ComparisonResult compare(QualType Type, const Value &Val1,
355+
const Environment &Env1, const Value &Val2,
356+
const Environment &Env2) override {
357357
const auto *Decl = Type->getAsCXXRecordDecl();
358358
if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
359359
Decl->getName() != "SpecialBool")
360-
return false;
360+
return ComparisonResult::Unknown;
361361

362362
auto *IsSet1 = cast_or_null<BoolValue>(Val1.getProperty("is_set"));
363+
auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
363364
if (IsSet1 == nullptr)
364-
return true;
365+
return IsSet2 == nullptr ? ComparisonResult::Same
366+
: ComparisonResult::Different;
365367

366-
auto *IsSet2 = cast_or_null<BoolValue>(Val2.getProperty("is_set"));
367368
if (IsSet2 == nullptr)
368-
return false;
369+
return ComparisonResult::Different;
369370

370371
return Env1.flowConditionImplies(*IsSet1) ==
371-
Env2.flowConditionImplies(*IsSet2);
372+
Env2.flowConditionImplies(*IsSet2)
373+
? ComparisonResult::Same
374+
: ComparisonResult::Different;
372375
}
373376

374377
// Always returns `true` to accept the `MergedVal`.
@@ -509,18 +512,19 @@ class OptionalIntAnalysis final
509512
}
510513
}
511514

512-
bool compareEquivalent(QualType Type, const Value &Val1,
513-
const Environment &Env1, const Value &Val2,
514-
const Environment &Env2) override {
515+
ComparisonResult compare(QualType Type, const Value &Val1,
516+
const Environment &Env1, const Value &Val2,
517+
const Environment &Env2) override {
515518
// Nothing to say about a value that does not model an `OptionalInt`.
516519
if (!Type->isRecordType() ||
517520
Type->getAsCXXRecordDecl()->getQualifiedNameAsString() != "OptionalInt")
518-
return false;
521+
return ComparisonResult::Unknown;
519522

520523
auto *Prop1 = Val1.getProperty("has_value");
521524
auto *Prop2 = Val2.getProperty("has_value");
522525
assert(Prop1 != nullptr && Prop2 != nullptr);
523-
return areEquivalentValues(*Prop1, *Prop2);
526+
return areEquivalentValues(*Prop1, *Prop2) ? ComparisonResult::Same
527+
: ComparisonResult::Different;
524528
}
525529

526530
bool merge(QualType Type, const Value &Val1, const Environment &Env1,
@@ -1182,12 +1186,12 @@ class TopAnalysis final : public DataflowAnalysis<TopAnalysis, NoopLattice> {
11821186
}
11831187
}
11841188

1185-
bool compareEquivalent(QualType Type, const Value &Val1,
1186-
const Environment &Env1, const Value &Val2,
1187-
const Environment &Env2) override {
1189+
ComparisonResult compare(QualType Type, const Value &Val1,
1190+
const Environment &Env1, const Value &Val2,
1191+
const Environment &Env2) override {
11881192
// Changes to a sound approximation, which allows us to test whether we can
11891193
// (soundly) converge for some loops.
1190-
return false;
1194+
return ComparisonResult::Unknown;
11911195
}
11921196
};
11931197

0 commit comments

Comments
 (0)