- Revision
- 278251
- Author
- [email protected]
- Date
- 2021-05-29 23:51:57 -0700 (Sat, 29 May 2021)
Log Message
Stop using UncheckedLock in JSC::VMInspector
https://bugs.webkit.org/show_bug.cgi?id=226427
Reviewed by Mark Lam.
Source/_javascript_Core:
Stop using UncheckedLock in JSC::VMInspector, as it is being phased out in favor
of Lock, which supports Clang thread safety analysis.
* tools/HeapVerifier.cpp:
(JSC::HeapVerifier::checkIfRecorded):
* tools/SigillCrashAnalyzer.cpp:
(JSC::SigillCrashAnalyzer::analyze):
* tools/VMInspector.cpp:
(JSC::VMInspector::isValidExecutableMemory):
(JSC::VMInspector::codeBlockForMachinePC):
(JSC::VMInspector::lock): Deleted.
* tools/VMInspector.h:
(JSC::VMInspector::WTF_RETURNS_LOCK):
(JSC::VMInspector::WTF_REQUIRES_LOCK):
(JSC::VMInspector::getLock): Deleted.
(JSC::VMInspector::iterate): Deleted.
Source/WTF:
Add Lock::tryLockWithTimeout(), similar to tryLock() but gives up after a
specified timeout. This used to be implemented in VMInspector but I think
Lock is a better place for it.
* wtf/Lock.cpp:
(WTF::Lock::tryLockWithTimeout):
* wtf/Lock.h:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (278250 => 278251)
--- trunk/Source/_javascript_Core/ChangeLog 2021-05-30 06:13:30 UTC (rev 278250)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-05-30 06:51:57 UTC (rev 278251)
@@ -1,5 +1,29 @@
2021-05-29 Chris Dumez <[email protected]>
+ Stop using UncheckedLock in JSC::VMInspector
+ https://bugs.webkit.org/show_bug.cgi?id=226427
+
+ Reviewed by Mark Lam.
+
+ Stop using UncheckedLock in JSC::VMInspector, as it is being phased out in favor
+ of Lock, which supports Clang thread safety analysis.
+
+ * tools/HeapVerifier.cpp:
+ (JSC::HeapVerifier::checkIfRecorded):
+ * tools/SigillCrashAnalyzer.cpp:
+ (JSC::SigillCrashAnalyzer::analyze):
+ * tools/VMInspector.cpp:
+ (JSC::VMInspector::isValidExecutableMemory):
+ (JSC::VMInspector::codeBlockForMachinePC):
+ (JSC::VMInspector::lock): Deleted.
+ * tools/VMInspector.h:
+ (JSC::VMInspector::WTF_RETURNS_LOCK):
+ (JSC::VMInspector::WTF_REQUIRES_LOCK):
+ (JSC::VMInspector::getLock): Deleted.
+ (JSC::VMInspector::iterate): Deleted.
+
+2021-05-29 Chris Dumez <[email protected]>
+
Stop using UncheckedLock in JSC::ConcurrentJSLock
https://bugs.webkit.org/show_bug.cgi?id=226428
Modified: trunk/Source/_javascript_Core/tools/HeapVerifier.cpp (278250 => 278251)
--- trunk/Source/_javascript_Core/tools/HeapVerifier.cpp 2021-05-30 06:13:30 UTC (rev 278250)
+++ trunk/Source/_javascript_Core/tools/HeapVerifier.cpp 2021-05-30 06:51:57 UTC (rev 278251)
@@ -442,16 +442,13 @@
{
HeapCell* candidateHeapCell = reinterpret_cast<HeapCell*>(candidateCell);
- VMInspector& inspector = VMInspector::instance();
- auto expectedLocker = inspector.lock(Seconds(2));
- if (!expectedLocker) {
- ASSERT(expectedLocker.error() == VMInspector::Error::TimedOut);
+ auto& inspector = VMInspector::instance();
+ if (!inspector.getLock().tryLockWithTimeout(2_s)) {
dataLog("ERROR: Timed out while waiting to iterate VMs.");
return;
}
-
- auto& locker = expectedLocker.value();
- inspector.iterate(locker, [&] (VM& vm) {
+ Locker locker { AdoptLock, inspector.getLock() };
+ inspector.iterate([&] (VM& vm) {
if (!vm.heap.m_verifier)
return VMInspector::FunctorStatus::Continue;
Modified: trunk/Source/_javascript_Core/tools/SigillCrashAnalyzer.cpp (278250 => 278251)
--- trunk/Source/_javascript_Core/tools/SigillCrashAnalyzer.cpp 2021-05-30 06:13:30 UTC (rev 278250)
+++ trunk/Source/_javascript_Core/tools/SigillCrashAnalyzer.cpp 2021-05-30 06:51:57 UTC (rev 278251)
@@ -226,20 +226,18 @@
// itself crashes.
context.dump();
- VMInspector& inspector = VMInspector::instance();
+ auto& inspector = VMInspector::instance();
// Use a timeout period of 2 seconds. The client is about to crash, and we don't
// want to turn the crash into a hang by re-trying the lock for too long.
- auto expectedLocker = inspector.lock(Seconds(2));
- if (!expectedLocker) {
- ASSERT(expectedLocker.error() == VMInspector::Error::TimedOut);
+ if (!inspector.getLock().tryLockWithTimeout(2_s)) {
log("ERROR: Unable to analyze SIGILL. Timed out while waiting to iterate VMs.");
break;
}
- auto& locker = expectedLocker.value();
+ Locker locker { AdoptLock, inspector.getLock() };
void* pc = context.machinePC.untaggedExecutableAddress();
- auto isInJITMemory = inspector.isValidExecutableMemory(locker, pc);
+ auto isInJITMemory = inspector.isValidExecutableMemory(pc);
if (!isInJITMemory) {
log("ERROR: Timed out: not able to determine if pc %p is in valid JIT executable memory", pc);
break;
@@ -265,7 +263,7 @@
log("instruction bits at pc %p is: 0x%08x", pc, wordAtPC);
#endif
- auto expectedCodeBlock = inspector.codeBlockForMachinePC(locker, pc);
+ auto expectedCodeBlock = inspector.codeBlockForMachinePC(pc);
if (!expectedCodeBlock) {
if (expectedCodeBlock.error() == VMInspector::Error::TimedOut)
log("ERROR: Timed out: not able to determine if pc %p is in a valid CodeBlock", pc);
Modified: trunk/Source/_javascript_Core/tools/VMInspector.cpp (278250 => 278251)
--- trunk/Source/_javascript_Core/tools/VMInspector.cpp 2021-05-30 06:13:30 UTC (rev 278250)
+++ trunk/Source/_javascript_Core/tools/VMInspector.cpp 2021-05-30 06:51:57 UTC (rev 278251)
@@ -36,10 +36,6 @@
#include <mutex>
#include <wtf/Expected.h>
-#if !OS(WINDOWS)
-#include <unistd.h>
-#endif
-
namespace JSC {
VMInspector& VMInspector::instance()
@@ -64,43 +60,16 @@
m_vmList.remove(vm);
}
-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
- // why we use unistd.h's sleep() instead of its alternatives.
-
- // We'll be doing sleep(1) between tries below. Hence, sleepPerRetry is 1.
- unsigned maxRetries = (timeout < Seconds::infinity()) ? timeout.value() : UINT_MAX;
-
- 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 = UncheckedLockHolder::tryLock(m_lock);
- }
-
- if (!locker)
- return makeUnexpected(Error::TimedOut);
- return locker;
-}
-
#if ENABLE(JIT)
-template<typename LockType>
-static bool ensureIsSafeToLock(LockType& lock)
+static bool ensureIsSafeToLock(Lock& lock)
{
- unsigned maxRetries = 2;
+ static constexpr unsigned maxRetries = 2;
unsigned tryCount = 0;
- while (tryCount <= maxRetries) {
- bool success = lock.tryLock();
- if (success) {
+ while (tryCount++ <= maxRetries) {
+ if (lock.tryLock()) {
lock.unlock();
return true;
}
- tryCount++;
}
return false;
}
@@ -113,7 +82,7 @@
inspector.iterate(func);
}
-auto VMInspector::isValidExecutableMemory(const UncheckedLockHolder&, void* machinePC) -> Expected<bool, Error>
+auto VMInspector::isValidExecutableMemory(void* machinePC) -> Expected<bool, Error>
{
#if ENABLE(JIT)
bool found = false;
@@ -145,7 +114,7 @@
#endif
}
-auto VMInspector::codeBlockForMachinePC(const UncheckedLockHolder&, void* machinePC) -> Expected<CodeBlock*, Error>
+auto VMInspector::codeBlockForMachinePC(void* machinePC) -> Expected<CodeBlock*, Error>
{
#if ENABLE(JIT)
CodeBlock* codeBlock = nullptr;
Modified: trunk/Source/_javascript_Core/tools/VMInspector.h (278250 => 278251)
--- trunk/Source/_javascript_Core/tools/VMInspector.h 2021-05-30 06:13:30 UTC (rev 278250)
+++ trunk/Source/_javascript_Core/tools/VMInspector.h 2021-05-30 06:51:57 UTC (rev 278251)
@@ -48,7 +48,7 @@
void add(VM*);
void remove(VM*);
- UncheckedLock& getLock() { return m_lock; }
+ Lock& getLock() WTF_RETURNS_LOCK(m_lock) { return m_lock; }
enum class FunctorStatus {
Continue,
@@ -55,16 +55,20 @@
Done
};
- template <typename Functor>
- void iterate(const UncheckedLockHolder&, const Functor& functor) { iterate(functor); }
+ template <typename Functor> void iterate(const Functor& functor) WTF_REQUIRES_LOCK(m_lock)
+ {
+ for (VM* vm = m_vmList.head(); vm; vm = vm->next()) {
+ FunctorStatus status = functor(*vm);
+ if (status == FunctorStatus::Done)
+ return;
+ }
+ }
JS_EXPORT_PRIVATE static void forEachVM(Function<FunctorStatus(VM&)>&&);
- Expected<UncheckedLockHolder, Error> lock(Seconds timeout = Seconds::infinity());
+ Expected<bool, Error> isValidExecutableMemory(void*) WTF_REQUIRES_LOCK(m_lock);
+ Expected<CodeBlock*, Error> codeBlockForMachinePC(void*) WTF_REQUIRES_LOCK(m_lock);
- 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*);
JS_EXPORT_PRIVATE static void edenGC(VM*);
@@ -92,17 +96,8 @@
static bool verifyCell(VM&, JSCell*);
private:
- template <typename Functor> void iterate(const Functor& functor)
- {
- for (VM* vm = m_vmList.head(); vm; vm = vm->next()) {
- FunctorStatus status = functor(*vm);
- if (status == FunctorStatus::Done)
- return;
- }
- }
-
- UncheckedLock m_lock;
- DoublyLinkedList<VM> m_vmList;
+ Lock m_lock;
+ DoublyLinkedList<VM> m_vmList WTF_GUARDED_BY_LOCK(m_lock);
};
} // namespace JSC
Modified: trunk/Source/WTF/ChangeLog (278250 => 278251)
--- trunk/Source/WTF/ChangeLog 2021-05-30 06:13:30 UTC (rev 278250)
+++ trunk/Source/WTF/ChangeLog 2021-05-30 06:51:57 UTC (rev 278251)
@@ -1,5 +1,20 @@
2021-05-29 Chris Dumez <[email protected]>
+ Stop using UncheckedLock in JSC::VMInspector
+ https://bugs.webkit.org/show_bug.cgi?id=226427
+
+ Reviewed by Mark Lam.
+
+ Add Lock::tryLockWithTimeout(), similar to tryLock() but gives up after a
+ specified timeout. This used to be implemented in VMInspector but I think
+ Lock is a better place for it.
+
+ * wtf/Lock.cpp:
+ (WTF::Lock::tryLockWithTimeout):
+ * wtf/Lock.h:
+
+2021-05-29 Chris Dumez <[email protected]>
+
Adopt clang thread safety annotations in WTF::DataMutex
https://bugs.webkit.org/show_bug.cgi?id=226431
Modified: trunk/Source/WTF/wtf/Lock.cpp (278250 => 278251)
--- trunk/Source/WTF/wtf/Lock.cpp 2021-05-30 06:13:30 UTC (rev 278250)
+++ trunk/Source/WTF/wtf/Lock.cpp 2021-05-30 06:51:57 UTC (rev 278251)
@@ -29,6 +29,12 @@
#include <wtf/LockAlgorithmInlines.h>
#include <wtf/StackShotProfiler.h>
+#if OS(WINDOWS)
+#include <windows.h>
+#else
+#include <unistd.h>
+#endif
+
namespace WTF {
static constexpr bool profileLockContention = false;
@@ -63,5 +69,24 @@
DefaultLockAlgorithm::safepointSlow(m_byte);
}
+bool Lock::tryLockWithTimeout(Seconds timeout)
+{
+ // 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
+ // why we use unistd.h's sleep() instead of its alternatives.
+
+ // We'll be doing sleep(1) between tries below. Hence, sleepPerRetry is 1.
+ unsigned maxRetries = (timeout < Seconds::infinity()) ? timeout.value() : std::numeric_limits<unsigned>::max();
+ unsigned tryCount = 0;
+ while (!tryLock() && tryCount++ <= maxRetries) {
+#if OS(WINDOWS)
+ Sleep(1000);
+#else
+ ::sleep(1);
+#endif
+ }
+ return isHeld();
+}
+
} // namespace WTF
Modified: trunk/Source/WTF/wtf/Lock.h (278250 => 278251)
--- trunk/Source/WTF/wtf/Lock.h 2021-05-30 06:13:30 UTC (rev 278250)
+++ trunk/Source/WTF/wtf/Lock.h 2021-05-30 06:51:57 UTC (rev 278251)
@@ -29,6 +29,7 @@
#include <wtf/LockAlgorithm.h>
#include <wtf/Locker.h>
#include <wtf/Noncopyable.h>
+#include <wtf/Seconds.h>
#include <wtf/ThreadSafetyAnalysis.h>
namespace TestWebKitAPI {
@@ -162,6 +163,7 @@
void lock() WTF_ACQUIRES_LOCK() { UncheckedLock::lock(); }
bool tryLock() WTF_ACQUIRES_LOCK_IF(true) { return UncheckedLock::tryLock(); }
bool try_lock() WTF_ACQUIRES_LOCK_IF(true) { return UncheckedLock::try_lock(); } // NOLINT: Intentional deviation to support std::scoped_lock.
+ WTF_EXPORT_PRIVATE bool tryLockWithTimeout(Seconds timeout) WTF_ACQUIRES_LOCK_IF(true);
void unlock() WTF_RELEASES_LOCK() { UncheckedLock::unlock(); }
void unlockFairly() WTF_RELEASES_LOCK() { UncheckedLock::unlockFairly(); }
void safepoint() { UncheckedLock::safepoint(); }
