[libc][malloc] Ensure a minimum block alignment of 4

[mysterymath]

Most platforms inherently have a size_t alignment of 4, but this isn't true on every platform LLVM has some degree of backend support for. Accordingly, it's simple enough to just set the min alignment of Block to 4 and lose the static_assert.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Daniel Thornburgh (mysterymath)

Changes

Most platforms inherently have a size_t alignment of 4, but this isn't true on every platform LLVM has some degree of backend support for. Accordingly, it's simple enough to just set the min alignment of Block to 4 and lose the static_assert.

---

Full diff: https://github.com/llvm/llvm-project/pull/169447.diff

1 Files Affected:

• (modified) libc/src/__support/block.h (+4-4)

diff --git a/libc/src/__support/block.h b/libc/src/__support/block.h
index b0d6576093244..faab409166c6e 100644
--- a/libc/src/__support/block.h
+++ b/libc/src/__support/block.h
@@ -90,7 +90,10 @@ using cpp::optional;
 ///
 /// The next offset of a block matches the previous offset of its next block.
 /// The first block in a list is denoted by having a previous offset of `0`.
-class Block {
+///
+// Ensure a minimum alignment of 4, since at least 2 bits must be available in
+// block sizes for flags.
+class alignas(cpp::max(alignof(size_t), size_t{4})) Block {
   // Masks for the contents of the next_ field.
   static constexpr size_t PREV_FREE_MASK = 1 << 0;
   static constexpr size_t LAST_MASK = 1 << 1;
@@ -360,9 +363,6 @@ class Block {
   static constexpr size_t PREV_FIELD_SIZE = sizeof(prev_);
 };
 
-static_assert(alignof(Block) >= 4,
-              "at least 2 bits must be available in block sizes for flags");
-
 LIBC_INLINE
 optional<Block *> Block::init(ByteSpan region) {
   if (!region.data())

[SchrodingerZhu]

@mysterymath shouldn't it be better to align block to max_align_t, which is effectively long double for most platforms?

[jhuber6]

The consensus around allocation alignments is a little weird, its not specified in the standard anywhere. Generally, dynamic memory is supposed to be aligned to the largest memory required by a primitive type, i.e. 16 bytes for an x87 long double. However, this wastes a lot of space and isn't required for all cases, so some implementations lower this. At some point LLVM tried to codify that returned pointers from malloc were aligned but that broke a lot of uses of jemalloc and other optimized allocators that tried to save memory.

[SchrodingerZhu]

But would that be against C11 standard?

POSIX standard (issue 8) says "The pointer returned if the allocation succeeds shall be suitably aligned so that it may be assigned to a pointer to any type of object with a fundamental alignment requirement and then used to access such an object in the space allocated"

ISO/IEC 9899:201X says "A fundamental alignment is represented by an alignment less than or equal to the greatest alignment supported by the implementation in all contexts, which is equal to alignof(max_align_t)."

I know the standard is a bit awkward as we do have aligned_alloc to any situation where alignment is a serious consideration, but going against it also has potential complications.

---

I understand that many platforms we support have considerations of limited space. We can document and opt-in this on baremetal and gpu, align with standard otherwise unless the user opt-in a different minimal alignment.

[SchrodingerZhu]

It seems that some modern allocators (e.g. mimalloc) does relax this for allocation sizes less than alignof(max_align_t).
https://github.com/microsoft/mimalloc/blob/09a27098aa6e9286518bd9c74e6ffa7199c3f04e/src/alloc.c#L47

[mysterymath]

Aha, didn't think this would trigger much discussion! This is specifically for the alignment of the Block structure, but the way we've implemented things, that actually has only indirect bearing on the alignment of the returned pointers carved out of blocks, as Block locations are adjusted such that the ends of the block headers (i.e., the ends of the Block structs) are always aligned to max_align_t. (In particular, this means that Blocks themselves might be required to never align to max_align_t!)

The only alignment requirements for Block itself are that it be correctly alignment for its contents (two size_t), and that it be aligned to at least 4, so that the bottom two bits of block sizes are always logically zero and thus free to store flags. It must also be the case that max_align_t is a multiple of the block alignment, but that's true for any power of two.

[mysterymath]

Drafting this for now; I just realized from the above that the sizes are the important thing now; the Block alignment itself is somewhat immaterial, as Block placement is completely controlled by the Block init functions. Some of this logic bled through from the older implementation, and it doesn't quite make sense after I had refactored it, so I'll need to do a bit more surgery to make this internally consistent.

[mysterymath]

Alright, I've updated this to directly ensure that the usable space alignment within the block is 4 at minimum. This may waste a bit of space, but it's quite simpler, and it would only have any effect on platforms where alignof(max_align_t) is less than 4. This is exceedingly rare.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.