From 4802edd1ac7a5aea8c8488b5baec221d722cbdde Mon Sep 17 00:00:00 2001 From: Eli Friedman Date: Thu, 14 Apr 2022 15:20:37 -0700 Subject: [PATCH] Fix size of flexible array initializers, and re-enable assertions. In D123649, I got the formula for getFlexibleArrayInitChars slightly wrong: the flexible array elements can be contained in the tail padding of the struct. Fix the formula to account for that. With the fixed formula, we run into another issue: in some cases, we were emitting extra padding for flexible arrray initializers. Fix CGExprConstant so it uses a packed struct when necessary, to avoid this extra padding. Differential Revision: https://reviews.llvm.org/D123826 --- clang/include/clang/AST/Decl.h | 12 ++++++--- clang/lib/AST/Decl.cpp | 26 +++++++++++++++++-- clang/lib/CodeGen/CGDecl.cpp | 7 +---- clang/lib/CodeGen/CGExprConstant.cpp | 21 +++++++++++---- clang/lib/CodeGen/CodeGenModule.cpp | 7 +---- clang/test/CodeGen/flexible-array-init.c | 4 +-- clang/test/CodeGenCXX/flexible-array-init.cpp | 7 ++++- 7 files changed, 58 insertions(+), 26 deletions(-) diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h index 5816a91bd0c2..f93008cdd322 100644 --- a/clang/include/clang/AST/Decl.h +++ b/clang/include/clang/AST/Decl.h @@ -1591,12 +1591,18 @@ public: /// kind? QualType::DestructionKind needsDestruction(const ASTContext &Ctx) const; - /// If this variable declares a struct with a flexible array member, and - /// the flexible array member is initialized with one or more elements, - /// compute the number of bytes necessary to store those elements. + /// Whether this variable has a flexible array member initialized with one + /// or more elements. This can only be called for declarations where + /// hasInit() is true. /// /// (The standard doesn't allow initializing flexible array members; this is /// a gcc/msvc extension.) + bool hasFlexibleArrayInit(const ASTContext &Ctx) const; + + /// If hasFlexibleArrayInit is true, compute the number of additional bytes + /// necessary to store those elements. Otherwise, returns zero. + /// + /// This can only be called for declarations where hasInit() is true. CharUnits getFlexibleArrayInitChars(const ASTContext &Ctx) const; // Implement isa/cast/dyncast/etc. diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index 915d8db10aca..050c3ad2dafc 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -31,6 +31,7 @@ #include "clang/AST/PrettyDeclStackTrace.h" #include "clang/AST/PrettyPrinter.h" #include "clang/AST/Randstruct.h" +#include "clang/AST/RecordLayout.h" #include "clang/AST/Redeclarable.h" #include "clang/AST/Stmt.h" #include "clang/AST/TemplateBase.h" @@ -2722,6 +2723,21 @@ VarDecl::needsDestruction(const ASTContext &Ctx) const { return getType().isDestructedType(); } +bool VarDecl::hasFlexibleArrayInit(const ASTContext &Ctx) const { + assert(hasInit() && "Expect initializer to check for flexible array init"); + auto *Ty = getType()->getAs(); + if (!Ty || !Ty->getDecl()->hasFlexibleArrayMember()) + return false; + auto *List = dyn_cast(getInit()->IgnoreParens()); + if (!List) + return false; + const Expr *FlexibleInit = List->getInit(List->getNumInits() - 1); + auto InitTy = Ctx.getAsConstantArrayType(FlexibleInit->getType()); + if (!InitTy) + return false; + return InitTy->getSize() != 0; +} + CharUnits VarDecl::getFlexibleArrayInitChars(const ASTContext &Ctx) const { assert(hasInit() && "Expect initializer to check for flexible array init"); auto *Ty = getType()->getAs(); @@ -2730,11 +2746,17 @@ CharUnits VarDecl::getFlexibleArrayInitChars(const ASTContext &Ctx) const { auto *List = dyn_cast(getInit()->IgnoreParens()); if (!List) return CharUnits::Zero(); - auto FlexibleInit = List->getInit(List->getNumInits() - 1); + const Expr *FlexibleInit = List->getInit(List->getNumInits() - 1); auto InitTy = Ctx.getAsConstantArrayType(FlexibleInit->getType()); if (!InitTy) return CharUnits::Zero(); - return Ctx.getTypeSizeInChars(InitTy); + CharUnits FlexibleArraySize = Ctx.getTypeSizeInChars(InitTy); + const ASTRecordLayout &RL = Ctx.getASTRecordLayout(Ty->getDecl()); + CharUnits FlexibleArrayOffset = + Ctx.toCharUnitsFromBits(RL.getFieldOffset(RL.getFieldCount() - 1)); + if (FlexibleArrayOffset + FlexibleArraySize < RL.getSize()) + return CharUnits::Zero(); + return FlexibleArrayOffset + FlexibleArraySize - RL.getSize(); } MemberSpecializationInfo *VarDecl::getMemberSpecializationInfo() const { diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp index 322602606ebe..e47450f2ba8f 100644 --- a/clang/lib/CodeGen/CGDecl.cpp +++ b/clang/lib/CodeGen/CGDecl.cpp @@ -342,7 +342,7 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D, if (!Init) { if (!getLangOpts().CPlusPlus) CGM.ErrorUnsupported(D.getInit(), "constant l-value expression"); - else if (!D.getFlexibleArrayInitChars(getContext()).isZero()) + else if (D.hasFlexibleArrayInit(getContext())) CGM.ErrorUnsupported(D.getInit(), "flexible array initializer"); else if (HaveInsertPoint()) { // Since we have a static initializer, this global variable can't @@ -354,17 +354,12 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D, return GV; } -#if 0 - // FIXME: The following check doesn't handle flexible array members - // inside tail padding (which don't actually increase the size of - // the struct). #ifndef NDEBUG CharUnits VarSize = CGM.getContext().getTypeSizeInChars(D.getType()) + D.getFlexibleArrayInitChars(getContext()); CharUnits CstSize = CharUnits::fromQuantity( CGM.getDataLayout().getTypeAllocSize(Init->getType())); assert(VarSize == CstSize && "Emitted constant has unexpected size"); -#endif #endif // The initializer may differ in type from the global. Rewrite diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp index 92122f0df7b9..91fdb88d6bb7 100644 --- a/clang/lib/CodeGen/CGExprConstant.cpp +++ b/clang/lib/CodeGen/CGExprConstant.cpp @@ -439,22 +439,33 @@ llvm::Constant *ConstantAggregateBuilder::buildFrom( // Can't emit as an array, carry on to emit as a struct. } + // The size of the constant we plan to generate. This is usually just + // the size of the initialized type, but in AllowOversized mode (i.e. + // flexible array init), it can be larger. CharUnits DesiredSize = Utils.getSize(DesiredTy); + if (Size > DesiredSize) { + assert(AllowOversized && "Elems are oversized"); + DesiredSize = Size; + } + + // The natural alignment of an unpacked LLVM struct with the given elements. CharUnits Align = CharUnits::One(); for (llvm::Constant *C : Elems) Align = std::max(Align, Utils.getAlignment(C)); + + // The natural size of an unpacked LLVM struct with the given elements. CharUnits AlignedSize = Size.alignTo(Align); bool Packed = false; ArrayRef UnpackedElems = Elems; llvm::SmallVector UnpackedElemStorage; - if ((DesiredSize < AlignedSize && !AllowOversized) || - DesiredSize.alignTo(Align) != DesiredSize) { - // The natural layout would be the wrong size; force use of a packed layout. + if (DesiredSize < AlignedSize || DesiredSize.alignTo(Align) != DesiredSize) { + // The natural layout would be too big; force use of a packed layout. NaturalLayout = false; Packed = true; } else if (DesiredSize > AlignedSize) { - // The constant would be too small. Add padding to fix it. + // The natural layout would be too small. Add padding to fix it. (This + // is ignored if we choose a packed layout.) UnpackedElemStorage.assign(Elems.begin(), Elems.end()); UnpackedElemStorage.push_back(Utils.getPadding(DesiredSize - Size)); UnpackedElems = UnpackedElemStorage; @@ -482,7 +493,7 @@ llvm::Constant *ConstantAggregateBuilder::buildFrom( // If we're using the packed layout, pad it out to the desired size if // necessary. if (Packed) { - assert((SizeSoFar <= DesiredSize || AllowOversized) && + assert(SizeSoFar <= DesiredSize && "requested size is too small for contents"); if (SizeSoFar < DesiredSize) PackedElems.push_back(Utils.getPadding(DesiredSize - SizeSoFar)); diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index 5184a363f18f..bf5f2d1b4271 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -4615,7 +4615,7 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D, T = D->getType(); if (getLangOpts().CPlusPlus) { - if (!InitDecl->getFlexibleArrayInitChars(getContext()).isZero()) + if (InitDecl->hasFlexibleArrayInit(getContext())) ErrorUnsupported(D, "flexible array initializer"); Init = EmitNullConstant(T); NeedsGlobalCtor = true; @@ -4631,17 +4631,12 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D, if (getLangOpts().CPlusPlus && !NeedsGlobalDtor) DelayedCXXInitPosition.erase(D); -#if 0 - // FIXME: The following check doesn't handle flexible array members - // inside tail padding (which don't actually increase the size of - // the struct). #ifndef NDEBUG CharUnits VarSize = getContext().getTypeSizeInChars(ASTTy) + InitDecl->getFlexibleArrayInitChars(getContext()); CharUnits CstSize = CharUnits::fromQuantity( getDataLayout().getTypeAllocSize(Init->getType())); assert(VarSize == CstSize && "Emitted constant has unexpected size"); -#endif #endif } } diff --git a/clang/test/CodeGen/flexible-array-init.c b/clang/test/CodeGen/flexible-array-init.c index 5974d475f816..b2cf959f7e12 100644 --- a/clang/test/CodeGen/flexible-array-init.c +++ b/clang/test/CodeGen/flexible-array-init.c @@ -18,7 +18,5 @@ struct __attribute((packed, aligned(4))) { char a; int x; char z[]; } d = { 1, 2 struct __attribute((packed, aligned(4))) { char a; int x; char z[]; } e = { 1, 2, { 13, 15, 17, 19 } }; // CHECK: @e ={{.*}} <{ i8, i32, [4 x i8] }> <{ i8 1, i32 2, [4 x i8] c"\0D\0F\11\13" }> -// FIXME: This global should be 6 bytes, not 8. Not likely to matter in most -// cases, but still a bug. struct { int x; char y[]; } f = { 1, { 13, 15 } }; -// CHECK: @f ={{.*}} global { i32, [2 x i8] } { i32 1, [2 x i8] c"\0D\0F" } +// CHECK: @f ={{.*}} global <{ i32, [2 x i8] }> <{ i32 1, [2 x i8] c"\0D\0F" }> diff --git a/clang/test/CodeGenCXX/flexible-array-init.cpp b/clang/test/CodeGenCXX/flexible-array-init.cpp index af8486d9d3b5..5840cbd8ab78 100644 --- a/clang/test/CodeGenCXX/flexible-array-init.cpp +++ b/clang/test/CodeGenCXX/flexible-array-init.cpp @@ -1,6 +1,7 @@ // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck %s // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL1 %s // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL2 %s +// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL3 %s struct A { int x; int y[]; }; A a = { 1, 7, 11 }; @@ -9,7 +10,7 @@ A a = { 1, 7, 11 }; A b = { 1, { 13, 15 } }; // CHECK: @b ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 13, i32 15] } -int f(); +char f(); #ifdef FAIL1 A c = { f(), { f(), f() } }; // expected-error {{cannot compile this flexible array initializer yet}} #endif @@ -18,3 +19,7 @@ void g() { static A d = { f(), { f(), f() } }; // expected-error {{cannot compile this flexible array initializer yet}} } #endif +#ifdef FAIL3 +struct B { int x; char y; char z[]; }; +B e = {f(), f(), f(), f()}; // expected-error {{cannot compile this flexible array initializer yet}} +#endif