This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd4fb829b7180: [clang][dataflow] Relax validity assumptions
in `UncheckedOptionalAccessModel`. (authored by ymandel).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142710/new/
https://reviews.llvm.org/D142710
Files:
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
@@ -11,7 +11,6 @@
#include "TestingSupport.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/DenseSet.h"
@@ -2124,6 +2123,139 @@
)");
}
+TEST_P(UncheckedOptionalAccessTest, SwapUnmodeledLocLeft) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct L { $ns::$optional<int> hd; L* tl; };
+
+ void target() {
+ $ns::$optional<int> foo = 3;
+ L bar;
+
+ // Any `tl` beyond the first is not modeled.
+ bar.tl->tl->hd.swap(foo);
+
+ bar.tl->tl->hd.value(); // [[unsafe]]
+ foo.value(); // [[unsafe]]
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, SwapUnmodeledLocRight) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct L { $ns::$optional<int> hd; L* tl; };
+
+ void target() {
+ $ns::$optional<int> foo = 3;
+ L bar;
+
+ // Any `tl` beyond the first is not modeled.
+ foo.swap(bar.tl->tl->hd);
+
+ bar.tl->tl->hd.value(); // [[unsafe]]
+ foo.value(); // [[unsafe]]
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, SwapUnmodeledValueLeftSet) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct S { int x; };
+ struct A { $ns::$optional<S> late; };
+ struct B { A f3; };
+ struct C { B f2; };
+ struct D { C f1; };
+
+ void target() {
+ $ns::$optional<S> foo = S{3};
+ D bar;
+
+ bar.f1.f2.f3.late.swap(foo);
+
+ bar.f1.f2.f3.late.value();
+ foo.value(); // [[unsafe]]
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, SwapUnmodeledValueLeftUnset) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct S { int x; };
+ struct A { $ns::$optional<S> late; };
+ struct B { A f3; };
+ struct C { B f2; };
+ struct D { C f1; };
+
+ void target() {
+ $ns::$optional<S> foo;
+ D bar;
+
+ bar.f1.f2.f3.late.swap(foo);
+
+ bar.f1.f2.f3.late.value(); // [[unsafe]]
+ foo.value(); // [[unsafe]]
+ }
+ )");
+}
+
+// fixme: use recursion instead of depth.
+TEST_P(UncheckedOptionalAccessTest, SwapUnmodeledValueRightSet) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct S { int x; };
+ struct A { $ns::$optional<S> late; };
+ struct B { A f3; };
+ struct C { B f2; };
+ struct D { C f1; };
+
+ void target() {
+ $ns::$optional<S> foo = S{3};
+ D bar;
+
+ foo.swap(bar.f1.f2.f3.late);
+
+ bar.f1.f2.f3.late.value();
+ foo.value(); // [[unsafe]]
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, SwapUnmodeledValueRightUnset) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct S { int x; };
+ struct A { $ns::$optional<S> late; };
+ struct B { A f3; };
+ struct C { B f2; };
+ struct D { C f1; };
+
+ void target() {
+ $ns::$optional<S> foo;
+ D bar;
+
+ foo.swap(bar.f1.f2.f3.late);
+
+ bar.f1.f2.f3.late.value(); // [[unsafe]]
+ foo.value(); // [[unsafe]]
+ }
+ )");
+}
+
TEST_P(UncheckedOptionalAccessTest, UniquePtrToOptional) {
// We suppress diagnostics for optionals in smart pointers (other than
// `optional` itself).
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -521,48 +521,54 @@
transferAssignment(E, State.Env.getBoolLiteralValue(false), State);
}
-void transferSwap(const StorageLocation &OptionalLoc1,
- const StorageLocation &OptionalLoc2,
- LatticeTransferState &State) {
- auto *OptionalVal1 = State.Env.getValue(OptionalLoc1);
- assert(OptionalVal1 != nullptr);
+void transferSwap(const Expr &E1, SkipPast E1Skip, const Expr &E2,
+ Environment &Env) {
+ // We account for cases where one or both of the optionals are not modeled,
+ // either lacking associated storage locations, or lacking values associated
+ // to such storage locations.
+ auto *Loc1 = Env.getStorageLocation(E1, E1Skip);
+ auto *Loc2 = Env.getStorageLocation(E2, SkipPast::Reference);
+
+ if (Loc1 == nullptr) {
+ if (Loc2 != nullptr)
+ Env.setValue(*Loc2, createOptionalValue(Env, Env.makeAtomicBoolValue()));
+ return;
+ }
+ if (Loc2 == nullptr) {
+ Env.setValue(*Loc1, createOptionalValue(Env, Env.makeAtomicBoolValue()));
+ return;
+ }
- auto *OptionalVal2 = State.Env.getValue(OptionalLoc2);
- assert(OptionalVal2 != nullptr);
+ // Both expressions have locations, though they may not have corresponding
+ // values. In that case, we create a fresh value at this point. Note that if
+ // two branches both do this, they will not share the value, but it at least
+ // allows for local reasoning about the value. To avoid the above, we would
+ // need *lazy* value allocation.
+ // FIXME: allocate values lazily, instead of just creating a fresh value.
+ auto *Val1 = Env.getValue(*Loc1);
+ if (Val1 == nullptr)
+ Val1 = &createOptionalValue(Env, Env.makeAtomicBoolValue());
- State.Env.setValue(OptionalLoc1, *OptionalVal2);
- State.Env.setValue(OptionalLoc2, *OptionalVal1);
+ auto *Val2 = Env.getValue(*Loc2);
+ if (Val2 == nullptr)
+ Val2 = &createOptionalValue(Env, Env.makeAtomicBoolValue());
+
+ Env.setValue(*Loc1, *Val2);
+ Env.setValue(*Loc2, *Val1);
}
void transferSwapCall(const CXXMemberCallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
assert(E->getNumArgs() == 1);
-
- auto *OptionalLoc1 = State.Env.getStorageLocation(
- *E->getImplicitObjectArgument(), SkipPast::ReferenceThenPointer);
- assert(OptionalLoc1 != nullptr);
-
- auto *OptionalLoc2 =
- State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference);
- assert(OptionalLoc2 != nullptr);
-
- transferSwap(*OptionalLoc1, *OptionalLoc2, State);
+ transferSwap(*E->getImplicitObjectArgument(), SkipPast::ReferenceThenPointer,
+ *E->getArg(0), State.Env);
}
void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
assert(E->getNumArgs() == 2);
-
- auto *OptionalLoc1 =
- State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference);
- assert(OptionalLoc1 != nullptr);
-
- auto *OptionalLoc2 =
- State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference);
- assert(OptionalLoc2 != nullptr);
-
- transferSwap(*OptionalLoc1, *OptionalLoc2, State);
+ transferSwap(*E->getArg(0), SkipPast::Reference, *E->getArg(1), State.Env);
}
BoolValue &evaluateEquality(Environment &Env, BoolValue &EqVal, BoolValue &LHS,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
