[PATCH] D133570: [Clang] changing behavior of constant array emission [AsmPrinter] changing Size from unsigned to uint64_t

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat created this revision.
Herald added a subscriber: hiraditya.
Herald added a project: All.
OfekShochat requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133570

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp


Index: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -3164,7 +3164,7 @@
  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) {
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -1222,29 +1222,18 @@
   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 Elts;
-if (fillC && fillC->isNullValue())
-  Elts.reserve(NumInitableElts + 1);
-else
-  Elts.reserve(NumElements);
+SmallVector Inits;
+Inits.reserve(NumInitableElts + 1);
 
 llvm::Type *CommonElementType = nullptr;
 for (unsigned i = 0; i < NumInitableElts; ++i) {
@@ -1256,13 +1245,33 @@
 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(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 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) {


Index: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -3164,7 +3164,7 @@
  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) {
Index: clang/lib/CodeGen/CGExprConstant.c

[PATCH] D133570: #57353 on github

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133570

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133570: #57353 on github

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133570

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133570: #57353 on github

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat added a comment.

hello! thank you so much, Ill do that now, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133570

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133570: Clang codegen, fixes issue with emitting partially initialized constant arrays larger than 2^32

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133570

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133570: Clang codegen, fixes issue with emitting partially initialized constant arrays larger than 2^32

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133570

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133570: Clang codegen, fixes issue with emitting partially initialized constant arrays larger than 2^32

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat added a subscriber: eli.friedman.
OfekShochat added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133570

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133570: Clang codegen, fixes issue with emitting partially initialized constant arrays larger than 2^32

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133570

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133570: Clang codegen, fixes issue with emitting partially initialized constant arrays larger than 2^32

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133570

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133648: [Clang] changing behavior of constant array emission

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat created this revision.
OfekShochat added a reviewer: klimek.
Herald added a subscriber: hiraditya.
Herald added a project: All.
OfekShochat requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133648

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/test/CodeGen/big-array-init.c
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp


Index: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -3164,7 +3164,7 @@
  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) {
Index: clang/test/CodeGen/big-array-init.c
===
--- /dev/null
+++ clang/test/CodeGen/big-array-init.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -O0 -triple x86_64-unknown-linux-gnu -S -emit-llvm -o - 
| FileCheck %s
+/* checks for #57353, arrays that are larger than 2^32 were emitting wrongly. 
*/
+
+// CHECK: @bad_char = global <{ i8, [4294967295 x i8] }> <{ i8 1, [4294967295 
x i8] zeroinitializer }>, align 16
+char bad_char[4294967296] = {1};
+// CHECK: @bad_int = global <{ i32, [1073741823 x i32] }> <{ i32 1, 
[1073741823 x i32] zeroinitializer }>, align 16
+int bad_int[1073741824] = {1};
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -836,8 +836,8 @@
   QualType ElementType;
 
   InitializedEntity ElementEntity = Entity;
-  unsigned NumInits = ILE->getNumInits();
-  unsigned NumElements = NumInits;
+  uint64_t NumInits = ILE->getNumInits();
+  uint64_t NumElements = NumInits;
   if (const ArrayType *AType = SemaRef.Context.getAsArrayType(ILE->getType())) 
{
 ElementType = AType->getElementType();
 if (const auto *CAType = dyn_cast(AType))
@@ -857,7 +857,7 @@
 ElementType = ILE->getType();
 
   bool SkipEmptyInitChecks = false;
-  for (unsigned Init = 0; Init != NumElements; ++Init) {
+  for (uint64_t Init = 0; Init != NumElements; ++Init) {
 if (hadError)
   return;
 
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -391,7 +391,7 @@
 
 static llvm::Constant *
 EmitArrayConstant(CodeGenModule &CGM, llvm::ArrayType *DesiredType,
-  llvm::Type *CommonElementType, unsigned ArrayBound,
+  llvm::Type *CommonElementType, uint64_t ArrayBound,
   SmallVectorImpl &Elements,
   llvm::Constant *Filler);
 
@@ -945,11 +945,11 @@
 
 static llvm::Constant *
 EmitArrayConstant(CodeGenModule &CGM, llvm::ArrayType *DesiredType,
-  llvm::Type *CommonElementType, unsigned ArrayBound,
+  llvm::Type *CommonElementType, uint64_t ArrayBound,
   SmallVectorImpl &Elements,
   llvm::Constant *Filler) {
   // Figure out how long the initial prefix of non-zero elements is.
-  unsigned NonzeroLength = ArrayBound;
+  uint64_t NonzeroLength = ArrayBound;
   if (Elements.size() < NonzeroLength && Filler->isNullValue())
 NonzeroLength = Elements.size();
   if (NonzeroLength == Elements.size()) {
@@ -1223,11 +1223,11 @@
 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 NumElements = CAT->getSize().getZExtValue();
 
 // 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((uint64_t)NumInitElements, 
NumElements);
 
 QualType EltType = CAT->getElementType();
 


Index: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -3164,7 +3164,7 @@
  const

[PATCH] D133648: Clang, increase upper bound of partially initialized array sizes

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat updated this revision to Diff 459642.
OfekShochat added a comment.

Clang, increase upper bound of partially initialized array sizes

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133648

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/test/CodeGen/big-array-init.c
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp


Index: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -3164,7 +3164,7 @@
  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) {
@@ -3391,7 +3391,7 @@
uint64_t Offset,
AsmPrinter::AliasMapTy *AliasList) {
   emitGlobalAliasInline(AP, Offset, AliasList);
-  uint64_t Size = DL.getTypeAllocSize(CV->getType());
+  unsigned Size = DL.getTypeAllocSize(CV->getType());
 
   // Globals with sub-elements such as combinations of arrays and structs
   // are handled recursively by emitGlobalConstantImpl. Keep track of the
Index: clang/test/CodeGen/big-array-init.c
===
--- /dev/null
+++ clang/test/CodeGen/big-array-init.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -O0 -triple x86_64-unknown-linux-gnu -S -emit-llvm -o - 
| FileCheck %s
+/* checks for #57353, arrays that are larger than 2^32 were emitting wrongly. 
*/
+
+// CHECK: @bad_char = global <{ i8, [4294967295 x i8] }> <{ i8 1, [4294967295 
x i8] zeroinitializer }>, align 16
+char bad_char[4294967296] = {1};
+// CHECK: @bad_int = global <{ i32, [1073741823 x i32] }> <{ i32 1, 
[1073741823 x i32] zeroinitializer }>, align 16
+int bad_int[1073741824] = {1};
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -836,8 +836,8 @@
   QualType ElementType;
 
   InitializedEntity ElementEntity = Entity;
-  unsigned NumInits = ILE->getNumInits();
-  unsigned NumElements = NumInits;
+  uint64_t NumInits = ILE->getNumInits();
+  uint64_t NumElements = NumInits;
   if (const ArrayType *AType = SemaRef.Context.getAsArrayType(ILE->getType())) 
{
 ElementType = AType->getElementType();
 if (const auto *CAType = dyn_cast(AType))
@@ -857,7 +857,7 @@
 ElementType = ILE->getType();
 
   bool SkipEmptyInitChecks = false;
-  for (unsigned Init = 0; Init != NumElements; ++Init) {
+  for (uint64_t Init = 0; Init != NumElements; ++Init) {
 if (hadError)
   return;
 
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -391,7 +391,7 @@
 
 static llvm::Constant *
 EmitArrayConstant(CodeGenModule &CGM, llvm::ArrayType *DesiredType,
-  llvm::Type *CommonElementType, unsigned ArrayBound,
+  llvm::Type *CommonElementType, uint64_t ArrayBound,
   SmallVectorImpl &Elements,
   llvm::Constant *Filler);
 
@@ -945,11 +945,11 @@
 
 static llvm::Constant *
 EmitArrayConstant(CodeGenModule &CGM, llvm::ArrayType *DesiredType,
-  llvm::Type *CommonElementType, unsigned ArrayBound,
+  llvm::Type *CommonElementType, uint64_t ArrayBound,
   SmallVectorImpl &Elements,
   llvm::Constant *Filler) {
   // Figure out how long the initial prefix of non-zero elements is.
-  unsigned NonzeroLength = ArrayBound;
+  uint64_t NonzeroLength = ArrayBound;
   if (Elements.size() < NonzeroLength && Filler->isNullValue())
 NonzeroLength = Elements.size();
   if (NonzeroLength == Elements.size()) {
@@ -1223,11 +1223,11 @@
 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 NumElements = CAT->getSize().getZExtValue();
 
 // Initialising an array requires us to automatically
 // initialise any elements that have not been initialised explicitly
-unsigned NumInitableElts = std::min(NumInitElements, NumElements);
+uint64_t NumI

[PATCH] D133648: Clang, increase upper bound of partially initialized array sizes

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat marked an inline comment as done.
OfekShochat added a comment.

I created a diff for the AsmPrinter part, its at D133845 





Comment at: clang/lib/Sema/SemaInit.cpp:866
 ElementEntity.getKind() == InitializedEntity::EK_VectorElement)
   ElementEntity.setElementIndex(Init);
 

efriedma wrote:
> shafik wrote:
> > `setElementIndex(...)` takes `unsigned` as well and therefore 
> > `InitializedEntity` also uses `unsigned`.
> > 
> > I briefly looked at this with another bug 
> > https://github.com/llvm/llvm-project/issues/57317
> > 
> > and I believe the 32 bit assumption is made in a lot of this code in this 
> > area. 
> You don't need to solve everything at once...
oh, right, should I change that after I split the diff in AsmPrinter to another 
patch?



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3167
   // 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());

efriedma wrote:
> Can you split this into a separate patch (with an appropriate testcase)?
yep


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133648

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133648: Clang, increase upper bound of partially initialized array sizes

[Ofek Shochat via Phabricator via cfe-commits]

OfekShochat marked an inline comment as done.
OfekShochat added a comment.

@efriedma would you be able to commit this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133648

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits