Clang codegen, fixes issue with emitting partially initialized constant arrays larger than 2^32

[OfekShochat]

Summary

fixes issue with emitting partially initialized constant arrays larger than 2^32.
issue #57353 on github.

[OfekShochat]

its my first time contributing. this is probably very bad and goes against a lot of guidelines, but I wouldnt resist to fix this

[OfekShochat]

trying to fix the problem. probably going to split this into two patches

[aaron.ballman]

In D133570#3779856, @OfekShochat wrote:

its my first time contributing. this is probably very bad and goes against a lot of guidelines, but I wouldnt resist to fix this

Welcome, and thank you for the patch! I'm going to add a few more reviewers who typically look at codegen changes, but I do have some high-level suggestions. Can you edit the title of the review (there's an "edit revision" link in the upper-right of the review) so that there's a short description of what is being fixed in the title? The title is what most reviewers see in our inbox/review list, so knowing what the patch is about helps reviewers immensely (also, we use the title of the review as the title of the commit message). Also, we ask that all changes to the code base come with test coverage showing what has been fixed, so please add some test coverage (most likely to clang/test/CodeGen/). Finally, we like to add information to our release notes about bug fixes so users know about the improvements, so please add a short release note (to clang/docs/ReleaseNotes.rst).

[OfekShochat]

hello! thank you so much, Ill do that now, thanks! just a question, what are your guidelines when it comes to commit naming/number etc?

[OfekShochat]

hmm, one problem, seems like nor clang and nor gcc can compile the example in the github issue fully, only to assembly. is there something like c -> llir, llir -> asm checks? in the issue, it did succeed to compile to either llir and asm, just not correctly

[aaron.ballman]

In D133570#3780071, @OfekShochat wrote:

hello! thank you so much, Ill do that now, thanks! just a question, what are your guidelines when it comes to commit naming/number etc?

Thanks, the new title is great! I'm not certain I understand your question, but our full developer policy can be found at: https://llvm.org/docs/DeveloperPolicy.html which may help.

[OfekShochat]

hello again, I added two tests, one for llvm and one for clang. how do I run those specifically so I see I didnt do something wrong?

[aaron.ballman]

In D133570#3780171, @OfekShochat wrote:

hello again, I added two tests, one for llvm and one for clang. how do I run those specifically so I see I didnt do something wrong?

The short answer is: you can run the check-all to run the LLVM and Clang tests together. For more details, our testing documentation can be found at: https://llvm.org/docs/TestingGuide.html though we also have some details at: https://clang.llvm.org/hacking.html#testing as well.

[efriedma]

clang/lib/CodeGen/CGExprConstant.cpp

1225

Note that getNumInits() itself is returning "unsigned". (Looks easy to fix, though.)

1265

Need to be careful here: in general, you can't just fill everything with zero. Need to check how getArrayFiller() is lowered. (For example, consider class X; int X::*a[10] = { 0 };.)

[OfekShochat]

yep @eli.friedman. probably gonna scratch this idea, makes more sense to actually tackle the problem itself, which is that getArrayFiller doesnt return anything. trying to actually get to the problem, but its quite hard, Visit's are thrown everywhere, and I cant really look at the problem. tryEmitPrivateForVarInit is called, and then it calls a Visit, which somehow transfers it to VisitInitListExpr. that thing worked just because it worked around that problem, even if not intentionally. though, I think Im closing in onto the problem

[efriedma]

FYI, the "Visit" method comes from the base class StmtVisitor; it just forwards to the relevant method. (For example, if you call "Visit" with an InitListExpr, it calls VisitInitListExpr.)

[OfekShochat]

Yeah. But it's really not clear how it does that, as it gets all the functions from the generated file

[OfekShochat]

hello, solved the issue. it was, quite expectedly, a u64/unsigned problem in parsing, not codegen. no need to refactor this function. I will close this, thanks for the feedback and sorry for the hassle

clang/lib/CodeGen/CGExprConstant.cpp

public:
   llvm::Constant *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E,
                                                 QualType T) {
     return Visit(E->getSubExpr(), T);
   }
 
   llvm::Constant *EmitArrayInitialization(InitListExpr *ILE, QualType T) {
     auto *CAT = CGM.getContext().getAsConstantArrayType(ILE->getType());
     assert(CAT && "can't emit array init for non-constant-bound array");
-    unsigned NumInitElements = ILE->getNumInits();
-    unsigned NumElements = CAT->getSize().getZExtValue();
+    uint64_t NumInitElements = ILE->getNumInits();
+    uint64_t NumElements = CAT->getSize().getZExtValue();
+    printf("hi\n");
 
     // Initialising an array requires us to automatically
     // initialise any elements that have not been initialised explicitly
-    unsigned NumInitableElts = std::min(NumInitElements, NumElements);
+    uint64_t NumInitableElts = std::min(NumInitElements, NumElements);
 
     QualType EltType = CAT->getElementType();
 
-    // Initialize remaining array elements.
-    llvm::Constant *fillC = nullptr;
-    if (Expr *filler = ILE->getArrayFiller()) {
-      fillC = Emitter.tryEmitAbstractForMemory(filler, EltType);
-      if (!fillC)
-        return nullptr;
-    }
-
-    // Copy initializer elements.
-    SmallVector<llvm::Constant*, 16> Elts;
-    if (fillC && fillC->isNullValue())
-      Elts.reserve(NumInitableElts + 1);
-    else
-      Elts.reserve(NumElements);
+    SmallVector<llvm::Constant *, 16> Inits;
+    Inits.reserve(NumInitableElts + 1);
 
     llvm::Type *CommonElementType = nullptr;
     for (unsigned i = 0; i < NumInitableElts; ++i) {
       Expr *Init = ILE->getInit(i);
       llvm::Constant *C = Emitter.tryEmitPrivateForMemory(Init, EltType);
       if (!C)
         return nullptr;
       if (i == 0)
         CommonElementType = C->getType();
       else if (C->getType() != CommonElementType)
         CommonElementType = nullptr;
-      Elts.push_back(C);
+      Inits.push_back(C);
     }
 
-    llvm::ArrayType *Desired =
+    uint64_t TrailingZeroes = NumElements - NumInitElements;
+
+    // If all elements have the same type, just emit an array constant.
+    if (CommonElementType && TrailingZeroes == 0)
+      return llvm::ConstantArray::get(
+          llvm::ArrayType::get(CommonElementType, NumElements), Inits);
+
+    llvm::ArrayType *DesiredType =
         cast<llvm::ArrayType>(CGM.getTypes().ConvertType(ILE->getType()));
-    return EmitArrayConstant(CGM, Desired, CommonElementType, NumElements, Elts,
-                             fillC);
+
+    auto *FillerType =
+        CommonElementType ? CommonElementType : DesiredType->getElementType();
+    FillerType = llvm::ArrayType::get(FillerType, TrailingZeroes);
+    
+    Inits.push_back(llvm::ConstantAggregateZero::get(FillerType));
+
+    llvm::SmallVector<llvm::Type *, 16> Types;
+    for (llvm::Constant *Elt : Inits)
+      Types.push_back(Elt->getType());
+
+    llvm::StructType *SType =
+        llvm::StructType::get(CGM.getLLVMContext(), Types, true);
+
+    return llvm::ConstantStruct::get(SType, Inits);
   }
 
   llvm::Constant *EmitRecordInitialization(InitListExpr *ILE, QualType T) {
     return ConstStructBuilder::BuildStruct(Emitter, ILE, T);
   }
 
   llvm::Constant *VisitImplicitValueInitExpr(ImplicitValueInitExpr* E,
                                              QualType T) {

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (unsigned Padding = Size - EmittedSize)
     AP.OutStreamer->emitZeros(Padding);
 }
 
 static void emitGlobalConstantStruct(const DataLayout &DL,
                                      const ConstantStruct *CS, AsmPrinter &AP,
                                      const Constant *BaseCV, uint64_t Offset,
                                      AsmPrinter::AliasMapTy *AliasList) {
   // Print the fields in successive locations. Pad to align if needed!
-  unsigned Size = DL.getTypeAllocSize(CS->getType());
+  uint64_t Size = DL.getTypeAllocSize(CS->getType());
   const StructLayout *Layout = DL.getStructLayout(CS->getType());
   uint64_t SizeSoFar = 0;
   for (unsigned I = 0, E = CS->getNumOperands(); I != E; ++I) {
     const Constant *Field = CS->getOperand(I);
 
     // Print the actual field value.
     emitGlobalConstantImpl(DL, Field, AP, BaseCV, Offset + SizeSoFar,
                            AliasList);