Changeset 238421 in webkit


Ignore:
Timestamp:
Nov 21, 2018 9:37:54 AM (5 years ago)
Author:
mark.lam@apple.com
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/JavaScriptCore:

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:
Location:
trunk
Files:
1 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r238414 r238421  
     12018-11-20  Mark Lam  <mark.lam@apple.com>
     2
     3        Remove invalid assertion in VMTraps::SignalSender's SignalAction.
     4        https://bugs.webkit.org/show_bug.cgi?id=191856
     5        <rdar://problem/46089992>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        * stress/regress-191856.js: Added.
     10        - this test is skipped for now until we have a fix for webkit.org/b/191855.
     11
    1122018-11-21  Dominik Infuehr  <dinfuehr@igalia.com>
    213

  • trunk/Source/JavaScriptCore/ChangeLog

    r238414 r238421  
     12018-11-20  Mark Lam  <mark.lam@apple.com>
     2
     3        Remove invalid assertion in VMTraps::SignalSender's SignalAction.
     4        https://bugs.webkit.org/show_bug.cgi?id=191856
     5        <rdar://problem/46089992>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        The ASSERT(vm.traps().needTrapHandling()) assertion in SignalSender's SigAction
     10        function is invalid because we can't be sure that the trap has been handled yet
     11        by the time the trap fires.  This is because the main thread may also check traps
     12        (in LLInt, baseline JIT and VM runtime code).  There's a race to handle the trap.
     13        Hence, the SigAction cannot assume that the trap still needs handling by the time
     14        it is executed.  This patch removed the invalid assertion.
     15
     16        Also renamed m_trapSet to m_condition because it is a AutomaticThreadCondition,
     17        and all the ways it is used is as a condvar.  The m_trapSet name doesn't seem
     18        appropriate nor meaningful.
     19
     20        * runtime/VMTraps.cpp:
     21        (JSC::VMTraps::tryInstallTrapBreakpoints):
     22        - Added a !needTrapHandling() check as an optimization: there's no need to install
     23          VMTrap breakpoints if someone already beat us to handling the trap (remember,
     24          the main thread is racing against the VMTraps signalling thread to handle the
     25          trap too).  We only need to install the VMTraps breakpoints if we need DFG/FTL
     26          compiled code to deopt so that they can check and handle pending traps.  If the
     27          trap has already been handled, it's better to not deopt any DFG/FTL functions.
     28
     29        (JSC::VMTraps::willDestroyVM):
     30        (JSC::VMTraps::fireTrap):
     31        (JSC::VMTraps::VMTraps):
     32        * runtime/VMTraps.h:
     33
    1342018-11-21  Dominik Infuehr  <dinfuehr@igalia.com>
    235

  • trunk/Source/JavaScriptCore/runtime/VMTraps.cpp

    r235605 r238421  
    149149            return; // Let the SignalSender try again later.
    150150
     151        if (!needTrapHandling()) {
     152            // Too late. Someone else already handled the trap.
     153            return;
     154        }
     155
    151156        if (!foundCodeBlock->hasInstalledVMTrapBreakpoints())
    152157            foundCodeBlock->installVMTrapBreakpoints();
    … …  
    191196    using Base = AutomaticThread;
    192197    SignalSender(const AbstractLocker& locker, VM& vm)
    193         : Base(locker, vm.traps().m_lock, vm.traps().m_trapSet.copyRef())
     198        : Base(locker, vm.traps().m_lock, vm.traps().m_condition.copyRef())
    194199        , m_vm(vm)
    195200    {
    … …  
    212217                ASSERT(currentCodeBlock->hasInstalledVMTrapBreakpoints());
    213218                VM& vm = *currentCodeBlock->vm();
    214                 ASSERT(vm.traps().needTrapHandling()); // We should have already jettisoned this code block when we handled the trap.
    215 
    216                 // We are in JIT code so it's safe to aquire this lock.
     219
     220                // We are in JIT code so it's safe to acquire this lock.
    217221                auto codeBlockSetLocker = holdLock(vm.heap.codeBlockSet().getLock());
    218222                bool sawCurrentCodeBlock = false;
    … …  
    280284            if (traps().m_isShuttingDown)
    281285                return WorkResult::Stop;
    282             traps().m_trapSet->waitFor(*traps().m_lock, 1_ms);
     286            traps().m_condition->waitFor(*traps().m_lock, 1_ms);
    283287        }
    284288        return WorkResult::Continue;
    … …  
    300304            auto locker = holdLock(*m_lock);
    301305            if (!m_signalSender->tryStop(locker))
    302                 m_trapSet->notifyAll(locker);
     306                m_condition->notifyAll(locker);
    303307        }
    304308        m_signalSender->join();
    … …  
    321325    if (!Options::usePollingTraps()) {
    322326        // sendSignal() can loop until it has confirmation that the mutator thread
    323         // has received the trap request. We'll call it from another trap so that
     327        // has received the trap request. We'll call it from another thread so that
    324328        // fireTrap() does not block.
    325329        auto locker = holdLock(*m_lock);
    326330        if (!m_signalSender)
    327331            m_signalSender = adoptRef(new SignalSender(locker, vm()));
    328         m_trapSet->notifyAll(locker);
     332        m_condition->notifyAll(locker);
    329333    }
    330334#endif
    … …  
    385389VMTraps::VMTraps()
    386390    : m_lock(Box<Lock>::create())
    387     , m_trapSet(AutomaticThreadCondition::create())
     391    , m_condition(AutomaticThreadCondition::create())
    388392{
    389393}

  • trunk/Source/JavaScriptCore/runtime/VMTraps.h

    r233123 r238421  
    147147
    148148    Box<Lock> m_lock;
    149     Ref<AutomaticThreadCondition> m_trapSet;
     149    Ref<AutomaticThreadCondition> m_condition;
    150150    union {
    151151        BitField m_needTrapHandling { 0 };
Note: See TracChangeset for help on using the changeset viewer.