[clang] [Clang] Extend lifetime bound analysis to support assignments (PR #96475)

Haojian Wu via cfe-commits cfe-commits at lists.llvm.org
Fri Jun 28 07:26:20 PDT 2024


https://github.com/hokein updated https://github.com/llvm/llvm-project/pull/96475

>From 97d1b80680112c3fa271501427a18273aed61dbe Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Mon, 24 Jun 2024 10:58:53 +0200
Subject: [PATCH 1/2] [clang] Extend the existing lifetimebound check for
 assignments.

Currently we only detect the builtin pointer type.
---
 clang/include/clang/Basic/DiagnosticGroups.td |  4 +-
 .../clang/Basic/DiagnosticSemaKinds.td        |  5 ++
 clang/lib/Sema/CheckExprLifetime.cpp          | 60 ++++++++++++++-----
 clang/lib/Sema/CheckExprLifetime.h            | 18 +++++-
 clang/lib/Sema/SemaExpr.cpp                   |  4 ++
 clang/lib/Sema/SemaInit.cpp                   |  2 +-
 clang/test/Parser/compound_literal.c          |  5 +-
 clang/test/SemaCXX/attr-lifetimebound.cpp     |  6 ++
 clang/test/SemaCXX/warn-dangling-local.cpp    |  2 +
 9 files changed, 83 insertions(+), 23 deletions(-)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 1c4f305fb5d00..9ae579c2c2a11 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -430,6 +430,7 @@ def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
 def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
 def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
+def DanglingAssignment: DiagGroup<"dangling-assignment">;
 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DanglingInitializerList : DiagGroup<"dangling-initializer-list">;
@@ -437,7 +438,8 @@ def DanglingGsl : DiagGroup<"dangling-gsl">;
 def ReturnStackAddress : DiagGroup<"return-stack-address">;
 // Name of this warning in GCC
 def : DiagGroup<"return-local-addr", [ReturnStackAddress]>;
-def Dangling : DiagGroup<"dangling", [DanglingField,
+def Dangling : DiagGroup<"dangling", [DanglingAssignment,
+                                      DanglingField,
                                       DanglingInitializerList,
                                       DanglingGsl,
                                       ReturnStackAddress]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 33412e14897b6..f613243cb8460 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10120,6 +10120,11 @@ def warn_new_dangling_initializer_list : Warning<
   "the allocated initializer list}0 "
   "will be destroyed at the end of the full-expression">,
   InGroup<DanglingInitializerList>;
+def warn_dangling_pointer_assignment : Warning<
+   "object backing the pointer %0 "
+   "will be destroyed at the end of the full-expression">,
+   InGroup<DanglingAssignment>;
+
 def warn_unsupported_lifetime_extension : Warning<
   "lifetime extension of "
   "%select{temporary|backing array of initializer list}0 created "
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 54e2f1c22536d..67fbd5e449d4a 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -8,6 +8,8 @@
 
 #include "CheckExprLifetime.h"
 #include "clang/AST/Expr.h"
+#include "clang/Basic/DiagnosticSema.h"
+#include "clang/Sema/Initialization.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/PointerIntPair.h"
 
@@ -964,11 +966,26 @@ static bool pathOnlyInitializesGslPointer(IndirectLocalPath &Path) {
   return false;
 }
 
-void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
+void checkExprLifetime(Sema &SemaRef, const CheckingEntity &CEntity,
                        Expr *Init) {
-  LifetimeResult LR = getEntityLifetime(&Entity);
-  LifetimeKind LK = LR.getInt();
-  const InitializedEntity *ExtendingEntity = LR.getPointer();
+  LifetimeKind LK = LK_FullExpression;
+
+  const AssignedEntity *AEntity = nullptr;
+  // Local variables for initialized entity.
+  const InitializedEntity *InitEntity = nullptr;
+  const InitializedEntity *ExtendingEntity = nullptr;
+  if (auto IEntityP = std::get_if<const InitializedEntity *>(&CEntity)) {
+    InitEntity = *IEntityP;
+    auto LTResult = getEntityLifetime(InitEntity);
+    LK = LTResult.getInt();
+    ExtendingEntity = LTResult.getPointer();
+  } else if (auto AEntityP = std::get_if<const AssignedEntity *>(&CEntity)) {
+    AEntity = *AEntityP;
+    if (AEntity->LHS->getType()->isPointerType()) // builtin pointer type
+      LK = LK_Extended;
+  } else {
+    llvm_unreachable("unexpected kind");
+  }
 
   // If this entity doesn't have an interesting lifetime, don't bother looking
   // for temporaries within its initializer.
@@ -1028,6 +1045,7 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
 
       switch (shouldLifetimeExtendThroughPath(Path)) {
       case PathLifetimeKind::Extend:
+        assert(InitEntity && "Expect only on initializing the entity");
         // Update the storage duration of the materialized temporary.
         // FIXME: Rebuild the expression instead of mutating it.
         MTE->setExtendingDecl(ExtendingEntity->getDecl(),
@@ -1036,6 +1054,7 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
         return true;
 
       case PathLifetimeKind::ShouldExtend:
+        assert(InitEntity && "Expect only on initializing the entity");
         // We're supposed to lifetime-extend the temporary along this path (per
         // the resolution of DR1815), but we don't support that yet.
         //
@@ -1053,16 +1072,23 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
         if (pathContainsInit(Path))
           return false;
 
-        SemaRef.Diag(DiagLoc, diag::warn_dangling_variable)
-            << RK << !Entity.getParent()
-            << ExtendingEntity->getDecl()->isImplicit()
-            << ExtendingEntity->getDecl() << Init->isGLValue() << DiagRange;
+        if (InitEntity) {
+          SemaRef.Diag(DiagLoc, diag::warn_dangling_variable)
+              << RK << !InitEntity->getParent()
+              << ExtendingEntity->getDecl()->isImplicit()
+              << ExtendingEntity->getDecl() << Init->isGLValue() << DiagRange;
+        } else {
+          SemaRef.Diag(DiagLoc, diag::warn_dangling_pointer_assignment)
+              << AEntity->LHS << DiagRange;
+        }
         break;
       }
       break;
     }
 
     case LK_MemInitializer: {
+      assert(InitEntity && "Expect only on initializing the entity");
+
       if (isa<MaterializeTemporaryExpr>(L)) {
         // Under C++ DR1696, if a mem-initializer (or a default member
         // initializer used by the absence of one) would lifetime-extend a
@@ -1077,7 +1103,7 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
                 << true;
             return false;
           }
-          bool IsSubobjectMember = ExtendingEntity != &Entity;
+          bool IsSubobjectMember = ExtendingEntity != InitEntity;
           SemaRef.Diag(DiagLoc, shouldLifetimeExtendThroughPath(Path) !=
                                         PathLifetimeKind::NoExtend
                                     ? diag::err_dangling_member
@@ -1137,6 +1163,7 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
     }
 
     case LK_New:
+      assert(InitEntity && "Expect only on initializing the entity");
       if (isa<MaterializeTemporaryExpr>(L)) {
         if (IsGslPtrInitWithGslTempOwner)
           SemaRef.Diag(DiagLoc, diag::warn_dangling_lifetime_pointer)
@@ -1145,7 +1172,7 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
           SemaRef.Diag(DiagLoc, RK == RK_ReferenceBinding
                                     ? diag::warn_new_dangling_reference
                                     : diag::warn_new_dangling_initializer_list)
-              << !Entity.getParent() << DiagRange;
+              << !InitEntity->getParent() << DiagRange;
       } else {
         // We can't determine if the allocation outlives the local declaration.
         return false;
@@ -1154,13 +1181,14 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
 
     case LK_Return:
     case LK_StmtExprResult:
+      assert(InitEntity && "Expect only on initializing the entity");
       if (auto *DRE = dyn_cast<DeclRefExpr>(L)) {
         // We can't determine if the local variable outlives the statement
         // expression.
         if (LK == LK_StmtExprResult)
           return false;
         SemaRef.Diag(DiagLoc, diag::warn_ret_stack_addr_ref)
-            << Entity.getType()->isReferenceType() << DRE->getDecl()
+            << InitEntity->getType()->isReferenceType() << DRE->getDecl()
             << isa<ParmVarDecl>(DRE->getDecl()) << DiagRange;
       } else if (isa<BlockExpr>(L)) {
         SemaRef.Diag(DiagLoc, diag::err_ret_local_block) << DiagRange;
@@ -1172,8 +1200,8 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
         SemaRef.Diag(DiagLoc, diag::warn_ret_addr_label) << DiagRange;
       } else if (auto *CLE = dyn_cast<CompoundLiteralExpr>(L)) {
         SemaRef.Diag(DiagLoc, diag::warn_ret_stack_addr_ref)
-            << Entity.getType()->isReferenceType() << CLE->getInitializer() << 2
-            << DiagRange;
+            << InitEntity->getType()->isReferenceType() << CLE->getInitializer()
+            << 2 << DiagRange;
       } else {
         // P2748R5: Disallow Binding a Returned Glvalue to a Temporary.
         // [stmt.return]/p6: In a function whose return type is a reference,
@@ -1181,12 +1209,12 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
         // a return statement that binds the returned reference to a temporary
         // expression ([class.temporary]) is ill-formed.
         if (SemaRef.getLangOpts().CPlusPlus26 &&
-            Entity.getType()->isReferenceType())
+            InitEntity->getType()->isReferenceType())
           SemaRef.Diag(DiagLoc, diag::err_ret_local_temp_ref)
-              << Entity.getType()->isReferenceType() << DiagRange;
+              << InitEntity->getType()->isReferenceType() << DiagRange;
         else
           SemaRef.Diag(DiagLoc, diag::warn_ret_local_temp_addr_ref)
-              << Entity.getType()->isReferenceType() << DiagRange;
+              << InitEntity->getType()->isReferenceType() << DiagRange;
       }
       break;
     }
diff --git a/clang/lib/Sema/CheckExprLifetime.h b/clang/lib/Sema/CheckExprLifetime.h
index f9cd3f55d3dbf..0882d41b35b6c 100644
--- a/clang/lib/Sema/CheckExprLifetime.h
+++ b/clang/lib/Sema/CheckExprLifetime.h
@@ -15,13 +15,25 @@
 #include "clang/AST/Expr.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Sema.h"
+#include <variant>
 
 namespace clang::sema {
 
+/// Describes an entity that is being assigned.
+struct AssignedEntity {
+  // The left-hand side expression of the assignment.
+  Expr *LHS = nullptr;
+};
+
+using CheckingEntity =
+    std::variant<const InitializedEntity *, const AssignedEntity *>;
+
 /// Check that the lifetime of the given expr (and its subobjects) is
-/// sufficient for initializing the entity, and perform lifetime extension
-/// (when permitted) if not.
-void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
+/// sufficient for initializing or assigning to the entity.
+///
+/// If the entity is being initialized and its lifetime is insufficient, perform
+/// lifetime extension (when permitted).
+void checkExprLifetime(Sema &SemaRef, const CheckingEntity &CEntity,
                        Expr *Init);
 
 } // namespace clang::sema
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index db44cfe1288b6..5c8b0682dcacc 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "CheckExprLifetime.h"
 #include "TreeTransform.h"
 #include "UsedDeclVisitor.h"
 #include "clang/AST/ASTConsumer.h"
@@ -13778,6 +13779,9 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
 
   CheckForNullPointerDereference(*this, LHSExpr);
 
+  AssignedEntity AE{LHSExpr};
+  checkExprLifetime(*this, &AE, RHS.get());
+
   if (getLangOpts().CPlusPlus20 && LHSType.isVolatileQualified()) {
     if (CompoundType.isNull()) {
       // C++2a [expr.ass]p5:
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index 26c65b34f4cc4..db2bacbad8a61 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -7287,7 +7287,7 @@ PerformConstructorInitialization(Sema &S,
 
 void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
                                     Expr *Init) {
-  return sema::checkExprLifetime(*this, Entity, Init);
+  return sema::checkExprLifetime(*this, &Entity, Init);
 }
 
 static void DiagnoseNarrowingInInitList(Sema &S,
diff --git a/clang/test/Parser/compound_literal.c b/clang/test/Parser/compound_literal.c
index 808ca63f97d9e..281a6d7ada430 100644
--- a/clang/test/Parser/compound_literal.c
+++ b/clang/test/Parser/compound_literal.c
@@ -1,8 +1,9 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// RUN: %clang_cc1 -fsyntax-only -verify -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -verify -x c++ -Wno-dangling-assignment %s
 // expected-no-diagnostics
 int main(void) {
   char *s;
-  s = (char []){"whatever"};
+  // In C++ mode, the cast creates a "char [4]" array temporary here.
+  s = (char []){"whatever"};  // dangling!
   s = (char(*)){s};
 }
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index 5fcda6f23dab7..7c572ecef52ba 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -40,6 +40,12 @@ namespace usage_ok {
   int *p = A().class_member(); // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
   int *q = A(); // expected-warning {{temporary whose address is used as value of local variable 'q' will be destroyed at the end of the full-expression}}
   int *r = A(1); // expected-warning {{temporary whose address is used as value of local variable 'r' will be destroyed at the end of the full-expression}}
+
+  void test_assignment() {
+    p = A().class_member(); // expected-warning {{object backing the pointer p will be destroyed at the end of the full-expression}}
+    q = A(); // expected-warning {{object backing the pointer q will be destroyed at the end of the full-expression}}
+    r = A(1); // expected-warning {{object backing the pointer r will be destroyed at the end of the full-expression}}
+  }
 }
 
 # 1 "<std>" 1 3
diff --git a/clang/test/SemaCXX/warn-dangling-local.cpp b/clang/test/SemaCXX/warn-dangling-local.cpp
index 5c1d56972945c..a946b8a241a38 100644
--- a/clang/test/SemaCXX/warn-dangling-local.cpp
+++ b/clang/test/SemaCXX/warn-dangling-local.cpp
@@ -4,8 +4,10 @@ using T = int[];
 
 void f() {
   int *p = &(int&)(int&&)0; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  p =  &(int&)(int&&)0; // expected-warning {{object backing the pointer p will be destroyed at the end of the full-expression}}
 
   int *q = (int *const &)T{1, 2, 3}; // expected-warning {{temporary whose address is used as value of local variable 'q' will be destroyed at the end of the full-expression}}
+  q = (int *const &)T{1, 2, 3}; // expected-warning {{object backing the pointer q will be destroyed at the end of the full-expression}}
 
   // FIXME: We don't warn here because the 'int*' temporary is not const, but
   // it also can't have actually changed since it was created, so we could

>From 78485d8f62eb36ecf7176f37ad19c43e2eea6a09 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Fri, 28 Jun 2024 16:25:23 +0200
Subject: [PATCH 2/2] Address review comments.

---
 clang/lib/Sema/CheckExprLifetime.cpp      | 10 +++++++---
 clang/test/SemaCXX/attr-lifetimebound.cpp |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 67fbd5e449d4a..e50fa4facdff6 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -992,6 +992,8 @@ void checkExprLifetime(Sema &SemaRef, const CheckingEntity &CEntity,
   if (LK == LK_FullExpression)
     return;
 
+  // FIXME: consider moving the TemporaryVisitor and visitLocalsRetained*
+  // functions to a dedicated class.
   auto TemporaryVisitor = [&](IndirectLocalPath &Path, Local L,
                               ReferenceKind RK) -> bool {
     SourceRange DiagRange = nextPathEntryRange(Path, 0, L);
@@ -1089,7 +1091,7 @@ void checkExprLifetime(Sema &SemaRef, const CheckingEntity &CEntity,
     case LK_MemInitializer: {
       assert(InitEntity && "Expect only on initializing the entity");
 
-      if (isa<MaterializeTemporaryExpr>(L)) {
+      if (MTE) {
         // Under C++ DR1696, if a mem-initializer (or a default member
         // initializer used by the absence of one) would lifetime-extend a
         // temporary, the program is ill-formed.
@@ -1280,8 +1282,10 @@ void checkExprLifetime(Sema &SemaRef, const CheckingEntity &CEntity,
                                           TemporaryVisitor,
                                           EnableLifetimeWarnings);
   else
-    visitLocalsRetainedByInitializer(Path, Init, TemporaryVisitor, false,
-                                     EnableLifetimeWarnings);
+    visitLocalsRetainedByInitializer(
+        Path, Init, TemporaryVisitor,
+        // Don't revisit the sub inits for the intialization case.
+        /*RevisitSubinits=*/!InitEntity, EnableLifetimeWarnings);
 }
 
 } // namespace clang::sema
diff --git a/clang/test/SemaCXX/attr-lifetimebound.cpp b/clang/test/SemaCXX/attr-lifetimebound.cpp
index 7c572ecef52ba..70bc545c07bd9 100644
--- a/clang/test/SemaCXX/attr-lifetimebound.cpp
+++ b/clang/test/SemaCXX/attr-lifetimebound.cpp
@@ -43,6 +43,7 @@ namespace usage_ok {
 
   void test_assignment() {
     p = A().class_member(); // expected-warning {{object backing the pointer p will be destroyed at the end of the full-expression}}
+    p = {A().class_member()}; // expected-warning {{object backing the pointer p will be destroyed at the end of the full-expression}}
     q = A(); // expected-warning {{object backing the pointer q will be destroyed at the end of the full-expression}}
     r = A(1); // expected-warning {{object backing the pointer r will be destroyed at the end of the full-expression}}
   }



More information about the cfe-commits mailing list