[CodeGen] Fix clang crash on aggregate initialization of array of labels

[johannes]

Summary

Fix PR43700

The ConstantEmitter in AggExprEmitter::EmitArrayInit was initialized
with the CodeGenFunction set to null, which caused the crash.
Also simplify another call, and make the CGF member a const pointer
since it is public but only assigned in the constructor.

[nickie]

LGTM, for all that it's worth.

clang/lib/CodeGen/CGExpr.cpp

1021 ↗

(On Diff #229497)

I don't think that const is necessary here. We're talking about the function's parameter. Why isn't E a *const too?

[johannes]

Remove unnecessary const on parameter

[johannes]

clang/lib/CodeGen/CGExpr.cpp

1021 ↗

(On Diff #229497)

Right, this was left over from when I thought it was necessary. Removed the const because it is surprising.

[johannes]

update commit message

[johannes]

Committed as 1ac700cdef787383ad49a0e37d9894491ef19480 - this should be a safe fix

[xbolva00]

I think this was commited too soon. You should receive LGTM from clang maintainer(s).

In D70302#1747372, @nickie wrote:

LGTM, for all that it's worth.

[rnk]

Fix looks good post commit, but we should enhance the test.

clang/test/CodeGen/label-array-aggregate-init.c

1

It's best practice to filecheck for something, even if this used to crash.

[johannes]

In D70302#1763025, @rnk wrote:

Fix looks good post commit, but we should enhance the test.

I see, apologies for the premature commit.

clang/test/CodeGen/label-array-aggregate-init.c

1

I guess the test should make sure that the array is constructed successfully after codegen.
I'm unsure about the best way to test that.

[rnk]

In D70302#1764261, @johannes wrote:

I see, apologies for the premature commit.

No problem, LLVM's code review practices are somewhat opaque.

clang/test/CodeGen/label-array-aggregate-init.c

1

I went ahead and did it in rG536cedaecbe586ec9cf86d5102872adc27e6ea23.

clang/lib/CodeGen/CGExprAgg.cpp

CharUnits elementAlign =
     DestPtr.getAlignment().alignmentOfArrayElement(elementSize);
 
   // Consider initializing the array by copying from a global. For this to be
   // more efficient than per-element initialization, the size of the elements
   // with explicit initializers should be large enough.
   if (NumInitElements * elementSize.getQuantity() > 16 &&
       elementType.isTriviallyCopyableType(CGF.getContext())) {
     CodeGen::CodeGenModule &CGM = CGF.CGM;
-    ConstantEmitter Emitter(CGM);
+    ConstantEmitter Emitter(CGF);
     LangAS AS = ArrayQTy.getAddressSpace();
     if (llvm::Constant *C = Emitter.tryEmitForInitializer(E, AS, ArrayQTy)) {
       auto GV = new llvm::GlobalVariable(
           CGM.getModule(), C->getType(),
           CGM.isTypeConstant(ArrayQTy, /* ExcludeCtorDtor= */ true),
           llvm::GlobalValue::PrivateLinkage, C, "constinit",
           /* InsertBefore= */ nullptr, llvm::GlobalVariable::NotThreadLocal,
           CGM.getContext().getTargetAddressSpace(AS));

clang/lib/CodeGen/CGExprScalar.cpp

public:
   // C++
   Value *VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *E) {
     return EmitLoadOfLValue(E);
   }
   Value *VisitSourceLocExpr(SourceLocExpr *SLE) {
     auto &Ctx = CGF.getContext();
     APValue Evaluated =
         SLE->EvaluateInContext(Ctx, CGF.CurSourceLocExprScope.getDefaultExpr());
-    return ConstantEmitter(CGF.CGM, &CGF)
-        .emitAbstract(SLE->getLocation(), Evaluated, SLE->getType());
+    return ConstantEmitter(CGF).emitAbstract(SLE->getLocation(), Evaluated,
+                                             SLE->getType());
   }
 
   Value *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *DAE) {
     CodeGenFunction::CXXDefaultArgExprScope Scope(CGF, DAE);
     return Visit(DAE->getExpr());
   }
   Value *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *DIE) {
     CodeGenFunction::CXXDefaultInitExprScope Scope(CGF, DIE);

clang/lib/CodeGen/ConstantEmitter.h

 #include "CodeGenModule.h"
 
 namespace clang {
 namespace CodeGen {
 
 class ConstantEmitter {
 public:
   CodeGenModule &CGM;
-  CodeGenFunction *CGF;
+  CodeGenFunction *const CGF;
 
 private:
   bool Abstract = false;
 
   /// Whether non-abstract components of the emitter have been initialized.
   bool InitializedNonAbstract = false;
 
   /// Whether the emitter has been finalized.

clang/test/CodeGen/label-array-aggregate-init.c

+// RUN: %clang -cc1 -emit-llvm %s -o /dev/null
+
+int main() {
+L:
+  (void)(void *[]){ &&L, 0, 0 };
+}