Skip to content

Commit 6f73908

Browse files
cdumezcdumez
committed
Stop using UncheckedLock in JSC::VMInspector
https://bugs.webkit.org/show_bug.cgi?id=226427 Reviewed by Mark Lam. Source/JavaScriptCore: 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: Canonical link: https://commits.webkit.org/238288@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@278251 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent acd83c5 commit 6f73908

8 files changed

Lines changed: 94 additions & 69 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,27 @@
1+
2021-05-29 Chris Dumez <cdumez@apple.com>
2+
3+
Stop using UncheckedLock in JSC::VMInspector
4+
https://bugs.webkit.org/show_bug.cgi?id=226427
5+
6+
Reviewed by Mark Lam.
7+
8+
Stop using UncheckedLock in JSC::VMInspector, as it is being phased out in favor
9+
of Lock, which supports Clang thread safety analysis.
10+
11+
* tools/HeapVerifier.cpp:
12+
(JSC::HeapVerifier::checkIfRecorded):
13+
* tools/SigillCrashAnalyzer.cpp:
14+
(JSC::SigillCrashAnalyzer::analyze):
15+
* tools/VMInspector.cpp:
16+
(JSC::VMInspector::isValidExecutableMemory):
17+
(JSC::VMInspector::codeBlockForMachinePC):
18+
(JSC::VMInspector::lock): Deleted.
19+
* tools/VMInspector.h:
20+
(JSC::VMInspector::WTF_RETURNS_LOCK):
21+
(JSC::VMInspector::WTF_REQUIRES_LOCK):
22+
(JSC::VMInspector::getLock): Deleted.
23+
(JSC::VMInspector::iterate): Deleted.
24+
125
2021-05-29 Chris Dumez <cdumez@apple.com>
226

327
Stop using UncheckedLock in JSC::ConcurrentJSLock

‎Source/JavaScriptCore/tools/HeapVerifier.cpp‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -442,16 +442,13 @@ void HeapVerifier::checkIfRecorded(uintptr_t candidateCell)
442442
{
443443
HeapCell* candidateHeapCell = reinterpret_cast<HeapCell*>(candidateCell);
444444

445-
VMInspector& inspector = VMInspector::instance();
446-
auto expectedLocker = inspector.lock(Seconds(2));
447-
if (!expectedLocker) {
448-
ASSERT(expectedLocker.error() == VMInspector::Error::TimedOut);
445+
auto& inspector = VMInspector::instance();
446+
if (!inspector.getLock().tryLockWithTimeout(2_s)) {
449447
dataLog("ERROR: Timed out while waiting to iterate VMs.");
450448
return;
451449
}
452-
453-
auto& locker = expectedLocker.value();
454-
inspector.iterate(locker, [&] (VM& vm) {
450+
Locker locker { AdoptLock, inspector.getLock() };
451+
inspector.iterate([&] (VM& vm) {
455452
if (!vm.heap.m_verifier)
456453
return VMInspector::FunctorStatus::Continue;
457454

‎Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -226,20 +226,18 @@ auto SigillCrashAnalyzer::analyze(SignalContext& context) -> CrashSource
226226
// itself crashes.
227227
context.dump();
228228

229-
VMInspector& inspector = VMInspector::instance();
229+
auto& inspector = VMInspector::instance();
230230

231231
// Use a timeout period of 2 seconds. The client is about to crash, and we don't
232232
// want to turn the crash into a hang by re-trying the lock for too long.
233-
auto expectedLocker = inspector.lock(Seconds(2));
234-
if (!expectedLocker) {
235-
ASSERT(expectedLocker.error() == VMInspector::Error::TimedOut);
233+
if (!inspector.getLock().tryLockWithTimeout(2_s)) {
236234
log("ERROR: Unable to analyze SIGILL. Timed out while waiting to iterate VMs.");
237235
break;
238236
}
239-
auto& locker = expectedLocker.value();
237+
Locker locker { AdoptLock, inspector.getLock() };
240238

241239
void* pc = context.machinePC.untaggedExecutableAddress();
242-
auto isInJITMemory = inspector.isValidExecutableMemory(locker, pc);
240+
auto isInJITMemory = inspector.isValidExecutableMemory(pc);
243241
if (!isInJITMemory) {
244242
log("ERROR: Timed out: not able to determine if pc %p is in valid JIT executable memory", pc);
245243
break;
@@ -265,7 +263,7 @@ auto SigillCrashAnalyzer::analyze(SignalContext& context) -> CrashSource
265263
log("instruction bits at pc %p is: 0x%08x", pc, wordAtPC);
266264
#endif
267265

268-
auto expectedCodeBlock = inspector.codeBlockForMachinePC(locker, pc);
266+
auto expectedCodeBlock = inspector.codeBlockForMachinePC(pc);
269267
if (!expectedCodeBlock) {
270268
if (expectedCodeBlock.error() == VMInspector::Error::TimedOut)
271269
log("ERROR: Timed out: not able to determine if pc %p is in a valid CodeBlock", pc);

‎Source/JavaScriptCore/tools/VMInspector.cpp‎

Lines changed: 6 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,6 @@
3636
#include <mutex>
3737
#include <wtf/Expected.h>
3838

39-
#if !OS(WINDOWS)
40-
#include <unistd.h>
41-
#endif
42-
4339
namespace JSC {
4440

4541
VMInspector& VMInspector::instance()
@@ -64,43 +60,16 @@ void VMInspector::remove(VM* vm)
6460
m_vmList.remove(vm);
6561
}
6662

67-
auto VMInspector::lock(Seconds timeout) -> Expected<UncheckedLockHolder, Error>
68-
{
69-
// This function may be called from a signal handler (e.g. via visit()). Hence,
70-
// it should only use APIs that are safe to call from signal handlers. This is
71-
// why we use unistd.h's sleep() instead of its alternatives.
72-
73-
// We'll be doing sleep(1) between tries below. Hence, sleepPerRetry is 1.
74-
unsigned maxRetries = (timeout < Seconds::infinity()) ? timeout.value() : UINT_MAX;
75-
76-
Expected<UncheckedLockHolder, Error> locker = UncheckedLockHolder::tryLock(m_lock);
77-
unsigned tryCount = 0;
78-
while (!locker && tryCount < maxRetries) {
79-
// We want the version of sleep from unistd.h. Cast to disambiguate.
80-
#if !OS(WINDOWS)
81-
(static_cast<unsigned (*)(unsigned)>(sleep))(1);
82-
#endif
83-
locker = UncheckedLockHolder::tryLock(m_lock);
84-
}
85-
86-
if (!locker)
87-
return makeUnexpected(Error::TimedOut);
88-
return locker;
89-
}
90-
9163
#if ENABLE(JIT)
92-
template<typename LockType>
93-
static bool ensureIsSafeToLock(LockType& lock)
64+
static bool ensureIsSafeToLock(Lock& lock)
9465
{
95-
unsigned maxRetries = 2;
66+
static constexpr unsigned maxRetries = 2;
9667
unsigned tryCount = 0;
97-
while (tryCount <= maxRetries) {
98-
bool success = lock.tryLock();
99-
if (success) {
68+
while (tryCount++ <= maxRetries) {
69+
if (lock.tryLock()) {
10070
lock.unlock();
10171
return true;
10272
}
103-
tryCount++;
10473
}
10574
return false;
10675
}
@@ -113,7 +82,7 @@ void VMInspector::forEachVM(Function<FunctorStatus(VM&)>&& func)
11382
inspector.iterate(func);
11483
}
11584

116-
auto VMInspector::isValidExecutableMemory(const UncheckedLockHolder&, void* machinePC) -> Expected<bool, Error>
85+
auto VMInspector::isValidExecutableMemory(void* machinePC) -> Expected<bool, Error>
11786
{
11887
#if ENABLE(JIT)
11988
bool found = false;
@@ -145,7 +114,7 @@ auto VMInspector::isValidExecutableMemory(const UncheckedLockHolder&, void* mach
145114
#endif
146115
}
147116

148-
auto VMInspector::codeBlockForMachinePC(const UncheckedLockHolder&, void* machinePC) -> Expected<CodeBlock*, Error>
117+
auto VMInspector::codeBlockForMachinePC(void* machinePC) -> Expected<CodeBlock*, Error>
149118
{
150119
#if ENABLE(JIT)
151120
CodeBlock* codeBlock = nullptr;

‎Source/JavaScriptCore/tools/VMInspector.h‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -48,22 +48,26 @@ class VMInspector {
4848
void add(VM*);
4949
void remove(VM*);
5050

51-
UncheckedLock& getLock() { return m_lock; }
51+
Lock& getLock() WTF_RETURNS_LOCK(m_lock) { return m_lock; }
5252

5353
enum class FunctorStatus {
5454
Continue,
5555
Done
5656
};
5757

58-
template <typename Functor>
59-
void iterate(const UncheckedLockHolder&, const Functor& functor) { iterate(functor); }
58+
template <typename Functor> void iterate(const Functor& functor) WTF_REQUIRES_LOCK(m_lock)
59+
{
60+
for (VM* vm = m_vmList.head(); vm; vm = vm->next()) {
61+
FunctorStatus status = functor(*vm);
62+
if (status == FunctorStatus::Done)
63+
return;
64+
}
65+
}
6066

6167
JS_EXPORT_PRIVATE static void forEachVM(Function<FunctorStatus(VM&)>&&);
6268

63-
Expected<UncheckedLockHolder, Error> lock(Seconds timeout = Seconds::infinity());
64-
65-
Expected<bool, Error> isValidExecutableMemory(const UncheckedLockHolder&, void*);
66-
Expected<CodeBlock*, Error> codeBlockForMachinePC(const UncheckedLockHolder&, void*);
69+
Expected<bool, Error> isValidExecutableMemory(void*) WTF_REQUIRES_LOCK(m_lock);
70+
Expected<CodeBlock*, Error> codeBlockForMachinePC(void*) WTF_REQUIRES_LOCK(m_lock);
6771

6872
JS_EXPORT_PRIVATE static bool currentThreadOwnsJSLock(VM*);
6973
JS_EXPORT_PRIVATE static void gc(VM*);
@@ -92,17 +96,8 @@ class VMInspector {
9296
static bool verifyCell(VM&, JSCell*);
9397

9498
private:
95-
template <typename Functor> void iterate(const Functor& functor)
96-
{
97-
for (VM* vm = m_vmList.head(); vm; vm = vm->next()) {
98-
FunctorStatus status = functor(*vm);
99-
if (status == FunctorStatus::Done)
100-
return;
101-
}
102-
}
103-
104-
UncheckedLock m_lock;
105-
DoublyLinkedList<VM> m_vmList;
99+
Lock m_lock;
100+
DoublyLinkedList<VM> m_vmList WTF_GUARDED_BY_LOCK(m_lock);
106101
};
107102

108103
} // namespace JSC

‎Source/WTF/ChangeLog‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,18 @@
1+
2021-05-29 Chris Dumez <cdumez@apple.com>
2+
3+
Stop using UncheckedLock in JSC::VMInspector
4+
https://bugs.webkit.org/show_bug.cgi?id=226427
5+
6+
Reviewed by Mark Lam.
7+
8+
Add Lock::tryLockWithTimeout(), similar to tryLock() but gives up after a
9+
specified timeout. This used to be implemented in VMInspector but I think
10+
Lock is a better place for it.
11+
12+
* wtf/Lock.cpp:
13+
(WTF::Lock::tryLockWithTimeout):
14+
* wtf/Lock.h:
15+
116
2021-05-29 Chris Dumez <cdumez@apple.com>
217

318
Adopt clang thread safety annotations in WTF::DataMutex

‎Source/WTF/wtf/Lock.cpp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@
2929
#include <wtf/LockAlgorithmInlines.h>
3030
#include <wtf/StackShotProfiler.h>
3131

32+
#if OS(WINDOWS)
33+
#include <windows.h>
34+
#else
35+
#include <unistd.h>
36+
#endif
37+
3238
namespace WTF {
3339

3440
static constexpr bool profileLockContention = false;
@@ -63,5 +69,24 @@ void UncheckedLock::safepointSlow()
6369
DefaultLockAlgorithm::safepointSlow(m_byte);
6470
}
6571

72+
bool Lock::tryLockWithTimeout(Seconds timeout)
73+
{
74+
// This function may be called from a signal handler (e.g. via visit()). Hence,
75+
// it should only use APIs that are safe to call from signal handlers. This is
76+
// why we use unistd.h's sleep() instead of its alternatives.
77+
78+
// We'll be doing sleep(1) between tries below. Hence, sleepPerRetry is 1.
79+
unsigned maxRetries = (timeout < Seconds::infinity()) ? timeout.value() : std::numeric_limits<unsigned>::max();
80+
unsigned tryCount = 0;
81+
while (!tryLock() && tryCount++ <= maxRetries) {
82+
#if OS(WINDOWS)
83+
Sleep(1000);
84+
#else
85+
::sleep(1);
86+
#endif
87+
}
88+
return isHeld();
89+
}
90+
6691
} // namespace WTF
6792

‎Source/WTF/wtf/Lock.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include <wtf/LockAlgorithm.h>
3030
#include <wtf/Locker.h>
3131
#include <wtf/Noncopyable.h>
32+
#include <wtf/Seconds.h>
3233
#include <wtf/ThreadSafetyAnalysis.h>
3334

3435
namespace TestWebKitAPI {
@@ -162,6 +163,7 @@ class WTF_CAPABILITY_LOCK Lock : UncheckedLock {
162163
void lock() WTF_ACQUIRES_LOCK() { UncheckedLock::lock(); }
163164
bool tryLock() WTF_ACQUIRES_LOCK_IF(true) { return UncheckedLock::tryLock(); }
164165
bool try_lock() WTF_ACQUIRES_LOCK_IF(true) { return UncheckedLock::try_lock(); } // NOLINT: Intentional deviation to support std::scoped_lock.
166+
WTF_EXPORT_PRIVATE bool tryLockWithTimeout(Seconds timeout) WTF_ACQUIRES_LOCK_IF(true);
165167
void unlock() WTF_RELEASES_LOCK() { UncheckedLock::unlock(); }
166168
void unlockFairly() WTF_RELEASES_LOCK() { UncheckedLock::unlockFairly(); }
167169
void safepoint() { UncheckedLock::safepoint(); }

0 commit comments

Comments
 (0)