This is an archive of the discontinued LLVM Phabricator instance.

[PR47636] Fix tryEmitPrivate to handle non-constantarraytypes
ClosedPublic

Authored by erichkeane on Sep 24 2020, 8:57 AM.

Details

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).

Diff Detail

Event Timeline

erichkeane requested review of this revision.Sep 24 2020, 8:57 AM
erichkeane created this revision.

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

rjmccall added inline comments.Sep 24 2020, 11:04 AM
clang/lib/CodeGen/CGExprConstant.cpp
2145–2146

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

erichkeane marked an inline comment as done.

@rjmccall Done!

rjmccall accepted this revision.Sep 24 2020, 12:07 PM

Thanks, LGTM.

This revision is now accepted and ready to land.Sep 24 2020, 12:07 PM
rsmith added inline comments.Sep 24 2020, 12:16 PM
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.

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 added inline comments.Sep 24 2020, 12:23 PM
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.

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!