[PR47636] Fix tryEmitPrivate to handle non-constantarraytypes

[erichkeane]

Summary

As mentioned in the bug report, tryEmitPrivate chokes on the
MaterializeTemporaryExpr in the reproducers, since it assumes that if
there are elements, than it must be a ConstantArrayType. However, the
MaterializeTemporaryExpr (which matches exactly the AST when it is NOT a
global/static) has an incomplete array type.

This changes the section where the number-of-elements is non-zero to
properly handle non-CAT types by just extracting it as an array type
(since all we needed was the element type out of it).

[erichkeane]

Note: @rsmith and @rjmccall were the last ones in this section based on Blame (by a wide margin!), so added both as reviewers.

[rjmccall]

clang/lib/CodeGen/CGExprConstant.cpp

2145–2146

AT is now just ArrayTy, and I think you can just check !Filler && !NumInitElts.

[erichkeane]

@rjmccall Done!

[rjmccall]

Thanks, LGTM.

[erichkeane]

Gah, forgot 'Differential Revision' again :/

[rsmith]

clang/test/CodeGenCXX/pr47636.cpp

2

The AST we generate for this is:

`-VarDecl 0x106e7630 <<stdin>:1:1, col:29> col:7 intu_rvref 'int (&&)[4]' listinit
  `-ExprWithCleanups 0x106e7908 <col:21, col:29> 'int []':'int []' xvalue
    `-MaterializeTemporaryExpr 0x106e78a8 <col:21, col:29> 'int []':'int []' xvalue extended by Var 0x106e7630 'intu_rvref' 'int (&&)[4]'
      `-InitListExpr 0x106e77c0 <col:21, col:29> 'int [4]'
        |-IntegerLiteral 0x106e76e0 <col:22> 'int' 1
        |-IntegerLiteral 0x106e7700 <col:24> 'int' 2
        |-IntegerLiteral 0x106e7720 <col:26> 'int' 3
        `-IntegerLiteral 0x106e7740 <col:28> 'int' 4

... which looks surprising to me -- MaterializeTemporaryExpr corresponds to allocating storage for an object of type T, so T being incomplete is at least surprising, and it would seem more consistent to complete the type of the MTE in the same way we would complete the type of a corresponding VarDecl.

We also crash while processing

constexpr int f() { int (&&intu_rvref)[] {1,2,3,4}; return intu_rvref[0]; }

... due to the same unexpected AST representation.

I don't know if we would need this CodeGen change in other cases; it seems like if we ever supported constant initialization of a class with a flexible array member, we'd want to do something like this. For now at least, we refuse to constant-evaluate things such as struct A { int n; int arr[]; } a = {1, 2, 3}; to an APValue and use the lowering-from-Expr path in CGExprConstant for such cases instead. So I suspect if we change the AST representation to use a complete type here, the code change here will be unreachable at least in the short term.

On the other hand, if we did choose to support constant initialization of flexible array members, it might be natural to treat objects of incomplete array type analogously to flexible array members, giving them an incomplete type whose size is determined from its initializer, and with that viewpoint the AST representation we're currently using wouldn't really be wrong. But I'm inclined to think that MaterializeTemporaryExpr should mirror the behavior of a VarDecl: the array bound should be filled in in the AST representation based on the initializer.

[rjmccall]

Yeah, that would also be a completely reasonable approach. We already preserve the source spelling of the incomplete array type in the appropriate expression.

[rsmith]

clang/lib/CodeGen/CGExprConstant.cpp

2145–2146

Filler is null if and only if NumElements == NumInitElts, and NumInitElts <= NumElements, so this condition reduces to if (!NumElements). EmitArrayConstant responds to that case by creating a ConstantAggregateZero of its destination type. So it seems to me that we probably don't need this special case at all.

[erichkeane]

Ok then, I'll take a look to see what it would take to make the MaterializeTemporaryExpr be created with the complete type here instead. Thanks!

clang/lib/CodeGen/CGExprConstant.cpp

2145–2146

I'll submit a patch to remove this branch then, thanks!

clang/lib/CodeGen/CGExprConstant.cpp

case APValue::AddrLabelDiff: {
     // address-of-label difference; make sure to truncate after
     // the subtraction.
     return llvm::ConstantExpr::getTruncOrBitCast(AddrLabelDiff, ResultType);
   }
   case APValue::Struct:
   case APValue::Union:
     return ConstStructBuilder::BuildStruct(*this, Value, DestType);
   case APValue::Array: {
-    const ConstantArrayType *CAT =
-        CGM.getContext().getAsConstantArrayType(DestType);
+    const ArrayType *ArrayTy = CGM.getContext().getAsArrayType(DestType);
     unsigned NumElements = Value.getArraySize();
     unsigned NumInitElts = Value.getArrayInitializedElts();
 
     // Emit array filler, if there is one.
     llvm::Constant *Filler = nullptr;
     if (Value.hasArrayFiller()) {
       Filler = tryEmitAbstractForMemory(Value.getArrayFiller(),
-                                        CAT->getElementType());
+                                        ArrayTy->getElementType());
       if (!Filler)
         return nullptr;
     }
 
     // Emit initializer elements.
     SmallVector<llvm::Constant*, 16> Elts;
     if (Filler && Filler->isNullValue())
       Elts.reserve(NumInitElts + 1);
     else
       Elts.reserve(NumElements);
 
     llvm::Type *CommonElementType = nullptr;
     for (unsigned I = 0; I < NumInitElts; ++I) {
       llvm::Constant *C = tryEmitPrivateForMemory(
-          Value.getArrayInitializedElt(I), CAT->getElementType());
+          Value.getArrayInitializedElt(I), ArrayTy->getElementType());
       if (!C) return nullptr;
 
       if (I == 0)
         CommonElementType = C->getType();
       else if (C->getType() != CommonElementType)
         CommonElementType = nullptr;
       Elts.push_back(C);
     }
 
     // This means that the array type is probably "IncompleteType" or some
     // type that is not ConstantArray.
-    if (CAT == nullptr && CommonElementType == nullptr && !NumInitElts) {
-      const ArrayType *AT = CGM.getContext().getAsArrayType(DestType);
-      CommonElementType = CGM.getTypes().ConvertType(AT->getElementType());
-      llvm::ArrayType *AType = llvm::ArrayType::get(CommonElementType,
-                                                    NumElements);
+    if (!Filler && !NumInitElts) {
+      CommonElementType = CGM.getTypes().ConvertType(ArrayTy->getElementType());
+      llvm::ArrayType *AType =
+          llvm::ArrayType::get(CommonElementType, NumElements);
       return llvm::ConstantAggregateZero::get(AType);
     }
 
     llvm::ArrayType *Desired =
         cast<llvm::ArrayType>(CGM.getTypes().ConvertType(DestType));
     return EmitArrayConstant(CGM, Desired, CommonElementType, NumElements, Elts,
                              Filler);
   }

clang/test/CodeGenCXX/pr47636.cpp

+// RUN: %clang_cc1 -o - -emit-llvm -triple x86_64-linux-pc %s | FileCheck %s
+int(&&intu_rvref)[] {1,2,3,4};
+// CHECK: @_ZGR10intu_rvref_ = internal global [4 x i32] [i32 1, i32 2, i32 3, i32 4]
+// CHECK: @intu_rvref = constant [4 x i32]* @_ZGR10intu_rvref_
+
+void foo() {
+  static const int(&&intu_rvref)[] {1,2,3,4};
+  // CHECK: @_ZZ3foovE10intu_rvref = internal constant [4 x i32]* @_ZGRZ3foovE10intu_rvref_
+  // CHECK: @_ZGRZ3foovE10intu_rvref_ = internal constant [4 x i32] [i32 1, i32 2, i32 3, i32 4]
+}