[llvm] Don't rely on undefined behavior to store how a `User` object's allocation is laid out (PR #105714)
Daniel Paoliello via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 22 11:31:40 PDT 2024
https://github.com/dpaoliello created https://github.com/llvm/llvm-project/pull/105714
In `User::operator new` a single allocation is created to store the `User` object itself, "intrusive" operands or a pointer for "hung off" operands, and the descriptor. After allocation, details about the layout (number of operands, how the operands are stored, if there is a descriptor) are stored in the `User` object by settings its fields. The `Value` and `User` constructors are then very careful not to initialize these fields so that the values set during allocation can be subsequently read. However, when the `User` object is returned from `operator new` [its value is technically "indeterminate" and so reading a field without first initializing it is undefined behavior (and will be erroneous in C++26)](https://en.cppreference.com/w/cpp/language/default_initialization#Indeterminate_and_erroneous_values).
We discovered this issue when trying to build LLVM using MSVC's [`/sdl` flag](https://learn.microsoft.com/en-us/cpp/build/reference/sdl-enable-additional-security-checks?view=msvc-170) which clears class fields after allocation (the docs say that this feature shouldn't be turned on for custom allocators and should only clear pointers, but that doesn't seem to match the implementation). MSVC's behavior both with and without the `/sdl` flag is standards conforming since a program is supposed to initialize storage before reading from it, thus the compiler implementation changing any values will never be observed in a well-formed program. The standard also provides no provisions for making storage bytes not indeterminate by setting them during allocation or `operator new`.
The fix for this is to move the fields that are set during `User::operator new` into a new object that is placed before the `User` object in that same allocation. This avoids the undefined behavior, but does increase the size of every `User` and derived type allocation by 1 pointer width.
>From 0c82c44e9c80230bbc577b26e956b72e0fe0c98a Mon Sep 17 00:00:00 2001
From: Daniel Paoliello <danpao at microsoft.com>
Date: Wed, 21 Aug 2024 14:07:11 -0700
Subject: [PATCH] Place operand info set during allocation into its own object
---
llvm/include/llvm/IR/InstrTypes.h | 2 +-
llvm/include/llvm/IR/OperandTraits.h | 9 +--
llvm/include/llvm/IR/User.h | 92 +++++++++++++++++++-------
llvm/include/llvm/IR/Value.h | 17 -----
llvm/lib/IR/User.cpp | 62 ++++++++++-------
llvm/lib/IR/Value.cpp | 4 +-
llvm/utils/LLVMVisualizers/llvm.natvis | 19 +++---
7 files changed, 123 insertions(+), 82 deletions(-)
diff --git a/llvm/include/llvm/IR/InstrTypes.h b/llvm/include/llvm/IR/InstrTypes.h
index afae564bf022d2..58717f50408201 100644
--- a/llvm/include/llvm/IR/InstrTypes.h
+++ b/llvm/include/llvm/IR/InstrTypes.h
@@ -1256,7 +1256,7 @@ class CallBase : public Instruction {
using Instruction::Instruction;
- bool hasDescriptor() const { return Value::HasDescriptor; }
+ bool hasDescriptor() const { return getHeader().HasDescriptor; }
unsigned getNumSubclassExtraOperands() const {
switch (getOpcode()) {
diff --git a/llvm/include/llvm/IR/OperandTraits.h b/llvm/include/llvm/IR/OperandTraits.h
index ffece6324aab02..306f5f97e9f2bf 100644
--- a/llvm/include/llvm/IR/OperandTraits.h
+++ b/llvm/include/llvm/IR/OperandTraits.h
@@ -32,10 +32,10 @@ struct FixedNumOperandTraits {
static_assert(
!std::is_polymorphic<SubClass>::value,
"adding virtual methods to subclasses of User breaks use lists");
- return reinterpret_cast<Use*>(U) - ARITY;
+ return static_cast<User *>(U)->getIntrusiveOperands();
}
static Use *op_end(SubClass* U) {
- return reinterpret_cast<Use*>(U);
+ return static_cast<User *>(U)->getIntrusiveOperands() + ARITY;
}
static unsigned operands(const User*) {
return ARITY;
@@ -70,10 +70,11 @@ struct VariadicOperandTraits {
static_assert(
!std::is_polymorphic<SubClass>::value,
"adding virtual methods to subclasses of User breaks use lists");
- return reinterpret_cast<Use*>(U) - static_cast<User*>(U)->getNumOperands();
+ return static_cast<User *>(U)->getIntrusiveOperands();
}
static Use *op_end(SubClass* U) {
- return reinterpret_cast<Use*>(U);
+ return static_cast<User *>(U)->getIntrusiveOperands() +
+ static_cast<User *>(U)->getNumOperands();
}
static unsigned operands(const User *U) {
return U->getNumOperands();
diff --git a/llvm/include/llvm/IR/User.h b/llvm/include/llvm/IR/User.h
index a9cf60151e5dc6..4436454499f9f2 100644
--- a/llvm/include/llvm/IR/User.h
+++ b/llvm/include/llvm/IR/User.h
@@ -44,11 +44,50 @@ struct OperandTraits;
class User : public Value {
template <unsigned>
friend struct HungoffOperandTraits;
+ template <typename SubClass, unsigned MINARITY>
+ friend struct VariadicOperandTraits;
+ template <typename SubClass, unsigned ARITY>
+ friend struct FixedNumOperandTraits;
LLVM_ATTRIBUTE_ALWAYS_INLINE static void *
allocateFixedOperandUser(size_t, unsigned, unsigned);
protected:
+ union Header {
+ // A `User` object, its `Header`, operands and descriptor are all allocated
+ // together (see `operator new`). Since the `User` object is after the other
+ // objects, we use the padding here to force `Header` to be the same size as
+ // a pointer so that the `User` object is aligned correctly.
+ std::size_t _padding;
+ struct Contents {
+ /// The number of operands in the subclass.
+ ///
+ /// This member is defined by this class, but not used for anything.
+ /// Subclasses can use it to store their number of operands, if they have
+ /// any.
+ ///
+ /// This is stored here to save space in User on 64-bit hosts. Since most
+ /// instances of Value have operands, 32-bit hosts aren't significantly
+ /// affected.
+ ///
+ /// Note, this should *NOT* be used directly by any class other than User.
+ /// User uses this value to find the Use list.
+ enum : unsigned { NumUserOperandsBits = 27 };
+ unsigned NumUserOperands : NumUserOperandsBits;
+ unsigned HasHungOffUses : 1;
+ unsigned HasDescriptor : 1;
+ } contents;
+ };
+ static_assert(sizeof(Header) == sizeof(void *),
+ "Header should be the same size as a pointer");
+
+ Header::Contents &getHeader() {
+ return (reinterpret_cast<Header *>(this) - 1)->contents;
+ }
+
+ const Header::Contents &getHeader() const {
+ return (reinterpret_cast<const Header *>(this) - 1)->contents;
+ }
/// Allocate a User with an operand pointer co-allocated.
///
/// This is used for subclasses which need to allocate a variable number
@@ -72,11 +111,12 @@ class User : public Value {
User(Type *ty, unsigned vty, Use *, unsigned NumOps)
: Value(ty, vty) {
- assert(NumOps < (1u << NumUserOperandsBits) && "Too many operands");
- NumUserOperands = NumOps;
+ assert(NumOps < (1u << Header::Contents::NumUserOperandsBits) &&
+ "Too many operands");
+ getHeader().NumUserOperands = NumOps;
// If we have hung off uses, then the operand list should initially be
// null.
- assert((!HasHungOffUses || !getOperandList()) &&
+ assert((!getHeader().HasHungOffUses || !getOperandList()) &&
"Error in initializing hung off uses for User");
}
@@ -139,40 +179,48 @@ class User : public Value {
private:
const Use *getHungOffOperands() const {
- return *(reinterpret_cast<const Use *const *>(this) - 1);
+ return *(reinterpret_cast<const Use *const *>(&getHeader()) - 1);
}
- Use *&getHungOffOperands() { return *(reinterpret_cast<Use **>(this) - 1); }
+ Use *&getHungOffOperands() {
+ return *(reinterpret_cast<Use **>(&getHeader()) - 1);
+ }
const Use *getIntrusiveOperands() const {
- return reinterpret_cast<const Use *>(this) - NumUserOperands;
+ auto &operandInfo = getHeader();
+ return reinterpret_cast<const Use *>(&operandInfo) -
+ operandInfo.NumUserOperands;
}
Use *getIntrusiveOperands() {
- return reinterpret_cast<Use *>(this) - NumUserOperands;
+ auto &operandInfo = getHeader();
+ return reinterpret_cast<Use *>(&operandInfo) - operandInfo.NumUserOperands;
}
void setOperandList(Use *NewList) {
- assert(HasHungOffUses &&
+ assert(getHeader().HasHungOffUses &&
"Setting operand list only required for hung off uses");
getHungOffOperands() = NewList;
}
public:
const Use *getOperandList() const {
- return HasHungOffUses ? getHungOffOperands() : getIntrusiveOperands();
+ return getHeader().HasHungOffUses ? getHungOffOperands()
+ : getIntrusiveOperands();
}
Use *getOperandList() {
return const_cast<Use *>(static_cast<const User *>(this)->getOperandList());
}
+ unsigned getNumOperands() const { return getHeader().NumUserOperands; }
+
Value *getOperand(unsigned i) const {
- assert(i < NumUserOperands && "getOperand() out of range!");
+ assert(i < getNumOperands() && "getOperand() out of range!");
return getOperandList()[i];
}
void setOperand(unsigned i, Value *Val) {
- assert(i < NumUserOperands && "setOperand() out of range!");
+ assert(i < getNumOperands() && "setOperand() out of range!");
assert((!isa<Constant>((const Value*)this) ||
isa<GlobalValue>((const Value*)this)) &&
"Cannot mutate a constant with setOperand!");
@@ -180,16 +228,14 @@ class User : public Value {
}
const Use &getOperandUse(unsigned i) const {
- assert(i < NumUserOperands && "getOperandUse() out of range!");
+ assert(i < getNumOperands() && "getOperandUse() out of range!");
return getOperandList()[i];
}
Use &getOperandUse(unsigned i) {
- assert(i < NumUserOperands && "getOperandUse() out of range!");
+ assert(i < getNumOperands() && "getOperandUse() out of range!");
return getOperandList()[i];
}
- unsigned getNumOperands() const { return NumUserOperands; }
-
/// Returns the descriptor co-allocated with this User instance.
ArrayRef<const uint8_t> getDescriptor() const;
@@ -206,16 +252,18 @@ class User : public Value {
/// 1 operand before delete.
void setGlobalVariableNumOperands(unsigned NumOps) {
assert(NumOps <= 1 && "GlobalVariable can only have 0 or 1 operands");
- NumUserOperands = NumOps;
+ getHeader().NumUserOperands = NumOps;
}
/// Subclasses with hung off uses need to manage the operand count
/// themselves. In these instances, the operand count isn't used to find the
/// OperandList, so there's no issue in having the operand count change.
void setNumHungOffUseOperands(unsigned NumOps) {
- assert(HasHungOffUses && "Must have hung off uses to use this method");
- assert(NumOps < (1u << NumUserOperandsBits) && "Too many operands");
- NumUserOperands = NumOps;
+ assert(getHeader().HasHungOffUses &&
+ "Must have hung off uses to use this method");
+ assert(NumOps < (1u << Header::Contents::NumUserOperandsBits) &&
+ "Too many operands");
+ getHeader().NumUserOperands = NumOps;
}
/// A droppable user is a user for which uses can be dropped without affecting
@@ -233,11 +281,9 @@ class User : public Value {
op_iterator op_begin() { return getOperandList(); }
const_op_iterator op_begin() const { return getOperandList(); }
- op_iterator op_end() {
- return getOperandList() + NumUserOperands;
- }
+ op_iterator op_end() { return getOperandList() + getNumOperands(); }
const_op_iterator op_end() const {
- return getOperandList() + NumUserOperands;
+ return getOperandList() + getNumOperands();
}
op_range operands() {
return op_range(op_begin(), op_end());
diff --git a/llvm/include/llvm/IR/Value.h b/llvm/include/llvm/IR/Value.h
index 945081b77e9536..43eca2f428d8ad 100644
--- a/llvm/include/llvm/IR/Value.h
+++ b/llvm/include/llvm/IR/Value.h
@@ -92,27 +92,10 @@ class Value {
unsigned short SubclassData;
protected:
- /// The number of operands in the subclass.
- ///
- /// This member is defined by this class, but not used for anything.
- /// Subclasses can use it to store their number of operands, if they have
- /// any.
- ///
- /// This is stored here to save space in User on 64-bit hosts. Since most
- /// instances of Value have operands, 32-bit hosts aren't significantly
- /// affected.
- ///
- /// Note, this should *NOT* be used directly by any class other than User.
- /// User uses this value to find the Use list.
- enum : unsigned { NumUserOperandsBits = 27 };
- unsigned NumUserOperands : NumUserOperandsBits;
-
// Use the same type as the bitfield above so that MSVC will pack them.
unsigned IsUsedByMD : 1;
unsigned HasName : 1;
unsigned HasMetadata : 1; // Has metadata attached to this?
- unsigned HasHungOffUses : 1;
- unsigned HasDescriptor : 1;
private:
Type *VTy;
diff --git a/llvm/lib/IR/User.cpp b/llvm/lib/IR/User.cpp
index 637af7aaa24530..959447f95066a2 100644
--- a/llvm/lib/IR/User.cpp
+++ b/llvm/lib/IR/User.cpp
@@ -48,7 +48,7 @@ bool User::replaceUsesOfWith(Value *From, Value *To) {
//===----------------------------------------------------------------------===//
void User::allocHungoffUses(unsigned N, bool IsPhi) {
- assert(HasHungOffUses && "alloc must have hung off uses");
+ assert(getHeader().HasHungOffUses && "alloc must have hung off uses");
static_assert(alignof(Use) >= alignof(BasicBlock *),
"Alignment is insufficient for 'hung-off-uses' pieces");
@@ -65,7 +65,7 @@ void User::allocHungoffUses(unsigned N, bool IsPhi) {
}
void User::growHungoffUses(unsigned NewNumUses, bool IsPhi) {
- assert(HasHungOffUses && "realloc must have hung off uses");
+ assert(getHeader().HasHungOffUses && "realloc must have hung off uses");
unsigned OldNumUses = getNumOperands();
@@ -102,8 +102,8 @@ ArrayRef<const uint8_t> User::getDescriptor() const {
}
MutableArrayRef<uint8_t> User::getDescriptor() {
- assert(HasDescriptor && "Don't call otherwise!");
- assert(!HasHungOffUses && "Invariant!");
+ assert(getHeader().HasDescriptor && "Don't call otherwise!");
+ assert(!getHeader().HasHungOffUses && "Invariant!");
auto *DI = reinterpret_cast<DescriptorInfo *>(getIntrusiveOperands()) - 1;
assert(DI->SizeInBytes != 0 && "Should not have had a descriptor otherwise!");
@@ -122,7 +122,8 @@ bool User::isDroppable() const {
void *User::allocateFixedOperandUser(size_t Size, unsigned Us,
unsigned DescBytes) {
- assert(Us < (1u << NumUserOperandsBits) && "Too many operands");
+ assert(Us < (1u << Header::Contents::NumUserOperandsBits) &&
+ "Too many operands");
static_assert(sizeof(DescriptorInfo) % sizeof(void *) == 0, "Required below");
@@ -131,14 +132,20 @@ void *User::allocateFixedOperandUser(size_t Size, unsigned Us,
assert(DescBytesToAllocate % sizeof(void *) == 0 &&
"We need this to satisfy alignment constraints for Uses");
- uint8_t *Storage = static_cast<uint8_t *>(
- ::operator new(Size + sizeof(Use) * Us + DescBytesToAllocate));
+ uint8_t *Storage = static_cast<uint8_t *>(::operator new(
+ Size + sizeof(Header) + sizeof(Use) * Us + DescBytesToAllocate));
Use *Start = reinterpret_cast<Use *>(Storage + DescBytesToAllocate);
Use *End = Start + Us;
- User *Obj = reinterpret_cast<User*>(End);
- Obj->NumUserOperands = Us;
- Obj->HasHungOffUses = false;
- Obj->HasDescriptor = DescBytes != 0;
+ Header *OI = reinterpret_cast<Header *>(End);
+ User *Obj = reinterpret_cast<User *>(OI + 1);
+ Obj->getHeader().NumUserOperands = Us;
+ Obj->getHeader().HasHungOffUses = false;
+ Obj->getHeader().HasDescriptor = DescBytes != 0;
+ assert(&(OI->contents) == &Obj->getHeader() &&
+ "getHeader() is returning the wrong location");
+ assert(Start == Obj->getIntrusiveOperands() &&
+ "getIntrusiveOperands() is returning the wrong location");
+
for (; Start != End; Start++)
new (Start) Use(Obj);
@@ -160,12 +167,17 @@ void *User::operator new(size_t Size, unsigned Us, unsigned DescBytes) {
void *User::operator new(size_t Size) {
// Allocate space for a single Use*
- void *Storage = ::operator new(Size + sizeof(Use *));
+ void *Storage = ::operator new(Size + sizeof(Header) + sizeof(Use *));
Use **HungOffOperandList = static_cast<Use **>(Storage);
- User *Obj = reinterpret_cast<User *>(HungOffOperandList + 1);
- Obj->NumUserOperands = 0;
- Obj->HasHungOffUses = true;
- Obj->HasDescriptor = false;
+ Header *OI = reinterpret_cast<Header *>(HungOffOperandList + 1);
+ User *Obj = reinterpret_cast<User *>(OI + 1);
+ Obj->getHeader().NumUserOperands = 0;
+ Obj->getHeader().HasHungOffUses = true;
+ Obj->getHeader().HasDescriptor = false;
+ assert(&(OI->contents) == &Obj->getHeader() &&
+ "getHeader() is returning the wrong location");
+ assert(HungOffOperandList == &Obj->getHungOffOperands() &&
+ "getHungOffOperands() is returning the wrong location");
*HungOffOperandList = nullptr;
return Obj;
}
@@ -180,24 +192,24 @@ LLVM_NO_SANITIZE_MEMORY_ATTRIBUTE void User::operator delete(void *Usr) {
// Hung off uses use a single Use* before the User, while other subclasses
// use a Use[] allocated prior to the user.
User *Obj = static_cast<User *>(Usr);
- if (Obj->HasHungOffUses) {
- assert(!Obj->HasDescriptor && "not supported!");
+ if (Obj->getHeader().HasHungOffUses) {
+ assert(!Obj->getHeader().HasDescriptor && "not supported!");
- Use **HungOffOperandList = static_cast<Use **>(Usr) - 1;
+ Use **HungOffOperandList = &(Obj->getHungOffOperands());
// drop the hung off uses.
- Use::zap(*HungOffOperandList, *HungOffOperandList + Obj->NumUserOperands,
+ Use::zap(*HungOffOperandList, *HungOffOperandList + Obj->getNumOperands(),
/* Delete */ true);
::operator delete(HungOffOperandList);
- } else if (Obj->HasDescriptor) {
- Use *UseBegin = static_cast<Use *>(Usr) - Obj->NumUserOperands;
- Use::zap(UseBegin, UseBegin + Obj->NumUserOperands, /* Delete */ false);
+ } else if (Obj->getHeader().HasDescriptor) {
+ Use *UseBegin = Obj->getIntrusiveOperands();
+ Use::zap(UseBegin, UseBegin + Obj->getNumOperands(), /* Delete */ false);
auto *DI = reinterpret_cast<DescriptorInfo *>(UseBegin) - 1;
uint8_t *Storage = reinterpret_cast<uint8_t *>(DI) - DI->SizeInBytes;
::operator delete(Storage);
} else {
- Use *Storage = static_cast<Use *>(Usr) - Obj->NumUserOperands;
- Use::zap(Storage, Storage + Obj->NumUserOperands,
+ Use *Storage = Obj->getIntrusiveOperands();
+ Use::zap(Storage, Storage + Obj->getNumOperands(),
/* Delete */ false);
::operator delete(Storage);
}
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index b2ee75811fbb7d..b8259b75a8efa0 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -52,8 +52,8 @@ static inline Type *checkType(Type *Ty) {
Value::Value(Type *ty, unsigned scid)
: SubclassID(scid), HasValueHandle(0), SubclassOptionalData(0),
- SubclassData(0), NumUserOperands(0), IsUsedByMD(false), HasName(false),
- HasMetadata(false), VTy(checkType(ty)), UseList(nullptr) {
+ SubclassData(0), IsUsedByMD(false), HasName(false), HasMetadata(false),
+ VTy(checkType(ty)), UseList(nullptr) {
static_assert(ConstantFirstVal == 0, "!(SubclassID < ConstantFirstVal)");
// FIXME: Why isn't this in the subclass gunk??
// Note, we cannot call isa<CallInst> before the CallInst has been
diff --git a/llvm/utils/LLVMVisualizers/llvm.natvis b/llvm/utils/LLVMVisualizers/llvm.natvis
index d83ae8013c51e2..a968eb8f7773a4 100644
--- a/llvm/utils/LLVMVisualizers/llvm.natvis
+++ b/llvm/utils/LLVMVisualizers/llvm.natvis
@@ -329,8 +329,7 @@ For later versions of Visual Studio, no setup is required.
<DisplayString Condition="HasName">$(Type) {*VTy} {this->getName()} {SubclassData}</DisplayString>
<DisplayString Condition="!HasName">$(Type) {*VTy} anon {SubclassData}</DisplayString>
<Expand>
- <Item Name="[Inst]" Condition="SubclassID > InstructionVal">(Instruction*)this</Item>
- <Item Name="Operands">(User*)this</Item>
+ <Item Name="[Inst]" Condition="SubclassID > InstructionVal">(llvm::Instruction*)this</Item>
<LinkedListItems>
<HeadPointer>UseList</HeadPointer>
<NextPointer>Next</NextPointer>
@@ -357,15 +356,15 @@ For later versions of Visual Studio, no setup is required.
<DisplayString Condition="HasName">$(Type) {*VTy} {this->getName()} {SubclassData}</DisplayString>
<DisplayString Condition="!HasName">$(Type) {*VTy} anon {SubclassData}</DisplayString>
<Expand>
- <Item Name="[Value]">(Value*)this,nd</Item>
+ <Item Name="[Value]">(llvm::User*)this,nd</Item>
<Item Name="[Type]">*VTy</Item>
- <ArrayItems Condition="!HasHungOffUses">
- <Size>NumUserOperands</Size>
- <ValuePointer>(llvm::Use*)this - NumUserOperands</ValuePointer>
+ <ArrayItems Condition="!((llvm::User::Header*)this - 1)->contents.HasHungOffUses">
+ <Size>((llvm::User::Header*)this - 1)->contents.NumUserOperands</Size>
+ <ValuePointer>(llvm::Use*)((llvm::User::Header*)this - 1) - ((llvm::User::Header*)this - 1)->contents.NumUserOperands</ValuePointer>
</ArrayItems>
- <ArrayItems Condition="HasHungOffUses">
- <Size>NumUserOperands</Size>
- <ValuePointer>*((llvm::Use**)this - 1)</ValuePointer>
+ <ArrayItems Condition="((llvm::User::Header*)this - 1)->contents.HasHungOffUses">
+ <Size>((llvm::User::Header*)this - 1)->contents.NumUserOperands</Size>
+ <ValuePointer>*((llvm::Use**)((llvm::User::Header*)this - 1) - 1)</ValuePointer>
</ArrayItems>
</Expand>
</Type>
@@ -373,7 +372,7 @@ For later versions of Visual Studio, no setup is required.
<Type Name="llvm::Instruction">
<DisplayString>{getOpcodeName(SubclassID - InstructionVal)}</DisplayString>
<Expand>
- <Item Name="[User]">(User*)this,nd</Item>
+ <Item Name="[User]">(llvm::User*)this,nd</Item>
</Expand>
</Type>
More information about the llvm-commits
mailing list