Changeset 249279 in webkit


Ignore:
Timestamp:
Aug 29, 2019 10:04:07 AM (5 years ago)
Author:
mark.lam@apple.com
Message:

Remove a bad assertion in ByteCodeParser::inlineCall().
https://bugs.webkit.org/show_bug.cgi?id=201292
<rdar://problem/54121659>

Reviewed by Michael Saboff.

In the DFG bytecode parser, we've already computed the inlining cost of a candidate
inlining target, and determine that it is worth inlining before invoking
ByteCodeParser::inlineCall(). However, in ByteCodeParser::inlineCall(), it
recomputes the inlining cost again only for the purpose of asserting that it isn't
too high.

Not consider a badly written test that does the following:

function bar() {

...
foo(); Call in a hot loop here.
...

}

bar(); <===== foo is inlineable into bar here.
noInline(foo);
<===== Change mind, and make foo not inlineable.
bar();

With this bad test, the following racy scenario can occur:

  1. the first invocation of bar() gets hot, and a concurrent compile is kicked off.
  2. the compiler thread computes foo()'s inliningCost() and determines that it is worthy to be inlined, and will imminently call inlineCall().
  3. the mutator calls the noInline() test utility on foo(), thereby making it NOT inlineable.
  4. the compiler thread calls inlineCall(). In inlineCall(), it re-computes the inliningCost for foo() and now finds that it is not inlineable. An assertion failure follows.

Technically, the test is in error because noInline() shouldn't be used that way.
However, fuzzers that are not clued into noInline()'s proper usage may generate
code like this.

On the other hand, ByteCodeParser::inlineCall() should not be recomputing that the
inlining cost and asserting on it. The only reason inlineCall() is invoked is
because it was already previously determined that a target function is inlineable
based on its inlining cost. Today, in practice, I don't think we have any real
world condition where the mutator can affect the inlining cost of a target
function midway through execution. So, this assertion isn't a problem if no one
writes a test that abuses noInline(). However, should things change such that the
mutator is able to affect the inlining cost of a target function, then it is
incorrect for the compiler to assume that the inlining cost is immutable. Once
the compiler decides to inline a function, it should just follow through.

This patch removes this assertion in ByteCodeParser::inlineCall(). It is an
annoyance at best (for fuzzers), and at worst, incorrect if the mutator gains the
ability to affect the inlining cost of a target function.

  • dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::inlineCall):

Location:
trunk/Source/JavaScriptCore
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/ChangeLog

    r249247 r249279  
     12019-08-29  Mark Lam  <mark.lam@apple.com>
     2
     3        Remove a bad assertion in ByteCodeParser::inlineCall().
     4        https://bugs.webkit.org/show_bug.cgi?id=201292
     5        <rdar://problem/54121659>
     6
     7        Reviewed by Michael Saboff.
     8
     9        In the DFG bytecode parser, we've already computed the inlining cost of a candidate
     10        inlining target, and determine that it is worth inlining before invoking
     11        ByteCodeParser::inlineCall().  However, in ByteCodeParser::inlineCall(), it
     12        recomputes the inlining cost again only for the purpose of asserting that it isn't
     13        too high.
     14
     15        Not consider a badly written test that does the following:
     16
     17            function bar() {
     18                ...
     19                foo(); // Call in a hot loop here.
     20                ...
     21            }
     22
     23            bar(); // <===== foo is inlineable into bar here.
     24            noInline(foo); // <===== Change mind, and make foo not inlineable.
     25            bar();
     26
     27        With this bad test, the following racy scenario can occur:
     28
     29        1. the first invocation of bar() gets hot, and a concurrent compile is kicked off.
     30        2. the compiler thread computes foo()'s inliningCost() and determines that it is
     31           worthy to be inlined, and will imminently call inlineCall().
     32        3. the mutator calls the noInline() test utility on foo(), thereby making it NOT
     33           inlineable.
     34        4. the compiler thread calls inlineCall().  In inlineCall(), it re-computes the
     35           inliningCost for foo() and now finds that it is not inlineable.  An assertion
     36           failure follows.
     37
     38        Technically, the test is in error because noInline() shouldn't be used that way.
     39        However, fuzzers that are not clued into noInline()'s proper usage may generate
     40        code like this.
     41
     42        On the other hand, ByteCodeParser::inlineCall() should not be recomputing that the
     43        inlining cost and asserting on it.  The only reason inlineCall() is invoked is
     44        because it was already previously determined that a target function is inlineable
     45        based on its inlining cost.  Today, in practice, I don't think we have any real
     46        world condition where the mutator can affect the inlining cost of a target
     47        function midway through execution.  So, this assertion isn't a problem if no one
     48        writes a test that abuses noInline().  However, should things change such that the
     49        mutator is able to affect the inlining cost of a target function, then it is
     50        incorrect for the compiler to assume that the inlining cost is immutable.  Once
     51        the compiler decides to inline a function, it should just follow through.
     52
     53        This patch removes this assertion in ByteCodeParser::inlineCall().  It is an
     54        annoyance at best (for fuzzers), and at worst, incorrect if the mutator gains the
     55        ability to affect the inlining cost of a target function.
     56
     57        * dfg/DFGByteCodeParser.cpp:
     58        (JSC::DFG::ByteCodeParser::inlineCall):
     59
    1602019-08-28  Mark Lam  <mark.lam@apple.com>
    261
  • trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

    r249075 r249279  
    15671567    const Instruction* savedCurrentInstruction = m_currentInstruction;
    15681568    CodeSpecializationKind specializationKind = InlineCallFrame::specializationKindFor(kind);
    1569    
    1570     ASSERT(inliningCost(callee, argumentCountIncludingThis, kind) != UINT_MAX);
    1571    
     1569
    15721570    CodeBlock* codeBlock = callee.functionExecutable()->baselineCodeBlockFor(specializationKind);
    15731571    insertChecks(codeBlock);
Note: See TracChangeset for help on using the changeset viewer.