[libc-commits] [libc] [libc] Use Block as a byte-backed proxy (PR #201001)

[via libc-commits]

================
@@ -275,79 +285,90 @@ class Block {
     // So the maximum distance would be G - L. As a special case, if L is 1
     // (unaligned), the max distance is G - 1.
     //
-    // This block's usable space is aligned to MIN_ALIGN >= 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))
+    // This block's usable space is aligned to MIN_ALIGN >= header alignment.
+    // With zero padding, the next block's usable space is HEADER_SIZE past it,
+    // which is aligned to header alignment. Thus the max padding needed is
+    // alignment - alignof(size_t).
+    if (add_overflow(size, alignment - alignof(size_t), size))
       return 0;
     return size;
   }
 
-  // This is the return type for `allocate` which can split one block into up to
-  // three blocks.
-  struct BlockInfo {
-    // This is the newly aligned block. It will have the alignment requested by
-    // a call to `allocate` and at most `size`.
-    Block *block;
-
-    // If the usable_space in the new block was not aligned according to the
-    // `alignment` parameter, we will need to split into this block and the
-    // `block` to ensure `block` is properly aligned. In this case, `prev` will
-    // be a pointer to this new "padding" block. `prev` will be nullptr if no
-    // new block was created or we were able to merge the block before the
-    // original block with the "padding" block.
-    Block *prev;
-
-    // This is the remainder of the next block after splitting the `block`
-    // according to `size`. This can happen if there's enough space after the
-    // `block`.
-    Block *next;
-  };
+  struct BlockInfo;
 
   // 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);
+  static BlockInfo allocate(BlockRef 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 = MIN_ALIGN) {
-    return align_up(ptr + sizeof(Block), usable_space_alignment) -
-           sizeof(Block);
+    return align_up(ptr + HEADER_SIZE, usable_space_alignment) - HEADER_SIZE;
   }
   LIBC_INLINE static uintptr_t
   prev_possible_block_start(uintptr_t ptr,
                             size_t usable_space_alignment = MIN_ALIGN) {
-    return align_down(ptr, usable_space_alignment) - sizeof(Block);
+    return align_down(ptr, usable_space_alignment) - HEADER_SIZE;
   }
 
+  /// Only for testing.
+  static constexpr size_t PREV_FIELD_SIZE = sizeof(size_t);
+
 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) ==
+  LIBC_INLINE static BlockRef as_block(ByteSpan bytes) {
+    LIBC_ASSERT(reinterpret_cast<uintptr_t>(bytes.data()) % alignof(size_t) ==
                     0 &&
                 "block start must be suitably aligned");
-    return ::new (bytes.data()) Block(bytes.size(), /*is_last=*/false);
+    BlockRef block(bytes.data());
+    block.store_next(bytes.size());
+    return block;
   }
 
   LIBC_INLINE static void make_last_block(cpp::byte *start) {
-    LIBC_ASSERT(reinterpret_cast<uintptr_t>(start) % alignof(Block) == 0 &&
+    LIBC_ASSERT(reinterpret_cast<uintptr_t>(start) % alignof(size_t) == 0 &&
                 "block start must be suitably aligned");
-    ::new (start) Block(sizeof(Block), /*is_last=*/true);
+    BlockRef last(start);
+    last.store_next(HEADER_SIZE | LAST_MASK);
+  }
+
+  LIBC_INLINE cpp::byte *field_ptr(size_t offset) const {
+    cpp::byte *ptr = header_ptr + offset;
+    LIBC_ASSERT(reinterpret_cast<uintptr_t>(ptr) % alignof(size_t) == 0 &&
+                "block metadata fields must be aligned");
+    // BlockRef points to block header which should be well-aligned. However,
+    // the compiler may not know this information. Adding assume aligned to
+    // tell the compiler to emit a single load/store even when unaligned access
+    // is disallowed.
+#if __has_builtin(__builtin_assume_aligned)
+    return reinterpret_cast<cpp::byte *>(
+        __builtin_assume_aligned(ptr, alignof(size_t)));
+#else
+    return ptr;
+#endif
+  }
+
+  LIBC_INLINE size_t load_field(size_t offset) const {
+    size_t value;
+    inline_memcpy(&value, field_ptr(offset), sizeof(value));
+    return value;
+  }
+
+  LIBC_INLINE void store_field(size_t offset, size_t value) const {
+    inline_memcpy(field_ptr(offset), &value, sizeof(value));
----------------
PiJoules wrote:

Hmm, I think I'm being overly pedantic here (so feel free to ignore bc this is me going off on a tangent), but I've been wondering why use `inline_memcpy` here instead of either a normal deref or placement new.

I believe a normal deref via `*reinterpret_cast<size_t *>(field_ptr(offset)) = value` wouldn't suffice because it would break aliasing/lifetime rules to write a value to this address via `store_prev` if a prior allocation had written something to it. That object lifetime would need to be destroyed first. We could then do a placement new instead via `new (field_ptr(offset)) size_t(value)` but I think it would then be possible for clang to reorder writes due to type aliasing rules. For example, let's say the user writes a float to the end of a usable_space of Block A which would overlap the prev of Block B:

```
*user_float_ptr = 3.14f;  // user_float_ptr points to the prev of block B
a_block.mark_free();  // calls placement new
```

Then since clang knows `user_float_ptr` and `prev` (which is `size_t*`) cannot alias to the same addr since they're different types, then it's possible for clang to reorder the write from the placement new which would be invoked via `mark_free` *before* the float write, which leads to unintended behavior since we'd write a float where we'd expect a normal `prev` value.

Using `memcpy` (or more specifically `inline_memcpy` which I believe lowers to one of the different flavors of some target-specific `__builtin_memcpy`) would start the object lifetime with at least C++20 for trivially copyable types, but it wouldn't do so for C++17. So I think both `inline_memcpy` for the load and store are the only legal ways to do this.

Probably don't need to take any action on this, was just curious as to why neither the deref nor placement new was used here.

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