[clang] Diagnose dangling references for parenthesized aggregate initialization.

[hokein]

Unlike brace initialization, the parenthesized aggregate initialization in C++20 does not extend the lifetime of a temporary object bound to a reference in an aggreate. This can lead to dangling references:

struct A { const int& r; };
 A a1(1); // well-formed, but results in a dangling reference.

With this patch, clang will diagnose this common dangling issues.

Fixes #101957

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

Fixes #101957

---

Full diff: https://github.com/llvm/llvm-project/pull/117690.diff

5 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/CheckExprLifetime.cpp (+32)
• (modified) clang/test/AST/ByteCode/records.cpp (+4-2)
• (added) clang/test/SemaCXX/cxx20-warn-dangling-paren-list-agg-init.cpp (+42)
• (modified) clang/test/SemaCXX/paren-list-agg-init.cpp (+4)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 954fe61f3d1d69..af9ab752a0e22e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -587,6 +587,8 @@ Improvements to Clang's diagnostics
 - For an rvalue reference bound to a temporary struct with an integer member, Clang will detect constant integer overflow
   in the initializer for the integer member (#GH46755).
 
+- Clang now diagnoses dangling references for C++20's parenthesized aggregate initialization (#101957).
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 6cdd4dc629e50a..16b5fba40692e2 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -13,6 +13,7 @@
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/STLExtras.h"
 
 namespace clang::sema {
 namespace {
@@ -203,6 +204,7 @@ struct IndirectLocalPathEntry {
     GslPointerInit,
     GslPointerAssignment,
     DefaultArg,
+    ParenAggInit,
   } Kind;
   Expr *E;
   union {
@@ -961,6 +963,16 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
   if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init))
     return visitFunctionCallArguments(Path, Init, Visit);
 
+  if (auto *CPE = dyn_cast<CXXParenListInitExpr>(Init)) {
+    Path.push_back({IndirectLocalPathEntry::ParenAggInit, CPE});
+    for (auto *I : CPE->getInitExprs()) {
+      if (I->isGLValue())
+        visitLocalsRetainedByReferenceBinding(Path, I, RK_ReferenceBinding,
+                                              Visit);
+      else
+        visitLocalsRetainedByInitializer(Path, I, Visit, true);
+    }
+  }
   switch (Init->getStmtClass()) {
   case Stmt::UnaryOperatorClass: {
     auto *UO = cast<UnaryOperator>(Init);
@@ -1057,6 +1069,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
     case IndirectLocalPathEntry::GslReferenceInit:
     case IndirectLocalPathEntry::GslPointerInit:
     case IndirectLocalPathEntry::GslPointerAssignment:
+    case IndirectLocalPathEntry::ParenAggInit:
       // These exist primarily to mark the path as not permitting or
       // supporting lifetime extension.
       break;
@@ -1193,6 +1206,24 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity,
         if (pathContainsInit(Path))
           return false;
 
+        // In C++20, parenthesized aggregate initialization does not extend the
+        // lifetime of a temporary object bound to a reference. This can lead to
+        // dangling references and clang diagnoses these cases:
+        //
+        //    struct A { const int& r; };
+        //    A a1(1); // well-formed, but results in a dangling reference.
+        //
+        // However, to reduce noise, we suppress diagnostics for cases where
+        // both the aggregate and the temporary object are destroyed at the end
+        // of the full expression:
+        //    f(A(1));
+        if (!Path.empty() &&
+            llvm::any_of(llvm::ArrayRef(Path).drop_front(), [](auto &P) {
+              return P.Kind == IndirectLocalPathEntry::ParenAggInit;
+            })) {
+          return false;
+        }
+
         SemaRef.Diag(DiagLoc, diag::warn_dangling_variable)
             << RK << !InitEntity->getParent()
             << ExtendingEntity->getDecl()->isImplicit()
@@ -1368,6 +1399,7 @@ checkExprLifetimeImpl(Sema &SemaRef, const InitializedEntity *InitEntity,
       switch (Elem.Kind) {
       case IndirectLocalPathEntry::AddressOf:
       case IndirectLocalPathEntry::LValToRVal:
+      case IndirectLocalPathEntry::ParenAggInit:
         // These exist primarily to mark the path as not permitting or
         // supporting lifetime extension.
         break;
diff --git a/clang/test/AST/ByteCode/records.cpp b/clang/test/AST/ByteCode/records.cpp
index 2eeaafc04c516c..4601aface135ee 100644
--- a/clang/test/AST/ByteCode/records.cpp
+++ b/clang/test/AST/ByteCode/records.cpp
@@ -1015,11 +1015,13 @@ namespace ParenInit {
   };
 
   /// Not constexpr!
-  O o1(0);
+  O o1(0); // both-warning {{temporary whose address is used as value of}}
+  // FIXME: the secondary warning message is bogus, would be nice to suppress it.
   constinit O o2(0); // both-error {{variable does not have a constant initializer}} \
                      // both-note {{required by 'constinit' specifier}} \
                      // both-note {{reference to temporary is not a constant expression}} \
-                     // both-note {{temporary created here}}
+                     // both-note {{temporary created here}} \
+                     // both-warning {{temporary whose address is used as value}}
 
 
   /// Initializing an array.
diff --git a/clang/test/SemaCXX/cxx20-warn-dangling-paren-list-agg-init.cpp b/clang/test/SemaCXX/cxx20-warn-dangling-paren-list-agg-init.cpp
new file mode 100644
index 00000000000000..8db0d5407cdbce
--- /dev/null
+++ b/clang/test/SemaCXX/cxx20-warn-dangling-paren-list-agg-init.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+
+namespace std {
+template <class T> struct remove_reference { typedef T type; };
+template <class T> struct remove_reference<T&> { typedef T type; };
+template <class T> struct remove_reference<T&&> { typedef T type; };
+
+template <class T> typename remove_reference<T>::type &&move(T &&t);
+} // namespace std
+
+// dcl.init 16.6.2.2
+struct A {
+  int a;
+  int&& r;
+};
+
+int f();
+int n = 10;
+
+// well-formed, but dangling reference
+A a2(1, f()); // expected-warning {{temporary whose address is used as value}}
+// well-formed, but dangling reference             
+A a4(1.0, 1); // expected-warning {{temporary whose address is used as value}}            
+A a5(1.0, std::move(n));  // OK
+
+
+
+struct B {
+  const int& r;
+};
+B test(int local) {
+  return B(1); // expected-warning {{returning address}}
+  return B(local); // expected-warning {{address of stack memory}}
+}
+
+void f(B b);
+void test2(int local) {
+  // No diagnostic on the following cases where both the aggregate object and
+  // temporary end at the end of the full expression.
+  f(B(1));
+  f(B(local));
+}
diff --git a/clang/test/SemaCXX/paren-list-agg-init.cpp b/clang/test/SemaCXX/paren-list-agg-init.cpp
index cc2a9d88dd4a6e..61afba85e1dff9 100644
--- a/clang/test/SemaCXX/paren-list-agg-init.cpp
+++ b/clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -116,6 +116,7 @@ void foo(int n) { // expected-note {{declared here}}
   B b2(A(1), {}, 1);
   // beforecxx20-warning@-1 {{aggregate initialization of type 'A' from a parenthesized list of values is a C++20 extension}}
   // beforecxx20-warning@-2 {{aggregate initialization of type 'B' from a parenthesized list of values is a C++20 extension}}
+  // expected-warning@-3 {{temporary whose address is used as value of local variable 'b2' will be destroyed at the end of the full-expression}}
 
   C c(A(1), 1, 2, 3, 4);
   // expected-error@-1 {{array initializer must be an initializer list}}
@@ -262,9 +263,12 @@ struct O {
 
 O o1(0, 0, 0); // no-error
 // beforecxx20-warning@-1 {{aggregate initialization of type 'O' from a parenthesized list of values is a C++20 extension}}
+// expected-warning@-2 {{temporary whose address is used as value of local variable 'o1' will be destroyed at the end of the full-expression}}
+// expected-warning@-3 {{temporary whose address is used as value of local variable 'o1' will be destroyed at the end of the full-expression}}
 
 O o2(0, 0); // no-error
 // beforecxx20-warning@-1 {{aggregate initialization of type 'O' from a parenthesized list of values is a C++20 extension}}
+// expected-warning@-2 {{temporary whose address is used as value of local variable 'o2' will be destroyed at the end of the full-expression}}
 
 O o3(0);
 // expected-error@-1 {{reference member of type 'int &&' uninitialized}}

[Xazax-hun]

LGTM, could you add a summary of the change to the commit message? I know the context is available in the linked issue, but I'd like the gist of it to be preserved in the commit history directly.

[hokein]

LGTM, could you add a summary of the change to the commit message? I know the context is available in the linked issue, but I'd like the gist of it to be preserved in the commit history directly.

Done.

[usx95]

Wow. This looks quite nice now without the special handling in NoExtend case.
Was the missing piece Path.pop_back(); ?
(nit: I prefer to add more commits instead of overwriting previous one to allow for incremental reviews.)

[usx95]

nit: Use RevertToOldSizeRAII.

[hokein]

Done.

[hokein]

Wow. This looks quite nice now without the special handling in NoExtend case. Was the missing piece Path.pop_back(); ? (nit: I prefer to add more commits instead of overwriting previous one to allow for incremental reviews.)

I realized the special handling code wasn’t needed at all -- it might have been a leftover from my earlier experiments. Keeping it would actually break the nested test case.

As for the missing Path.pop_back(), I spotted it while reading the code.