Skip to content

[libc] Fix malloc Block alignment issue on riscv32 #117815

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 6 commits into from
Jan 16, 2025
Merged

[libc] Fix malloc Block alignment issue on riscv32 #117815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
33fdf85
[libc] Fix malloc Block alignment issue on riscv32
mysterymath Jan 8, 2025
bde7ae4
Add more usable space alignment assertions
mysterymath Jan 14, 2025
7945755
Address overflow cases in init
mysterymath Jan 14, 2025
9e71857
Handle overflow cases in split
mysterymath Jan 14, 2025
43814e2
Revert change away from kN
mysterymath Jan 14, 2025
1c22ad1
Clarify max size calculaton comment
mysterymath Jan 15, 2025
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
1 change: 1 addition & 0 deletions libc/src/__support/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ add_header_library(
libc.src.__support.CPP.optional
libc.src.__support.CPP.span
libc.src.__support.CPP.type_traits
libc.src.__support.math_extras
)

add_object_library(
Expand Down
253 changes: 123 additions & 130 deletions libc/src/__support/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "src/__support/CPP/type_traits.h"
#include "src/__support/libc_assert.h"
#include "src/__support/macros/config.h"
#include "src/__support/math_extras.h"

#include <stdint.h>

Expand All @@ -40,24 +41,10 @@ LIBC_INLINE constexpr size_t align_down(size_t value, size_t alignment) {
return (value / alignment) * alignment;
}

/// Returns the value rounded down to the nearest multiple of alignment.
template <typename T>
LIBC_INLINE constexpr T *align_down(T *value, size_t alignment) {
return reinterpret_cast<T *>(
align_down(reinterpret_cast<size_t>(value), alignment));
}

/// Returns the value rounded up to the nearest multiple of alignment.
/// Returns the value rounded up to the nearest multiple of alignment. May wrap
/// around.
LIBC_INLINE constexpr size_t align_up(size_t value, size_t alignment) {
__builtin_add_overflow(value, alignment - 1, &value);
return align_down(value, alignment);
}

/// Returns the value rounded up to the nearest multiple of alignment.
template <typename T>
LIBC_INLINE constexpr T *align_up(T *value, size_t alignment) {
return reinterpret_cast<T *>(
align_up(reinterpret_cast<size_t>(value), alignment));
return align_down(value + alignment - 1, alignment);
}

using ByteSpan = cpp::span<LIBC_NAMESPACE::cpp::byte>;
Expand All @@ -68,8 +55,8 @@ using cpp::optional;
/// The blocks store their offsets to the previous and next blocks. The latter
/// is also the block's size.
///
/// Blocks will always be aligned to a `ALIGNMENT` boundary. Block sizes will
/// always be rounded up to a multiple of `ALIGNMENT`.
/// All blocks have their usable space aligned to some multiple of max_align_t.
/// This also implies that block outer sizes are aligned to max_align_t.
///
/// As an example, the diagram below represents two contiguous `Block`s. The
/// indices indicate byte offsets:
Expand Down Expand Up @@ -129,8 +116,9 @@ class Block {
Block(const Block &other) = delete;
Block &operator=(const Block &other) = delete;

/// Creates the first block for a given memory region, followed by a sentinel
/// last block. Returns the first block.
/// Initializes a given memory region into a first block and a sentinel last
/// block. Returns the first block, which has its usable space aligned to
/// max_align_t.
Comment on lines +119 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a LIBC_ASSERT to the end of init that checks the alignment?

Copy link
Contributor Author

@mysterymath mysterymath Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is an invariant for all Blocks, so I've checked for this in the block constructor. init necessarily calls out to that constructor, so that should be sufficient to establish it for init.

static optional<Block *> init(ByteSpan region);

/// @returns A pointer to a `Block`, given a pointer to the start of the
Expand Down Expand Up @@ -186,11 +174,19 @@ class Block {
}

/// @returns A pointer to the usable space inside this block.
///
/// Aligned to some multiple of max_align_t.
LIBC_INLINE cpp::byte *usable_space() {
return reinterpret_cast<cpp::byte *>(this) + BLOCK_OVERHEAD;
auto *s = reinterpret_cast<cpp::byte *>(this) + BLOCK_OVERHEAD;
LIBC_ASSERT(reinterpret_cast<uintptr_t>(s) % alignof(max_align_t) == 0 &&
"usable space must be aligned to a multiple of max_align_t");
return s;
}
LIBC_INLINE const cpp::byte *usable_space() const {
return reinterpret_cast<const cpp::byte *>(this) + BLOCK_OVERHEAD;
const auto *s = reinterpret_cast<const cpp::byte *>(this) + BLOCK_OVERHEAD;
LIBC_ASSERT(reinterpret_cast<uintptr_t>(s) % alignof(max_align_t) == 0 &&
"usable space must be aligned to a multiple of max_align_t");
Copy link
Member

Choose a reason for hiding this comment

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

llvm-project/libc/src/__support/block.h:188: Assertion failed: 'reinterpret_cast<uintptr_t>(s) % alignof(max_align_t) == 0 && "usable space must be aligned to a multiple of max_align_t"' in function: 'const cpp::byte *__llvm_libc_20_0_0_git::Block::usable_space() const'

return s;
}

// @returns The region of memory the block manages, including the header.
Expand All @@ -201,11 +197,12 @@ class Block {
/// Attempts to split this block.
///
/// If successful, the block will have an inner size of at least
/// `new_inner_size`, rounded to ensure that the split point is on an
/// ALIGNMENT boundary. The remaining space will be returned as a new block.
/// Note that the prev_ field of the next block counts as part of the inner
/// size of the returnd block.
optional<Block *> split(size_t new_inner_size);
/// `new_inner_size`. The remaining space will be returned as a new block,
/// with usable space aligned to `usable_space_alignment`. Note that the prev_
/// field of the next block counts as part of the inner size of the block.
/// `usable_space_alignment` must be a multiple of max_align_t.
optional<Block *> split(size_t new_inner_size,
size_t usable_space_alignment = alignof(max_align_t));

/// Merges this block with the one that comes after it.
bool merge_next();
Expand Down Expand Up @@ -248,46 +245,56 @@ class Block {
/// nullptr.
LIBC_INLINE void mark_last() { next_ |= LAST_MASK; }

LIBC_INLINE constexpr Block(size_t outer_size) : next_(outer_size) {
LIBC_INLINE Block(size_t outer_size) : next_(outer_size) {
LIBC_ASSERT(outer_size % ALIGNMENT == 0 && "block sizes must be aligned");
LIBC_ASSERT(is_usable_space_aligned(alignof(max_align_t)) &&
"usable space must be aligned to a multiple of max_align_t");
}

LIBC_INLINE bool is_usable_space_aligned(size_t alignment) const {
return reinterpret_cast<uintptr_t>(usable_space()) % alignment == 0;
}

/// @returns The new inner size of this block that would give the usable
/// space of the next block the given alignment.
LIBC_INLINE size_t padding_for_alignment(size_t alignment) const {
if (is_usable_space_aligned(alignment))
// Returns the minimum inner size necessary for a block of that size to
// always be able to allocate at the given size and alignment.
//
// Returns 0 if there is no such size.
LIBC_INLINE static size_t min_size_for_allocation(size_t alignment,
size_t size) {
LIBC_ASSERT(alignment >= alignof(max_align_t) &&
alignment % alignof(max_align_t) == 0 &&
"alignment must be multiple of max_align_t");

if (alignment == alignof(max_align_t))
return size;

// We must create a new block inside this one (splitting). This requires a
// block header in addition to the requested size.
if (add_overflow(size, sizeof(Block), size))
return 0;

// We need to ensure we can always split this block into a "padding" block
// and the aligned block. To do this, we need enough extra space for at
// least one block.
//
// |block |usable_space |
// |........|......................................|
// ^
// Alignment requirement
// Beyond that, padding space may need to remain in this block to ensure
// that the usable space of the next block is aligned.
//
// Consider a position P of some lesser alignment, L, with maximal distance
// to the next position of some greater alignment, G, where G is a multiple
// of L. P must be one L unit past a G-aligned point. If it were one L-unit
// earlier, its distance would be zero. If it were one L-unit later, its
// distance would not be maximal. If it were not some integral number of L
// units away, it would not be L-aligned.
//
// |block |space |block |usable_space |
// |........|........|........|....................|
// ^
// Alignment requirement
// So the maximum distance would be G - L. As a special case, if L is 1
// (unaligned), the max distance is G - 1.
//
alignment = cpp::max(alignment, ALIGNMENT);
uintptr_t start = reinterpret_cast<uintptr_t>(usable_space());
uintptr_t next_usable_space = align_up(start + BLOCK_OVERHEAD, alignment);
uintptr_t next_block = next_usable_space - BLOCK_OVERHEAD;
return next_block - start + sizeof(prev_);
// This block's usable space is aligned to max_align_t >= Block. With zero
// padding, the next block's usable space is sizeof(Block) past it, which is
// a point aligned to Block. Thus the max padding needed is alignment -
// alignof(Block).
if (add_overflow(size, alignment - alignof(Block), size))
return 0;
return size;
}

// Check that we can `allocate` a block with a given alignment and size from
// this existing block.
bool can_allocate(size_t alignment, size_t size) const;

// This is the return type for `allocate` which can split one block into up to
// three blocks.
struct BlockInfo {
Expand All @@ -309,21 +316,31 @@ class Block {
Block *next;
};

// Divide a block into up to 3 blocks according to `BlockInfo`. This should
// only be called if `can_allocate` returns true.
// Divide a block into up to 3 blocks according to `BlockInfo`. Behavior is
// undefined if allocation is not possible for the given size and alignment.
static BlockInfo allocate(Block *block, size_t alignment, size_t size);

// These two functions may wrap around.
LIBC_INLINE static uintptr_t next_possible_block_start(
uintptr_t ptr, size_t usable_space_alignment = alignof(max_align_t)) {
return align_up(ptr + sizeof(Block), usable_space_alignment) -
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be overthinking here, but is it possible for ptr + sizeof(Block) causing next_possible_block_start to also overflow? I imagine it's very unlikely, but I'm thinking it's possible through Block::split if min_outer_size is large enough there(?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to disregard this since this is probably me nitpicking on potential overflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These conveniently return the correct answer in modulo arithmetic, so I modified their uses to detect overflow and react accordingly.

sizeof(Block);
}
LIBC_INLINE static uintptr_t prev_possible_block_start(
uintptr_t ptr, size_t usable_space_alignment = alignof(max_align_t)) {
return align_down(ptr, usable_space_alignment) - sizeof(Block);
}

private:
/// Construct a block to represent a span of bytes. Overwrites only enough
/// memory for the block header; the rest of the span is left alone.
LIBC_INLINE static Block *as_block(ByteSpan bytes) {
LIBC_ASSERT(reinterpret_cast<uintptr_t>(bytes.data()) % alignof(Block) ==
0 &&
"block start must be suitably aligned");
return ::new (bytes.data()) Block(bytes.size());
}

/// Like `split`, but assumes the caller has already checked to parameters to
/// ensure the split will succeed.
Block *split_impl(size_t new_inner_size);

/// Offset from this block to the previous block. 0 if this is the first
/// block. This field is only alive when the previous block is free;
/// otherwise, its memory is reused as part of the previous block's usable
Expand All @@ -343,81 +360,54 @@ class Block {
/// previous block is free.
/// * If the `last` flag is set, the block is the sentinel last block. It is
/// summarily considered used and has no next block.
} __attribute__((packed, aligned(cpp::max(alignof(max_align_t), size_t{4}))));
};

inline constexpr size_t Block::BLOCK_OVERHEAD =
align_up(sizeof(Block), ALIGNMENT);

LIBC_INLINE ByteSpan get_aligned_subspan(ByteSpan bytes, size_t alignment) {
if (bytes.data() == nullptr)
return ByteSpan();

auto unaligned_start = reinterpret_cast<uintptr_t>(bytes.data());
auto aligned_start = align_up(unaligned_start, alignment);
auto unaligned_end = unaligned_start + bytes.size();
auto aligned_end = align_down(unaligned_end, alignment);

if (aligned_end <= aligned_start)
return ByteSpan();

return bytes.subspan(aligned_start - unaligned_start,
aligned_end - aligned_start);
}

LIBC_INLINE
optional<Block *> Block::init(ByteSpan region) {
optional<ByteSpan> result = get_aligned_subspan(region, ALIGNMENT);
if (!result)
if (!region.data())
return {};

uintptr_t start = reinterpret_cast<uintptr_t>(region.data());
uintptr_t end = start + region.size();
if (end < start)
return {};

uintptr_t block_start = next_possible_block_start(start);
if (block_start < start)
return {};

region = result.value();
// Two blocks are allocated: a free block and a sentinel last block.
if (region.size() < 2 * BLOCK_OVERHEAD)
uintptr_t last_start = prev_possible_block_start(end);
if (last_start >= end)
return {};

if (cpp::numeric_limits<size_t>::max() < region.size())
if (block_start + sizeof(Block) > last_start)
return {};

Block *block = as_block(region.first(region.size() - BLOCK_OVERHEAD));
Block *last = as_block(region.last(BLOCK_OVERHEAD));
auto *last_start_ptr = reinterpret_cast<cpp::byte *>(last_start);
Block *block =
as_block({reinterpret_cast<cpp::byte *>(block_start), last_start_ptr});
Block *last = as_block({last_start_ptr, sizeof(Block)});
block->mark_free();
last->mark_last();
return block;
}

LIBC_INLINE
bool Block::can_allocate(size_t alignment, size_t size) const {
if (inner_size() < size)
return false;
if (is_usable_space_aligned(alignment))
return true;

// Alignment isn't met, so a padding block is needed. Determine amount of
// inner_size() consumed by the padding block.
size_t padding_size = padding_for_alignment(alignment) - sizeof(prev_);

// Check that there is room for the allocation in the following aligned block.
size_t aligned_inner_size = inner_size() - padding_size - BLOCK_OVERHEAD;
return size <= aligned_inner_size;
}

LIBC_INLINE
Block::BlockInfo Block::allocate(Block *block, size_t alignment, size_t size) {
LIBC_ASSERT(
block->can_allocate(alignment, size) &&
"Calls to this function for a given alignment and size should only be "
"done if `can_allocate` for these parameters returns true.");
LIBC_ASSERT(alignment % alignof(max_align_t) == 0 &&
"alignment must be a multiple of max_align_t");

BlockInfo info{block, /*prev=*/nullptr, /*next=*/nullptr};

if (!info.block->is_usable_space_aligned(alignment)) {
Block *original = info.block;
optional<Block *> maybe_aligned_block =
original->split(info.block->padding_for_alignment(alignment));
// The padding block has no minimum size requirement.
optional<Block *> maybe_aligned_block = original->split(0, alignment);
LIBC_ASSERT(maybe_aligned_block.has_value() &&
"This split should always result in a new block. The check in "
"`can_allocate` ensures that we have enough space here to make "
"two blocks.");
"it should always be possible to split for alignment");

if (Block *prev = original->prev_free()) {
// If there is a free block before this, we can merge the current one with
Expand All @@ -441,37 +431,40 @@ Block::BlockInfo Block::allocate(Block *block, size_t alignment, size_t size) {
}

LIBC_INLINE
optional<Block *> Block::split(size_t new_inner_size) {
optional<Block *> Block::split(size_t new_inner_size,
size_t usable_space_alignment) {
LIBC_ASSERT(usable_space_alignment % alignof(max_align_t) == 0 &&
"alignment must be a multiple of max_align_t");
if (used())
return {};
// The prev_ field of the next block is always available, so there is a
// minimum size to a block created through splitting.
if (new_inner_size < sizeof(prev_))
new_inner_size = sizeof(prev_);

size_t old_inner_size = inner_size();
new_inner_size =
align_up(new_inner_size - sizeof(prev_), ALIGNMENT) + sizeof(prev_);
if (old_inner_size < new_inner_size)
return {};

if (old_inner_size - new_inner_size < BLOCK_OVERHEAD)
// Compute the minimum outer size that produces a block of at least
// `new_inner_size`.
size_t min_outer_size = outer_size(cpp::max(new_inner_size, sizeof(prev_)));

uintptr_t start = reinterpret_cast<uintptr_t>(this);
uintptr_t next_block_start =
next_possible_block_start(start + min_outer_size, usable_space_alignment);
if (next_block_start < start)
return {};
size_t new_outer_size = next_block_start - start;
LIBC_ASSERT(new_outer_size % alignof(max_align_t) == 0 &&
"new size must be aligned to max_align_t");

return split_impl(new_inner_size);
}
if (outer_size() < new_outer_size ||
outer_size() - new_outer_size < sizeof(Block))
return {};

LIBC_INLINE
Block *Block::split_impl(size_t new_inner_size) {
size_t outer_size1 = outer_size(new_inner_size);
LIBC_ASSERT(outer_size1 % ALIGNMENT == 0 && "new size must be aligned");
ByteSpan new_region = region().subspan(outer_size1);
ByteSpan new_region = region().subspan(new_outer_size);
next_ &= ~SIZE_MASK;
next_ |= outer_size1;
next_ |= new_outer_size;

Block *new_block = as_block(new_region);
mark_free(); // Free status for this block is now stored in new_block.
new_block->next()->prev_ = new_region.size();

LIBC_ASSERT(new_block->is_usable_space_aligned(usable_space_alignment) &&
"usable space must have requested alignment");
return new_block;
}

Expand Down
Loading
Loading