[PATCH] D148274: [clang] Fix overly aggressive lifetime checks for parenthesized aggregate initialization

[Alan Zhao via Phabricator via cfe-commits]

ayzhao added inline comments.


================
Comment at: clang/lib/Sema/SemaInit.cpp:5364-5368
     if (const ConstantArrayType *CAT =
             S.getASTContext().getAsConstantArrayType(Entity.getType()))
       ArrayLength = CAT->getSize().getZExtValue();
     else
       ArrayLength = Args.size();
----------------
rsmith wrote:
> What happens if the array is of `VariableArrayType` or `DependentSizedArrayType`? I guess we shouldn't get here in the latter case, but the former case seems possible, and presumably shouldn't result in constructing a value of `ConstantArrayType`. [Test](https://godbolt.org/z/377TWzn7r):
> 
> ```
> constexpr int f(int n, int i) {
>     int arr[n](1, 2, 3);
>     return arr[i];
> }
> 
> constexpr int a = f(1, 2);
> constexpr int b = f(4, 3);
> ```
> 
> GCC appears to leave the type alone in this case, and treats the evaluation as UB if `n` is less than the number of initializers given. That matches what GCC does for a `{...}` initializer of a VLA. We should probably match what Clang does for `{...}` initialization of a VLA and reject.
Clang simply does not allow braced-initialization for `VariableArrayType`, even if `n` is greater than or equal to the number of initializers given:  https://godbolt.org/z/MaPjvn694. I updated the patch to make parenthesized aggregate initialization do the same thing.


================
Comment at: clang/lib/Sema/SemaInit.cpp:5391-5393
+    ResultType = S.Context.getConstantArrayType(
+        AT->getElementType(), llvm::APInt(/*numBits=*/32, ArrayLength),
+        /*SizeExpr=*/nullptr, ArrayType::Normal, 0);
----------------
rsmith wrote:
> It would be nice to use the original type here in the case where we didn't add an array bound, so we preserve type sugar (typedefs etc). Also, do we ever need to preserve type qualifiers from the original entity's type?
> It would be nice to use the original type here in the case where we didn't add an array bound, so we preserve type sugar (typedefs etc).

Done 

> Also, do we ever need to preserve type qualifiers from the original entity's type?

I don't think so. In the case of `IncompleteArrayType`, which is the only situation where we would need to deduce the `ConstantArrayType` based on the number of arguments, the corresponding `QualType` object is always created with `Quals = 0` [0].

[0]: https://github.com/llvm/llvm-project/blob/c95533a7be2858893ec32b8abaa37a2d912ebe63/clang/lib/AST/ASTContext.cpp#L3888


================
Comment at: clang/lib/Sema/SemaInit.cpp:5401
+        InitializedEntity SubEntity =
+            InitializedEntity::InitializeBase(S.getASTContext(), &Base, false);
+        if (EntityIndexToProcess < Args.size()) {
----------------
rsmith wrote:
> Does this have the same wrong-lifetime-kind problem as members did?
I don't think so:

1. This will never initialize a reference because it's initializing a base class.
1. In the case where the constructor of a base class accepts a reference parameter, the lifetime is not extended: https://godbolt.org/z/MaqhTr1rP


================
Comment at: clang/lib/Sema/SemaInit.cpp:5476
+          InitializedEntity SubEntity =
+              InitializedEntity::InitializeMemberFromDefaultMemberInitializer(
+                  FD);
----------------
rsmith wrote:
> Does this entity kind do the right thing for lifetime warnings? (I'm not sure why this is a distinct kind of `InitializedEntity`; the thing that changes here is not the entity, it's how it's initialized.)
This now uses `InitializeMemberFromParenAggInit(..)` (before, it would emit [the same error it emits for braced init lists](https://godbolt.org/z/d3qfzh73z)). I also added a test for this.


================
Comment at: clang/lib/Sema/SemaInit.cpp:5486-5487
+          //   The remaining elements...otherwise are value initialzed
+          InitializedEntity SubEntity =
+              InitializedEntity::InitializeMember(FD, &Entity);
+          InitializationKind SubKind = InitializationKind::CreateValue(
----------------
rsmith wrote:
> Is there any possibility of lifetime warnings here? I don't *think* value initialization can ever create problems, but it would seem more obviously right to use the parenthesized aggregate initialization entity kind here.
GCC rejects value initialization of references: https://godbolt.org/z/nhhehrf7r

This patch currently rejects value initialization of references, but I updated the diagnostics to match those Clang emits with braced initialization: https://godbolt.org/z/rbh17ecqe


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148274/new/

https://reviews.llvm.org/D148274