[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