jfb created this revision.
jfb added reviewers: rsmith, hubert.reinterpretcast.
Herald added subscribers: cfe-commits, ributzka, dexonsmith, jkorous.
Herald added a project: clang.
jfb marked an inline comment as done.
jfb added inline comments.
================
Comment at: clang/test/CodeGen/union-tail-padding.c:9
+// 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
+
----------------
This test, while it might be surprising, is testing the current behavior of
clang. I haven't validated that it's correct for each standard.
>From a discussion in D68115 <https://reviews.llvm.org/D68115>, it seems like
>union tail padding is often surprising to folks, compiler implementors
>included. This new diagnostic, -Wunion-tail-padding, emits a diagnostic when a
>union's tail is left undefined when develoeprs might have reasonably expected
>that it would not be. I've included -Wunion-tail-padding under the umbrella of
>a new diagnostic group, -Wpadding, since it often seems like padding in
>general is a source of pain. In particular, padding can leak secret
>information to malicious attackers. Here we focus on the places where padding
>is *unitialized*, not where a structure or union has padding.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D80055
Files:
clang/include/clang/Basic/DiagnosticFrontendKinds.td
clang/include/clang/Basic/DiagnosticGroups.td
clang/lib/CodeGen/CGExprConstant.cpp
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/CodeGen/ConstantEmitter.h
clang/test/CodeGen/union-tail-padding.c
Index: clang/test/CodeGen/union-tail-padding.c
===================================================================
--- /dev/null
+++ 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
Index: clang/lib/CodeGen/ConstantEmitter.h
===================================================================
--- clang/lib/CodeGen/ConstantEmitter.h
+++ 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
Index: clang/lib/CodeGen/CodeGenModule.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ 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 {
Index: clang/lib/CodeGen/CGExprConstant.cpp
===================================================================
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ 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<RecordType>()->getDecl();
const CXXRecordDecl *CD = dyn_cast<CXXRecordDecl>(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<Expr*>(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)
Index: clang/include/clang/Basic/DiagnosticGroups.td
===================================================================
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ 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">;
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -260,6 +260,10 @@
"as the %select{aliasee|resolver}2">,
InGroup<IgnoredAttributes>;
+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<UnionTailPadding>;
+
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"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
