Reland "base/allocator: Respect malloc() alignment guarantees in PartitionAlloc."

This reverts commit 25c28325.

Reason for revert: Updated the failing test.

> Original change's description:
> > base/allocator: Respect malloc() alignment guarantees in PartitionAlloc.
> >
> > Generally speaking, malloc() must return data suitably aligned for
> > std::max_align_t. In practice this means 16 bytes on 64 bit
> > architectures, whereas PartitionAlloc only provides 8 there. This is
> > problematic to use it as a malloc() replacement.
> >
> > This CL makes PartitionAlloc respect the alignment guarantees on 64 bit
> > architectures.
> >
> > Bug: 787153,998048
> > Change-Id: I860f9d0439d59f8d494306452b8cdbff515c8300
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2219498
> > Commit-Queue: Benoit L <lizeb@chromium.org>
> > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > Reviewed-by: Takashi Sakamoto <tasak@google.com>
> > Cr-Commit-Position: refs/heads/master@{#772662}
>
> TBR=tasak@google.com,haraken@chromium.org,lizeb@chromium.org
>
> Change-Id: Ic3266b40748d22cb25d91ab041db79da42851f4f
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 787153, 998048
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220085
> Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
> Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#772669}

# Not skipping CQ checks because this is a reland.

Bug: 787153, 998048
Change-Id: I15257083b8e651c2ec0516563e5b8728bc5f158d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2218280
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772748}

base/allocator/partition_allocator/partition_alloc.cc

@@ -62,7 +62,8 @@ static_assert(kGenericMaxDirectMapped <=
                   (1UL << 31) + kPageAllocationGranularity,
               "maximum direct mapped allocation");
 // Check that some of our zanier calculations worked out as expected.
-static_assert(kGenericSmallestBucket == 8, "generic smallest bucket");
+static_assert(kGenericSmallestBucket == alignof(std::max_align_t),
+              "generic smallest bucket");
 static_assert(kGenericMaxBucketed == 983040, "generic max bucketed");
 static_assert(kMaxSystemPagesPerSlotSpan < (1 << 8),
               "System pages per slot span must be less than 128.");

base/allocator/partition_allocator/partition_alloc_constants.h

@@ -6,6 +6,7 @@
 #define BASE_ALLOCATOR_PARTITION_ALLOCATOR_PARTITION_ALLOC_CONSTANTS_H_
 
 #include <limits.h>
+#include <cstddef>
 
 #include "base/allocator/partition_allocator/page_allocator_constants.h"
 #include "base/logging.h"
@@ -136,7 +137,18 @@ static const size_t kNumPartitionPagesPerSuperPage =
 // In terms of allocation sizes, order 0 covers 0, order 1 covers 1, order 2
 // covers 2->3, order 3 covers 4->7, order 4 covers 8->15.
 
-static const size_t kGenericMinBucketedOrder = 4;  // 8 bytes.
+static_assert(alignof(std::max_align_t) <= 16,
+              "PartitionAlloc doesn't support a fundamental alignment larger "
+              "than 16 bytes.");
+// PartitionAlloc should return memory properly aligned for any type, to behave
+// properly as a generic allocator. This is not strictly required as long as
+// types are explicitly allocated with PartitionAlloc, but is to use it as a
+// malloc() implementation, and generally to match malloc()'s behavior.
+//
+// In practice, this means 8 bytes alignment on 32 bit architectures, and 16
+// bytes on 64 bit ones.
+static const size_t kGenericMinBucketedOrder =
+    alignof(std::max_align_t) == 16 ? 5 : 4;  // 2^(order - 1), that is 16 or 8.
 // The largest bucketed order is 1 << (20 - 1), storing [512 KiB, 1 MiB):
 static const size_t kGenericMaxBucketedOrder = 20;
 static const size_t kGenericNumBucketedOrders =

base/allocator/partition_allocator/partition_alloc_unittest.cc

@@ -8,6 +8,7 @@
 #include <string.h>
 
 #include <algorithm>
+#include <cstddef>
 #include <limits>
 #include <memory>
 #include <vector>
@@ -2340,6 +2341,34 @@ TEST_F(PartitionAllocTest, Alignment) {
     generic_allocator.root()->Free(ptr);
 }
 
+TEST_F(PartitionAllocTest, FundamentalAlignment) {
+  // See the test above for details. Essentially, checking the bucket size is
+  // sufficient to ensure that alignment will always be respected, as long as
+  // the fundamental alignment is <= 16 bytes.
+  size_t fundamental_alignment = alignof(std::max_align_t);
+  for (size_t size = 0; size < base::kSystemPageSize; size++) {
+    // Allocate several pointers, as the first one in use in a size class will
+    // be aligned on a page boundary.
+    void* ptr = generic_allocator.root()->Alloc(size, "");
+    void* ptr2 = generic_allocator.root()->Alloc(size, "");
+    void* ptr3 = generic_allocator.root()->Alloc(size, "");
+
+    EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % fundamental_alignment,
+              static_cast<uintptr_t>(0));
+    EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr2) % fundamental_alignment,
+              static_cast<uintptr_t>(0));
+    EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr3) % fundamental_alignment,
+              static_cast<uintptr_t>(0));
+
+    EXPECT_EQ(PartitionAllocGetSize(ptr) % fundamental_alignment,
+              static_cast<uintptr_t>(0));
+
+    generic_allocator.root()->Free(ptr);
+    generic_allocator.root()->Free(ptr2);
+    generic_allocator.root()->Free(ptr3);
+  }
+}
+
 }  // namespace internal
 }  // namespace base
 

third_party/blink/renderer/platform/wtf/allocator/partitions_test.cc

@@ -3,9 +3,11 @@
 // found in the LICENSE file.
 
 #include "third_party/blink/renderer/platform/wtf/allocator/partitions.h"
+
+#include <vector>
+
 #include "base/allocator/partition_allocator/memory_reclaimer.h"
 #include "build/build_config.h"
-
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace WTF {
@@ -22,16 +24,27 @@ class PartitionsTest : public ::testing::Test {
 };
 
 TEST_F(PartitionsTest, MemoryIsInitiallyCommitted) {
+  // std::vector to explicitly not use PartitionAlloc.
+  std::vector<void*> allocated_pointers;
+
   size_t committed_before = Partitions::TotalSizeOfCommittedPages();
-  void* data = Partitions::BufferMalloc(1, "");
-  ASSERT_TRUE(data);
+  // Need to allocate enough memory to require a new super page. Unless nothing
+  // else in the process has allocated anything, this can be after several
+  // iterations.
+  while (Partitions::TotalSizeOfCommittedPages() == committed_before) {
+    void* data = Partitions::BufferMalloc(100, "");
+    ASSERT_TRUE(data);
+    allocated_pointers.push_back(data);
+  }
   size_t committed_after = Partitions::TotalSizeOfCommittedPages();
 
   // No buffer data committed initially, hence committed size increases.
   EXPECT_GT(committed_after, committed_before);
   // Increase is larger than the allocation.
-  EXPECT_GT(committed_after, committed_before + 1);
-  Partitions::BufferFree(data);
+  EXPECT_GT(committed_after, committed_before + allocated_pointers.size());
+
+  for (void* data : allocated_pointers)
+    Partitions::BufferFree(data);
 
   // Decommit is not triggered by deallocation.
   size_t committed_after_free = Partitions::TotalSizeOfCommittedPages();
@@ -39,12 +52,19 @@ TEST_F(PartitionsTest, MemoryIsInitiallyCommitted) {
 }
 
 TEST_F(PartitionsTest, Decommit) {
+  std::vector<void*> allocated_pointers;
+
   size_t committed_before = Partitions::TotalSizeOfCommittedPages();
-  void* data = Partitions::BufferMalloc(1, "");
-  ASSERT_TRUE(data);
-  Partitions::BufferFree(data);
+  while (Partitions::TotalSizeOfCommittedPages() == committed_before) {
+    void* data = Partitions::BufferMalloc(100, "");
+    ASSERT_TRUE(data);
+    allocated_pointers.push_back(data);
+  }
   size_t committed_after = Partitions::TotalSizeOfCommittedPages();
 
+  for (void* data : allocated_pointers)
+    Partitions::BufferFree(data);
+
   // Decommit is not triggered by deallocation.
   EXPECT_GT(committed_after, committed_before);
   // Decommit works.