[clang] Fix 2 bugs with parenthesized aggregate initialization

[ayzhao]

Summary

• Fix an issue where temporaries initialized via parenthesized aggregate initialization don't get destroyed.
• Fix an issue where aggregate initialization omits calls to class members' move constructors after a TreeTransform. This occurs because the CXXConstructExpr wrapping the call to the move constructor gets unboxed during a TreeTransform of the wrapping FunctionalCastExpr (as with a InitListExpr), but unlike InitListExpr, we dont reperform the InitializationSequence for the list's expressions to regenerate the CXXConstructExpr. This patch fixes this bug by treating CXXParenListInitExpr identically to InitListExpr in this regard.

Fixes #61145

[ayzhao]

clean up test

[ayzhao]

remove extra semicolon

[rsmith]

clang/lib/Sema/SemaExprCXX.cpp

1582–1596 ↗

(On Diff #506755)

The input to this function should be a syntactic initializer such as a ParenListExpr, not an already-type-checked semantic initializer such as a CXXParenListInitExpr. The right place to do this unwrapping is in TreeTransform::TransformInitializer, where we unwrap various other kinds of semantic initializer.

[ayzhao]

clang/lib/Sema/SemaExprCXX.cpp

1582–1596 ↗

(On Diff #506755)

The right place to do this unwrapping is in TreeTransform::TransformInitializer, where we unwrap various other kinds of semantic initializer.

So the CXXParenListInitExpr for S never gets passed to TreeTransform::TransformInitializer - rather, TransformCXXParenListInitExpr(...) calls TransformExprs(...) on its subexpressions. If the subexpression happens to be a CXXConstructExpr, then TransformCXXConstructExpr will call TransformInitializer(...), which is why we saw the CXXConstructExpr getting deleted

Interestingly enough, if we initialize S with a braced init-list (i.e. S{(V&&) Vec}), we never end up calling TransformCXXConstructExpr(...) since we revert to the syntatic form..

The input to this function should be a syntactic initializer such as a ParenListExpr, not an already-type-checked semantic initializer such as a CXXParenListInitExpr.

We can probably do this by returning a ParenListExpr in TransformCXXParenListInitExpr(...). This does cause some problems though. If we use a ParenListExpr, how would we know when to bail out in line 1551 (!isa<InitListExpr>(Exprs[0]) && !isa<CXXParenListInitExpr>(Exprs[0]))? Surely, not every instance of a ParenListExpr would be caused by a CXXParenListInitExpr, and if this were the case, that would be a pretty brittle assumption. Furthermore, we never create a ParenListExpr for a corresponding CXXParenListInitExpr.

As an alternative, we could probably do something similar to InitListExpr, which has separate semantic and syntactic forms. Would that work?

Also, in general, are the TransformFooExpr(...) functions supposed to return semantic or syntactic results? I seem to see a mix of both...

[rsmith]

clang/lib/Sema/SemaExprCXX.cpp

1582–1596 ↗

(On Diff #506755)

OK, I've looked at this a bit more. It looks to me like there are problems in two places:

1. TreeTransform::RebuildCXXParenListInitExpr. When transforming the subexpressions of a CXXParenListInitExpr, we call TransformExprs passing IsCall=true, which means we call TransformInitializer on each of them, which reverts them to syntactic form. So we need to redo the semantic analysis for forming a CXXParenListInitExpr here, and can't just create a new CXXParenListInitExpr node directly. Probably the easiest way to handle that would be to produce a ParenListExpr here instead.

2. Expr::getSubExprAsWritten (reached via here) is not properly handling CXXParenListExpr -- it should be stepping over it to the single argument of the expression, which was the as-written subexpression.

But looking more into (2), it seems like there's a representational oddity here, too. We use a CXXFunctionalCastExpr wrapping a CXXParenListExpr to model both the case of Aggregate(a) (which is a functional cast, equivalent by definition to (Aggregate)a) and the case of Aggregate(a, b) (which is not a functional cast). I don't think that's necessarily a problem -- we also incorrectly use CXXFunctionalCastExpr wrapping an InitListExpr for Aggregate{a, b} -- but it means that getSubExprAsWritten will need to be careful to only unwrap in the case where the parenthesized aggregate initialization expression had exactly one element, and it's not ideal from a source fidelity perspective.

In the long term, I think we should split CXXTemporaryObjectExpr into two -- an outer wrapper representing an explicit type construction expression of the form T(...) or T{...} (excluding the case T(a) which is a functional cast expression) and an inner CXXConstructExpr representing the actual constructor call. And then we can represent parenthesized aggregate initialization as either a CXXTemporaryObjectExpr wrapping a CXXParenListInitExpr (when the number of arguments is not equal to 1) or a CXXFunctionalCastExpr wrapping a CXXParenListInitExpr (when the number of arguments is equal to 1) -- and similarly we can represent the case of (Aggregate)a as a CastExpr wrapping a CXXParenListInitExpr. But for now I think we can leave the representation as-is and make TreeTranform handle it.

(Incidentally, the name CXXParenListInitExpr seems confusing here -- there aren't necessarily any parentheses involved, and the name really ought to mention aggregate initialization. CXXAggregateInitExpr would make more sense to me.)

If we use a ParenListExpr, how would we know when to bail out in line 1551

We should never see a ParenListExpr there (that would represent T((a, b)), not T(a, b), which would result in a comma expression not a ParenListExpr). Instead, the caller of BuildCXXTypeConstructExpr should convert the ParenListExpr into a list of expressions passed as Exprs, and pass the locations of the parentheses as LParenOrBraceLoc and RParenOrBraceLoc.

In particular, I think it would make sense for TransformCXXFunctionalCastExpr to detect when it's in this special case where the thing it's transforming isn't a functional cast expression, and call a different Rebuild... overload that takes a list of expressions instead of a single expression. (It would make some sense to call RebuildCXXTemporaryObjectExpr in this case.)

Also, in general, are the TransformFooExpr(...) functions supposed to return semantic or syntactic results? I seem to see a mix of both...

The intent is that TreeTransform on an expression removes all implicit nodes -- implicit conversions, semantic forms for initializers, and so on -- and returns the node to how it would have been before it was given to Sema as a subexpression. That means that the Sema functionality for building an expression with a given set of arguments doesn't need to handle the case where they've already been implicitly converted in some way, which gives us consistent behavior between the initial parse and template instantiation.

That means that when we see an expression being used as an initializer, we want to strip off all the purely semantic pieces and revert it back to a syntactic form -- that happens when transforming the initializer of a variable, an argument to a function, etc -- and also when we transform an expression that performed some kinds of conversions on its operands, we need to redo those conversions when transforming it.

[ayzhao]

use ParenListExpr and fix getSubExprAsWritten(...)

[ayzhao]

clang/lib/Sema/SemaExprCXX.cpp

1582–1596 ↗

(On Diff #506755)

Patch has been updated per this comment. PTAL.

[ayzhao]

fix stray whitespace change

[rsmith]

LGTM, thanks!

clang/lib/AST/Expr.cpp

if (auto *Materialize = dyn_cast<MaterializeTemporaryExpr>(E))
     return Materialize->getSubExpr();
 
   if (auto *Binder = dyn_cast<CXXBindTemporaryExpr>(E))
     return Binder->getSubExpr();
 
   if (auto *Full = dyn_cast<FullExpr>(E))
     return Full->getSubExpr();
 
+  if (auto *CPLIE = dyn_cast<CXXParenListInitExpr>(E);
+      CPLIE && CPLIE->getInitExprs().size() == 1)
+    return CPLIE->getInitExprs()[0];
+
   return E;
 }
 } // namespace
 
 Expr *CastExpr::getSubExprAsWritten() {
   const Expr *SubExpr = nullptr;
 
   for (const CastExpr *E = this; E; E = dyn_cast<ImplicitCastExpr>(SubExpr)) {

clang/lib/Sema/SemaInit.cpp

auto *CPLIE = CXXParenListInitExpr::Create(
         S.getASTContext(), InitExprs, ResultType, Args.size(),
         Kind.getLocation(), SR.getBegin(), SR.getEnd());
     if (ArrayFiller)
       CPLIE->setArrayFiller(ArrayFiller);
     if (InitializedFieldInUnion)
       CPLIE->setInitializedFieldInUnion(InitializedFieldInUnion);
     *Result = CPLIE;
     S.Diag(Kind.getLocation(),
-           diag::warn_cxx17_compat_aggregate_init_paren_list)
         << Kind.getLocation() << SR << ResultType;
   }
 
   return;
 }
 
 /// Attempt a user-defined conversion between two types (C++ [dcl.init]),
 /// which enumerates all conversion functions and performs overload resolution
case SK_OCLZeroOpaqueType: {
       break;
     }
     case SK_ParenthesizedListInit: {
       CurInit = nullptr;
       TryOrBuildParenListInitialization(S, Entity, Kind, Args, *this,
                                         /*VerifyOnly=*/false, &CurInit);
       if (CurInit.get() && ResultType)
         *ResultType = CurInit.get()->getType();
+      if (shouldBindAsTemporary(Entity))
+        CurInit = S.MaybeBindToTemporary(CurInit.get());
       break;
     }
     }
   }
 
   // Check whether the initializer has a shorter lifetime than the initialized
   // entity, and if not, either lifetime-extend or warn as appropriate.
   if (auto *Init = CurInit.get())

clang/lib/Sema/TreeTransform.h

   ///
   /// By default, performs semantic analysis to build the new expression.
   /// Subclasses may override this routine to provide different behavior.
   ExprResult RebuildCXXFunctionalCastExpr(TypeSourceInfo *TInfo,
                                           SourceLocation LParenLoc,
                                           Expr *Sub,
                                           SourceLocation RParenLoc,
                                           bool ListInitialization) {
+    // If Sub is a ParenListExpr, then Sub is the syntatic form of a
+    // CXXParenListInitExpr. Pass its expanded arguments so that the
+    // CXXParenListInitExpr can be rebuilt.
+    if (auto *PLE = dyn_cast<ParenListExpr>(Sub))
+      return getSema().BuildCXXTypeConstructExpr(
+          TInfo, LParenLoc, MultiExprArg(PLE->getExprs(), PLE->getNumExprs()),
+          RParenLoc, ListInitialization);
     return getSema().BuildCXXTypeConstructExpr(TInfo, LParenLoc,
                                                MultiExprArg(&Sub, 1), RParenLoc,
                                                ListInitialization);
   }
 
   /// Build a new C++ __builtin_bit_cast expression.
   ///
   /// By default, performs semantic analysis to build the new expression.

   ///
   /// By default, produces the fallback value for the fold-expression, or
   /// produce an error if there is no fallback value.
   ExprResult RebuildEmptyCXXFoldExpr(SourceLocation EllipsisLoc,
                                      BinaryOperatorKind Operator) {
     return getSema().BuildEmptyCXXFoldExpr(EllipsisLoc, Operator);
   }
 
-  ExprResult RebuildCXXParenListInitExpr(ArrayRef<Expr *> Args, QualType T,
-                                         unsigned NumUserSpecifiedExprs,
-                                         SourceLocation InitLoc,
-                                         SourceLocation LParenLoc,
-                                         SourceLocation RParenLoc) {
-    return CXXParenListInitExpr::Create(getSema().Context, Args, T,
-                                        NumUserSpecifiedExprs, InitLoc,
-                                        LParenLoc, RParenLoc);
-  }
-
   /// Build a new atomic operation expression.
   ///
   /// By default, performs semantic analysis to build the new expression.
   /// Subclasses may override this routine to provide different behavior.
   ExprResult RebuildAtomicExpr(SourceLocation BuiltinLoc, MultiExprArg SubExprs,
                                AtomicExpr::AtomicOp Op,
                                SourceLocation RParenLoc) {
     // Use this for all of the locations, since we don't know the difference

 ExprResult
 TreeTransform<Derived>::TransformCXXParenListInitExpr(CXXParenListInitExpr *E) {
   SmallVector<Expr *, 4> TransformedInits;
   ArrayRef<Expr *> InitExprs = E->getInitExprs();
   if (TransformExprs(InitExprs.data(), InitExprs.size(), true,
                      TransformedInits))
     return ExprError();
 
-  return getDerived().RebuildCXXParenListInitExpr(
-      TransformedInits, E->getType(), E->getUserSpecifiedInitExprs().size(),
-      E->getInitLoc(), E->getBeginLoc(), E->getEndLoc());
+  return getDerived().RebuildParenListExpr(E->getBeginLoc(), TransformedInits,
+                                           E->getEndLoc());
 }
 
 template<typename Derived>
 ExprResult
 TreeTransform<Derived>::TransformCXXStdInitializerListExpr(
     CXXStdInitializerListExpr *E) {
   return getDerived().TransformExpr(E->getSubExpr());
 }

clang/test/CodeGen/paren-list-agg-init.cpp

 // CHECK-DAG: [[UNION_U:%.*]] = type { [[STRUCT_A]] }
 // CHECK-DAG: [[STR:@.*]] = private unnamed_addr constant [6 x i8] {{.*}}foo18{{.*}}, align 1
 union U {
   unsigned : 1;
   A a;
   char b;
 };
 
+
+namespace gh61145 {
+  // CHECK-DAG: [[STRUCT_VEC:%.*]] = type { i8 }
+  struct Vec {
+    Vec();
+    Vec(Vec&&);
+    ~Vec();
+  };
+
+  // CHECK-DAG: [[STRUCT_S1:%.*]] = type { [[STRUCT_VEC]] }
+  struct S1 {
+    Vec v;
+  };
+
+  // CHECK-DAG: [[STRUCT_S2:%.*]] = type { [[STRUCT_VEC]], i8 }
+  struct S2 {
+    Vec v;
+    char c;
+  };
+}
+
 // CHECK-DAG: [[A1:@.*a1.*]] = internal constant [[STRUCT_A]] { i8 3, double 2.000000e+00 }, align 8
 constexpr A a1(3.1, 2.0);
 // CHECK-DAG: [[A2:@.*a2.*]] = internal constant [[STRUCT_A]] { i8 99, double 0.000000e+00 }, align 8
 constexpr auto a2 = static_cast<A>('c');
 // CHECK-DAG: [[B1:@.*b1.*]] = internal constant [[STRUCT_B]] { [[STRUCT_A]] { i8 99, double 0.000000e+00 }, i32 0 }, align 8
 constexpr B b1(A('c'));
 // CHECK-DAG: [[C1:@.*c1.*]] = internal constant { [[STRUCT_A]], i32, [4 x i8], i8, double, i32 } { [[STRUCT_A]] { i8 99, double 0.000000e+00 }, i32 0, [4 x i8] undef, i8 3, double 2.000000e+00, i32 0 }, align
 constexpr C c1(b1, a1);

 // CHECK-NEXT: [[A:%.*a.*]] = getelementptr inbounds [[STRUCT_G]], ptr [[G]], i32 0, i32 0
 // CHECK-NEXT: store i32 2, ptr [[A]], align 4
 // CHECK-NEXT: [[F:%.*f.*]] = getelementptr inbounds [[STRUCT_G]], ptr [[G]], i32 0, i32 1
 // CHECk-NEXT: call void @{{.*F.*}}(ptr noundef nonnull align 1 dereferenceable(1)) [[F]], ie32 noundef 1)
 // CHECK: ret void
 void foo19() {
   G g(2);
 }
+
+namespace gh61145 {
+  // a.k.a. void make1<0>()
+  // CHECK: define {{.*}} void @_ZN7gh611455make1ILi0EEEvv
+  // CHECK-NEXT: entry:
+  // CHECK-NEXT: [[V:%.*v.*]] = alloca [[STRUCT_VEC]], align 1
+  // CHECK-NEXT: [[AGG_TMP_ENSURED:%.*agg.tmp.ensured.*]] = alloca [[STRUCT_S1]], align 1
+  // a.k.a. Vec::Vec()
+  // CHECK-NEXT: call void @_ZN7gh611453VecC1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // CHECK-NEXT: [[V1:%.*v1.*]] = getelementptr inbounds [[STRUCT_S1]], ptr [[AGG_TMP_ENSURED]], i32 0, i32 0
+  // a.k.a. Vec::Vec(Vec&&)
+  // CHECK-NEXT: call void @_ZN7gh611453VecC1EOS0_(ptr noundef nonnull align 1 dereferenceable(1) [[V1]], ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // a.k.a. S1::~S1()
+  // CHECK-NEXT: call void @_ZN7gh611452S1D1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[AGG_TMP_ENSURED]])
+  // a.k.a.Vec::~Vec()
+  // CHECK-NEXT: call void @_ZN7gh611453VecD1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // CHECK-NEXT: ret void
+  template <int I>
+  void make1() {
+    Vec v;
+    S1((Vec&&) v);
+  }
+
+  // a.k.a. void make1<0>()
+  // CHECK: define {{.*}} void @_ZN7gh611455make2ILi0EEEvv
+  // CHECK-NEXT: entry:
+  // CHECK-NEXT: [[V:%.*v.*]] = alloca [[STRUCT_VEC]], align 1
+  // CHECK-NEXT: [[AGG_TMP_ENSURED:%.*agg.tmp.ensured.*]] = alloca [[STRUCT_S2]], align 1
+  // a.k.a. Vec::Vec()
+  // CHECK-NEXT: call void @_ZN7gh611453VecC1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // CHECK-NEXT: [[V1:%.*v1.*]] = getelementptr inbounds [[STRUCT_S2]], ptr [[AGG_TMP_ENSURED]], i32 0, i32 0
+  // a.k.a. Vec::Vec(Vec&&)
+  // CHECK-NEXT: call void @_ZN7gh611453VecC1EOS0_(ptr noundef nonnull align 1 dereferenceable(1) [[V1]], ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // CHECK-NEXT: [[C:%.*c.*]] = getelementptr inbounds [[STRUCT_S2]], ptr [[AGG_TMP_ENSURED]], i32 0, i32
+  // CHECK-NEXT: store i8 0, ptr [[C]], align 1
+  // a.k.a. S2::~S2()
+  // CHECK-NEXT: call void @_ZN7gh611452S2D1Ev(ptr noundef nonnull align 1 dereferenceable(2) [[AGG_TMP_ENSURED]])
+  // a.k.a. Vec::~Vec()
+  // CHECK-NEXT: call void @_ZN7gh611453VecD1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // CHECK-NEXT: ret void
+  template <int I>
+  void make2() {
+    Vec v;
+    S2((Vec&&) v, 0);
+  }
+
+  void foo() {
+    make1<0>();
+    make2<0>();
+  }
+}

clang/test/SemaCXX/paren-list-agg-init.cpp

union U {
   int a;
   char* b;
 };
 
 template <typename T, char CH>
 void bar() {
   T t = 0;
   A a(CH, 1.1); // OK; C++ paren list constructors are supported in semantic tree transformations.
-  // beforecxx20-warning@-1 {{aggregate initialization of type 'A' from a parenthesized list of values is a C++20 extension}}
+  // beforecxx20-warning@-1 2{{aggregate initialization of type 'A' from a parenthesized list of values is a C++20 extension}}
 }
 
 template <class T, class... Args>
 T Construct(Args... args) {
   return T(args...); // OK; variadic arguments can be used in paren list initializers.
   // beforecxx20-warning@-1 {{aggregate initialization of type 'A' from a parenthesized list of values is a C++20 extension}}
 }
 
void foo() {
 
   constexpr F f1(1);
   // expected-error@-1 {{constexpr variable 'f1' must be initialized by a constant expression}}
   // beforecxx20-warning@-2 {{aggregate initialization of type 'const F' from a parenthesized list of values is a C++20 extension}}
 
   constexpr F f2(1, 1); // OK: f2.b is initialized by a constant expression.
   // beforecxx20-warning@-1 {{aggregate initialization of type 'const F' from a parenthesized list of values is a C++20 extension}}
 
-  bar<char, 1>();
+  bar<int, 'a'>();
+  // beforecxx20-note@-1 {{in instantiation of function template specialization 'bar<int, 'a'>' requested here}}
 
   G<char> g('b', 'b');
   // beforecxx20-warning@-1 {{aggregate initialization of type 'G<char>' from a parenthesized list of values is a C++20 extension}}
 
   A a7 = Construct<A>('i', 2.2);
   // beforecxx20-note@-1 {{in instantiation of function template specialization 'Construct<A, char, double>' requested here}}
 
   int arr4[](1, 2);