Changeset 287991 in webkit

Timestamp:

Jan 13, 2022 1:12:09 PM (2 years ago)

Author:

fpizlo@apple.com

Message:

[libpas] add assertions that we aren't switching to a NULL lock
​https://bugs.webkit.org/show_bug.cgi?id=235190

Reviewed by Yusuke Suzuki.

This adds a pas_panic call when pas_local_allocator_stop sees a NULL page->lock_ptr. That's one
possible explanation of a very rare crash I'm seeing where return_memory_to_page fails its assertion
that we are holding the page lock.

This also adds TESTING asserts in a bunch of other places. The PAS_TESTING_ASSERTS about this are in
places that are perf-sensitive, so we probably cannot assert in production. The hope behind those is
that it will help to catch this issue in test_pas.

• libpas/src/libpas/pas_local_allocator.c:

(stop_impl):

• libpas/src/libpas/pas_segregated_page.c:

(pas_segregated_page_switch_lock_and_rebias_while_ineligible_impl):

• libpas/src/libpas/pas_segregated_page_inlines.h:

(pas_segregated_page_lock_with_unbias_not_utility):
(pas_segregated_page_lock_with_unbias):
(pas_segregated_page_lock):
(pas_segregated_page_switch_lock_impl):
(pas_segregated_page_switch_lock_with_mode):

Location:

trunk/Source/bmalloc

Files:

• ChangeLog (modified) (1 diff)
• libpas/src/libpas/pas_local_allocator.c (modified) (1 diff)
• libpas/src/libpas/pas_segregated_page.c (modified) (2 diffs)
• libpas/src/libpas/pas_segregated_page_inlines.h (modified) (6 diffs)

trunk/Source/bmalloc/ChangeLog

@@ +1 @@
+2022-01-13  Filip Pizlo  <fpizlo@apple.com>
+
+        [libpas] add assertions that we aren't switching to a NULL lock
+        https://bugs.webkit.org/show_bug.cgi?id=235190
+
+        Reviewed by Yusuke Suzuki.
+
+        This adds a pas_panic call when pas_local_allocator_stop sees a NULL page->lock_ptr. That's one
+        possible explanation of a very rare crash I'm seeing where return_memory_to_page fails its assertion
+        that we are holding the page lock.
+
+        This also adds TESTING asserts in a bunch of other places. The PAS_TESTING_ASSERTS about this are in
+        places that are perf-sensitive, so we probably cannot assert in production. The hope behind those is
+        that it will help to catch this issue in test_pas.
+
+        * libpas/src/libpas/pas_local_allocator.c:
+        (stop_impl):
+        * libpas/src/libpas/pas_segregated_page.c:
+        (pas_segregated_page_switch_lock_and_rebias_while_ineligible_impl):
+        * libpas/src/libpas/pas_segregated_page_inlines.h:
+        (pas_segregated_page_lock_with_unbias_not_utility):
+        (pas_segregated_page_lock_with_unbias):
+        (pas_segregated_page_lock):
+        (pas_segregated_page_switch_lock_impl):
+        (pas_segregated_page_switch_lock_with_mode):
+
 2022-01-12  Filip Pizlo  <fpizlo@apple.com>
 

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

@@ -181 +181 @@
     if (!pas_segregated_page_switch_lock_with_mode(page, &held_lock, page_lock_mode, page_config))
         return false;
+
+    if (!pas_segregated_page_config_is_utility(page_config) && !held_lock)
+        pas_panic("Should be holding a lock after pas_segregated_page_switch_lock_with_mode in stop_impl\n");
     
     page_config.specialized_local_allocator_return_memory_to_page(

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

@@ -1 +1 @@
 /*
- * Copyright (c) 2018-2021 Apple Inc. All rights reserved.
+ * Copyright (c) 2018-2022 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -126 +126 @@
     
         page_lock = page->lock_ptr;
+        PAS_TESTING_ASSERT(page_lock);
 
         if (*held_lock == page_lock && *held_lock == &cache_node->page_lock) {

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

@@ -1 +1 @@
 /*
- * Copyright (c) 2018-2021 Apple Inc. All rights reserved.
+ * Copyright (c) 2018-2022 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -131 +131 @@
     pas_lock* lock_ptr)
 {
+    PAS_TESTING_ASSERT(lock_ptr);
+    
     *held_lock = lock_ptr;
     
@@ -152 +154 @@
     }
 
+    PAS_TESTING_ASSERT(lock_ptr);
+
     return pas_segregated_page_lock_with_unbias_not_utility(page, held_lock, lock_ptr);
 }
@@ -171 +175 @@
         
         lock_ptr = page->lock_ptr;
-        
+
         if (pas_segregated_page_lock_with_unbias(page, &held_lock_ignored, lock_ptr, page_config))
             return;
@@ -209 +213 @@
     held_lock_value = *held_lock;
     page_lock = page->lock_ptr;
+
+    PAS_TESTING_ASSERT(page_lock);
     
     if (PAS_LIKELY(held_lock_value == page_lock)) {
@@ -233 +239 @@
 
     switch (lock_mode) {
-    case pas_lock_lock_mode_try_lock:
-        return pas_lock_switch_with_mode(held_lock, page->lock_ptr, pas_lock_lock_mode_try_lock);
+    case pas_lock_lock_mode_try_lock: {
+        pas_lock* page_lock;
+        page_lock = page->lock_ptr;
+        PAS_TESTING_ASSERT(page_lock);
+        return pas_lock_switch_with_mode(held_lock, page_lock, pas_lock_lock_mode_try_lock);
+    }
 
     case pas_lock_lock_mode_lock: {