From 02562804d074a4d6e041e00572483fe25932186e Mon Sep 17 00:00:00 2001 From: Yitzhak Mandelbaum Date: Tue, 3 Jan 2023 15:41:38 +0000 Subject: [PATCH] [clang][dataflow] Fix handling of `DeclRefExpr`s to `BindingDecl`s. The invariants around `ReferenceValues` are subtle (arguably, too much so). That includes that we need to take care not to double wrap them -- in cases where we wrap a loc in an `ReferenceValue` we need to be sure that the pointee isn't already a `ReferenceValue`. `BindingDecl` introduces another situation in which this can arise. Previously, the code did not properly handle `BindingDecl`, resulting in double-wrapped values, which broke other invariants (at least, that struct values have an `AggregateStorageLocation`). This patch adjusts the interpretation of `DeclRefExpr` to take `BindingDecl`'s peculiarities into account. It also fixes the two tests which should have caught this issue but were themselves (subtly) buggy. Differential Revision: https://reviews.llvm.org/D140897 --- clang/lib/Analysis/FlowSensitive/Transfer.cpp | 31 +++++++++++++++++----- .../Analysis/FlowSensitive/TransferTest.cpp | 11 ++++---- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 0e6c484..74dd598 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -108,7 +108,8 @@ static BoolValue &unpackValue(BoolValue &V, Environment &Env) { } // Unpacks the value (if any) associated with `E` and updates `E` to the new -// value, if any unpacking occured. +// value, if any unpacking occured. Also, does the lvalue-to-rvalue conversion, +// by skipping past the reference. static Value *maybeUnpackLValueExpr(const Expr &E, Environment &Env) { // FIXME: this is too flexible: it _allows_ a reference, while it should // _require_ one, since lvalues should always be wrapped in `ReferenceValue`. @@ -146,7 +147,9 @@ public: if (LHSLoc == nullptr) break; - auto *RHSVal = Env.getValue(*RHS, SkipPast::Reference); + // No skipping should be necessary, because any lvalues should have + // already been stripped off in evaluating the LValueToRValue cast. + auto *RHSVal = Env.getValue(*RHS, SkipPast::None); if (RHSVal == nullptr) break; @@ -196,9 +199,17 @@ public: if (DeclLoc == nullptr) return; - if (VD->getType()->isReferenceType()) { - assert(isa_and_nonnull(Env.getValue((*DeclLoc))) && - "reference-typed declarations map to `ReferenceValue`s"); + // If the value is already an lvalue, don't double-wrap it. + if (isa_and_nonnull(Env.getValue(*DeclLoc))) { + // We only expect to encounter a `ReferenceValue` for a reference type + // (always) or for `BindingDecl` (sometimes). For the latter, we can't + // rely on type, because their type does not indicate whether they are a + // reference type. The assert is not strictly necessary, since we don't + // depend on its truth to proceed. But, it verifies our assumptions, + // which, if violated, probably indicate a problem elsewhere. + assert((VD->getType()->isReferenceType() || isa(VD)) && + "Only reference-typed declarations or `BindingDecl`s should map " + "to `ReferenceValue`s"); Env.setStorageLocation(*S, *DeclLoc); } else { auto &Loc = Env.createStorageLocation(*S); @@ -238,6 +249,7 @@ public: if (D.getType()->isReferenceType()) { // Initializing a reference variable - do not create a reference to // reference. + // FIXME: reuse the ReferenceValue instead of creating a new one. if (auto *InitExprLoc = Env.getStorageLocation(*InitExpr, SkipPast::Reference)) { auto &Val = @@ -264,6 +276,9 @@ public: Env.setValue(Loc, *Val); } + // `DecompositionDecl` must be handled after we've interpreted the loc + // itself, because the binding expression refers back to the + // `DecompositionDecl` (even though it has no written name). if (const auto *Decomp = dyn_cast(&D)) { // If VarDecl is a DecompositionDecl, evaluate each of its bindings. This // needs to be evaluated after initializing the values in the storage for @@ -288,7 +303,8 @@ public: // types. The holding var declarations appear *after* this statement, // so we have to create a location for them here to share with `B`. We // don't visit the binding, because we know it will be a DeclRefExpr - // to `VD`. + // to `VD`. Note that, by construction of the AST, `VD` will always be + // a reference -- either lvalue or rvalue. auto &VDLoc = Env.createStorageLocation(*VD); Env.setStorageLocation(*VD, VDLoc); Env.setStorageLocation(*B, VDLoc); @@ -320,7 +336,8 @@ public: case CK_LValueToRValue: { // When an L-value is used as an R-value, it may result in sharing, so we - // need to unpack any nested `Top`s. + // need to unpack any nested `Top`s. We also need to strip off the + // `ReferenceValue` associated with the lvalue. auto *SubExprVal = maybeUnpackLValueExpr(*SubExpr, Env); if (SubExprVal == nullptr) break; diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 5d56d98..f626eb0 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -3893,6 +3893,8 @@ TEST(TransferTest, StructuredBindingAssignFromTupleLikeType) { const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz"); ASSERT_THAT(BazDecl, NotNull()); + // BindingDecls always map to references -- either lvalue or rvalue, so + // we still need to skip here. const Value *BoundFooValue = Env1.getValue(*BoundFooDecl, SkipPast::Reference); ASSERT_THAT(BoundFooValue, NotNull()); @@ -3904,13 +3906,13 @@ TEST(TransferTest, StructuredBindingAssignFromTupleLikeType) { EXPECT_TRUE(isa(BoundBarValue)); // Test that a `DeclRefExpr` to a `BindingDecl` works as expected. - EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::Reference), BoundFooValue); + EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::None), 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); + EXPECT_EQ(Env2.getValue(*BazDecl, SkipPast::None), BoundFooValue); }); } @@ -3988,16 +3990,15 @@ TEST(TransferTest, StructuredBindingAssignRefFromTupleLikeType) { // 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); + EXPECT_EQ(Env1.getValue(*BazDecl, SkipPast::None), 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); + EXPECT_EQ(Env2.getValue(*BazDecl, SkipPast::None), BoundFooValue); }); } -// TODO: ref binding TEST(TransferTest, BinaryOperatorComma) { std::string Code = R"( -- 2.7.4