Stop using UncheckedLock in JSC::WasmCalleeRegistry

https://bugs.webkit.org/show_bug.cgi?id=226412

Reviewed by Darin Adler.

Stop using UncheckedLock in JSC::WasmCalleeRegistry, as it is being phased out in favor of
Lock, which supports Clang thread safety analysis.

* runtime/SamplingProfiler.cpp:
(JSC::FrameWalker::FrameWalker):
(JSC::FrameWalker::recordJITFrame):
(JSC::CFrameWalker::CFrameWalker):
(JSC::SamplingProfiler::takeSample):
* wasm/WasmCalleeRegistry.h:
(JSC::Wasm::CalleeRegistry::WTF_RETURNS_LOCK):
(JSC::Wasm::CalleeRegistry::WTF_REQUIRES_LOCK):
(JSC::Wasm::CalleeRegistry::getLock): Deleted.
(JSC::Wasm::CalleeRegistry::isValidCallee): Deleted.
* wasm/WasmFaultSignalHandler.cpp:
(JSC::Wasm::trapHandler):


Canonical link: https://commits.webkit.org/238278@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@278241 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,26 @@
+2021-05-29  Chris Dumez  <cdumez@apple.com>
+
+        Stop using UncheckedLock in JSC::WasmCalleeRegistry
+        https://bugs.webkit.org/show_bug.cgi?id=226412
+
+        Reviewed by Darin Adler.
+
+        Stop using UncheckedLock in JSC::WasmCalleeRegistry, as it is being phased out in favor of
+        Lock, which supports Clang thread safety analysis.
+
+        * runtime/SamplingProfiler.cpp:
+        (JSC::FrameWalker::FrameWalker):
+        (JSC::FrameWalker::recordJITFrame):
+        (JSC::CFrameWalker::CFrameWalker):
+        (JSC::SamplingProfiler::takeSample):
+        * wasm/WasmCalleeRegistry.h:
+        (JSC::Wasm::CalleeRegistry::WTF_RETURNS_LOCK):
+        (JSC::Wasm::CalleeRegistry::WTF_REQUIRES_LOCK):
+        (JSC::Wasm::CalleeRegistry::getLock): Deleted.
+        (JSC::Wasm::CalleeRegistry::isValidCallee): Deleted.
+        * wasm/WasmFaultSignalHandler.cpp:
+        (JSC::Wasm::trapHandler):
+
 2021-05-29  Mark Lam  <mark.lam@apple.com>
 
         VM::isTerminationException() should only be run on a non-null exception value.

Source/JavaScriptCore/runtime/SamplingProfiler.cpp

@@ -75,13 +75,12 @@ ALWAYS_INLINE static void reportStats()
 
 class FrameWalker {
 public:
-    FrameWalker(VM& vm, CallFrame* callFrame, const AbstractLocker& codeBlockSetLocker, const AbstractLocker& machineThreadsLocker, const Optional<UncheckedLockHolder>& wasmCalleeLocker)
+    FrameWalker(VM& vm, CallFrame* callFrame, const AbstractLocker& codeBlockSetLocker, const AbstractLocker& machineThreadsLocker)
         : m_vm(vm)
         , m_callFrame(callFrame)
         , m_entryFrame(vm.topEntryFrame)
         , m_codeBlockSetLocker(codeBlockSetLocker)
         , m_machineThreadsLocker(machineThreadsLocker)
-        , m_wasmCalleeLocker(wasmCalleeLocker)
     {
     }
 
@@ -124,8 +123,9 @@ class FrameWalker {
         stackTrace[m_depth] = UnprocessedStackFrame(codeBlock, unsafeCallee, callSiteIndex);
 #if ENABLE(WEBASSEMBLY)
         if (Wasm::isSupported() && unsafeCallee.isWasm()) {
+            assertIsHeld(Wasm::CalleeRegistry::singleton().getLock());
             auto* wasmCallee = unsafeCallee.asWasmCallee();
-            if (Wasm::CalleeRegistry::singleton().isValidCallee(*m_wasmCalleeLocker, wasmCallee)) {
+            if (Wasm::CalleeRegistry::singleton().isValidCallee(wasmCallee)) {
                 // At this point, Wasm::Callee would be dying (ref count is 0), but its fields are still live.
                 // And we can safely copy Wasm::IndexOrName even when any lock is held by suspended threads.
                 stackTrace[m_depth].wasmIndexOrName = wasmCallee->indexOrName();
@@ -201,7 +201,6 @@ class FrameWalker {
     EntryFrame* m_entryFrame;
     const AbstractLocker& m_codeBlockSetLocker;
     const AbstractLocker& m_machineThreadsLocker;
-    const Optional<UncheckedLockHolder>& m_wasmCalleeLocker;
     bool m_bailingOut { false };
     size_t m_depth { 0 };
 };
@@ -210,8 +209,8 @@ class CFrameWalker : public FrameWalker {
 public:
     typedef FrameWalker Base;
 
-    CFrameWalker(VM& vm, void* machineFrame, CallFrame* callFrame, const AbstractLocker& codeBlockSetLocker, const AbstractLocker& machineThreadsLocker, const Optional<UncheckedLockHolder>& wasmCalleeLocker)
-        : Base(vm, callFrame, codeBlockSetLocker, machineThreadsLocker, wasmCalleeLocker)
+    CFrameWalker(VM& vm, void* machineFrame, CallFrame* callFrame, const AbstractLocker& codeBlockSetLocker, const AbstractLocker& machineThreadsLocker)
+        : Base(vm, callFrame, codeBlockSetLocker, machineThreadsLocker)
         , m_machineFrame(machineFrame)
     {
     }
@@ -353,10 +352,10 @@ void SamplingProfiler::takeSample(const AbstractLocker&, Seconds& stackTraceProc
         Locker machineThreadsLocker { m_vm.heap.machineThreads().getLock() };
         Locker codeBlockSetLocker { m_vm.heap.codeBlockSet().getLock() };
         Locker executableAllocatorLocker { ExecutableAllocator::singleton().getLock() };
-        Optional<UncheckedLockHolder> wasmCalleesLocker;
+        Optional<LockHolder> wasmCalleesLocker;
 #if ENABLE(WEBASSEMBLY)
         if (Wasm::isSupported())
-            wasmCalleesLocker = Locker { Wasm::CalleeRegistry::singleton().getLock() };
+            wasmCalleesLocker.emplace(Wasm::CalleeRegistry::singleton().getLock());
 #endif
 
         auto didSuspend = m_jscExecutionThread->suspend();
@@ -407,11 +406,11 @@ void SamplingProfiler::takeSample(const AbstractLocker&, Seconds& stackTraceProc
             bool wasValidWalk;
             bool didRunOutOfVectorSpace;
             if (Options::sampleCCode()) {
-                CFrameWalker walker(m_vm, machineFrame, callFrame, codeBlockSetLocker, machineThreadsLocker, wasmCalleesLocker);
+                CFrameWalker walker(m_vm, machineFrame, callFrame, codeBlockSetLocker, machineThreadsLocker);
                 walkSize = walker.walk(m_currentFrames, didRunOutOfVectorSpace);
                 wasValidWalk = walker.wasValidWalk();
             } else {
-                FrameWalker walker(m_vm, callFrame, codeBlockSetLocker, machineThreadsLocker, wasmCalleesLocker);
+                FrameWalker walker(m_vm, callFrame, codeBlockSetLocker, machineThreadsLocker);
                 walkSize = walker.walk(m_currentFrames, didRunOutOfVectorSpace);
                 wasValidWalk = walker.wasValidWalk();
             }

Source/JavaScriptCore/wasm/WasmCalleeRegistry.h

@@ -41,7 +41,7 @@ class CalleeRegistry {
     static void initialize();
     static CalleeRegistry& singleton();
 
-    UncheckedLock& getLock() { return m_lock; }
+    Lock& getLock() WTF_RETURNS_LOCK(m_lock) { return m_lock; }
 
     void registerCallee(Callee* callee)
     {
@@ -55,12 +55,12 @@ class CalleeRegistry {
         m_calleeSet.remove(callee);
     }
 
-    const HashSet<Callee*>& allCallees(const AbstractLocker&)
+    const HashSet<Callee*>& allCallees() WTF_REQUIRES_LOCK(m_lock)
     {
         return m_calleeSet;
     }
 
-    bool isValidCallee(const AbstractLocker&, Callee* callee)
+    bool isValidCallee(Callee* callee)  WTF_REQUIRES_LOCK(m_lock)
     {
         if (!HashSet<Callee*>::isValidValue(callee))
             return false;
@@ -70,8 +70,8 @@ class CalleeRegistry {
     CalleeRegistry() = default;
 
 private:
-    UncheckedLock m_lock;
-    HashSet<Callee*> m_calleeSet;
+    Lock m_lock;
+    HashSet<Callee*> m_calleeSet WTF_GUARDED_BY_LOCK(m_lock);
 };
 
 } } // namespace JSC::Wasm

Source/JavaScriptCore/wasm/WasmFaultSignalHandler.cpp

@@ -84,7 +84,7 @@ static SignalAction trapHandler(Signal signal, SigInfo& sigInfo, PlatformRegiste
                     return true;
                 auto& calleeRegistry = CalleeRegistry::singleton();
                 Locker locker { calleeRegistry.getLock() };
-                for (auto* callee : calleeRegistry.allCallees(locker)) {
+                for (auto* callee : calleeRegistry.allCallees()) {
                     auto [start, end] = callee->range();
                     dataLogLnIf(WasmFaultSignalHandlerInternal::verbose, "function start: ", RawPointer(start), " end: ", RawPointer(end));
                     if (start <= faultingInstruction && faultingInstruction < end) {