[Webkit-unassigned] [Bug 201292] New: Remove a bad assertion in ByteCodeParser::inlineCall().

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=201292

            Bug ID: 201292
           Summary: Remove a bad assertion in
                    ByteCodeParser::inlineCall().
           Product: WebKit
           Version: WebKit Nightly Build
          Hardware: Unspecified
                OS: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: JavaScriptCore
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: mark.lam at apple.com

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.  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.

Hence, we should just remove the 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.

<rdar://problem/54121659>

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190829/59788c47/attachment.html>