Address nopsledder review comments

Author: mysterymath

libc/src/__support/CMakeLists.txt

@@ -14,25 +14,47 @@ add_header_library(
     libc.src.__support.CPP.type_traits
 )
 
-add_header_library(
+add_object_library(
   freelist
   HDRS
     freelist.h
+  SRCS
+    freelist.cpp
   DEPENDS
+    .block
     libc.src.__support.fixedvector
     libc.src.__support.CPP.array
     libc.src.__support.CPP.cstddef
     libc.src.__support.CPP.new
     libc.src.__support.CPP.span
 )
 
+add_object_library(
+  freetrie
+  HDRS
+    freetrie.h
+  SRCS
+    freetrie.cpp
+  DEPENDS
+    .block
+    .freelist
+)
+
+add_header_library(
+  freestore
+  HDRS
+    freestore.h
+  DEPENDS
+    .freetrie
+)
+
 add_header_library(
   freelist_heap
   HDRS
     freelist_heap.h
   DEPENDS
     .block
-    .freelist
+    .freestore
     libc.src.__support.CPP.cstddef
     libc.src.__support.CPP.array
     libc.src.__support.CPP.optional

libc/src/__support/freelist.cpp

@@ -0,0 +1,42 @@
+//===-- Implementation for freelist ---------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "freelist.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+void FreeList::push(Node *node) {
+  if (begin_) {
+    LIBC_ASSERT(Block<>::from_usable_space(node)->outer_size() ==
+                    begin_->block()->outer_size() &&
+                "freelist entries must have the same size");
+    // Since the list is circular, insert the node immediately before begin_.
+    node->prev = begin_->prev;
+    node->next = begin_;
+    begin_->prev->next = node;
+    begin_->prev = node;
+  } else {
+    begin_ = node->prev = node->next = node;
+  }
+}
+
+void FreeList::remove(Node *node) {
+  LIBC_ASSERT(begin_ && "cannot remove from empty list");
+  if (node == node->next) {
+    LIBC_ASSERT(node == begin_ &&
+                "a self-referential node must be the only element");
+    begin_ = nullptr;
+  } else {
+    node->prev->next = node->next;
+    node->next->prev = node->prev;
+    if (begin_ == node)
+      begin_ = node->next;
+  }
+}
+
+} // namespace LIBC_NAMESPACE_DECL

libc/src/__support/freelist.h

@@ -14,7 +14,8 @@
 namespace LIBC_NAMESPACE_DECL {
 
 /// A circularly-linked FIFO list storing free Blocks. All Blocks on a list
-/// are the same size.
+/// are the same size. The blocks are referenced by Nodes in the list; the list
+/// refers to these, but it does not own them.
 ///
 /// Allocating free blocks in FIFO order maximizes the amount of time before a
 /// free block is reused. This in turn maximizes the number of opportunities for
@@ -25,12 +26,15 @@ class FreeList {
   class Node {
   public:
     /// @returns The block containing this node.
-    Block<> *block() const {
-      return const_cast<Block<> *>(Block<>::from_usable_space(this));
+    LIBC_INLINE const Block<> *block() const {
+      return Block<>::from_usable_space(this);
     }
 
+    /// @returns The block containing this node.
+    LIBC_INLINE Block<> *block() { return Block<>::from_usable_space(this); }
+
     /// @returns The inner size of blocks in the list containing this node.
-    size_t size() const { return block()->inner_size(); }
+    LIBC_INLINE size_t size() const { return block()->inner_size(); }
 
   private:
     // Circularly linked pointers to adjacent nodes.
@@ -39,33 +43,39 @@ class FreeList {
     friend class FreeList;
   };
 
-  constexpr FreeList() : FreeList(nullptr) {}
-  constexpr FreeList(Node *begin) : begin_(begin) {}
+  LIBC_INLINE constexpr FreeList() : FreeList(nullptr) {}
+  LIBC_INLINE constexpr FreeList(Node *begin) : begin_(begin) {}
 
-  bool empty() const { return !begin_; }
+  LIBC_INLINE bool empty() const { return !begin_; }
 
   /// @returns The inner size of blocks in the list.
-  size_t size() const {
+  LIBC_INLINE size_t size() const {
     LIBC_ASSERT(begin_ && "empty lists have no size");
     return begin_->size();
   }
 
   /// @returns The first node in the list.
-  Node *begin() { return begin_; }
+  LIBC_INLINE Node *begin() { return begin_; }
 
   /// @returns The first block in the list.
-  Block<> *front() { return begin_->block(); }
+  LIBC_INLINE Block<> *front() { return begin_->block(); }
 
   /// Push a block to the back of the list.
   /// The block must be large enough to contain a node.
-  void push(Block<> *block);
+  LIBC_INLINE void push(Block<> *block) {
+    LIBC_ASSERT(!block->used() &&
+                "only free blocks can be placed on free lists");
+    LIBC_ASSERT(block->inner_size_free() >= sizeof(FreeList) &&
+                "block too small to accomodate free list node");
+    push(new (block->usable_space()) Node);
+  }
 
   /// Push an already-constructed node to the back of the list.
   /// This allows pushing derived node types with additional data.
   void push(Node *node);
 
   /// Pop the first node from the list.
-  void pop();
+  LIBC_INLINE void pop() { remove(begin_); }
 
   /// Remove an arbitrary node from the list.
   void remove(Node *node);
@@ -74,44 +84,6 @@ class FreeList {
   Node *begin_;
 };
 
-LIBC_INLINE void FreeList::push(Block<> *block) {
-  LIBC_ASSERT(!block->used() && "only free blocks can be placed on free lists");
-  LIBC_ASSERT(block->inner_size_free() >= sizeof(FreeList) &&
-              "block too small to accomodate free list node");
-  push(new (block->usable_space()) Node);
-}
-
-LIBC_INLINE void FreeList::push(Node *node) {
-  if (begin_) {
-    LIBC_ASSERT(Block<>::from_usable_space(node)->outer_size() ==
-                    begin_->block()->outer_size() &&
-                "freelist entries must have the same size");
-    // Since the list is circular, insert the node immediately before begin_.
-    node->prev = begin_->prev;
-    node->next = begin_;
-    begin_->prev->next = node;
-    begin_->prev = node;
-  } else {
-    begin_ = node->prev = node->next = node;
-  }
-}
-
-LIBC_INLINE void FreeList::pop() { remove(begin_); }
-
-LIBC_INLINE void FreeList::remove(Node *node) {
-  LIBC_ASSERT(begin_ && "cannot remove from empty list");
-  if (node == node->next) {
-    LIBC_ASSERT(node == begin_ &&
-                "a self-referential node must be the only element");
-    begin_ = nullptr;
-  } else {
-    node->prev->next = node->next;
-    node->next->prev = node->prev;
-    if (begin_ == node)
-      begin_ = node->next;
-  }
-}
-
 } // namespace LIBC_NAMESPACE_DECL
 
 #endif // LLVM_LIBC_SRC___SUPPORT_FREELIST_H

libc/src/__support/freelist_heap.h

@@ -32,8 +32,6 @@ LIBC_INLINE constexpr bool IsPow2(size_t x) { return x && (x & (x - 1)) == 0; }
 
 class FreeListHeap {
 public:
-  using BlockType = Block<>;
-
   constexpr FreeListHeap() : begin(&_end), end(&__llvm_libc_heap_limit) {}
 
   constexpr FreeListHeap(span<cpp::byte> region)
@@ -90,8 +88,11 @@ LIBC_INLINE void *FreeListHeap::allocate_impl(size_t alignment, size_t size) {
   if (!is_initialized)
     init();
 
-  size_t request_size =
-      alignment <= alignof(max_align_t) ? size : size + alignment - 1;
+  size_t request_size = size;
+  if (alignment > alignof(max_align_t)) {
+    if (add_overflow(size, alignment - 1, size))
+      return nullptr;
+  }
 
   Block<> *block = free_store.remove_best_fit(request_size);
   if (!block)
@@ -194,9 +195,12 @@ LIBC_INLINE void *FreeListHeap::realloc(void *ptr, size_t size) {
 }
 
 LIBC_INLINE void *FreeListHeap::calloc(size_t num, size_t size) {
-  void *ptr = allocate(num * size);
+  size_t bytes;
+  if (__builtin_mul_overflow(num, size, &bytes))
+    return nullptr;
+  void *ptr = allocate(bytes);
   if (ptr != nullptr)
-    LIBC_NAMESPACE::inline_memset(ptr, 0, num * size);
+    LIBC_NAMESPACE::inline_memset(ptr, 0, bytes);
   return ptr;
 }
 

libc/src/__support/freestore.h

@@ -17,9 +17,15 @@ namespace LIBC_NAMESPACE_DECL {
 /// removed in logarithmic time.
 class FreeStore {
 public:
+  FreeStore() = default;
+  FreeStore(const FreeStore &other) = delete;
+  FreeStore &operator=(const FreeStore &other) = delete;
+
   /// Sets the range of possible block sizes. This can only be called when the
   /// trie is empty.
-  void set_range(FreeTrie::SizeRange range) { large_trie.set_range(range); }
+  LIBC_INLINE void set_range(FreeTrie::SizeRange range) {
+    large_trie.set_range(range);
+  }
 
   /// Insert a free block. If the block is too small to be tracked, nothing
   /// happens.
@@ -42,10 +48,10 @@ class FreeStore {
   static constexpr size_t NUM_SMALL_SIZES =
       (MIN_LARGE_OUTER_SIZE - MIN_OUTER_SIZE) / ALIGNMENT;
 
-  static bool too_small(Block<> *block) {
+  LIBC_INLINE static bool too_small(Block<> *block) {
     return block->outer_size() < MIN_OUTER_SIZE;
   }
-  static bool is_small(Block<> *block) {
+  LIBC_INLINE static bool is_small(Block<> *block) {
     return block->outer_size() < MIN_LARGE_OUTER_SIZE;
   }
 

libc/src/__support/freetrie.cpp

@@ -0,0 +1,60 @@
+//===-- Implementation for freetrie ---------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "freetrie.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+void FreeTrie::remove(Node *node) {
+  LIBC_ASSERT(!empty() && "cannot remove from empty trie");
+  FreeList list = node;
+  list.pop();
+  Node *new_node = static_cast<Node *>(list.begin());
+  if (!new_node) {
+    // The freelist is empty. Replace the subtrie root with an arbitrary leaf.
+    // This is legal because there is no relationship between the size of the
+    // root and its children.
+    Node *leaf = node;
+    while (leaf->lower || leaf->upper)
+      leaf = leaf->lower ? leaf->lower : leaf->upper;
+    if (leaf == node) {
+      // If the root is a leaf, then removing it empties the subtrie.
+      replace_node(node, nullptr);
+      return;
+    }
+
+    replace_node(leaf, nullptr);
+    new_node = leaf;
+  }
+
+  // Copy the trie links to the new head.
+  new_node->lower = node->lower;
+  new_node->upper = node->upper;
+  new_node->parent = node->parent;
+  replace_node(node, new_node);
+  return;
+}
+
+void FreeTrie::replace_node(Node *node, Node *new_node) {
+  if (node->parent) {
+    Node *&parent_child =
+        node->parent->lower == node ? node->parent->lower : node->parent->upper;
+    LIBC_ASSERT(parent_child == node &&
+                "no reference to child node found in parent");
+    parent_child = new_node;
+  } else {
+    LIBC_ASSERT(root == node && "non-root node had no parent");
+    root = new_node;
+  }
+  if (node->lower)
+    node->lower->parent = new_node;
+  if (node->upper)
+    node->upper->parent = new_node;
+}
+
+} // namespace LIBC_NAMESPACE_DECL