This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd34fbf2d9bf4: [clang][dataflow] In optional model, implement
`widen` and make `compare` sound. (authored by ymandel).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140344/new/
https://reviews.llvm.org/D140344
Files:
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2748,19 +2748,6 @@
}
}
)");
-
- // FIXME: Add support for operator==.
- // ExpectDiagnosticsFor(R"(
- // #include "unchecked_optional_access_test.h"
- //
- // void target($ns::$optional<int> opt1, $ns::$optional<int> opt2) {
- // if (opt1 == opt2) {
- // if (opt1.has_value()) {
- // opt2.value();
- // }
- // }
- // }
- // )");
}
TEST_P(UncheckedOptionalAccessTest, JoinDistinctValues) {
@@ -2846,7 +2833,7 @@
)code");
}
-TEST_P(UncheckedOptionalAccessTest, ReassignValueInLoop) {
+TEST_P(UncheckedOptionalAccessTest, AccessValueInLoop) {
ExpectDiagnosticsFor(R"(
#include "unchecked_optional_access_test.h"
@@ -2857,7 +2844,9 @@
}
}
)");
+}
+TEST_P(UncheckedOptionalAccessTest, ReassignValueInLoopWithCheckSafe) {
ExpectDiagnosticsFor(R"(
#include "unchecked_optional_access_test.h"
@@ -2871,7 +2860,9 @@
}
}
)");
+}
+TEST_P(UncheckedOptionalAccessTest, ReassignValueInLoopNoCheckUnsafe) {
ExpectDiagnosticsFor(
R"(
#include "unchecked_optional_access_test.h"
@@ -2885,7 +2876,60 @@
}
}
)");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ReassignValueInLoopToUnsetUnsafe) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ void target() {
+ $ns::$optional<int> opt = 3;
+ while (Make<bool>())
+ opt = $ns::nullopt;
+ $ns::$optional<int> opt2 = $ns::nullopt;
+ if (opt.has_value())
+ opt2 = $ns::$optional<int>(3);
+ opt2.value(); // [[unsafe]]
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ReassignValueInLoopToSetUnsafe) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ void target() {
+ $ns::$optional<int> opt = $ns::nullopt;
+ while (Make<bool>())
+ opt = $ns::$optional<int>(3);
+ $ns::$optional<int> opt2 = $ns::nullopt;
+ if (!opt.has_value())
+ opt2 = $ns::$optional<int>(3);
+ opt2.value(); // [[unsafe]]
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ReassignValueInLoopToUnknownUnsafe) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ void target() {
+ $ns::$optional<int> opt = $ns::nullopt;
+ while (Make<bool>())
+ opt = Make<$ns::$optional<int>>();
+ $ns::$optional<int> opt2 = $ns::nullopt;
+ if (!opt.has_value())
+ opt2 = $ns::$optional<int>(3);
+ opt2.value(); // [[unsafe]]
+ }
+ )");
+}
+TEST_P(UncheckedOptionalAccessTest, ReassignValueInLoopBadConditionUnsafe) {
ExpectDiagnosticsFor(
R"(
#include "unchecked_optional_access_test.h"
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -27,6 +27,7 @@
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Casting.h"
+#include "llvm/Support/ErrorHandling.h"
#include <cassert>
#include <memory>
#include <utility>
@@ -835,7 +836,14 @@
const Value &Val2, const Environment &Env2) {
if (!isOptionalType(Type))
return ComparisonResult::Unknown;
- return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2)
+ bool MustNonEmpty1 = isNonEmptyOptional(Val1, Env1);
+ bool MustNonEmpty2 = isNonEmptyOptional(Val2, Env2);
+ if (MustNonEmpty1 && MustNonEmpty2) return ComparisonResult::Same;
+ // If exactly one is true, then they're different, no reason to check whether
+ // they're definitely empty.
+ if (MustNonEmpty1 || MustNonEmpty2) return ComparisonResult::Different;
+ // Check if they're both definitely empty.
+ return (isEmptyOptional(Val1, Env1) && isEmptyOptional(Val2, Env2))
? ComparisonResult::Same
: ComparisonResult::Different;
}
@@ -848,16 +856,48 @@
Environment &MergedEnv) {
if (!isOptionalType(Type))
return true;
-
+ // FIXME: uses same approach as join for `BoolValues`. Requires non-const
+ // values, though, so will require updating the interface.
auto &HasValueVal = MergedEnv.makeAtomicBoolValue();
- if (isNonEmptyOptional(Val1, Env1) && isNonEmptyOptional(Val2, Env2))
+ bool MustNonEmpty1 = isNonEmptyOptional(Val1, Env1);
+ bool MustNonEmpty2 = isNonEmptyOptional(Val2, Env2);
+ if (MustNonEmpty1 && MustNonEmpty2)
MergedEnv.addToFlowCondition(HasValueVal);
- else if (isEmptyOptional(Val1, Env1) && isEmptyOptional(Val2, Env2))
+ else if (
+ // Only make the costly calls to `isEmptyOptional` if we got "unknown"
+ // (false) for both calls to `isNonEmptyOptional`.
+ !MustNonEmpty1 && !MustNonEmpty2 && isEmptyOptional(Val1, Env1) &&
+ isEmptyOptional(Val2, Env2))
MergedEnv.addToFlowCondition(MergedEnv.makeNot(HasValueVal));
setHasValue(MergedVal, HasValueVal);
return true;
}
+Value *UncheckedOptionalAccessModel::widen(QualType Type, Value &Prev,
+ const Environment &PrevEnv,
+ Value &Current,
+ Environment &CurrentEnv) {
+ switch (compare(Type, Prev, PrevEnv, Current, CurrentEnv)) {
+ case ComparisonResult::Same:
+ return &Prev;
+ case ComparisonResult::Different:
+ if (auto *PrevHasVal =
+ cast_or_null<BoolValue>(Prev.getProperty("has_value"))) {
+ if (isa<TopBoolValue>(PrevHasVal))
+ return &Prev;
+ }
+ if (auto *CurrentHasVal =
+ cast_or_null<BoolValue>(Current.getProperty("has_value"))) {
+ if (isa<TopBoolValue>(CurrentHasVal))
+ return &Current;
+ }
+ return &createOptionalValue(CurrentEnv, CurrentEnv.makeTopBoolValue());
+ case ComparisonResult::Unknown:
+ return nullptr;
+ }
+ llvm_unreachable("all cases covered in switch");
+}
+
UncheckedOptionalAccessDiagnoser::UncheckedOptionalAccessDiagnoser(
UncheckedOptionalAccessModelOptions Options)
: DiagnoseMatchSwitch(buildDiagnoseMatchSwitch(Options)) {}
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
@@ -62,6 +62,9 @@
const Value &Val2, const Environment &Env2, Value &MergedVal,
Environment &MergedEnv) override;
+ Value *widen(QualType Type, Value &Prev, const Environment &PrevEnv,
+ Value &Current, Environment &CurrentEnv) override;
+
private:
CFGMatchSwitch<TransferState<NoopLattice>> TransferMatchSwitch;
};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
