Title: [238421] trunk
- Revision
- 238421
- Author
- [email protected]
- Date
- 2018-11-21 09:37:54 -0800 (Wed, 21 Nov 2018)
Log Message
Remove invalid assertion in VMTraps::SignalSender's SignalAction.
https://bugs.webkit.org/show_bug.cgi?id=191856
<rdar://problem/46089992>
Reviewed by Yusuke Suzuki.
JSTests:
* stress/regress-191856.js: Added.
- this test is skipped for now until we have a fix for webkit.org/b/191855.
Source/_javascript_Core:
The ASSERT(vm.traps().needTrapHandling()) assertion in SignalSender's SigAction
function is invalid because we can't be sure that the trap has been handled yet
by the time the trap fires. This is because the main thread may also check traps
(in LLInt, baseline JIT and VM runtime code). There's a race to handle the trap.
Hence, the SigAction cannot assume that the trap still needs handling by the time
it is executed. This patch removed the invalid assertion.
Also renamed m_trapSet to m_condition because it is a AutomaticThreadCondition,
and all the ways it is used is as a condvar. The m_trapSet name doesn't seem
appropriate nor meaningful.
* runtime/VMTraps.cpp:
(JSC::VMTraps::tryInstallTrapBreakpoints):
- Added a !needTrapHandling() check as an optimization: there's no need to install
VMTrap breakpoints if someone already beat us to handling the trap (remember,
the main thread is racing against the VMTraps signalling thread to handle the
trap too). We only need to install the VMTraps breakpoints if we need DFG/FTL
compiled code to deopt so that they can check and handle pending traps. If the
trap has already been handled, it's better to not deopt any DFG/FTL functions.
(JSC::VMTraps::willDestroyVM):
(JSC::VMTraps::fireTrap):
(JSC::VMTraps::VMTraps):
* runtime/VMTraps.h:
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (238420 => 238421)
--- trunk/JSTests/ChangeLog 2018-11-21 17:36:32 UTC (rev 238420)
+++ trunk/JSTests/ChangeLog 2018-11-21 17:37:54 UTC (rev 238421)
@@ -1,3 +1,14 @@
+2018-11-20 Mark Lam <[email protected]>
+
+ Remove invalid assertion in VMTraps::SignalSender's SignalAction.
+ https://bugs.webkit.org/show_bug.cgi?id=191856
+ <rdar://problem/46089992>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/regress-191856.js: Added.
+ - this test is skipped for now until we have a fix for webkit.org/b/191855.
+
2018-11-21 Dominik Infuehr <[email protected]>
Enable JIT on ARM/Linux
Added: trunk/JSTests/stress/regress-191856.js (0 => 238421)
--- trunk/JSTests/stress/regress-191856.js (rev 0)
+++ trunk/JSTests/stress/regress-191856.js 2018-11-21 17:37:54 UTC (rev 238421)
@@ -0,0 +1,8 @@
+//@ skip
+//@ requireOptions("--watchdog=100")
+
+// FIMXE: skipping this test for now because it takes too long to run until we have a fix
+// for https://bugs.webkit.org/show_bug.cgi?id=191855.
+
+for (let i=0; i<1000; i++)
+ import(0);
Modified: trunk/Source/_javascript_Core/ChangeLog (238420 => 238421)
--- trunk/Source/_javascript_Core/ChangeLog 2018-11-21 17:36:32 UTC (rev 238420)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-11-21 17:37:54 UTC (rev 238421)
@@ -1,3 +1,36 @@
+2018-11-20 Mark Lam <[email protected]>
+
+ Remove invalid assertion in VMTraps::SignalSender's SignalAction.
+ https://bugs.webkit.org/show_bug.cgi?id=191856
+ <rdar://problem/46089992>
+
+ Reviewed by Yusuke Suzuki.
+
+ The ASSERT(vm.traps().needTrapHandling()) assertion in SignalSender's SigAction
+ function is invalid because we can't be sure that the trap has been handled yet
+ by the time the trap fires. This is because the main thread may also check traps
+ (in LLInt, baseline JIT and VM runtime code). There's a race to handle the trap.
+ Hence, the SigAction cannot assume that the trap still needs handling by the time
+ it is executed. This patch removed the invalid assertion.
+
+ Also renamed m_trapSet to m_condition because it is a AutomaticThreadCondition,
+ and all the ways it is used is as a condvar. The m_trapSet name doesn't seem
+ appropriate nor meaningful.
+
+ * runtime/VMTraps.cpp:
+ (JSC::VMTraps::tryInstallTrapBreakpoints):
+ - Added a !needTrapHandling() check as an optimization: there's no need to install
+ VMTrap breakpoints if someone already beat us to handling the trap (remember,
+ the main thread is racing against the VMTraps signalling thread to handle the
+ trap too). We only need to install the VMTraps breakpoints if we need DFG/FTL
+ compiled code to deopt so that they can check and handle pending traps. If the
+ trap has already been handled, it's better to not deopt any DFG/FTL functions.
+
+ (JSC::VMTraps::willDestroyVM):
+ (JSC::VMTraps::fireTrap):
+ (JSC::VMTraps::VMTraps):
+ * runtime/VMTraps.h:
+
2018-11-21 Dominik Infuehr <[email protected]>
Enable JIT on ARM/Linux
Modified: trunk/Source/_javascript_Core/runtime/VMTraps.cpp (238420 => 238421)
--- trunk/Source/_javascript_Core/runtime/VMTraps.cpp 2018-11-21 17:36:32 UTC (rev 238420)
+++ trunk/Source/_javascript_Core/runtime/VMTraps.cpp 2018-11-21 17:37:54 UTC (rev 238421)
@@ -148,6 +148,11 @@
if (!locker)
return; // Let the SignalSender try again later.
+ if (!needTrapHandling()) {
+ // Too late. Someone else already handled the trap.
+ return;
+ }
+
if (!foundCodeBlock->hasInstalledVMTrapBreakpoints())
foundCodeBlock->installVMTrapBreakpoints();
return;
@@ -190,7 +195,7 @@
public:
using Base = AutomaticThread;
SignalSender(const AbstractLocker& locker, VM& vm)
- : Base(locker, vm.traps().m_lock, vm.traps().m_trapSet.copyRef())
+ : Base(locker, vm.traps().m_lock, vm.traps().m_condition.copyRef())
, m_vm(vm)
{
static std::once_flag once;
@@ -211,9 +216,8 @@
}
ASSERT(currentCodeBlock->hasInstalledVMTrapBreakpoints());
VM& vm = *currentCodeBlock->vm();
- ASSERT(vm.traps().needTrapHandling()); // We should have already jettisoned this code block when we handled the trap.
- // We are in JIT code so it's safe to aquire this lock.
+ // We are in JIT code so it's safe to acquire this lock.
auto codeBlockSetLocker = holdLock(vm.heap.codeBlockSet().getLock());
bool sawCurrentCodeBlock = false;
vm.heap.forEachCodeBlockIgnoringJITPlans(codeBlockSetLocker, [&] (CodeBlock* codeBlock) {
@@ -279,7 +283,7 @@
auto locker = holdLock(*traps().m_lock);
if (traps().m_isShuttingDown)
return WorkResult::Stop;
- traps().m_trapSet->waitFor(*traps().m_lock, 1_ms);
+ traps().m_condition->waitFor(*traps().m_lock, 1_ms);
}
return WorkResult::Continue;
}
@@ -299,7 +303,7 @@
{
auto locker = holdLock(*m_lock);
if (!m_signalSender->tryStop(locker))
- m_trapSet->notifyAll(locker);
+ m_condition->notifyAll(locker);
}
m_signalSender->join();
m_signalSender = nullptr;
@@ -320,12 +324,12 @@
#if ENABLE(SIGNAL_BASED_VM_TRAPS)
if (!Options::usePollingTraps()) {
// sendSignal() can loop until it has confirmation that the mutator thread
- // has received the trap request. We'll call it from another trap so that
+ // has received the trap request. We'll call it from another thread so that
// fireTrap() does not block.
auto locker = holdLock(*m_lock);
if (!m_signalSender)
m_signalSender = adoptRef(new SignalSender(locker, vm()));
- m_trapSet->notifyAll(locker);
+ m_condition->notifyAll(locker);
}
#endif
}
@@ -384,7 +388,7 @@
VMTraps::VMTraps()
: m_lock(Box<Lock>::create())
- , m_trapSet(AutomaticThreadCondition::create())
+ , m_condition(AutomaticThreadCondition::create())
{
}
Modified: trunk/Source/_javascript_Core/runtime/VMTraps.h (238420 => 238421)
--- trunk/Source/_javascript_Core/runtime/VMTraps.h 2018-11-21 17:36:32 UTC (rev 238420)
+++ trunk/Source/_javascript_Core/runtime/VMTraps.h 2018-11-21 17:37:54 UTC (rev 238421)
@@ -146,7 +146,7 @@
#endif
Box<Lock> m_lock;
- Ref<AutomaticThreadCondition> m_trapSet;
+ Ref<AutomaticThreadCondition> m_condition;
union {
BitField m_needTrapHandling { 0 };
BitField m_trapsBitField;
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
