[clang] 4802edd - Fix size of flexible array initializers, and re-enable assertions.
Eli Friedman via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 15 12:10:05 PDT 2022
Author: Eli Friedman
Date: 2022-04-15T12:09:57-07:00
New Revision: 4802edd1ac7a5aea8c8488b5baec221d722cbdde
URL: https://github.com/llvm/llvm-project/commit/4802edd1ac7a5aea8c8488b5baec221d722cbdde
DIFF: https://github.com/llvm/llvm-project/commit/4802edd1ac7a5aea8c8488b5baec221d722cbdde.diff
LOG: 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
Added:
Modified:
clang/include/clang/AST/Decl.h
clang/lib/AST/Decl.cpp
clang/lib/CodeGen/CGDecl.cpp
clang/lib/CodeGen/CGExprConstant.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/test/CodeGen/flexible-array-init.c
clang/test/CodeGenCXX/flexible-array-init.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 5816a91bd0c2d..f93008cdd322d 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -1591,12 +1591,18 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
/// 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 915d8db10aca4..050c3ad2dafc6 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<RecordType>();
+ if (!Ty || !Ty->getDecl()->hasFlexibleArrayMember())
+ return false;
+ auto *List = dyn_cast<InitListExpr>(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<RecordType>();
@@ -2730,11 +2746,17 @@ CharUnits VarDecl::getFlexibleArrayInitChars(const ASTContext &Ctx) const {
auto *List = dyn_cast<InitListExpr>(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 322602606ebe9..e47450f2ba8fe 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
diff er in type from the global. Rewrite
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index 92122f0df7b9a..91fdb88d6bb7a 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<llvm::Constant*> UnpackedElems = Elems;
llvm::SmallVector<llvm::Constant*, 32> 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 5184a363f18f5..bf5f2d1b42719 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 5974d475f816b..b2cf959f7e122 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 af8486d9d3b52..5840cbd8ab781 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
More information about the cfe-commits
mailing list