This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
mboehme marked an inline comment as done.
Closed by commit rG3bc1ea5b0ac9: [clang][dataflow] Fix a bug in handling of
`operator->` for optional checker. (authored by mboehme).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150775/new/
https://reviews.llvm.org/D150775
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
@@ -2764,9 +2764,6 @@
}
TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
- // FIXME: Fix when to initialize `value`. All unwrapping should be safe in
- // this example, but `value` initialization is done multiple times during the
- // fixpoint iterations and joining the environment won't correctly merge
them.
ExpectDiagnosticsFor(
R"(
#include "unchecked_optional_access_test.h"
@@ -2786,7 +2783,7 @@
}
// Now we merge the two values. UncheckedOptionalAccessModel::merge()
will
// throw away the "value" property.
- foo->value(); // [[unsafe]]
+ foo->value();
}
)");
}
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -399,6 +399,18 @@
}
}
+void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
+ LatticeTransferState &State) {
+ if (auto *OptionalVal =
+ getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
+ if (auto *Loc = maybeInitializeOptionalValueMember(
+ UnwrapExpr->getType()->getPointeeType(), *OptionalVal, State.Env))
{
+ State.Env.setValueStrict(*UnwrapExpr,
+ State.Env.create<PointerValue>(*Loc));
+ }
+ }
+}
+
void transferMakeOptionalCall(const CallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
@@ -774,25 +786,22 @@
transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
})
- // optional::operator*, optional::operator->
- // FIXME: This does something slightly strange for `operator->`.
- // `transferUnwrapCall()` may create a new value of type `T` for the
- // `optional<T>`, and it associates that value with `E`. In the case of
- // `operator->`, `E` is a pointer. As a result, we associate an
- // expression of pointer type with a storage location of non-pointer type
- // `T`. This can confound other code that expects expressions of
- // pointer type to be associated with `PointerValue`s, such as the
- // centrally provided accessors `getImplicitObjectLocation()` and
- // `getBaseObjectLocation()`, and this is the reason we need to use our
- // own 'maybeSkipPointer()` and `getValueBehindPossiblePointer()` instead
- // of these accessors.
- .CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
+ // optional::operator*
+ .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("*"),
[](const CallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
transferUnwrapCall(E, E->getArg(0), State);
})
+ // optional::operator->
+ .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("->"),
+ [](const CallExpr *E,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ transferArrowOpCall(E, E->getArg(0), State);
+ })
+
// optional::has_value
.CaseOfCFGStmt<CXXMemberCallExpr>(
isOptionalMemberCallWithName("has_value"),
Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2764,9 +2764,6 @@
}
TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
- // FIXME: Fix when to initialize `value`. All unwrapping should be safe in
- // this example, but `value` initialization is done multiple times during the
- // fixpoint iterations and joining the environment won't correctly merge them.
ExpectDiagnosticsFor(
R"(
#include "unchecked_optional_access_test.h"
@@ -2786,7 +2783,7 @@
}
// Now we merge the two values. UncheckedOptionalAccessModel::merge() will
// throw away the "value" property.
- foo->value(); // [[unsafe]]
+ foo->value();
}
)");
}
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -399,6 +399,18 @@
}
}
+void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
+ LatticeTransferState &State) {
+ if (auto *OptionalVal =
+ getValueBehindPossiblePointer(*ObjectExpr, State.Env)) {
+ if (auto *Loc = maybeInitializeOptionalValueMember(
+ UnwrapExpr->getType()->getPointeeType(), *OptionalVal, State.Env)) {
+ State.Env.setValueStrict(*UnwrapExpr,
+ State.Env.create<PointerValue>(*Loc));
+ }
+ }
+}
+
void transferMakeOptionalCall(const CallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
@@ -774,25 +786,22 @@
transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
})
- // optional::operator*, optional::operator->
- // FIXME: This does something slightly strange for `operator->`.
- // `transferUnwrapCall()` may create a new value of type `T` for the
- // `optional<T>`, and it associates that value with `E`. In the case of
- // `operator->`, `E` is a pointer. As a result, we associate an
- // expression of pointer type with a storage location of non-pointer type
- // `T`. This can confound other code that expects expressions of
- // pointer type to be associated with `PointerValue`s, such as the
- // centrally provided accessors `getImplicitObjectLocation()` and
- // `getBaseObjectLocation()`, and this is the reason we need to use our
- // own 'maybeSkipPointer()` and `getValueBehindPossiblePointer()` instead
- // of these accessors.
- .CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
+ // optional::operator*
+ .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("*"),
[](const CallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
transferUnwrapCall(E, E->getArg(0), State);
})
+ // optional::operator->
+ .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("->"),
+ [](const CallExpr *E,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ transferArrowOpCall(E, E->getArg(0), State);
+ })
+
// optional::has_value
.CaseOfCFGStmt<CXXMemberCallExpr>(
isOptionalMemberCallWithName("has_value"),
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
