[clang] 0256280 - [clang][dataflow] Fix handling of `DeclRefExpr`s to `BindingDecl`s.

Yitzhak Mandelbaum via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 1 05:23:42 PST 2023


Author: Yitzhak Mandelbaum
Date: 2023-02-01T13:23:23Z
New Revision: 02562804d074a4d6e041e00572483fe25932186e

URL: https://github.com/llvm/llvm-project/commit/02562804d074a4d6e041e00572483fe25932186e
DIFF: https://github.com/llvm/llvm-project/commit/02562804d074a4d6e041e00572483fe25932186e.diff

LOG: [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

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/Transfer.cpp
    clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 0e6c484b67e7d..74dd59851ad44 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 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       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 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     if (DeclLoc == nullptr)
       return;
 
-    if (VD->getType()->isReferenceType()) {
-      assert(isa_and_nonnull<ReferenceValue>(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<ReferenceValue>(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<BindingDecl>(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 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     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 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
         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<DecompositionDecl>(&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 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
           // 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 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
 
     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 5d56d98235f15..f626eb0c01548 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<IntegerValue>(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"(


        


More information about the cfe-commits mailing list