[libc-commits] [libc] [NFC][libc][malloc] Refactor Block (PR #100445)
Daniel Thornburgh via libc-commits
libc-commits at lists.llvm.org
Wed Jul 24 13:25:27 PDT 2024
https://github.com/mysterymath updated https://github.com/llvm/llvm-project/pull/100445
>From 5e5d3be45cfb964f29cfee332029bc90c351908e Mon Sep 17 00:00:00 2001
From: Daniel Thornburgh <dthorn at google.com>
Date: Tue, 23 Jul 2024 15:55:14 -0700
Subject: [PATCH 1/2] [NFC][libc][malloc] Refactor Block
This decreases the surface area of the block implementation in
preparation for deeper changes to its implementation.
- Remove dead member functions.
- Remove last() check from next(), as described in its comment.
- Rework object lifetimes such that only block headers are actually
considered live. This simplifies their implementation.
- The allocated storage becomes live at the outer call to
malloc-family functions via a special case in the C++ standard.
- Add asserts for flag properties required by the implementation.
- Remove static from member functions that don't invalidate the block.
---
libc/src/__support/block.h | 147 +++++++------------
libc/src/__support/freelist_heap.h | 4 +-
libc/test/src/__support/block_test.cpp | 190 ++++---------------------
3 files changed, 76 insertions(+), 265 deletions(-)
diff --git a/libc/src/__support/block.h b/libc/src/__support/block.h
index 86cb4bd7ad582..442740441e9da 100644
--- a/libc/src/__support/block.h
+++ b/libc/src/__support/block.h
@@ -159,13 +159,10 @@ class Block {
return reinterpret_cast<const cpp::byte *>(this) + BLOCK_OVERHEAD;
}
- /// Marks the block as free and merges it with any free neighbors.
- ///
- /// This method is static in order to consume and replace the given block
- /// pointer. If neither member is free, the returned pointer will point to the
- /// original block. Otherwise, it will point to the new, larger block created
- /// by merging adjacent free blocks together.
- static void free(Block *&block);
+ // @returns The region of memory the block manages, including the header.
+ ByteSpan region() {
+ return {reinterpret_cast<cpp::byte *>(this), outer_size()};
+ }
/// Attempts to split this block.
///
@@ -176,16 +173,10 @@ class Block {
/// This method may fail if the remaining space is too small to hold a new
/// block. If this method fails for any reason, the original block is
/// unmodified.
- ///
- /// This method is static in order to consume and replace the given block
- /// pointer with a pointer to the new, smaller block.
- static optional<Block *> split(Block *&block, size_t new_inner_size);
+ optional<Block *> split(size_t new_inner_size);
/// Merges this block with the one that comes after it.
- ///
- /// This method is static in order to consume and replace the given block
- /// pointer with a pointer to the new, larger block.
- static bool merge_next(Block *&block);
+ bool merge_next();
/// Fetches the block immediately after this one.
///
@@ -206,20 +197,10 @@ class Block {
/// @endcode
Block *next() const;
- /// @copydoc `next`.
- static Block *next_block(const Block *block) {
- return block == nullptr ? nullptr : block->next();
- }
-
/// @returns The block immediately before this one, or a null pointer if this
/// is the first block.
Block *prev() const;
- /// @copydoc `prev`.
- static Block *prev_block(const Block *block) {
- return block == nullptr ? nullptr : block->prev();
- }
-
/// Indicates whether the block is in use.
///
/// @returns `true` if the block is in use or `false` if not.
@@ -242,9 +223,6 @@ class Block {
/// Marks this block as the last one in the chain.
constexpr void mark_last() { next_ |= LAST_MASK; }
- /// Clears the last bit from this block.
- void clear_last() { next_ &= ~LAST_MASK; }
-
/// @brief Checks if a block is valid.
///
/// @returns `true` if and only if the following conditions are met:
@@ -314,10 +292,8 @@ class Block {
static BlockInfo allocate(Block *block, size_t alignment, size_t size);
private:
- /// Consumes the block and returns as a span of bytes.
- static ByteSpan as_bytes(Block *&&block);
-
- /// Consumes the span of bytes and uses it to construct and return a block.
+ /// 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.
static Block *as_block(size_t prev_outer_size, ByteSpan bytes);
/// Returns a `BlockStatus` that is either VALID or indicates the reason why
@@ -329,7 +305,7 @@ class Block {
/// Like `split`, but assumes the caller has already checked to parameters to
/// ensure the split will succeed.
- static Block *split_impl(Block *&block, size_t new_inner_size);
+ Block *split_impl(size_t new_inner_size);
/// Offset from this block to the previous block. 0 if this is the first
/// block.
@@ -389,20 +365,6 @@ Block<OffsetType, kAlign>::init(ByteSpan region) {
return block;
}
-template <typename OffsetType, size_t kAlign>
-void Block<OffsetType, kAlign>::free(Block *&block) {
- if (block == nullptr)
- return;
-
- block->mark_free();
- Block *prev = block->prev();
-
- if (merge_next(prev))
- block = prev;
-
- merge_next(block);
-}
-
template <typename OffsetType, size_t kAlign>
bool Block<OffsetType, kAlign>::can_allocate(size_t alignment,
size_t size) const {
@@ -436,7 +398,7 @@ Block<OffsetType, kAlign>::allocate(Block *block, size_t alignment,
Block *original = info.block;
optional<Block *> maybe_aligned_block =
- Block::split(original, adjustment - BLOCK_OVERHEAD);
+ original->split(adjustment - BLOCK_OVERHEAD);
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 "
@@ -445,7 +407,7 @@ Block<OffsetType, kAlign>::allocate(Block *block, size_t alignment,
if (Block *prev = original->prev()) {
// If there is a block before this, we can merge the current one with the
// newly created one.
- merge_next(prev);
+ prev->merge_next();
} else {
// Otherwise, this was the very first block in the chain. Now we can make
// it the new first block.
@@ -459,7 +421,7 @@ Block<OffsetType, kAlign>::allocate(Block *block, size_t alignment,
}
// Now get a block for the requested size.
- if (optional<Block *> next = Block::split(info.block, size))
+ if (optional<Block *> next = info.block->split(size))
info.next = *next;
return info;
@@ -467,14 +429,11 @@ Block<OffsetType, kAlign>::allocate(Block *block, size_t alignment,
template <typename OffsetType, size_t kAlign>
optional<Block<OffsetType, kAlign> *>
-Block<OffsetType, kAlign>::split(Block *&block, size_t new_inner_size) {
- if (block == nullptr)
+Block<OffsetType, kAlign>::split(size_t new_inner_size) {
+ if (used())
return {};
- if (block->used())
- return {};
-
- size_t old_inner_size = block->inner_size();
+ size_t old_inner_size = inner_size();
new_inner_size = align_up(new_inner_size, ALIGNMENT);
if (old_inner_size < new_inner_size)
return {};
@@ -482,61 +441,58 @@ Block<OffsetType, kAlign>::split(Block *&block, size_t new_inner_size) {
if (old_inner_size - new_inner_size < BLOCK_OVERHEAD)
return {};
- return split_impl(block, new_inner_size);
+ return split_impl(new_inner_size);
}
template <typename OffsetType, size_t kAlign>
Block<OffsetType, kAlign> *
-Block<OffsetType, kAlign>::split_impl(Block *&block, size_t new_inner_size) {
- size_t prev_outer_size = block->prev_;
+Block<OffsetType, kAlign>::split_impl(size_t new_inner_size) {
size_t outer_size1 = new_inner_size + BLOCK_OVERHEAD;
- bool is_last = block->last();
- ByteSpan bytes = as_bytes(cpp::move(block));
- Block *block1 = as_block(prev_outer_size, bytes.subspan(0, outer_size1));
- Block *block2 = as_block(outer_size1, bytes.subspan(outer_size1));
-
- if (is_last)
- block2->mark_last();
- else
- block2->next()->prev_ = block2->next_;
-
- block = cpp::move(block1);
- return block2;
+ bool is_last = last();
+ ByteSpan new_region = region().subspan(outer_size1);
+ LIBC_ASSERT(!used() && "used blocks cannot be split");
+ // The low order bits of outer_size1 should both be zero, and is the correct
+ // value for the flags is false.
+ next_ = outer_size1;
+ LIBC_ASSERT(!used() && !last() && "incorrect first split flags");
+ Block *new_block = as_block(outer_size1, new_region);
+
+ if (is_last) {
+ new_block->mark_last();
+ } else {
+ // The two flags are both false, so next_ is a plain size.
+ LIBC_ASSERT(!new_block->used() && !new_block->last() &&
+ "flags disrupt use of size");
+ new_block->next()->prev_ = new_block->next_;
+ }
+ return new_block;
}
template <typename OffsetType, size_t kAlign>
-bool Block<OffsetType, kAlign>::merge_next(Block *&block) {
- if (block == nullptr)
+bool Block<OffsetType, kAlign>::merge_next() {
+ if (last())
return false;
- if (block->last())
+ if (used() || next()->used())
return false;
- Block *next = block->next();
- if (block->used() || next->used())
- return false;
+ // Extend the size and copy the last() flag from the next block to this one.
+ next_ &= SIZE_MASK;
+ next_ += next()->next_;
- size_t prev_outer_size = block->prev_;
- bool is_last = next->last();
- ByteSpan prev_bytes = as_bytes(cpp::move(block));
- ByteSpan next_bytes = as_bytes(cpp::move(next));
- size_t outer_size = prev_bytes.size() + next_bytes.size();
- cpp::byte *merged = ::new (prev_bytes.data()) cpp::byte[outer_size];
- block = as_block(prev_outer_size, ByteSpan(merged, outer_size));
-
- if (is_last)
- block->mark_last();
- else
- block->next()->prev_ = block->next_;
+ if (!last()) {
+ // The two flags are both false, so next_ is a plain size.
+ LIBC_ASSERT(!used() && !last() && "flags disrupt use of size");
+ next()->prev_ = next_;
+ }
return true;
}
template <typename OffsetType, size_t kAlign>
Block<OffsetType, kAlign> *Block<OffsetType, kAlign>::next() const {
- uintptr_t addr =
- last() ? 0 : reinterpret_cast<uintptr_t>(this) + outer_size();
- return reinterpret_cast<Block *>(addr);
+ return reinterpret_cast<Block *>(reinterpret_cast<uintptr_t>(this) +
+ outer_size());
}
template <typename OffsetType, size_t kAlign>
@@ -555,13 +511,6 @@ constexpr Block<OffsetType, kAlign>::Block(size_t prev_outer_size,
next_ = outer_size;
}
-template <typename OffsetType, size_t kAlign>
-ByteSpan Block<OffsetType, kAlign>::as_bytes(Block *&&block) {
- size_t block_size = block->outer_size();
- cpp::byte *bytes = new (cpp::move(block)) cpp::byte[block_size];
- return {bytes, block_size};
-}
-
template <typename OffsetType, size_t kAlign>
Block<OffsetType, kAlign> *
Block<OffsetType, kAlign>::as_block(size_t prev_outer_size, ByteSpan bytes) {
diff --git a/libc/src/__support/freelist_heap.h b/libc/src/__support/freelist_heap.h
index ce4f14b431585..dc2fe43e44af2 100644
--- a/libc/src/__support/freelist_heap.h
+++ b/libc/src/__support/freelist_heap.h
@@ -196,12 +196,12 @@ template <size_t NUM_BUCKETS> void FreeListHeap<NUM_BUCKETS>::free(void *ptr) {
// Remove from freelist and merge
freelist_.remove_chunk(block_to_span(prev));
chunk_block = chunk_block->prev();
- BlockType::merge_next(chunk_block);
+ chunk_block->merge_next();
}
if (next != nullptr && !next->used()) {
freelist_.remove_chunk(block_to_span(next));
- BlockType::merge_next(chunk_block);
+ chunk_block->merge_next();
}
// Add back to the freelist
freelist_.add_chunk(block_to_span(chunk_block));
diff --git a/libc/test/src/__support/block_test.cpp b/libc/test/src/__support/block_test.cpp
index 04704482b5147..fcd9d06a5a8d2 100644
--- a/libc/test/src/__support/block_test.cpp
+++ b/libc/test/src/__support/block_test.cpp
@@ -52,7 +52,6 @@ TEST_FOR_EACH_BLOCK_TYPE(CanCreateSingleAlignedBlock) {
EXPECT_EQ(block->outer_size(), kN);
EXPECT_EQ(block->inner_size(), kN - BlockType::BLOCK_OVERHEAD);
EXPECT_EQ(block->prev(), static_cast<BlockType *>(nullptr));
- EXPECT_EQ(block->next(), static_cast<BlockType *>(nullptr));
EXPECT_FALSE(block->used());
EXPECT_TRUE(block->last());
}
@@ -96,7 +95,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CanSplitBlock) {
ASSERT_TRUE(result.has_value());
auto *block1 = *result;
- result = BlockType::split(block1, kSplitN);
+ result = block1->split(kSplitN);
ASSERT_TRUE(result.has_value());
auto *block2 = *result;
@@ -128,7 +127,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CanSplitBlockUnaligned) {
split_addr += alignof(BlockType) - (split_addr % alignof(BlockType));
uintptr_t split_len = split_addr - (uintptr_t)&bytes;
- result = BlockType::split(block1, kSplitN);
+ result = block1->split(kSplitN);
ASSERT_TRUE(result.has_value());
BlockType *block2 = *result;
@@ -161,11 +160,11 @@ TEST_FOR_EACH_BLOCK_TYPE(CanSplitMidBlock) {
ASSERT_TRUE(result.has_value());
BlockType *block1 = *result;
- result = BlockType::split(block1, kSplit1);
+ result = block1->split(kSplit1);
ASSERT_TRUE(result.has_value());
BlockType *block2 = *result;
- result = BlockType::split(block1, kSplit2);
+ result = block1->split(kSplit2);
ASSERT_TRUE(result.has_value());
BlockType *block3 = *result;
@@ -184,7 +183,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CannotSplitTooSmallBlock) {
ASSERT_TRUE(result.has_value());
BlockType *block = *result;
- result = BlockType::split(block, kSplitN);
+ result = block->split(kSplitN);
ASSERT_FALSE(result.has_value());
}
@@ -197,13 +196,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CannotSplitBlockWithoutHeaderSpace) {
ASSERT_TRUE(result.has_value());
BlockType *block = *result;
- result = BlockType::split(block, kSplitN);
- ASSERT_FALSE(result.has_value());
-}
-
-TEST_FOR_EACH_BLOCK_TYPE(CannotSplitNull) {
- BlockType *block = nullptr;
- auto result = BlockType::split(block, 1);
+ result = block->split(kSplitN);
ASSERT_FALSE(result.has_value());
}
@@ -216,7 +209,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CannotMakeBlockLargerInSplit) {
ASSERT_TRUE(result.has_value());
BlockType *block = *result;
- result = BlockType::split(block, block->inner_size() + 1);
+ result = block->split(block->inner_size() + 1);
ASSERT_FALSE(result.has_value());
}
@@ -229,8 +222,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CannotMakeSecondBlockLargerInSplit) {
ASSERT_TRUE(result.has_value());
BlockType *block = *result;
- result = BlockType::split(block, block->inner_size() -
- BlockType::BLOCK_OVERHEAD + 1);
+ result = block->split(block->inner_size() - BlockType::BLOCK_OVERHEAD + 1);
ASSERT_FALSE(result.has_value());
}
@@ -243,7 +235,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CanMakeZeroSizeFirstBlock) {
ASSERT_TRUE(result.has_value());
BlockType *block = *result;
- result = BlockType::split(block, 0);
+ result = block->split(0);
ASSERT_TRUE(result.has_value());
EXPECT_EQ(block->inner_size(), static_cast<size_t>(0));
}
@@ -257,8 +249,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CanMakeZeroSizeSecondBlock) {
ASSERT_TRUE(result.has_value());
BlockType *block1 = *result;
- result = BlockType::split(block1,
- block1->inner_size() - BlockType::BLOCK_OVERHEAD);
+ result = block1->split(block1->inner_size() - BlockType::BLOCK_OVERHEAD);
ASSERT_TRUE(result.has_value());
BlockType *block2 = *result;
@@ -293,7 +284,7 @@ TEST_FOR_EACH_BLOCK_TYPE(CannotSplitUsedBlock) {
BlockType *block = *result;
block->mark_used();
- result = BlockType::split(block, kSplitN);
+ result = block->split(kSplitN);
ASSERT_FALSE(result.has_value());
}
@@ -309,14 +300,14 @@ TEST_FOR_EACH_BLOCK_TYPE(CanMergeWithNextBlock) {
ASSERT_TRUE(result.has_value());
BlockType *block1 = *result;
- result = BlockType::split(block1, kSplit1);
+ result = block1->split(kSplit1);
ASSERT_TRUE(result.has_value());
- result = BlockType::split(block1, kSplit2);
+ result = block1->split(kSplit2);
ASSERT_TRUE(result.has_value());
BlockType *block3 = *result;
- EXPECT_TRUE(BlockType::merge_next(block3));
+ EXPECT_TRUE(block3->merge_next());
EXPECT_EQ(block1->next(), block3);
EXPECT_EQ(block3->prev(), block1);
@@ -334,16 +325,11 @@ TEST_FOR_EACH_BLOCK_TYPE(CannotMergeWithFirstOrLastBlock) {
BlockType *block1 = *result;
// Do a split, just to check that the checks on next/prev are different...
- result = BlockType::split(block1, kSplitN);
+ result = block1->split(kSplitN);
ASSERT_TRUE(result.has_value());
BlockType *block2 = *result;
- EXPECT_FALSE(BlockType::merge_next(block2));
-}
-
-TEST_FOR_EACH_BLOCK_TYPE(CannotMergeNull) {
- BlockType *block = nullptr;
- EXPECT_FALSE(BlockType::merge_next(block));
+ EXPECT_FALSE(block2->merge_next());
}
TEST_FOR_EACH_BLOCK_TYPE(CannotMergeUsedBlock) {
@@ -356,131 +342,11 @@ TEST_FOR_EACH_BLOCK_TYPE(CannotMergeUsedBlock) {
BlockType *block = *result;
// Do a split, just to check that the checks on next/prev are different...
- result = BlockType::split(block, kSplitN);
- ASSERT_TRUE(result.has_value());
-
- block->mark_used();
- EXPECT_FALSE(BlockType::merge_next(block));
-}
-
-TEST_FOR_EACH_BLOCK_TYPE(CanFreeSingleBlock) {
- constexpr size_t kN = 1024;
- alignas(BlockType::ALIGNMENT) array<byte, kN> bytes;
-
- auto result = BlockType::init(bytes);
+ result = block->split(kSplitN);
ASSERT_TRUE(result.has_value());
- BlockType *block = *result;
block->mark_used();
- BlockType::free(block);
- EXPECT_FALSE(block->used());
- EXPECT_EQ(block->outer_size(), kN);
-}
-
-TEST_FOR_EACH_BLOCK_TYPE(CanFreeBlockWithoutMerging) {
- constexpr size_t kN = 1024;
- constexpr size_t kSplit1 = 512;
- constexpr size_t kSplit2 = 256;
-
- alignas(BlockType::ALIGNMENT) array<byte, kN> bytes;
- auto result = BlockType::init(bytes);
- ASSERT_TRUE(result.has_value());
- BlockType *block1 = *result;
-
- result = BlockType::split(block1, kSplit1);
- ASSERT_TRUE(result.has_value());
- BlockType *block2 = *result;
-
- result = BlockType::split(block2, kSplit2);
- ASSERT_TRUE(result.has_value());
- BlockType *block3 = *result;
-
- block1->mark_used();
- block2->mark_used();
- block3->mark_used();
-
- BlockType::free(block2);
- EXPECT_FALSE(block2->used());
- EXPECT_NE(block2->prev(), static_cast<BlockType *>(nullptr));
- EXPECT_FALSE(block2->last());
-}
-
-TEST_FOR_EACH_BLOCK_TYPE(CanFreeBlockAndMergeWithPrev) {
- constexpr size_t kN = 1024;
- constexpr size_t kSplit1 = 512;
- constexpr size_t kSplit2 = 256;
-
- alignas(BlockType::ALIGNMENT) array<byte, kN> bytes;
- auto result = BlockType::init(bytes);
- ASSERT_TRUE(result.has_value());
- BlockType *block1 = *result;
-
- result = BlockType::split(block1, kSplit1);
- ASSERT_TRUE(result.has_value());
- BlockType *block2 = *result;
-
- result = BlockType::split(block2, kSplit2);
- ASSERT_TRUE(result.has_value());
- BlockType *block3 = *result;
-
- block2->mark_used();
- block3->mark_used();
-
- BlockType::free(block2);
- EXPECT_FALSE(block2->used());
- EXPECT_EQ(block2->prev(), static_cast<BlockType *>(nullptr));
- EXPECT_FALSE(block2->last());
-}
-
-TEST_FOR_EACH_BLOCK_TYPE(CanFreeBlockAndMergeWithNext) {
- constexpr size_t kN = 1024;
- constexpr size_t kSplit1 = 512;
- constexpr size_t kSplit2 = 256;
-
- alignas(BlockType::ALIGNMENT) array<byte, kN> bytes;
- auto result = BlockType::init(bytes);
- ASSERT_TRUE(result.has_value());
- BlockType *block1 = *result;
-
- result = BlockType::split(block1, kSplit1);
- ASSERT_TRUE(result.has_value());
- BlockType *block2 = *result;
-
- result = BlockType::split(block2, kSplit2);
- ASSERT_TRUE(result.has_value());
-
- block1->mark_used();
- block2->mark_used();
-
- BlockType::free(block2);
- EXPECT_FALSE(block2->used());
- EXPECT_NE(block2->prev(), static_cast<BlockType *>(nullptr));
- EXPECT_TRUE(block2->last());
-}
-
-TEST_FOR_EACH_BLOCK_TYPE(CanFreeUsedBlockAndMergeWithBoth) {
- constexpr size_t kN = 1024;
- constexpr size_t kSplit1 = 512;
- constexpr size_t kSplit2 = 256;
-
- alignas(BlockType::ALIGNMENT) array<byte, kN> bytes;
- auto result = BlockType::init(bytes);
- ASSERT_TRUE(result.has_value());
- BlockType *block1 = *result;
-
- result = BlockType::split(block1, kSplit1);
- ASSERT_TRUE(result.has_value());
- BlockType *block2 = *result;
-
- result = BlockType::split(block2, kSplit2);
- ASSERT_TRUE(result.has_value());
-
- block2->mark_used();
-
- BlockType::free(block2);
- EXPECT_FALSE(block2->used());
- EXPECT_EQ(block2->prev(), static_cast<BlockType *>(nullptr));
- EXPECT_TRUE(block2->last());
+ EXPECT_FALSE(block->merge_next());
}
TEST_FOR_EACH_BLOCK_TYPE(CanCheckValidBlock) {
@@ -493,11 +359,11 @@ TEST_FOR_EACH_BLOCK_TYPE(CanCheckValidBlock) {
ASSERT_TRUE(result.has_value());
BlockType *block1 = *result;
- result = BlockType::split(block1, kSplit1);
+ result = block1->split(kSplit1);
ASSERT_TRUE(result.has_value());
BlockType *block2 = *result;
- result = BlockType::split(block2, kSplit2);
+ result = block2->split(kSplit2);
ASSERT_TRUE(result.has_value());
BlockType *block3 = *result;
@@ -517,15 +383,15 @@ TEST_FOR_EACH_BLOCK_TYPE(CanCheckInvalidBlock) {
ASSERT_TRUE(result.has_value());
BlockType *block1 = *result;
- result = BlockType::split(block1, kSplit1);
+ result = block1->split(kSplit1);
ASSERT_TRUE(result.has_value());
BlockType *block2 = *result;
- result = BlockType::split(block2, kSplit2);
+ result = block2->split(kSplit2);
ASSERT_TRUE(result.has_value());
BlockType *block3 = *result;
- result = BlockType::split(block3, kSplit3);
+ result = block3->split(kSplit3);
ASSERT_TRUE(result.has_value());
// Corrupt a Block header.
@@ -630,7 +496,6 @@ TEST_FOR_EACH_BLOCK_TYPE(AllocateAlreadyAligned) {
// Check the next block.
EXPECT_NE(next, static_cast<BlockType *>(nullptr));
EXPECT_EQ(aligned_block->next(), next);
- EXPECT_EQ(next->next(), static_cast<BlockType *>(nullptr));
EXPECT_EQ(reinterpret_cast<byte *>(next) + next->outer_size(),
bytes.data() + bytes.size());
}
@@ -674,7 +539,6 @@ TEST_FOR_EACH_BLOCK_TYPE(AllocateNeedsAlignment) {
// Check the next block.
EXPECT_NE(next, static_cast<BlockType *>(nullptr));
EXPECT_EQ(aligned_block->next(), next);
- EXPECT_EQ(next->next(), static_cast<BlockType *>(nullptr));
EXPECT_EQ(reinterpret_cast<byte *>(next) + next->outer_size(), &*bytes.end());
}
@@ -687,7 +551,7 @@ TEST_FOR_EACH_BLOCK_TYPE(PreviousBlockMergedIfNotFirst) {
BlockType *block = *result;
// Split the block roughly halfway and work on the second half.
- auto result2 = BlockType::split(block, kN / 2);
+ auto result2 = block->split(kN / 2);
ASSERT_TRUE(result2.has_value());
BlockType *newblock = *result2;
ASSERT_EQ(newblock->prev(), block);
@@ -749,14 +613,12 @@ TEST_FOR_EACH_BLOCK_TYPE(CanRemergeBlockAllocations) {
EXPECT_NE(next, static_cast<BlockType *>(nullptr));
EXPECT_NE(next, static_cast<BlockType *>(nullptr));
EXPECT_EQ(aligned_block->next(), next);
- EXPECT_EQ(next->next(), static_cast<BlockType *>(nullptr));
ASSERT_TRUE(next->last());
// Now check for successful merges.
- EXPECT_TRUE(BlockType::merge_next(prev));
+ EXPECT_TRUE(prev->merge_next());
EXPECT_EQ(prev->next(), next);
- EXPECT_TRUE(BlockType::merge_next(prev));
- EXPECT_EQ(prev->next(), static_cast<BlockType *>(nullptr));
+ EXPECT_TRUE(prev->merge_next());
EXPECT_TRUE(prev->last());
// We should have the original buffer.
>From 4bace2b351e4a69aa89ded009ed69969a9c1d6a2 Mon Sep 17 00:00:00 2001
From: Daniel Thornburgh <dthorn at google.com>
Date: Wed, 24 Jul 2024 13:25:04 -0700
Subject: [PATCH 2/2] Make next() return nullptr if last; remove last()
---
libc/src/__support/block.h | 56 ++++++++------------------
libc/src/__support/freelist_heap.h | 2 +-
libc/test/src/__support/block_test.cpp | 9 ++---
3 files changed, 20 insertions(+), 47 deletions(-)
diff --git a/libc/src/__support/block.h b/libc/src/__support/block.h
index 442740441e9da..98ec5cf5151b8 100644
--- a/libc/src/__support/block.h
+++ b/libc/src/__support/block.h
@@ -178,23 +178,8 @@ class Block {
/// Merges this block with the one that comes after it.
bool merge_next();
- /// Fetches the block immediately after this one.
- ///
- /// For performance, this always returns a block pointer, even if the returned
- /// pointer is invalid. The pointer is valid if and only if `last()` is false.
- ///
- /// Typically, after calling `Init` callers may save a pointer past the end of
- /// the list using `next()`. This makes it easy to subsequently iterate over
- /// the list:
- /// @code{.cpp}
- /// auto result = Block<>::init(byte_span);
- /// Block<>* begin = *result;
- /// Block<>* end = begin->next();
- /// ...
- /// for (auto* block = begin; block != end; block = block->next()) {
- /// // Do something which each block.
- /// }
- /// @endcode
+ /// @returns The block immediately after this one, or a null pointer if this
+ /// is the last block.
Block *next() const;
/// @returns The block immediately before this one, or a null pointer if this
@@ -206,21 +191,14 @@ class Block {
/// @returns `true` if the block is in use or `false` if not.
bool used() const { return next_ & USED_MASK; }
- /// Indicates whether this block is the last block or not (i.e. whether
- /// `next()` points to a valid block or not). This is needed because
- /// `next()` points to the end of this block, whether there is a valid
- /// block there or not.
- ///
- /// @returns `true` is this is the last block or `false` if not.
- bool last() const { return next_ & LAST_MASK; }
-
/// Marks this block as in use.
void mark_used() { next_ |= USED_MASK; }
/// Marks this block as free.
void mark_free() { next_ &= ~USED_MASK; }
- /// Marks this block as the last one in the chain.
+ /// Marks this block as the last one in the chain. Makes next() return
+ /// nullptr.
constexpr void mark_last() { next_ |= LAST_MASK; }
/// @brief Checks if a block is valid.
@@ -448,41 +426,37 @@ template <typename OffsetType, size_t kAlign>
Block<OffsetType, kAlign> *
Block<OffsetType, kAlign>::split_impl(size_t new_inner_size) {
size_t outer_size1 = new_inner_size + BLOCK_OVERHEAD;
- bool is_last = last();
+ bool has_next = next();
ByteSpan new_region = region().subspan(outer_size1);
LIBC_ASSERT(!used() && "used blocks cannot be split");
// The low order bits of outer_size1 should both be zero, and is the correct
// value for the flags is false.
next_ = outer_size1;
- LIBC_ASSERT(!used() && !last() && "incorrect first split flags");
+ LIBC_ASSERT(!used() && next() && "incorrect first split flags");
Block *new_block = as_block(outer_size1, new_region);
- if (is_last) {
- new_block->mark_last();
- } else {
+ if (has_next) {
// The two flags are both false, so next_ is a plain size.
- LIBC_ASSERT(!new_block->used() && !new_block->last() &&
- "flags disrupt use of size");
+ LIBC_ASSERT(!new_block->used() && "flags disrupt use of size");
new_block->next()->prev_ = new_block->next_;
+ } else {
+ new_block->mark_last();
}
return new_block;
}
template <typename OffsetType, size_t kAlign>
bool Block<OffsetType, kAlign>::merge_next() {
- if (last())
- return false;
-
- if (used() || next()->used())
+ if (used() || !next() || next()->used())
return false;
// Extend the size and copy the last() flag from the next block to this one.
next_ &= SIZE_MASK;
next_ += next()->next_;
- if (!last()) {
+ if (next()) {
// The two flags are both false, so next_ is a plain size.
- LIBC_ASSERT(!used() && !last() && "flags disrupt use of size");
+ LIBC_ASSERT(!used() && next() && "flags disrupt use of size");
next()->prev_ = next_;
}
@@ -491,6 +465,8 @@ bool Block<OffsetType, kAlign>::merge_next() {
template <typename OffsetType, size_t kAlign>
Block<OffsetType, kAlign> *Block<OffsetType, kAlign>::next() const {
+ if (next_ & LAST_MASK)
+ return nullptr;
return reinterpret_cast<Block *>(reinterpret_cast<uintptr_t>(this) +
outer_size());
}
@@ -522,7 +498,7 @@ internal::BlockStatus Block<OffsetType, kAlign>::check_status() const {
if (reinterpret_cast<uintptr_t>(this) % ALIGNMENT != 0)
return internal::BlockStatus::MISALIGNED;
- if (!last() && (this >= next() || this != next()->prev()))
+ if (next() && (this >= next() || this != next()->prev()))
return internal::BlockStatus::NEXT_MISMATCHED;
if (prev() && (this <= prev() || this != prev()->next()))
diff --git a/libc/src/__support/freelist_heap.h b/libc/src/__support/freelist_heap.h
index dc2fe43e44af2..a2c714e15ba87 100644
--- a/libc/src/__support/freelist_heap.h
+++ b/libc/src/__support/freelist_heap.h
@@ -189,7 +189,7 @@ template <size_t NUM_BUCKETS> void FreeListHeap<NUM_BUCKETS>::free(void *ptr) {
BlockType *prev = chunk_block->prev();
BlockType *next = nullptr;
- if (!chunk_block->last())
+ if (chunk_block->next())
next = chunk_block->next();
if (prev != nullptr && !prev->used()) {
diff --git a/libc/test/src/__support/block_test.cpp b/libc/test/src/__support/block_test.cpp
index fcd9d06a5a8d2..ecce00b7926f9 100644
--- a/libc/test/src/__support/block_test.cpp
+++ b/libc/test/src/__support/block_test.cpp
@@ -52,8 +52,8 @@ TEST_FOR_EACH_BLOCK_TYPE(CanCreateSingleAlignedBlock) {
EXPECT_EQ(block->outer_size(), kN);
EXPECT_EQ(block->inner_size(), kN - BlockType::BLOCK_OVERHEAD);
EXPECT_EQ(block->prev(), static_cast<BlockType *>(nullptr));
+ EXPECT_EQ(block->next(), static_cast<BlockType *>(nullptr));
EXPECT_FALSE(block->used());
- EXPECT_TRUE(block->last());
}
TEST_FOR_EACH_BLOCK_TYPE(CanCreateUnalignedSingleBlock) {
@@ -102,11 +102,9 @@ TEST_FOR_EACH_BLOCK_TYPE(CanSplitBlock) {
EXPECT_EQ(block1->inner_size(), kSplitN);
EXPECT_EQ(block1->outer_size(), kSplitN + BlockType::BLOCK_OVERHEAD);
- EXPECT_FALSE(block1->last());
EXPECT_EQ(block2->outer_size(), kN - kSplitN - BlockType::BLOCK_OVERHEAD);
EXPECT_FALSE(block2->used());
- EXPECT_TRUE(block2->last());
EXPECT_EQ(block1->next(), block2);
EXPECT_EQ(block2->prev(), block1);
@@ -608,18 +606,17 @@ TEST_FOR_EACH_BLOCK_TYPE(CanRemergeBlockAllocations) {
// Check we have the appropriate blocks.
ASSERT_NE(prev, static_cast<BlockType *>(nullptr));
- ASSERT_FALSE(prev->last());
ASSERT_EQ(aligned_block->prev(), prev);
EXPECT_NE(next, static_cast<BlockType *>(nullptr));
EXPECT_NE(next, static_cast<BlockType *>(nullptr));
EXPECT_EQ(aligned_block->next(), next);
- ASSERT_TRUE(next->last());
+ EXPECT_EQ(next->next(), static_cast<BlockType *>(nullptr));
// Now check for successful merges.
EXPECT_TRUE(prev->merge_next());
EXPECT_EQ(prev->next(), next);
EXPECT_TRUE(prev->merge_next());
- EXPECT_TRUE(prev->last());
+ EXPECT_EQ(prev->next(), static_cast<BlockType *>(nullptr));
// We should have the original buffer.
EXPECT_EQ(reinterpret_cast<byte *>(prev), &*bytes.begin());
More information about the libc-commits
mailing list