ymandel updated this revision to Diff 481651.
ymandel added a comment.
Simplifies the implementation and removes mention of any potential CFG problem.
This new version does the work eagerly by forcing a location for the
synthesized variable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139544/new/
https://reviews.llvm.org/D139544
Files:
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.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
@@ -2860,6 +2860,59 @@
)");
}
+TEST_P(UncheckedOptionalAccessTest, StructuredBindingsFromStruct) {
+ ExpectDiagnosticsFor(R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct kv { $ns::$optional<int> opt; int x; };
+ int target() {
+ auto [contents, x] = Make<kv>();
+ return contents ? *contents : x;
+ }
+ )");
+
+ ExpectDiagnosticsFor(R"(
+ #include "unchecked_optional_access_test.h"
+
+ template <typename T1, typename T2>
+ struct pair { T1 fst; T2 snd; };
+ int target() {
+ auto [contents, x] = Make<pair<$ns::$optional<int>, int>>();
+ return contents ? *contents : x;
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, StructuredBindingsFromTupleLikeType) {
+ ExpectDiagnosticsFor(R"(
+ #include "unchecked_optional_access_test.h"
+
+ namespace std {
+ template <class> struct tuple_size;
+ template <size_t, class> struct tuple_element;
+ template <class...> class tuple;
+
+ template <class... T>
+ struct tuple_size<tuple<T...>> : integral_constant<size_t, sizeof...(T)> {};
+
+ template <size_t I, class... T>
+ struct tuple_element<I, tuple<T...>> {
+ using type = __type_pack_element<I, T...>;
+ };
+
+ template <class...> class tuple {};
+ template <size_t I, class... T>
+ typename tuple_element<I, tuple<T...>>::type get(tuple<T...>);
+ } // namespace std
+
+ std::tuple<$ns::$optional<const char *>, int> get_opt();
+ void target() {
+ auto [content, ck] = get_opt();
+ content ? *content : "";
+ }
+ )");
+}
+
// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3613,6 +3613,172 @@
});
}
+TEST(TransferTest, StructuredBindingAssignFromTupleLikeType) {
+ std::string Code = R"(
+ namespace std {
+ using size_t = int;
+ template <class> struct tuple_size;
+ template <std::size_t, class> struct tuple_element;
+ template <class...> class tuple;
+
+ namespace {
+ template <class T, T v>
+ struct size_helper { static const T value = v; };
+ } // namespace
+
+ template <class... T>
+ struct tuple_size<tuple<T...>> : size_helper<std::size_t, sizeof...(T)> {};
+
+ template <std::size_t I, class... T>
+ struct tuple_element<I, tuple<T...>> {
+ using type = __type_pack_element<I, T...>;
+ };
+
+ template <class...> class tuple {};
+
+ template <std::size_t I, class... T>
+ typename tuple_element<I, tuple<T...>>::type get(tuple<T...>);
+ } // namespace std
+
+ std::tuple<bool, int> makeTuple();
+
+ void target(bool B) {
+ auto [BoundFoo, BoundBar] = makeTuple();
+ bool Baz;
+ // Include if-then-else to test interaction of `BindingDecl` with join.
+ if (B) {
+ Baz = BoundFoo;
+ (void)BoundBar;
+ // [[p1]]
+ } else {
+ Baz = BoundFoo;
+ }
+ (void)0;
+ // [[p2]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1", "p2"));
+ const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
+
+ const ValueDecl *BoundFooDecl = findValueDecl(ASTCtx, "BoundFoo");
+ ASSERT_THAT(BoundFooDecl, NotNull());
+
+ const ValueDecl *BoundBarDecl = findValueDecl(ASTCtx, "BoundBar");
+ ASSERT_THAT(BoundBarDecl, NotNull());
+
+ const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
+ ASSERT_THAT(BazDecl, NotNull());
+
+ const Value *BoundFooValue =
+ Env1.getValue(*BoundFooDecl, SkipPast::Reference);
+ ASSERT_THAT(BoundFooValue, NotNull());
+ EXPECT_TRUE(isa<BoolValue>(BoundFooValue));
+
+ const Value *BoundBarValue =
+ Env1.getValue(*BoundBarDecl, SkipPast::Reference);
+ ASSERT_THAT(BoundBarValue, NotNull());
+ EXPECT_TRUE(isa<IntegerValue>(BoundBarValue));
+
+ // Test that a `DeclRefExpr` to a `BindingDecl` works as expected.
+ EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::Reference), BoundFooValue);
+
+ const Environment &Env2 = getEnvironmentAtAnnotation(Results, "p2");
+
+ // Test that `BoundFooDecl` retains the value we expect, after the join.
+ BoundFooValue = Env2.getValue(*BoundFooDecl, SkipPast::Reference);
+ EXPECT_EQ(Env2.getValue(*BazDecl, SkipPast::Reference), BoundFooValue);
+ });
+}
+
+TEST(TransferTest, StructuredBindingAssignRefFromTupleLikeType) {
+ std::string Code = R"(
+ namespace std {
+ using size_t = int;
+ template <class> struct tuple_size;
+ template <std::size_t, class> struct tuple_element;
+ template <class...> class tuple;
+
+ namespace {
+ template <class T, T v>
+ struct size_helper { static const T value = v; };
+ } // namespace
+
+ template <class... T>
+ struct tuple_size<tuple<T...>> : size_helper<std::size_t, sizeof...(T)> {};
+
+ template <std::size_t I, class... T>
+ struct tuple_element<I, tuple<T...>> {
+ using type = __type_pack_element<I, T...>;
+ };
+
+ template <class...> class tuple {};
+
+ template <std::size_t I, class... T>
+ typename tuple_element<I, tuple<T...>>::type get(tuple<T...>);
+ } // namespace std
+
+ std::tuple<bool, int> &getTuple();
+
+ void target(bool B) {
+ auto &[BoundFoo, BoundBar] = getTuple();
+ bool Baz;
+ // Include if-then-else to test interaction of `BindingDecl` with join.
+ if (B) {
+ Baz = BoundFoo;
+ (void)BoundBar;
+ // [[p1]]
+ } else {
+ Baz = BoundFoo;
+ }
+ (void)0;
+ // [[p2]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1", "p2"));
+ const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
+
+ const ValueDecl *BoundFooDecl = findValueDecl(ASTCtx, "BoundFoo");
+ ASSERT_THAT(BoundFooDecl, NotNull());
+
+ const ValueDecl *BoundBarDecl = findValueDecl(ASTCtx, "BoundBar");
+ ASSERT_THAT(BoundBarDecl, NotNull());
+
+ const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz");
+ ASSERT_THAT(BazDecl, NotNull());
+
+ const Value *BoundFooValue =
+ Env1.getValue(*BoundFooDecl, SkipPast::Reference);
+ ASSERT_THAT(BoundFooValue, NotNull());
+ EXPECT_TRUE(isa<BoolValue>(BoundFooValue));
+
+ const Value *BoundBarValue =
+ Env1.getValue(*BoundBarDecl, SkipPast::Reference);
+ ASSERT_THAT(BoundBarValue, NotNull());
+ EXPECT_TRUE(isa<IntegerValue>(BoundBarValue));
+
+ // Test that a `DeclRefExpr` to a `BindingDecl` (with reference type)
+ // works as expected. We don't test aliasing properties of the
+ // reference, because we don't model `std::get` and so have no way to
+ // equate separate references into the tuple.
+ EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::Reference), BoundFooValue);
+
+ const Environment &Env2 = getEnvironmentAtAnnotation(Results, "p2");
+
+ // Test that `BoundFooDecl` retains the value we expect, after the join.
+ BoundFooValue = Env2.getValue(*BoundFooDecl, SkipPast::Reference);
+ EXPECT_EQ(Env2.getValue(*BazDecl, SkipPast::Reference), BoundFooValue);
+ });
+}
+// TODO: ref binding
+
TEST(TransferTest, BinaryOperatorComma) {
std::string Code = R"(
void target(int Foo, int Bar) {
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -346,14 +346,12 @@
TypeErasedDataflowAnalysisState &State,
AnalysisContext &AC) {
switch (Elt.getKind()) {
- case CFGElement::Statement: {
+ case CFGElement::Statement:
builtinTransferStatement(Elt.castAs<CFGStmt>(), State, AC);
break;
- }
- case CFGElement::Initializer: {
+ case CFGElement::Initializer:
builtinTransferInitializer(Elt.castAs<CFGInitializer>(), State);
break;
- }
default:
// FIXME: Evaluate other kinds of `CFGElement`.
break;
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -188,13 +188,42 @@
}
}
+ // void VisitDeclRefExpr(const DeclRefExpr *S) {
+ // assert(S->getDecl() != nullptr);
+ // auto *DeclLoc = Env.getStorageLocation(*S->getDecl(), SkipPast::None);
+ // if (DeclLoc == nullptr) {
+ // // Lazily initialize the decl when it's a BindingDecl with a holding-var.
+ // // FIXME: this code is only necessary because the CFG places the
+ // // `DecompositionDecl` _before_ the holding var decls, so the normal eager
+ // // initialization fails.
+ // if (auto *B = dyn_cast<BindingDecl>(S->getDecl()))
+ // if (auto *VD = B->getHoldingVar())
+ // if (auto *Loc = Env.getStorageLocation(*VD, SkipPast::None)) {
+ // Env.setStorageLocation(*B, *Loc);
+ // DeclLoc = Loc;
+ // }
+ // if (DeclLoc == nullptr)
+ // return;
+ // }
+
+ // if (S->getDecl()->getType()->isReferenceType()) {
+ // Env.setStorageLocation(*S, *DeclLoc);
+ // } else {
+ // auto &Loc = Env.createStorageLocation(*S);
+ // auto &Val = Env.takeOwnership(std::make_unique<ReferenceValue>(*DeclLoc));
+ // Env.setStorageLocation(*S, Loc);
+ // Env.setValue(Loc, Val);
+ // }
+ // }
+
void VisitDeclRefExpr(const DeclRefExpr *S) {
- assert(S->getDecl() != nullptr);
- auto *DeclLoc = Env.getStorageLocation(*S->getDecl(), SkipPast::None);
+ const ValueDecl *VD = S->getDecl();
+ assert(VD != nullptr);
+ auto *DeclLoc = Env.getStorageLocation(*VD, SkipPast::None);
if (DeclLoc == nullptr)
return;
- if (S->getDecl()->getType()->isReferenceType()) {
+ if (VD->getType()->isReferenceType()) {
Env.setStorageLocation(*S, *DeclLoc);
} else {
auto &Loc = Env.createStorageLocation(*S);
@@ -213,8 +242,15 @@
if (D.hasGlobalStorage())
return;
- auto &Loc = Env.createStorageLocation(D);
- Env.setStorageLocation(D, Loc);
+ // The storage location for `D` could have been created earlier, before the
+ // variable's declaration statement (for example, in the case of
+ // BindingDecls).
+ auto *MaybeLoc = Env.getStorageLocation(D, SkipPast::None);
+ if (MaybeLoc == nullptr) {
+ MaybeLoc = &Env.createStorageLocation(D);
+ Env.setStorageLocation(D, *MaybeLoc);
+ }
+ auto &Loc = *MaybeLoc;
const Expr *InitExpr = D.getInit();
if (InitExpr == nullptr) {
@@ -258,24 +294,30 @@
// needs to be evaluated after initializing the values in the storage for
// VarDecl, as the bindings refer to them.
// FIXME: Add support for ArraySubscriptExpr.
- // FIXME: Consider adding AST nodes that are used for structured bindings
- // to the CFG.
+ // FIXME: Consider adding AST nodes used in BindingDecls to the CFG.
for (const auto *B : Decomp->bindings()) {
- auto *ME = dyn_cast_or_null<MemberExpr>(B->getBinding());
- if (ME == nullptr)
- continue;
-
- auto *DE = dyn_cast_or_null<DeclRefExpr>(ME->getBase());
- if (DE == nullptr)
- continue;
-
- // ME and its base haven't been visited because they aren't included in
- // the statements of the CFG basic block.
- VisitDeclRefExpr(DE);
- VisitMemberExpr(ME);
-
- if (auto *Loc = Env.getStorageLocation(*ME, SkipPast::Reference))
- Env.setStorageLocation(*B, *Loc);
+ if (auto *ME = dyn_cast_or_null<MemberExpr>(B->getBinding())) {
+ auto *DE = dyn_cast_or_null<DeclRefExpr>(ME->getBase());
+ if (DE == nullptr)
+ continue;
+
+ // ME and its base haven't been visited because they aren't included
+ // in the statements of the CFG basic block.
+ VisitDeclRefExpr(DE);
+ VisitMemberExpr(ME);
+
+ if (auto *Loc = Env.getStorageLocation(*ME, SkipPast::Reference))
+ Env.setStorageLocation(*B, *Loc);
+ } else if (auto *VD = B->getHoldingVar()) {
+ // Holding vars are used to back the BindingDecls of tuple-like
+ // types. The holding var declarations appear *after* this statement,
+ // so we have to create a location or them here to share with `B`. We
+ // don't visit the binding, because we know it will be a DeclRefExpr
+ // to `VD`.
+ auto &VDLoc = Env.createStorageLocation(*VD);
+ Env.setStorageLocation(*VD, VDLoc);
+ Env.setStorageLocation(*B, VDLoc);
+ }
}
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
