[Clang] Extend lifetime bound analysis to support assignments

clang/docs/ReleaseNotes.rst

@@ -626,6 +626,10 @@ Improvements to Clang's diagnostics
   used rather than when they are needed for constant evaluation or when code is generated for them.
   The check is now stricter to prevent crashes for some unsupported declarations (Fixes #GH95495).
 
+- Clang now diagnoses dangling cases where a pointer is assigned to a temporary
+  that will be destroyed at the end of the full expression.
+  Fixes #GH54492.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/include/clang/Basic/DiagnosticGroups.td

@@ -430,14 +430,16 @@ 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">;
 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]>;

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 "

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,17 +966,34 @@ static bool pathOnlyInitializesGslPointer(IndirectLocalPath &Path) {
   return false;
 }
 
-void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
-                       Expr *Init) {
-  LifetimeResult LR = getEntityLifetime(&Entity);
-  LifetimeKind LK = LR.getInt();
-  const InitializedEntity *ExtendingEntity = LR.getPointer();
-
+static void checkExprLifetimeImpl(
+    Sema &SemaRef,
+    llvm::PointerUnion<const InitializedEntity *, const AssignedEntity *>
+        CEntity,
+    Expr *Init) {
+  LifetimeKind LK = LK_FullExpression;
+
+  const AssignedEntity *AEntity = nullptr;
+  // Local variables for initialized entity.
+  const InitializedEntity *InitEntity = nullptr;
+  const InitializedEntity *ExtendingEntity = nullptr;
+  if (CEntity.is<const InitializedEntity *>()) {
+    InitEntity = CEntity.get<const InitializedEntity *>();
+    auto LTResult = getEntityLifetime(InitEntity);
+    LK = LTResult.getInt();
+    ExtendingEntity = LTResult.getPointer();
+  } else {
+    AEntity = CEntity.get<const AssignedEntity *>();
+    if (AEntity->LHS->getType()->isPointerType()) // builtin pointer type
+      LK = LK_Extended;
+  }
   // If this entity doesn't have an interesting lifetime, don't bother looking
   // for temporaries within its initializer.
   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);
@@ -1028,6 +1047,8 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
 
       switch (shouldLifetimeExtendThroughPath(Path)) {
       case PathLifetimeKind::Extend:
+        assert(InitEntity && "Lifetime extension should happen only for "
+                             "initialization and not assignment");
         // Update the storage duration of the materialized temporary.
         // FIXME: Rebuild the expression instead of mutating it.
         MTE->setExtendingDecl(ExtendingEntity->getDecl(),
@@ -1036,6 +1057,8 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
         return true;
 
       case PathLifetimeKind::ShouldExtend:
+        assert(InitEntity && "Lifetime extension should happen only for "
+                             "initialization and not assignment");
         // We're supposed to lifetime-extend the temporary along this path (per
         // the resolution of DR1815), but we don't support that yet.
         //
@@ -1053,17 +1076,24 @@ 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: {
-      if (isa<MaterializeTemporaryExpr>(L)) {
+      assert(InitEntity && "Expect only on initializing the entity");
+
+      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.
@@ -1077,7 +1107,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 +1167,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 +1176,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 +1185,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,21 +1204,21 @@ 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,
         // other than an invented function for std::is_convertible ([meta.rel]),
         // 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;
     }
@@ -1252,8 +1284,20 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
                                           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);
+}
+
+void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
+                       Expr *Init) {
+  checkExprLifetimeImpl(SemaRef, &Entity, Init);
+}
+
+void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity,
+                       Expr *Init) {
+  checkExprLifetimeImpl(SemaRef, &Entity, Init);
 }
 
 } // namespace clang::sema

clang/lib/Sema/CheckExprLifetime.h

@@ -18,12 +18,22 @@
 
 namespace clang::sema {
 
+/// Describes an entity that is being assigned.
+struct AssignedEntity {
+  // The left-hand side expression of the assignment.
+  Expr *LHS = nullptr;
+};
+
 /// 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,
                        Expr *Init);
 
+/// Check that the lifetime of the given expr (and its subobjects) is
+/// sufficient for assigning to the entity.
+void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity, Expr *Init);
+
 } // namespace clang::sema
 
 #endif // LLVM_CLANG_SEMA_CHECK_EXPR_LIFETIME_H

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:

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};
 }

clang/test/SemaCXX/attr-lifetimebound.cpp

@@ -40,6 +40,13 @@ 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}}
+    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

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