Skip to content

Reapply "[clang][CodeGen] Zero init unspecified fields in initializers in C" (#109898) #110051

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 14, 2024
Merged

Reapply "[clang][CodeGen] Zero init unspecified fields in initializers in C" (#109898) #110051

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
112b018
Reapply "[clang][CodeGen] Zero init unspecified fields in initializer…
yabinc Sep 25, 2024
56cad9a
Zero init padding between bitfields in CGExprAgg.cpp
yabinc Sep 27, 2024
5ffefd5
Init paddings between bitfields by storing 0 to StorageSize
yabinc Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions clang/docs/LanguageExtensions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5860,3 +5860,26 @@ specify the starting offset to begin embedding from. The resources is treated
as being empty if the specified offset is larger than the number of bytes in
the resource. The offset will be applied *before* any ``limit`` parameters are
applied.

Union and aggregate initialization in C
=======================================

In C23 (N2900), when an object is initialized from initializer ``= {}``, all
elements of arrays, all members of structs, and the first members of unions are
empty-initialized recursively. In addition, all padding bits are initialized to
zero.

Clang guarantees the following behaviors:

* ``1:`` Clang supports initializer ``= {}`` mentioned above in all C
standards.

* ``2:`` When unions are initialized from initializer ``= {}``, bytes outside
of the first members of unions are also initialized to zero.

* ``3:`` When unions, structures and arrays are initialized from initializer
``= { initializer-list }``, all members not explicitly initialized in
the initializer list are empty-initialized recursively. In addition, all
padding bits are initialized to zero.

Currently, the above extension only applies to C source code, not C++.
79 changes: 77 additions & 2 deletions clang/lib/CodeGen/CGExprAgg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include "CGCXXABI.h"
#include "CGObjCRuntime.h"
#include "CGRecordLayout.h"
#include "CodeGenFunction.h"
#include "CodeGenModule.h"
#include "ConstantEmitter.h"
Expand Down Expand Up @@ -64,6 +65,9 @@ class AggExprEmitter : public StmtVisitor<AggExprEmitter> {
void withReturnValueSlot(const Expr *E,
llvm::function_ref<RValue(ReturnValueSlot)> Fn);

void DoZeroInitPadding(const Address BaseAddr, uint64_t StartBitOffset,
uint64_t EndBitOffset);

public:
AggExprEmitter(CodeGenFunction &cgf, AggValueSlot Dest, bool IsResultUnused)
: CGF(cgf), Builder(CGF.Builder), Dest(Dest),
Expand Down Expand Up @@ -1698,6 +1702,10 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
// Prepare a 'this' for CXXDefaultInitExprs.
CodeGenFunction::FieldConstructionScope FCS(CGF, Dest.getAddress());

const bool ZeroInitPadding =
CGF.CGM.shouldZeroInitPadding() && !Dest.isZeroed();
const Address BaseAddr = Dest.getAddress().withElementType(CGF.Int8Ty);

if (record->isUnion()) {
// Only initialize one field of a union. The field itself is
// specified by the initializer list.
Expand All @@ -1722,16 +1730,26 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
if (NumInitElements) {
// Store the initializer into the field
EmitInitializationToLValue(InitExprs[0], FieldLoc);
if (ZeroInitPadding) {
uint64_t TotalSize = CGF.getContext().toBits(
Dest.getPreferredSize(CGF.getContext(), DestLV.getType()));
uint64_t FieldSize = CGF.getContext().getTypeSize(FieldLoc.getType());
DoZeroInitPadding(BaseAddr, FieldSize, TotalSize);
}
} else {
// Default-initialize to null.
EmitNullInitializationToLValue(FieldLoc);
if (ZeroInitPadding)
EmitNullInitializationToLValue(DestLV);
else
EmitNullInitializationToLValue(FieldLoc);
}

return;
}

// Here we iterate over the fields; this makes it simpler to both
// default-initialize fields and skip over unnamed fields.
const ASTRecordLayout &Layout = CGF.getContext().getASTRecordLayout(record);
uint64_t LastFieldBitOffset = 0;
for (const auto *field : record->fields()) {
// We're done once we hit the flexible array member.
if (field->getType()->isIncompleteArrayType())
Expand All @@ -1748,6 +1766,19 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
CGF.getTypes().isZeroInitializable(ExprToVisit->getType()))
break;

if (ZeroInitPadding) {
uint64_t StartBitOffset = Layout.getFieldOffset(field->getFieldIndex());
DoZeroInitPadding(BaseAddr, LastFieldBitOffset, StartBitOffset);
if (!field->isBitField()) {
LastFieldBitOffset =
StartBitOffset + CGF.getContext().getTypeSize(field->getType());
} else {
const CGRecordLayout &RL =
CGF.getTypes().getCGRecordLayout(field->getParent());
const CGBitFieldInfo &Info = RL.getBitFieldInfo(field);
LastFieldBitOffset = StartBitOffset + Info.Size;
}
}

LValue LV = CGF.EmitLValueForFieldInitialization(DestLV, field);
// We never generate write-barries for initialized fields.
Expand All @@ -1774,6 +1805,50 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
}
}
}
if (ZeroInitPadding) {
uint64_t TotalSize = CGF.getContext().toBits(
Dest.getPreferredSize(CGF.getContext(), DestLV.getType()));
DoZeroInitPadding(BaseAddr, LastFieldBitOffset, TotalSize);
}
}

void AggExprEmitter::DoZeroInitPadding(const Address BaseAddr,
uint64_t StartBitOffset,
uint64_t EndBitOffset) {
if (StartBitOffset >= EndBitOffset)
return;

auto InitBytes = [&](uint64_t Start, uint64_t End) {
Address Addr = CGF.Builder.CreateConstGEP(BaseAddr, Start);
llvm::Constant *SizeVal = CGF.Builder.getInt64(End - Start);
CGF.Builder.CreateMemSet(Addr, CGF.Builder.getInt8(0), SizeVal, false);
};
auto InitBits = [&](uint64_t Byte, uint64_t Start, uint64_t End) {
Address Addr = CGF.Builder.CreateConstGEP(BaseAddr, Byte);
llvm::Value *Val = Builder.CreateLoad(Addr);
Copy link
Collaborator

@efriedma-quic efriedma-quic Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the size of the loaded integer here? I can't find anything that explicitly sets it.

Copy link
Collaborator

@efriedma-quic efriedma-quic Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Ideally, we'd use load/store operations with the same width as the bitfield, as computed by CGBitFieldInfo, to make the result easier for LLVM to optimize.)

Copy link
Contributor Author

@yabinc yabinc Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally it was Int8Ty. Now changed to store 0 to Info.StorageSize before initializing bitfields. I tested in -O2 that the store was always merged with initialization for bitfields.
Not sure if I need to consider Info.VolatileStorageSize, but I ran check-clang and test-suite successfully.

Val = Builder.CreateAnd(Val, ~llvm::APInt::getBitsSet(8, Start, End));
Builder.CreateStore(Val, Addr);
};

uint64_t StartBit = StartBitOffset % 8;
uint64_t StartByte = StartBitOffset / 8;
uint64_t EndBit = EndBitOffset % 8;
uint64_t EndByte = EndBitOffset / 8;

if (StartByte < EndByte) {
if (StartBit != 0) {
InitBits(StartByte, StartBit, 8);
StartBit = 0;
StartByte++;
}
if (StartByte < EndByte) {
InitBytes(StartByte, EndByte);
StartByte = EndByte;
}
}
if (EndBit != 0) {
InitBits(EndByte, StartBit, EndBit);
}
}

void AggExprEmitter::VisitArrayInitLoopExpr(const ArrayInitLoopExpr *E,
Expand Down
107 changes: 96 additions & 11 deletions clang/lib/CodeGen/CGExprConstant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ using namespace CodeGen;
namespace {
class ConstExprEmitter;

llvm::Constant *getPadding(const CodeGenModule &CGM, CharUnits PadSize) {
llvm::Type *Ty = CGM.CharTy;
if (PadSize > CharUnits::One())
Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
if (CGM.shouldZeroInitPadding()) {
return llvm::Constant::getNullValue(Ty);
}
return llvm::UndefValue::get(Ty);
}

struct ConstantAggregateBuilderUtils {
CodeGenModule &CGM;

Expand All @@ -61,10 +71,7 @@ struct ConstantAggregateBuilderUtils {
}

llvm::Constant *getPadding(CharUnits PadSize) const {
llvm::Type *Ty = CGM.CharTy;
if (PadSize > CharUnits::One())
Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
return llvm::UndefValue::get(Ty);
return ::getPadding(CGM, PadSize);
}

llvm::Constant *getZeroes(CharUnits ZeroSize) const {
Expand Down Expand Up @@ -591,6 +598,11 @@ class ConstStructBuilder {
bool Build(const InitListExpr *ILE, bool AllowOverwrite);
bool Build(const APValue &Val, const RecordDecl *RD, bool IsPrimaryBase,
const CXXRecordDecl *VTableClass, CharUnits BaseOffset);
bool DoZeroInitPadding(const ASTRecordLayout &Layout, unsigned FieldNo,
const FieldDecl &Field, bool AllowOverwrite,
CharUnits &SizeSoFar, bool &ZeroFieldSize);
bool DoZeroInitPadding(const ASTRecordLayout &Layout, bool AllowOverwrite,
CharUnits SizeSoFar);
llvm::Constant *Finalize(QualType Ty);
};

Expand Down Expand Up @@ -715,6 +727,10 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) {
if (CXXRD->getNumBases())
return false;

const bool ZeroInitPadding = CGM.shouldZeroInitPadding();
bool ZeroFieldSize = false;
CharUnits SizeSoFar = CharUnits::Zero();

for (FieldDecl *Field : RD->fields()) {
++FieldNo;

Expand All @@ -732,8 +748,13 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) {
const Expr *Init = nullptr;
if (ElementNo < ILE->getNumInits())
Init = ILE->getInit(ElementNo++);
if (isa_and_nonnull<NoInitExpr>(Init))
if (isa_and_nonnull<NoInitExpr>(Init)) {
if (ZeroInitPadding &&
!DoZeroInitPadding(Layout, FieldNo, *Field, AllowOverwrite, SizeSoFar,
ZeroFieldSize))
return false;
continue;
}

// Zero-sized fields are not emitted, but their initializers may still
// prevent emission of this struct as a constant.
Expand All @@ -743,6 +764,11 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) {
continue;
}

if (ZeroInitPadding &&
!DoZeroInitPadding(Layout, FieldNo, *Field, AllowOverwrite, SizeSoFar,
ZeroFieldSize))
return false;

// When emitting a DesignatedInitUpdateExpr, a nested InitListExpr
// represents additional overwriting of our current constant value, and not
// a new constant to emit independently.
Expand All @@ -768,6 +794,10 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) {
if (!EltInit)
return false;

if (ZeroInitPadding && ZeroFieldSize)
SizeSoFar += CharUnits::fromQuantity(
CGM.getDataLayout().getTypeAllocSize(EltInit->getType()));

if (!Field->isBitField()) {
// Handle non-bitfield members.
if (!AppendField(Field, Layout.getFieldOffset(FieldNo), EltInit,
Expand All @@ -785,6 +815,9 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) {
}
}

if (ZeroInitPadding && !DoZeroInitPadding(Layout, AllowOverwrite, SizeSoFar))
return false;

return true;
}

Expand Down Expand Up @@ -849,6 +882,9 @@ bool ConstStructBuilder::Build(const APValue &Val, const RecordDecl *RD,

unsigned FieldNo = 0;
uint64_t OffsetBits = CGM.getContext().toBits(Offset);
const bool ZeroInitPadding = CGM.shouldZeroInitPadding();
bool ZeroFieldSize = false;
CharUnits SizeSoFar = CharUnits::Zero();

bool AllowOverwrite = false;
for (RecordDecl::field_iterator Field = RD->field_begin(),
Expand All @@ -870,6 +906,15 @@ bool ConstStructBuilder::Build(const APValue &Val, const RecordDecl *RD,
if (!EltInit)
return false;

if (ZeroInitPadding) {
if (!DoZeroInitPadding(Layout, FieldNo, **Field, AllowOverwrite,
SizeSoFar, ZeroFieldSize))
return false;
if (ZeroFieldSize)
SizeSoFar += CharUnits::fromQuantity(
CGM.getDataLayout().getTypeAllocSize(EltInit->getType()));
}

if (!Field->isBitField()) {
// Handle non-bitfield members.
if (!AppendField(*Field, Layout.getFieldOffset(FieldNo) + OffsetBits,
Expand All @@ -886,7 +931,49 @@ bool ConstStructBuilder::Build(const APValue &Val, const RecordDecl *RD,
return false;
}
}
if (ZeroInitPadding && !DoZeroInitPadding(Layout, AllowOverwrite, SizeSoFar))
return false;

return true;
}

bool ConstStructBuilder::DoZeroInitPadding(
const ASTRecordLayout &Layout, unsigned FieldNo, const FieldDecl &Field,
bool AllowOverwrite, CharUnits &SizeSoFar, bool &ZeroFieldSize) {
uint64_t StartBitOffset = Layout.getFieldOffset(FieldNo);
CharUnits StartOffset = CGM.getContext().toCharUnitsFromBits(StartBitOffset);
if (SizeSoFar < StartOffset)
if (!AppendBytes(SizeSoFar, getPadding(CGM, StartOffset - SizeSoFar),
AllowOverwrite))
return false;

if (!Field.isBitField()) {
CharUnits FieldSize = CGM.getContext().getTypeSizeInChars(Field.getType());
SizeSoFar = StartOffset + FieldSize;
ZeroFieldSize = FieldSize.isZero();
} else {
const CGRecordLayout &RL =
CGM.getTypes().getCGRecordLayout(Field.getParent());
const CGBitFieldInfo &Info = RL.getBitFieldInfo(&Field);
uint64_t EndBitOffset = StartBitOffset + Info.Size;
SizeSoFar = CGM.getContext().toCharUnitsFromBits(EndBitOffset);
if (EndBitOffset % CGM.getContext().getCharWidth() != 0) {
SizeSoFar++;
}
ZeroFieldSize = Info.Size == 0;
}
return true;
}

bool ConstStructBuilder::DoZeroInitPadding(const ASTRecordLayout &Layout,
bool AllowOverwrite,
CharUnits SizeSoFar) {
CharUnits TotalSize = Layout.getSize();
if (SizeSoFar < TotalSize)
if (!AppendBytes(SizeSoFar, getPadding(CGM, TotalSize - SizeSoFar),
AllowOverwrite))
return false;
SizeSoFar = TotalSize;
return true;
}

Expand Down Expand Up @@ -1127,12 +1214,10 @@ class ConstExprEmitter

assert(CurSize <= TotalSize && "Union size mismatch!");
if (unsigned NumPadBytes = TotalSize - CurSize) {
llvm::Type *Ty = CGM.CharTy;
if (NumPadBytes > 1)
Ty = llvm::ArrayType::get(Ty, NumPadBytes);

Elts.push_back(llvm::UndefValue::get(Ty));
Types.push_back(Ty);
llvm::Constant *Padding =
getPadding(CGM, CharUnits::fromQuantity(NumPadBytes));
Elts.push_back(Padding);
Types.push_back(Padding->getType());
}

llvm::StructType *STy = llvm::StructType::get(VMContext, Types, false);
Expand Down
Loading
Loading