Changeset 249279 in webkit
- Timestamp:
- Aug 29, 2019 10:04:07 AM (5 years ago)
- Location:
- trunk/Source/JavaScriptCore
- Files:
-
- 2 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/JavaScriptCore/ChangeLog
r249247 r249279 1 2019-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 1 60 2019-08-28 Mark Lam <mark.lam@apple.com> 2 61 -
trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
r249075 r249279 1567 1567 const Instruction* savedCurrentInstruction = m_currentInstruction; 1568 1568 CodeSpecializationKind specializationKind = InlineCallFrame::specializationKindFor(kind); 1569 1570 ASSERT(inliningCost(callee, argumentCountIncludingThis, kind) != UINT_MAX); 1571 1569 1572 1570 CodeBlock* codeBlock = callee.functionExecutable()->baselineCodeBlockFor(specializationKind); 1573 1571 insertChecks(codeBlock);
Note: See TracChangeset
for help on using the changeset viewer.