Changeset 261325 in webkit


Ignore:
Timestamp:
May 7, 2020, 12:25:32 PM (5 years ago)
Author:
mark.lam@apple.com
Message:

Add stack checks to the DFG and FTL bytecode parser.
​https://bugs.webkit.org/show_bug.cgi?id=211547
<rdar://problem/62958880>

Reviewed by Yusuke Suzuki.

Source/JavaScriptCore:

Inlining can cause some level of recursion of the DFG bytecode parser. We should
do a stack check at each inlining check before recursing. If a stack overflow
appears to be imminent, then just refuse to inline, and therefore, don't recurse
deeper into the parser.

This issue is more noticeable on ASan debug builds where stack frames can be
humongous.

Removed the SUPPRESS_ASAN on cloberrize() and the associated comment from r260692.
It was a mis-diagnosis. The stack checks are what we need.

  • dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::handleVarargsInlining):
(JSC::DFG::ByteCodeParser::handleInlining):

  • dfg/DFGClobberize.h:

(JSC::DFG::clobberize):

  • dfg/DFGGraph.h:

Source/WTF:

Added a StackCheck::Scope RAII object to help verify that the default reserved
zone size is at least adequate for known work loads. If this the StackCheck::Scope
assertions fail, then we either need more stack checks, or the reserved zone size
needs to be increased.

Note that the assertions are usually only on in Debug builds. Ideally, we would
want to measure the reserved zone size with a Release build. To do that, we
can just set VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE to 1 unconditionally in
StackCheck.h and rebuild.

  • wtf/StackCheck.h:

(WTF::StackCheck::Scope::Scope):
(WTF::StackCheck::Scope::~Scope):
(WTF::StackCheck::Scope::isSafeToRecurse):
(WTF::StackCheck::StackCheck):

Location:
trunk/Source
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r261315 r261325  
     12020-05-07  Mark Lam  <mark.lam@apple.com>
     2
     3        Add stack checks to the DFG and FTL bytecode parser.
     4        https://bugs.webkit.org/show_bug.cgi?id=211547
     5        <rdar://problem/62958880>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        Inlining can cause some level of recursion of the DFG bytecode parser.  We should
     10        do a stack check at each inlining check before recursing.  If a stack overflow
     11        appears to be imminent, then just refuse to inline, and therefore, don't recurse
     12        deeper into the parser.
     13
     14        This issue is more noticeable on ASan debug builds where stack frames can be
     15        humongous.
     16
     17        Removed the SUPPRESS_ASAN on cloberrize() and the associated comment from r260692.
     18        It was a mis-diagnosis.  The stack checks are what we need.
     19
     20        * dfg/DFGByteCodeParser.cpp:
     21        (JSC::DFG::ByteCodeParser::handleVarargsInlining):
     22        (JSC::DFG::ByteCodeParser::handleInlining):
     23        * dfg/DFGClobberize.h:
     24        (JSC::DFG::clobberize):
     25        * dfg/DFGGraph.h:
     26
    1272020-05-07  Darin Adler  <darin@apple.com>
    228

  • trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

    r261313 r261325  
    19271927{
    19281928    VERBOSE_LOG("Handling inlining (Varargs)...\nStack: ", currentCodeOrigin(), "\n");
     1929
     1930    StackCheck::Scope stackChecker(m_graph.m_stackChecker);
     1931    if (!stackChecker.isSafeToRecurse()) {
     1932        VERBOSE_LOG("Bailing inlining (compiler thread stack overflow eminent).\nStack: ", currentCodeOrigin(), "\n");
     1933        return false;
     1934    }
    19291935    if (callLinkStatus.maxArgumentCountIncludingThis() > Options::maximumVarargsForInlining()) {
    19301936        VERBOSE_LOG("Bailing inlining: too many arguments for varargs inlining.\n");
    … …  
    20712077{
    20722078    VERBOSE_LOG("Handling inlining...\nStack: ", currentCodeOrigin(), "\n");
     2079
     2080    StackCheck::Scope stackChecker(m_graph.m_stackChecker);
     2081    if (!stackChecker.isSafeToRecurse()) {
     2082        VERBOSE_LOG("Bailing inlining (compiler thread stack overflow eminent).\nStack: ", currentCodeOrigin(), "\n");
     2083        return CallOptimizationResult::DidNothing;
     2084    }
    20732085   
    20742086    CodeSpecializationKind specializationKind = InlineCallFrame::specializationKindFor(kind);

  • trunk/Source/JavaScriptCore/dfg/DFGClobberize.h

    r261313 r261325  
    4040namespace JSC { namespace DFG {
    4141
    42 // FIXME: SUPPRESS_ASAN is needed here because ASan can mistakenly think that
    43 // we're accesing out of invalid bounds stack memory when we're not. For example,
    44 // in the CheckIsConstant case below, we compute:
    45 //    AdjacencyList(AdjacencyList::Fixed, node->child1())
    46 //
    47 // 1. The AdjacencyList constructor takes an Edge value.
    48 // 2. node->child1() returns an Edge&.
    49 // 3. Clang generates a call to __asan_memcpy to copy the return value of
    50 //    node->child1() to a temp local on the stack used for passing the Edge
    51 //    argument to the AdjacencyList constructor.
    52 // 4. Inside __asan_memcpy, it attempts to write to the temp local Edge in
    53 //    clobberize's frame (not __asan_memcpy's frame), and ASan will wrongly
    54 //    flag this as an invalid out of stack bounds write.
    55 //
    56 // This manifested with a debug ASan build.
    57 // See <rdar://problem/62362403>.
    58 
    5942template<typename ReadFunctor, typename WriteFunctor, typename DefFunctor>
    60 SUPPRESS_ASAN void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFunctor& write, const DefFunctor& def)
     43void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFunctor& write, const DefFunctor& def)
    6144{
    6245    clobberize(graph, node, read, write, def, [] { });

  • trunk/Source/JavaScriptCore/dfg/DFGGraph.h

    r260730 r261325  
    11/*
    2  * Copyright (C) 2011-2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2011-2020 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4242#include <wtf/BitVector.h>
    4343#include <wtf/HashMap.h>
    44 #include <wtf/Vector.h>
     44#include <wtf/StackCheck.h>
    4545#include <wtf/StdLibExtras.h>
    4646#include <wtf/StdUnorderedMap.h>
     47#include <wtf/Vector.h>
    4748
    4849namespace WTF {
    … …  
    10661067    void nextPhase() { m_prefix.phaseNumber++; }
    10671068
     1069    StackCheck m_stackChecker;
    10681070    VM& m_vm;
    10691071    Plan& m_plan;
    10701072    CodeBlock* m_codeBlock;
    10711073    CodeBlock* m_profiledBlock;
    1072    
     1074
    10731075    Vector<RefPtr<BasicBlock>, 8> m_blocks;
    10741076    Vector<BasicBlock*, 1> m_roots;

  • trunk/Source/WTF/ChangeLog

    r261264 r261325  
     12020-05-07  Mark Lam  <mark.lam@apple.com>
     2
     3        Add stack checks to the DFG and FTL bytecode parser.
     4        https://bugs.webkit.org/show_bug.cgi?id=211547
     5        <rdar://problem/62958880>
     6
     7        Reviewed by Yusuke Suzuki.
     8
     9        Added a StackCheck::Scope RAII object to help verify that the default reserved
     10        zone size is at least adequate for known work loads.  If this the StackCheck::Scope
     11        assertions fail, then we either need more stack checks, or the reserved zone size
     12        needs to be increased.
     13
     14        Note that the assertions are usually only on in Debug builds.  Ideally, we would
     15        want to measure the reserved zone size with a Release build.  To do that, we
     16        can just set VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE to 1 unconditionally in
     17        StackCheck.h and rebuild.
     18
     19        * wtf/StackCheck.h:
     20        (WTF::StackCheck::Scope::Scope):
     21        (WTF::StackCheck::Scope::~Scope):
     22        (WTF::StackCheck::Scope::isSafeToRecurse):
     23        (WTF::StackCheck::StackCheck):
     24
    1252020-05-06  Yoshiaki Jitsukawa  <yoshiaki.jitsukawa@sony.com> and Fujii Hironori  <Hironori.Fujii@sony.com>
    226

  • trunk/Source/WTF/wtf/StackCheck.h

    r252239 r261325  
    11/*
    2  * Copyright (C) 2019 Apple Inc. All rights reserved.
     2 * Copyright (C) 2019-2020 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    3131namespace WTF {
    3232
     33// We only enable the reserved zone size check by default on ASSERT_ENABLED
     34// builds (which usually mean Debug builds). However, it is more valuable to
     35// run this test on Release builds. That said, we don't want to do pay this
     36// cost always because we may need to do stack checks on hot paths too.
     37// Note also that we're deliberately using RELEASE_ASSERTs if the verification
     38// is turned on. This makes it easier for us to turn this on for Release builds
     39// for a reserved zone size calibration test runs.
     40
     41#define VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE ASSERT_ENABLED
     42
    3343class StackCheck {
    3444    WTF_MAKE_FAST_ALLOCATED;
    3545public:
    36     StackCheck(const StackBounds& bounds = Thread::current().stack(), size_t minReservedZone = StackBounds::DefaultReservedZone)
     46    class Scope {
     47    public:
     48        Scope(StackCheck& checker)
     49            : m_checker(checker)
     50        {
     51            m_checker.isSafeToRecurse();
     52#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
     53            RELEASE_ASSERT(checker.m_ownerThread == &Thread::current());
     54
     55            // Make sure that the stack interval between checks never exceed the
     56            // reservedZone size. If the interval exceeds the reservedZone size,
     57            // then we either need to:
     58            // 1. break the interval into smaller pieces (i.e. insert more checks), or
     59            // 2. use a larger reservedZone size.
     60            uint8_t* currentStackCheckpoint = reinterpret_cast<uint8_t*>(currentStackPointer());
     61            uint8_t* previousStackCheckpoint = m_checker.m_lastStackCheckpoint;
     62            RELEASE_ASSERT(previousStackCheckpoint - currentStackCheckpoint > 0);
     63            RELEASE_ASSERT(previousStackCheckpoint - currentStackCheckpoint < static_cast<ptrdiff_t>(m_checker.m_reservedZone));
     64
     65            m_savedLastStackCheckpoint = m_checker.m_lastStackCheckpoint;
     66            m_checker.m_lastStackCheckpoint = currentStackCheckpoint;
     67#endif
     68        }
     69
     70#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
     71        ~Scope()
     72        {
     73            m_checker.m_lastStackCheckpoint = m_savedLastStackCheckpoint;
     74        }
     75#endif
     76
     77        bool isSafeToRecurse() { return m_checker.isSafeToRecurse(); }
     78
     79    private:
     80        StackCheck& m_checker;
     81#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
     82        uint8_t* m_savedLastStackCheckpoint;
     83#endif
     84    };
     85
     86    StackCheck(const StackBounds& bounds = Thread::current().stack(), size_t minReservedZone = defaultReservedZoneSize)
    3787        : m_stackLimit(bounds.recursionLimit(minReservedZone))
     88#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
     89        , m_ownerThread(&Thread::current())
     90        , m_lastStackCheckpoint(reinterpret_cast<uint8_t*>(currentStackPointer()))
     91        , m_reservedZone(minReservedZone)
     92#endif
    3893    { }
    3994
    … …  
    4196
    4297private:
     98#if ASAN_ENABLED
     99    static constexpr size_t defaultReservedZoneSize = StackBounds::DefaultReservedZone * 2;
     100#else
     101    static constexpr size_t defaultReservedZoneSize = StackBounds::DefaultReservedZone;
     102#endif
     103
    43104    void* m_stackLimit;
     105#if VERIFY_STACK_CHECK_RESERVED_ZONE_SIZE
     106    Thread* m_ownerThread;
     107    uint8_t* m_lastStackCheckpoint;
     108    size_t m_reservedZone;
     109#endif
     110
     111    friend class Scope;
    44112};
    45113
Note: See TracChangeset for help on using the changeset viewer.