[libc-commits] [libc] [libc] Fix malloc Block alignment issue on riscv32 (PR #117815)

[via libc-commits]

================
@@ -248,46 +259,48 @@ 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))
-      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
-    //
+  // 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 padding block to align the usable space of the next
+    // block. The next block's usable space can be found by advancing by
+    // sizeof(Block) then aligning up. The amount advanced is the amount of
+    // additional inner size required.
     //
-    // |block   |space   |block   |usable_space        |
-    // |........|........|........|....................|
-    //                            ^
-    //                            Alignment requirement
-    //
-    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_);
+    // The minimum advancment is sizeof(Block), since the resulting position may
+    // happen to be aligned. What is the maximum? Advancing by sizeof(Block)
+    // leaves the result still aligned to alignof(Block). So the most additional
+    // advancement required would be if the point is exactly alignof(Block) past
+    // alignment. The remaining size to the next alignment would then be
+    // alignment - alignof(Block). So the total maximum advancement required is
+    // sizeof(Block) + alignment - alignof(Block).
----------------
PiJoules wrote:

Had some trouble thinking about why the max is calculated this way but @mysterymath explained offline and I'm going to write down the explanation which can hopefully be added here to expand on the max calculation.

```
When picking a new size, we need to ensure we can always pick a point inside that block that is aligned to alignment. We also need to make sure that advancing by the requested size will ensure the address at the end of the new size is Block-aligned (so we can always allocate new Blocks after it).

Assuming the alignment is just one max_align_t, we can always return the size size the new inner size would always be aligned. Otherwise, we need to advance by a little extra space.

The minimum additional advancement is sizeof(Block), since the resulting position may happen to be aligned. This space is also needed just to hold the next block this can be partitioned into.

What is the maximum? We know that requesting an additional size by `alignment` will always ensure there's a point in the new allocation that will be aligned to `alignment`, so we can add that to our new size. However, let's say our new usable_space would already be aligned to `alignment`, then advancing by `alignment` would not be necessary since the usable_space would already be aligned. If instead the usable_space were one byte after an alignment point, then we'd only need to advance the size by at most `alignment - 1` byte to find a point in the new block that can be aligned to `alignment`. Over the contiguous memory region Blocks operate under, any Block split always ensures partitions are at minimum Block-aligned. Therefore, the most we need to advance is actually `alignment - alignof(Block)`. This way, even if advancing by `size + sizeof(Block)` happens to be aligned to `alignment`, we don't need to request the full `alignment`. Leaving out the `alignof(Block)` also ensures the partition after this one will always be Block-aligned.
```

Feel free to correct/expand on anything I got wrong with my understanding here.

https://github.com/llvm/llvm-project/pull/117815