Changeset 286516 in webkit

Timestamp:

Dec 3, 2021 2:48:23 PM (2 years ago)

Author:

fpizlo@apple.com

Message:

[libpas] Bitfit allocator has a wrong assertion when a page's max_free is enough for the size of an allocation, not enough for that allocation's size class, and the object of that size is not aligned to the currently requested alignment
​https://bugs.webkit.org/show_bug.cgi?id=233831

Reviewed by Yusuke Suzuki.

What a combination of conditions:

• We just failed bitfit allocation in a page, which gives us some max_free (aka largest_available), and the allocation had nontrivial alignment.
• The max_free is smaller than the size class.
• The max_free is larger than the requested size.
• The max_free object is not aligned to the requested alignment.

The code handles this fine, but has a wrong assertion about it.

This change fixes the assertion and adds a test that deterministically reproduced the issue.

• libpas/libpas.xcodeproj/project.pbxproj:
• libpas/src/libpas/pas_bitfit_allocator.c:

(pas_bitfit_allocator_finish_failing):

• libpas/src/libpas/pas_bitfit_allocator_inlines.h:

(pas_bitfit_allocator_try_allocate):

• libpas/src/test/BitfitTests.cpp: Added.

(std::getBitfitSizeClasses):
(std::assertSizeClasses):
(std::testAllocateAlignedSmallerThanSizeClassAndSmallerThanLargestAvailable):
(addBitfitTests):

• libpas/src/test/TestHarness.cpp:

(main):

Location:

trunk/Source/bmalloc

Files:

• ChangeLog (modified) (1 diff)
• libpas/libpas.xcodeproj/project.pbxproj (modified) (4 diffs)
• libpas/src/libpas/pas_bitfit_allocator.c (modified) (3 diffs)
• libpas/src/libpas/pas_bitfit_allocator_inlines.h (modified) (2 diffs)
• libpas/src/test/BitfitTests.cpp (added)
• libpas/src/test/TestHarness.cpp (modified) (2 diffs)

trunk/Source/bmalloc/ChangeLog

@@ +1 @@
+2021-12-03  Filip Pizlo  <fpizlo@apple.com>
+
+        [libpas] Bitfit allocator has a wrong assertion when a page's max_free is enough for the size of an allocation, not enough for that allocation's size class, and the object of that size is not aligned to the currently requested alignment
+        https://bugs.webkit.org/show_bug.cgi?id=233831
+
+        Reviewed by Yusuke Suzuki.
+
+        What a combination of conditions:
+        
+        - We just failed bitfit allocation in a page, which gives us some max_free (aka largest_available), and the allocation had nontrivial alignment.
+        - The max_free is smaller than the size class.
+        - The max_free is larger than the requested size.
+        - The max_free object is not aligned to the requested alignment.
+        
+        The code handles this fine, but has a wrong assertion about it.
+
+        This change fixes the assertion and adds a test that deterministically reproduced the issue.
+
+        * libpas/libpas.xcodeproj/project.pbxproj:
+        * libpas/src/libpas/pas_bitfit_allocator.c:
+        (pas_bitfit_allocator_finish_failing):
+        * libpas/src/libpas/pas_bitfit_allocator_inlines.h:
+        (pas_bitfit_allocator_try_allocate):
+        * libpas/src/test/BitfitTests.cpp: Added.
+        (std::getBitfitSizeClasses):
+        (std::assertSizeClasses):
+        (std::testAllocateAlignedSmallerThanSizeClassAndSmallerThanLargestAvailable):
+        (addBitfitTests):
+        * libpas/src/test/TestHarness.cpp:
+        (main):
+
 2021-12-02  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/bmalloc/libpas/libpas.xcodeproj/project.pbxproj

@@ -569 +569 @@
                 2C85DC4127128F0F00367905 /* pas_try_allocate_intrinsic.h in Headers */ = {isa = PBXBuildFile; fileRef = 2C85DC4027128F0F00367905 /* pas_try_allocate_intrinsic.h */; };
                 2C91E5502718DA9A00D67FF9 /* pas_size_lookup_mode.h in Headers */ = {isa = PBXBuildFile; fileRef = 2C91E54F2718DA9A00D67FF9 /* pas_size_lookup_mode.h */; };
+                2CE2AE35275A953E00D02BBC /* BitfitTests.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2CE2AE34275A953E00D02BBC /* BitfitTests.cpp */; };
 /* End PBXBuildFile section */
 
@@ -1253 +1254 @@
                 2C85DC4027128F0F00367905 /* pas_try_allocate_intrinsic.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = pas_try_allocate_intrinsic.h; sourceTree = "<group>"; };
                 2C91E54F2718DA9A00D67FF9 /* pas_size_lookup_mode.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = pas_size_lookup_mode.h; sourceTree = "<group>"; };
+                2CE2AE34275A953E00D02BBC /* BitfitTests.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = BitfitTests.cpp; sourceTree = "<group>"; };
 /* End PBXFileReference section */
 
@@ -1345 +1347 @@
                         children = (
                                 0FDEA45D228B651B0085E340 /* BitfieldVectorTests.cpp */,
+                                2CE2AE34275A953E00D02BBC /* BitfitTests.cpp */,
                                 0F53181022C954ED003F7B6A /* BitvectorTests.cpp */,
                                 0F31A66723E8B336002C0CA3 /* CartesianTreeTests.cpp */,
@@ -2585 +2588 @@
                                 0FD48B6723B589910026C46D /* IsoHeapPartialAndBaselineTests.cpp in Sources */,
                                 0F5B6094235E919900CAE629 /* IsoHeapReservedMemoryTests.cpp in Sources */,
+                                2CE2AE35275A953E00D02BBC /* BitfitTests.cpp in Sources */,
                                 0F5193E7266AE5D400483A2C /* JITHeapTests.cpp in Sources */,
                                 0FC64191213745FA0040CE5E /* LargeFreeHeapTests.cpp in Sources */,

trunk/Source/bmalloc/libpas/src/libpas/pas_bitfit_allocator.c

@@ -206 +206 @@
                                                      pas_bitfit_page_config* config)
 {
+    static const bool verbose = false;
+    
     pas_bitfit_directory* directory;
     pas_bitfit_size_class* size_class;
@@ -222 +224 @@
 
     view_index = view->index;
+
+    if (verbose) {
+        pas_log("Finishing failing in view %p, size = %zu, alignment = %zu, largest_available = %zu\n",
+                view, size, alignment, largest_available);
+    }
 
     /* If we're still on the view that the allocator was on and we found that this view no longer
@@ -243 +250 @@
             directory, index, largest_available >> config->base.min_align_shift,
             "processing on finish_failing");
-        
-        PAS_TESTING_ASSERT(largest_available < size);
+
+        /* If we're doing an aligned allocation, then we might now skip over this view even though the
+           size we were allocating would have fit. The reason why we're doing it is that the largest size
+           we could have fit is smaller than the size class, and although it's big enough for the size being
+           requested, it's not aligned properly. */
+        PAS_TESTING_ASSERT(largest_available < size
+                           || alignment > pas_page_base_config_min_align(config->base));
+        
         PAS_TESTING_ASSERT(largest_available < size_class->size);
         

trunk/Source/bmalloc/libpas/src/libpas/pas_bitfit_allocator_inlines.h

@@ -88 +88 @@
             allocator->view = view;
         }
+
+        if (verbose)
+            pas_log("Allocating in view %p\n", view);
 
         bytes_committed = 0;
@@ -179 +182 @@
         if (verbose)
             pas_log("bitfit allocation succeeded with %p\n", (void*)bitfit_result.u.result);
+
+        PAS_TESTING_ASSERT(pas_is_aligned(bitfit_result.u.result, alignment));
         
         return pas_fast_path_allocation_result_create_success(bitfit_result.u.result);

trunk/Source/bmalloc/libpas/src/test/TestHarness.cpp

@@ -336 +336 @@
 
 void addBitfieldVectorTests();
+void addBitfitTests();
 void addBitvectorTests();
 void addCartesianTreeTests();
@@ -698 +699 @@
     // Run the rest of the tests in alphabetical order.
     ADD_SUITE(BitfieldVector);
+    ADD_SUITE(Bitfit);
     ADD_SUITE(Bitvector);
     ADD_SUITE(CartesianTree);