diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td --- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td +++ b/clang/include/clang/Basic/DiagnosticFrontendKinds.td @@ -260,6 +260,10 @@ "as the %select{aliasee|resolver}2">, InGroup; +def warn_union_tail_padding_uninitialized : Warning< + "Initializing union %0 field %1 only initializes the first %2 of %3 bytes, leaving the remaining %4 bytes undefined">, + InGroup; + let CategoryName = "Instrumentation Issue" in { def warn_profile_data_out_of_date : Warning< "profile data may be out of date: of %0 function%s0, %1 %plural{1:has|:have}1" diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -820,6 +820,10 @@ UnusedPropertyIvar]>, DiagCategory<"Unused Entity Issue">; +def UnionTailPadding : DiagGroup<"union-tail-padding">; +def Padding : DiagGroup<"padding", [UnionTailPadding]>, + DiagCategory<"Padding Issue">; + // Format settings. def FormatInvalidSpecifier : DiagGroup<"format-invalid-specifier">; def FormatSecurity : DiagGroup<"format-security">; diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp --- a/clang/lib/CodeGen/CGExprConstant.cpp +++ b/clang/lib/CodeGen/CGExprConstant.cpp @@ -23,6 +23,7 @@ #include "clang/AST/RecordLayout.h" #include "clang/AST/StmtVisitor.h" #include "clang/Basic/Builtins.h" +#include "clang/Frontend/FrontendDiagnostic.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/Sequence.h" #include "llvm/IR/Constants.h" @@ -72,7 +73,7 @@ /// Incremental builder for an llvm::Constant* holding a struct or array /// constant. -class ConstantAggregateBuilder : private ConstantAggregateBuilderUtils { +class ConstantAggregateBuilder : public ConstantAggregateBuilderUtils { /// The elements of the constant. These two arrays must have the same size; /// Offsets[i] describes the offset of Elems[i] within the constant. The /// elements are kept in increasing offset order, and we ensure that there @@ -553,6 +554,7 @@ static llvm::Constant *BuildStruct(ConstantEmitter &Emitter, InitListExpr *ILE, QualType StructTy); static llvm::Constant *BuildStruct(ConstantEmitter &Emitter, + const SourceLocation loc, const APValue &Value, QualType ValTy); static bool UpdateStruct(ConstantEmitter &Emitter, ConstantAggregateBuilder &Const, CharUnits Offset, @@ -565,7 +567,8 @@ StartOffset(StartOffset) {} bool AppendField(const FieldDecl *Field, uint64_t FieldOffset, - llvm::Constant *InitExpr, bool AllowOverwrite = false); + llvm::Constant *InitExpr, bool AllowOverwrite, + const SourceLocation loc); bool AppendBytes(CharUnits FieldOffsetInChars, llvm::Constant *InitCst, bool AllowOverwrite = false); @@ -574,19 +577,41 @@ llvm::ConstantInt *InitExpr, bool AllowOverwrite = false); bool Build(InitListExpr *ILE, bool AllowOverwrite); - bool Build(const APValue &Val, const RecordDecl *RD, bool IsPrimaryBase, - const CXXRecordDecl *VTableClass, CharUnits BaseOffset); + bool Build(const SourceLocation loc, const APValue &Val, const RecordDecl *RD, + bool IsPrimaryBase, const CXXRecordDecl *VTableClass, + CharUnits BaseOffset); llvm::Constant *Finalize(QualType Ty); }; -bool ConstStructBuilder::AppendField( - const FieldDecl *Field, uint64_t FieldOffset, llvm::Constant *InitCst, - bool AllowOverwrite) { +bool ConstStructBuilder::AppendField(const FieldDecl *Field, + uint64_t FieldOffset, + llvm::Constant *InitCst, + bool AllowOverwrite, + const SourceLocation loc) { const ASTContext &Context = CGM.getContext(); CharUnits FieldOffsetInChars = Context.toCharUnitsFromBits(FieldOffset); - return AppendBytes(FieldOffsetInChars, InitCst, AllowOverwrite); + if (!AppendBytes(FieldOffsetInChars, InitCst, AllowOverwrite)) + return false; + + if (!Field->getParent()->isUnion()) + return true; + + const ASTRecordLayout &Layout = + CGM.getContext().getASTRecordLayout(Field->getParent()); + CharUnits UnionSize = Layout.getSize(); + CharUnits InitSize = Builder.getSize(InitCst); + if (UnionSize <= InitSize) + return true; + + // The tail padding of the union isn't being initialized. + CGM.getDiags().Report(loc, diag::warn_union_tail_padding_uninitialized) + << Field->getParent() << Field << (unsigned)InitSize.getQuantity() + << (unsigned)UnionSize.getQuantity() + << (unsigned)(UnionSize - InitSize).getQuantity(); + + return true; } bool ConstStructBuilder::AppendBytes(CharUnits FieldOffsetInChars, @@ -732,7 +757,7 @@ if (!Field->isBitField()) { // Handle non-bitfield members. if (!AppendField(Field, Layout.getFieldOffset(FieldNo), EltInit, - AllowOverwrite)) + AllowOverwrite, ILE->getExprLoc())) return false; // After emitting a non-empty field with [[no_unique_address]], we may // need to overwrite its tail padding. @@ -769,8 +794,8 @@ }; } -bool ConstStructBuilder::Build(const APValue &Val, const RecordDecl *RD, - bool IsPrimaryBase, +bool ConstStructBuilder::Build(const SourceLocation loc, const APValue &Val, + const RecordDecl *RD, bool IsPrimaryBase, const CXXRecordDecl *VTableClass, CharUnits Offset) { const ASTRecordLayout &Layout = CGM.getContext().getASTRecordLayout(RD); @@ -803,7 +828,7 @@ BaseInfo &Base = Bases[I]; bool IsPrimaryBase = Layout.getPrimaryBase() == Base.Decl; - Build(Val.getStructBase(Base.Index), Base.Decl, IsPrimaryBase, + Build(loc, Val.getStructBase(Base.Index), Base.Decl, IsPrimaryBase, VTableClass, Offset + Base.Offset); } } @@ -826,14 +851,14 @@ const APValue &FieldValue = RD->isUnion() ? Val.getUnionValue() : Val.getStructField(FieldNo); llvm::Constant *EltInit = - Emitter.tryEmitPrivateForMemory(FieldValue, Field->getType()); + Emitter.tryEmitPrivateForMemory(loc, FieldValue, Field->getType()); if (!EltInit) return false; if (!Field->isBitField()) { // Handle non-bitfield members. if (!AppendField(*Field, Layout.getFieldOffset(FieldNo) + OffsetBits, - EltInit, AllowOverwrite)) + EltInit, AllowOverwrite, loc)) return false; // After emitting a non-empty field with [[no_unique_address]], we may // need to overwrite its tail padding. @@ -869,6 +894,7 @@ } llvm::Constant *ConstStructBuilder::BuildStruct(ConstantEmitter &Emitter, + const SourceLocation loc, const APValue &Val, QualType ValTy) { ConstantAggregateBuilder Const(Emitter.CGM); @@ -876,7 +902,7 @@ const RecordDecl *RD = ValTy->castAs()->getDecl(); const CXXRecordDecl *CD = dyn_cast(RD); - if (!Builder.Build(Val, RD, false, CD, CharUnits::Zero())) + if (!Builder.Build(loc, Val, RD, false, CD, CharUnits::Zero())) return nullptr; return Builder.Finalize(ValTy); @@ -1351,10 +1377,11 @@ return validateAndPopAbstract(C, state); } -llvm::Constant * -ConstantEmitter::tryEmitAbstract(const APValue &value, QualType destType) { +llvm::Constant *ConstantEmitter::tryEmitAbstract(const SourceLocation loc, + const APValue &value, + QualType destType) { auto state = pushAbstract(); - auto C = tryEmitPrivate(value, destType); + auto C = tryEmitPrivate(loc, value, destType); return validateAndPopAbstract(C, state); } @@ -1375,7 +1402,7 @@ ConstantEmitter::emitAbstract(SourceLocation loc, const APValue &value, QualType destType) { auto state = pushAbstract(); - auto C = tryEmitPrivate(value, destType); + auto C = tryEmitPrivate(loc, value, destType); C = validateAndPopAbstract(C, state); if (!C) { CGM.Error(loc, @@ -1397,11 +1424,12 @@ return markIfFailed(tryEmitPrivateForMemory(E, destType)); } -llvm::Constant *ConstantEmitter::emitForInitializer(const APValue &value, +llvm::Constant *ConstantEmitter::emitForInitializer(const SourceLocation loc, + const APValue &value, LangAS destAddrSpace, QualType destType) { initializeNonAbstract(destAddrSpace); - auto C = tryEmitPrivateForMemory(value, destType); + auto C = tryEmitPrivateForMemory(loc, value, destType); assert(C && "couldn't emit constant value non-abstractly?"); return C; } @@ -1612,7 +1640,7 @@ // Try to emit the initializer. Note that this can allow some things that // are not allowed by tryEmitPrivateForMemory alone. if (auto value = D.evaluateValue()) { - return tryEmitPrivateForMemory(*value, destType); + return tryEmitPrivateForMemory(D.getInit()->getExprLoc(), *value, destType); } // FIXME: Implement C++11 [basic.start.init]p2: if the initializer of a @@ -1640,11 +1668,10 @@ return (C ? emitForMemory(C, destType) : nullptr); } -llvm::Constant * -ConstantEmitter::tryEmitAbstractForMemory(const APValue &value, - QualType destType) { +llvm::Constant *ConstantEmitter::tryEmitAbstractForMemory( + const SourceLocation loc, const APValue &value, QualType destType) { auto nonMemoryDestType = getNonMemoryType(CGM, destType); - auto C = tryEmitAbstract(value, nonMemoryDestType); + auto C = tryEmitAbstract(loc, value, nonMemoryDestType); return (C ? emitForMemory(C, destType) : nullptr); } @@ -1655,10 +1682,10 @@ return (C ? emitForMemory(C, destType) : nullptr); } -llvm::Constant *ConstantEmitter::tryEmitPrivateForMemory(const APValue &value, - QualType destType) { +llvm::Constant *ConstantEmitter::tryEmitPrivateForMemory( + const SourceLocation loc, const APValue &value, QualType destType) { auto nonMemoryDestType = getNonMemoryType(CGM, destType); - auto C = tryEmitPrivate(value, nonMemoryDestType); + auto C = tryEmitPrivate(loc, value, nonMemoryDestType); return (C ? emitForMemory(C, destType) : nullptr); } @@ -1706,7 +1733,7 @@ llvm::Constant *C; if (Success && !Result.HasSideEffects) - C = tryEmitPrivate(Result.Val, destType); + C = tryEmitPrivate(E->getExprLoc(), Result.Val, destType); else C = ConstExprEmitter(*this).Visit(const_cast(E), destType); @@ -2003,7 +2030,8 @@ return CGM.GetAddrOfGlobalTemporary(E, Inner); } -llvm::Constant *ConstantEmitter::tryEmitPrivate(const APValue &Value, +llvm::Constant *ConstantEmitter::tryEmitPrivate(const SourceLocation loc, + const APValue &Value, QualType DestType) { switch (Value.getKind()) { case APValue::None: @@ -2088,7 +2116,7 @@ } case APValue::Struct: case APValue::Union: - return ConstStructBuilder::BuildStruct(*this, Value, DestType); + return ConstStructBuilder::BuildStruct(*this, loc, Value, DestType); case APValue::Array: { const ConstantArrayType *CAT = CGM.getContext().getAsConstantArrayType(DestType); @@ -2098,7 +2126,7 @@ // Emit array filler, if there is one. llvm::Constant *Filler = nullptr; if (Value.hasArrayFiller()) { - Filler = tryEmitAbstractForMemory(Value.getArrayFiller(), + Filler = tryEmitAbstractForMemory(loc, Value.getArrayFiller(), CAT->getElementType()); if (!Filler) return nullptr; @@ -2114,7 +2142,7 @@ llvm::Type *CommonElementType = nullptr; for (unsigned I = 0; I < NumInitElts; ++I) { llvm::Constant *C = tryEmitPrivateForMemory( - Value.getArrayInitializedElt(I), CAT->getElementType()); + loc, Value.getArrayInitializedElt(I), CAT->getElementType()); if (!C) return nullptr; if (I == 0) diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -2458,8 +2458,9 @@ if (!V.isAbsent()) { // If possible, emit the APValue version of the initializer. In particular, // this gets the type of the constant right. - Init = Emitter.emitForInitializer( - GD->getAsAPValue(), GD->getType().getAddressSpace(), GD->getType()); + Init = Emitter.emitForInitializer(GD->getLocation(), GD->getAsAPValue(), + GD->getType().getAddressSpace(), + GD->getType()); } else { // As a fallback, directly construct the constant. // FIXME: This may get padding wrong under esoteric struct layout rules. @@ -5150,8 +5151,8 @@ if (Value) { // The temporary has a constant initializer, use it. emitter.emplace(*this); - InitialValue = emitter->emitForInitializer(*Value, AddrSpace, - MaterializedType); + InitialValue = emitter->emitForInitializer(Init->getExprLoc(), *Value, + AddrSpace, MaterializedType); Constant = isTypeConstant(MaterializedType, /*ExcludeCtor*/Value); Type = InitialValue->getType(); } else { diff --git a/clang/lib/CodeGen/ConstantEmitter.h b/clang/lib/CodeGen/ConstantEmitter.h --- a/clang/lib/CodeGen/ConstantEmitter.h +++ b/clang/lib/CodeGen/ConstantEmitter.h @@ -67,12 +67,13 @@ return Abstract; } - /// Try to emit the initiaizer of the given declaration as an abstract - /// constant. If this succeeds, the emission must be finalized. + /// Try to emit the initializer of the given declaration as an abstract + /// constant. If this succeeds, the emission must be finalized. llvm::Constant *tryEmitForInitializer(const VarDecl &D); llvm::Constant *tryEmitForInitializer(const Expr *E, LangAS destAddrSpace, QualType destType); - llvm::Constant *emitForInitializer(const APValue &value, LangAS destAddrSpace, + llvm::Constant *emitForInitializer(const SourceLocation loc, + const APValue &value, LangAS destAddrSpace, QualType destType); void finalize(llvm::GlobalVariable *global); @@ -107,8 +108,10 @@ llvm::Constant *tryEmitAbstract(const Expr *E, QualType T); llvm::Constant *tryEmitAbstractForMemory(const Expr *E, QualType T); - llvm::Constant *tryEmitAbstract(const APValue &value, QualType T); - llvm::Constant *tryEmitAbstractForMemory(const APValue &value, QualType T); + llvm::Constant *tryEmitAbstract(const SourceLocation loc, + const APValue &value, QualType T); + llvm::Constant *tryEmitAbstractForMemory(const SourceLocation loc, + const APValue &value, QualType T); llvm::Constant *emitNullForMemory(QualType T) { return emitNullForMemory(CGM, T); @@ -130,8 +133,10 @@ llvm::Constant *tryEmitPrivate(const Expr *E, QualType T); llvm::Constant *tryEmitPrivateForMemory(const Expr *E, QualType T); - llvm::Constant *tryEmitPrivate(const APValue &value, QualType T); - llvm::Constant *tryEmitPrivateForMemory(const APValue &value, QualType T); + llvm::Constant *tryEmitPrivate(const SourceLocation loc, const APValue &value, + QualType T); + llvm::Constant *tryEmitPrivateForMemory(const SourceLocation loc, + const APValue &value, QualType T); /// Get the address of the current location. This is a constant /// that will resolve, after finalization, to the address of the diff --git a/clang/test/CodeGen/union-tail-padding.c b/clang/test/CodeGen/union-tail-padding.c new file mode 100644 --- /dev/null +++ b/clang/test/CodeGen/union-tail-padding.c @@ -0,0 +1,87 @@ +// RUN: %clang_cc1 -Wunion-tail-padding -std=c99 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1 -Wunion-tail-padding -std=c11 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1 -Wunion-tail-padding -std=c17 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++98 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++03 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++11 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++14 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++17 -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify +// RUN: %clang_cc1 -Wunion-tail-padding -xc++ -std=c++2a -triple aarch64-apple-darwin %s -emit-llvm -o /dev/null -verify + +union OK { + char c; + unsigned char uc; +}; + +union OK ok1; +union OK ok2 = {}; +union OK ok3 = {42}; +union OK ok4 = {.c = 42}; +union OK ok5 = {.uc = 42}; +#if defined(__cplusplus) +OK ok6 = OK(); +#if __cplusplus >= 201103 +OK ok7 = OK{}; +#endif +#endif + +union Front { + int i; + long long ll; +}; + +union Front front1; +union Front front2 = {}; // expected-warning {{Initializing union 'Front' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +union Front front3 = {42}; // expected-warning {{Initializing union 'Front' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +union Front front4 = {.i = 42}; // expected-warning {{Initializing union 'Front' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +union Front front5 = {.ll = 42}; +#if defined(__cplusplus) +#if __cplusplus < 201103 +Front front6 = Front(); +#elif __cplusplus < 201703 +Front front6 = Front(); // expected-warning {{Initializing union 'Front' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +Front front7 = Front{}; // expected-warning {{Initializing union 'Front' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +#else +Front front6 = Front(); +Front front7 = Front{}; // expected-warning {{Initializing union 'Front' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +#endif +#endif + +union Back { + long long ll; + int i; +}; + +union Back back1; +union Back back2 = {}; +union Back back3 = {42}; +union Back back4 = {.i = 42}; // expected-warning {{Initializing union 'Back' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +union Back back5 = {.ll = 42}; +#if defined(__cplusplus) +Back back6 = Back(); +#if __cplusplus >= 201103 +Back back7 = Back{}; +#endif +#endif + +#if defined(__cplusplus) && __cplusplus >= 201103 +union FrontInit { + int i = 42; + long long ll; +}; +FrontInit frontinit1; // expected-warning {{Initializing union 'FrontInit' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +FrontInit frontinit2 = {}; // expected-warning {{Initializing union 'FrontInit' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +FrontInit frontinit3 = FrontInit(); // expected-warning {{Initializing union 'FrontInit' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +FrontInit frontinit4 = FrontInit{}; // expected-warning {{Initializing union 'FrontInit' field 'i' only initializes the first 4 of 8 bytes, leaving the remaining 4 bytes undefined}} +#endif + +#if defined(__cplusplus) && __cplusplus >= 201103 +union BackInit { + int i; + long long ll = 42; +}; +BackInit backinit1; +BackInit backinit2 = {}; +BackInit backinit3 = BackInit(); +BackInit backinit4 = BackInit{}; +#endif