Stop using UncheckedLock in WTF::MetaAllocator

https://bugs.webkit.org/show_bug.cgi?id=226396 Reviewed by Darin Adler. Stop using UncheckedLock in WTF::MetaAllocator, as it is being phased out in favor on Lock, which supports Clang thread safety analysis. Source/JavaScriptCore: * jit/ExecutableAllocator.cpp: (JSC::ExecutableAllocator::getLock const): * jit/ExecutableAllocator.h: (JSC::ExecutableAllocatorBase::WTF_RETURNS_LOCK): * tools/VMInspector.cpp: (JSC::VMInspector::isValidExecutableMemory): Source/WTF: * wtf/MetaAllocator.cpp: (WTF::MetaAllocator::release): (WTF::MetaAllocator::MetaAllocator): (WTF::MetaAllocator::allocate): (WTF::MetaAllocator::currentStatistics): * wtf/MetaAllocator.h: Canonical link: https://commits.webkit.org/238269@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@278232 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · May 29, 2021 · ebdfb12 · ebdfb12
1 parent 700c5aa
commit ebdfb12
Show file tree

Hide file tree

Showing 9 changed files with 66 additions and 34 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,20 @@
+2021-05-28  Chris Dumez  <cdumez@apple.com>
+
+        Stop using UncheckedLock in WTF::MetaAllocator
+        https://bugs.webkit.org/show_bug.cgi?id=226396
+
+        Reviewed by Darin Adler.
+
+        Stop using UncheckedLock in WTF::MetaAllocator, as it is being phased out in favor on Lock,
+        which supports Clang thread safety analysis.
+
+        * jit/ExecutableAllocator.cpp:
+        (JSC::ExecutableAllocator::getLock const):
+        * jit/ExecutableAllocator.h:
+        (JSC::ExecutableAllocatorBase::WTF_RETURNS_LOCK):
+        * tools/VMInspector.cpp:
+        (JSC::VMInspector::isValidExecutableMemory):
+
 2021-05-28  Chris Dumez  <cdumez@apple.com>
 
         Stop using UncheckedLock in JSDOMGlobalObject

diff --git a/Source/JavaScriptCore/jit/ExecutableAllocator.cpp b/Source/JavaScriptCore/jit/ExecutableAllocator.cpp
@@ -485,7 +485,7 @@ class FixedVMPoolExecutableAllocator final {
 #endif // ENABLE(JUMP_ISLANDS)
     }
 
-    UncheckedLock& getLock() { return m_lock; }
+    Lock& getLock() WTF_RETURNS_LOCK(m_lock) { return m_lock; }
 
     // Non atomic
     size_t bytesAllocated() 
@@ -547,7 +547,7 @@ class FixedVMPoolExecutableAllocator final {
     }
 
 #if ENABLE(JUMP_ISLANDS)
-    void handleWillBeReleased(const UncheckedLockHolder& locker, MetaAllocatorHandle& handle)
+    void handleWillBeReleased(const LockHolder& locker, MetaAllocatorHandle& handle)
     {
         if (m_islandsForJumpSourceLocation.isEmpty())
             return;
@@ -599,7 +599,7 @@ class FixedVMPoolExecutableAllocator final {
         return result;
     }
 
-    void freeJumpIslands(const UncheckedLockHolder&, Islands* islands)
+    void freeJumpIslands(const LockHolder&, Islands* islands)
     {
         for (CodeLocationLabel<ExecutableMemoryPtrTag> jumpIsland : islands->jumpIslands) {
             uintptr_t untaggedJumpIsland = bitwise_cast<uintptr_t>(jumpIsland.dataLocation());
@@ -610,14 +610,14 @@ class FixedVMPoolExecutableAllocator final {
         islands->jumpIslands.clear();
     }
 
-    void freeIslands(const UncheckedLockHolder& locker, Islands* islands)
+    void freeIslands(const LockHolder& locker, Islands* islands)
     {
         freeJumpIslands(locker, islands);
         m_islandsForJumpSourceLocation.remove(islands);
         delete islands;
     }
 
-    void* islandForJumpLocation(const UncheckedLockHolder& locker, uintptr_t jumpLocation, uintptr_t target, bool concurrently)
+    void* islandForJumpLocation(const LockHolder& locker, uintptr_t jumpLocation, uintptr_t target, bool concurrently)
     {
         Islands* islands = m_islandsForJumpSourceLocation.findExact(bitwise_cast<void*>(jumpLocation));
         if (islands) {
@@ -746,7 +746,7 @@ class FixedVMPoolExecutableAllocator final {
             return islandsPerPage;
         }
 
-        void release(const UncheckedLockHolder& locker, MetaAllocatorHandle& handle) final
+        void release(const LockHolder& locker, MetaAllocatorHandle& handle) final
         {
             m_fixedAllocator.handleWillBeReleased(locker, handle);
             Base::release(locker, handle);
@@ -847,7 +847,7 @@ class FixedVMPoolExecutableAllocator final {
     };
 #endif // ENABLE(JUMP_ISLANDS)
 
-    UncheckedLock m_lock;
+    Lock m_lock;
     PageReservation m_reservation;
 #if ENABLE(JUMP_ISLANDS)
     std::array<RegionAllocator, maxNumberOfRegions> m_allocators;
@@ -961,7 +961,7 @@ bool ExecutableAllocator::isValidExecutableMemory(const AbstractLocker& locker,
     return allocator->isInAllocatedMemory(locker, address);
 }
 
-UncheckedLock& ExecutableAllocator::getLock() const
+Lock& ExecutableAllocator::getLock() const
 {
     FixedVMPoolExecutableAllocator* allocator = g_jscConfig.fixedVMPoolExecutableAllocator;
     if (!allocator)

diff --git a/Source/JavaScriptCore/jit/ExecutableAllocator.h b/Source/JavaScriptCore/jit/ExecutableAllocator.h
@@ -77,7 +77,7 @@ class ExecutableAllocatorBase {
 
     static size_t committedByteCount() { return 0; }
 
-    UncheckedLock& getLock() const
+    Lock& getLock() const WTF_RETURNS_LOCK(m_lock)
     {
         return m_lock;
     }
@@ -87,7 +87,7 @@ class ExecutableAllocatorBase {
     ~ExecutableAllocatorBase() = default;
 
 private:
-    mutable UncheckedLock m_lock;
+    mutable Lock m_lock;
 };
 
 #if ENABLE(JIT)
@@ -176,7 +176,7 @@ class ExecutableAllocator : private ExecutableAllocatorBase {
 
     static size_t committedByteCount();
 
-    UncheckedLock& getLock() const;
+    Lock& getLock() const;
 
 #if ENABLE(JUMP_ISLANDS)
     JS_EXPORT_PRIVATE void* getJumpIslandTo(void* from, void* newDestination);

diff --git a/Source/JavaScriptCore/tools/VMInspector.cpp b/Source/JavaScriptCore/tools/VMInspector.cpp
@@ -64,7 +64,7 @@ void VMInspector::remove(VM* vm)
     m_vmList.remove(vm);
 }
 
-auto VMInspector::lock(Seconds timeout) -> Expected<Locker, Error>
+auto VMInspector::lock(Seconds timeout) -> Expected<UncheckedLockHolder, Error>
 {
     // This function may be called from a signal handler (e.g. via visit()). Hence,
     // it should only use APIs that are safe to call from signal handlers. This is
@@ -73,14 +73,14 @@ auto VMInspector::lock(Seconds timeout) -> Expected<Locker, Error>
     // We'll be doing sleep(1) between tries below. Hence, sleepPerRetry is 1.
     unsigned maxRetries = (timeout < Seconds::infinity()) ? timeout.value() : UINT_MAX;
 
-    Expected<Locker, Error> locker = Locker::tryLock(m_lock);
+    Expected<UncheckedLockHolder, Error> locker = UncheckedLockHolder::tryLock(m_lock);
     unsigned tryCount = 0;
     while (!locker && tryCount < maxRetries) {
         // We want the version of sleep from unistd.h. Cast to disambiguate.
 #if !OS(WINDOWS)
         (static_cast<unsigned (*)(unsigned)>(sleep))(1);
 #endif
-        locker = Locker::tryLock(m_lock);
+        locker = UncheckedLockHolder::tryLock(m_lock);
     }
 
     if (!locker)
@@ -109,11 +109,11 @@ static bool ensureIsSafeToLock(LockType& lock)
 void VMInspector::forEachVM(Function<FunctorStatus(VM&)>&& func)
 {
     VMInspector& inspector = instance();
-    Locker lock(inspector.getLock());
+    Locker lock { inspector.getLock() };
     inspector.iterate(func);
 }
 
-auto VMInspector::isValidExecutableMemory(const VMInspector::Locker&, void* machinePC) -> Expected<bool, Error>
+auto VMInspector::isValidExecutableMemory(const UncheckedLockHolder&, void* machinePC) -> Expected<bool, Error>
 {
 #if ENABLE(JIT)
     bool found = false;
@@ -145,7 +145,7 @@ auto VMInspector::isValidExecutableMemory(const VMInspector::Locker&, void* mach
 #endif
 }
 
-auto VMInspector::codeBlockForMachinePC(const VMInspector::Locker&, void* machinePC) -> Expected<CodeBlock*, Error>
+auto VMInspector::codeBlockForMachinePC(const UncheckedLockHolder&, void* machinePC) -> Expected<CodeBlock*, Error>
 {
 #if ENABLE(JIT)
     CodeBlock* codeBlock = nullptr;
@@ -172,7 +172,7 @@ auto VMInspector::codeBlockForMachinePC(const VMInspector::Locker&, void* machin
             return FunctorStatus::Continue; // Skip this VM.
         }
 
-        WTF::Locker locker { codeBlockSetLock };
+        Locker locker { codeBlockSetLock };
         vm.heap.forEachCodeBlockIgnoringJITPlans(locker, [&] (CodeBlock* cb) {
             JITCode* jitCode = cb->jitCode().get();
             if (!jitCode) {

diff --git a/Source/JavaScriptCore/tools/VMInspector.h b/Source/JavaScriptCore/tools/VMInspector.h
@@ -43,8 +43,6 @@ class VMInspector {
         TimedOut
     };
 
-    typedef WTF::Locker<UncheckedLock> Locker;
-
     static VMInspector& instance();
 
     void add(VM*);
@@ -58,14 +56,14 @@ class VMInspector {
     };
 
     template <typename Functor>
-    void iterate(const Locker&, const Functor& functor) { iterate(functor); }
+    void iterate(const UncheckedLockHolder&, const Functor& functor) { iterate(functor); }
 
     JS_EXPORT_PRIVATE static void forEachVM(Function<FunctorStatus(VM&)>&&);
 
-    Expected<Locker, Error> lock(Seconds timeout = Seconds::infinity());
+    Expected<UncheckedLockHolder, Error> lock(Seconds timeout = Seconds::infinity());
 
-    Expected<bool, Error> isValidExecutableMemory(const Locker&, void*);
-    Expected<CodeBlock*, Error> codeBlockForMachinePC(const Locker&, void*);
+    Expected<bool, Error> isValidExecutableMemory(const UncheckedLockHolder&, void*);
+    Expected<CodeBlock*, Error> codeBlockForMachinePC(const UncheckedLockHolder&, void*);
 
     JS_EXPORT_PRIVATE static bool currentThreadOwnsJSLock(VM*);
     JS_EXPORT_PRIVATE static void gc(VM*);

diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
@@ -1,3 +1,20 @@
+2021-05-28  Chris Dumez  <cdumez@apple.com>
+
+        Stop using UncheckedLock in WTF::MetaAllocator
+        https://bugs.webkit.org/show_bug.cgi?id=226396
+
+        Reviewed by Darin Adler.
+
+        Stop using UncheckedLock in WTF::MetaAllocator, as it is being phased out in favor on Lock,
+        which supports Clang thread safety analysis.
+
+        * wtf/MetaAllocator.cpp:
+        (WTF::MetaAllocator::release):
+        (WTF::MetaAllocator::MetaAllocator):
+        (WTF::MetaAllocator::allocate):
+        (WTF::MetaAllocator::currentStatistics):
+        * wtf/MetaAllocator.h:
+
 2021-05-28  Robin Morisset  <rmorisset@apple.com>
 
         Fix LikelyDenseUnsignedIntegerSet::clear()

diff --git a/Source/WTF/wtf/MetaAllocator.cpp b/Source/WTF/wtf/MetaAllocator.cpp
@@ -62,7 +62,7 @@ void MetaAllocatorTracker::release(MetaAllocatorHandle& handle)
     m_allocations.remove(&handle);
 }
 
-void MetaAllocator::release(const UncheckedLockHolder&, MetaAllocatorHandle& handle)
+void MetaAllocator::release(const LockHolder&, MetaAllocatorHandle& handle)
 {
     if (handle.sizeInBytes()) {
         MemoryPtr start = handle.start();
@@ -124,7 +124,7 @@ void MetaAllocatorHandle::dump(PrintStream& out) const
     out.print(RawPointer(start().untaggedPtr()), "...", RawPointer(end().untaggedPtr()));
 }
 
-MetaAllocator::MetaAllocator(UncheckedLock& lock, size_t allocationGranule, size_t pageSize)
+MetaAllocator::MetaAllocator(Lock& lock, size_t allocationGranule, size_t pageSize)
     : m_allocationGranule(allocationGranule)
     , m_pageSize(pageSize)
     , m_bytesAllocated(0)
@@ -154,7 +154,7 @@ MetaAllocator::MetaAllocator(UncheckedLock& lock, size_t allocationGranule, size
     ASSERT(static_cast<size_t>(1) << m_logAllocationGranule == m_allocationGranule);
 }
 
-RefPtr<MetaAllocatorHandle> MetaAllocator::allocate(const UncheckedLockHolder&, size_t sizeInBytes)
+RefPtr<MetaAllocatorHandle> MetaAllocator::allocate(const LockHolder&, size_t sizeInBytes)
 {
     if (!sizeInBytes)
         return nullptr;
@@ -198,7 +198,7 @@ RefPtr<MetaAllocatorHandle> MetaAllocator::allocate(const UncheckedLockHolder&,
     return handle;
 }
 
-MetaAllocator::Statistics MetaAllocator::currentStatistics(const UncheckedLockHolder&)
+MetaAllocator::Statistics MetaAllocator::currentStatistics(const LockHolder&)
 {
     Statistics result;
     result.bytesAllocated = m_bytesAllocated;

diff --git a/Source/WTF/wtf/MetaAllocator.h b/Source/WTF/wtf/MetaAllocator.h
@@ -65,7 +65,7 @@ class MetaAllocator {
     using FreeSpacePtr = MetaAllocatorPtr<FreeSpacePtrTag>;
     using MemoryPtr = MetaAllocatorHandle::MemoryPtr;
 
-    WTF_EXPORT_PRIVATE MetaAllocator(UncheckedLock&, size_t allocationGranule, size_t pageSize = WTF::pageSize());
+    WTF_EXPORT_PRIVATE MetaAllocator(Lock&, size_t allocationGranule, size_t pageSize = WTF::pageSize());
 
     WTF_EXPORT_PRIVATE virtual ~MetaAllocator();
 
@@ -74,7 +74,7 @@ class MetaAllocator {
         Locker locker { m_lock };
         return allocate(locker, sizeInBytes);
     }
-    WTF_EXPORT_PRIVATE RefPtr<MetaAllocatorHandle> allocate(const UncheckedLockHolder&, size_t sizeInBytes);
+    WTF_EXPORT_PRIVATE RefPtr<MetaAllocatorHandle> allocate(const LockHolder&, size_t sizeInBytes);
 
     void trackAllocations(MetaAllocatorTracker* tracker)
     {
@@ -98,7 +98,7 @@ class MetaAllocator {
         Locker locker { m_lock };
         return currentStatistics(locker);
     }
-    WTF_EXPORT_PRIVATE Statistics currentStatistics(const UncheckedLockHolder&);
+    WTF_EXPORT_PRIVATE Statistics currentStatistics(const LockHolder&);
 
     // Add more free space to the allocator. Call this directly from
     // the constructor if you wish to operate the allocator within a
@@ -134,7 +134,7 @@ class MetaAllocator {
     // as there are Handles that refer to it.
 
     // Release a MetaAllocatorHandle.
-    WTF_EXPORT_PRIVATE virtual void release(const UncheckedLockHolder&, MetaAllocatorHandle&);
+    WTF_EXPORT_PRIVATE virtual void release(const LockHolder&, MetaAllocatorHandle&);
 private:
 
     friend class MetaAllocatorHandle;
@@ -197,7 +197,7 @@ class MetaAllocator {
     size_t m_bytesReserved;
     size_t m_bytesCommitted;
 
-    UncheckedLock& m_lock;
+    Lock& m_lock;
 
     MetaAllocatorTracker* m_tracker { nullptr };
 

diff --git a/Tools/TestWebKitAPI/Tests/WTF/MetaAllocator.cpp b/Tools/TestWebKitAPI/Tests/WTF/MetaAllocator.cpp
@@ -129,7 +129,7 @@ class MetaAllocatorTest: public testing::Test {
         }
 
     private:
-        UncheckedLock m_lock;
+        Lock m_lock;
         MetaAllocatorTest* m_parent;
     };