[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–1604

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–1604

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–1604

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–1604

Patch has been updated per this comment. PTAL.

[ayzhao]

fix stray whitespace change

[rsmith]

LGTM, thanks!

clang/lib/Sema/SemaExprCXX.cpp

return CXXUnresolvedConstructExpr::Create(Context, Ty.getNonReferenceType(),
                                               Locs.getEnd());
   }
 
   // C++ [expr.type.conv]p1:
   // If the expression list is a parenthesized single expression, the type
   // conversion expression is equivalent (in definedness, and if defined in
   // meaning) to the corresponding cast expression.
   if (Exprs.size() == 1 && !ListInitialization &&
-      !isa<InitListExpr>(Exprs[0])) {
+      !isa<InitListExpr>(Exprs[0]) && !isa<CXXParenListInitExpr>(Exprs[0])) {
     Expr *Arg = Exprs[0];
     return BuildCXXFunctionalCastExpr(TInfo, Ty, LParenOrBraceLoc, Arg,
                                       RParenOrBraceLoc);
   }
 
   //   For an expression of the form T(), T shall not be an array type.
   QualType ElemTy = Ty;
   if (Ty->isArrayType()) {
Sema::BuildCXXTypeConstructExpr(TypeSourceInfo *TInfo,
   // C++17 [expr.type.conv]p2:
   //   If the type is cv void and the initializer is (), the expression is a
   //   prvalue of the specified type that performs no initialization.
   if (!Ty->isVoidType() &&
       RequireCompleteType(TyBeginLoc, ElemTy,
                           diag::err_invalid_incomplete_type_use, FullRange))
     return ExprError();
 
+  MultiExprArg ExprsToPass;
+  if (Exprs.size() == 1 && isa<CXXParenListInitExpr>(Exprs[0])) {
+    // C++20 [expr.static.cast]p4:
+    //   An expression E can be explicitly converted to a type T...if T is an
+    //   aggregate type ([dcl.init.aggr]) having a first element x and there is
+    //   an implicit conversion sequence from E to the type of x
+    //
+    // Unlike for InitListExprs, InitializationSequence(..) doesn't take the
+    // overall CXXParenListInitExpr as the first element, so we instead pass
+    // it the elements of the CXXParenListInitExpr and have it deduce the
+    // initialization type and generate a new CXXParenListInitExpr.
+    auto *CPLIE = cast<CXXParenListInitExpr>(Exprs[0]);
+    ExprsToPass =
+        MultiExprArg(const_cast<Expr **>(CPLIE->getInitExprs().begin()),
+                     CPLIE->getInitExprs().size());
+  } else {
     //   Otherwise, the expression is a prvalue of the specified type whose
     //   result object is direct-initialized (11.6) with the initializer.
-  InitializationSequence InitSeq(*this, Entity, Kind, Exprs);
-  ExprResult Result = InitSeq.Perform(*this, Entity, Kind, Exprs);
+    ExprsToPass = Exprs;
+  }
+  InitializationSequence InitSeq(*this, Entity, Kind, ExprsToPass);
+  ExprResult Result = InitSeq.Perform(*this, Entity, Kind, ExprsToPass);
 
   if (Result.isInvalid())
     return Result;
 
   Expr *Inner = Result.get();
   if (CXXBindTemporaryExpr *BTE = dyn_cast_or_null<CXXBindTemporaryExpr>(Inner))
     Inner = BTE->getSubExpr();
   if (!isa<CXXTemporaryObjectExpr>(Inner) &&
       !isa<CXXScalarValueInitExpr>(Inner)) {

clang/lib/Sema/SemaInit.cpp

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/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_S:%.*]] = type { [[STRUCT_VEC]] }
+  struct S {
+    Vec v;
+  };
+}
+
 // 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 make<1>()
+  // CHECK: define {{.*}} void @_ZN7gh611454makeILi0EEEvv
+  // CHECK-NEXT: entry:
+  // CHECK-NEXT: [[V:%.*v.*]] = alloca [[STRUCT_VEC]], align 1
+  // CHECK-NEXT: [[AGG_TMP_ENSURED:%.*agg.tmp.ensured.*]] = alloca [[STRUCT_S]], align 1
+  // CHECK-NEXT: call void @_ZN7gh611453VecC1Ev(ptr noundef nonnull align 1 dereferenceable(1) [[V]])
+  // a.k.a. Vec::Vec()
+  // CHECK-NEXT: [[V1:%.*v1.*]] = getelementptr inbounds [[STRUCT_S]], 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. S::~S();
+  // CHECK-NEXT: call void @_ZN7gh611451SD1Ev(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 make() {
+    Vec v;
+    S((Vec&&) v);
+  }
+
+  void foo() {
+    make<0>();
+  }
+}