Bug 130295 – Crash beneath operationTearOffActivation running this JS compression demo

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Oliver Hunt]

* STEPS TO REPRODUCE
Steps to reproduce:
1. Go to http://nerget.com/compression/
2. Select Order 2 (precise)
3. Hit compress

<rdar://problem/16332337> Crash beneath operationTearOffActivation running this JS compression demo

[Mark Lam]

The issue is above the LLINT and baselineJIT.  If I disable the DFG, the issue does not manifest.

Adding some printfs to the slow paths for creating and tearing off the activation, I see that on the DFG run, the activation is created by the LLINT, but at tear off time, it is seeing a bad activation pointer.

[Mark Lam]

The added printfs say that the tear off (and manifestation of the issue) is being done in the baseline JIT.  There was an OSR entry to DFG code, but it encountered a speculation check failure and OSR exited back to the baseline JIT where this issue manifested:

Optimized decompress#EDGKb2:[0x7ff00ea3c6e0->0x7ff00ea30920->0x11605a970, NoneFunctionCall, 324] using DFGMode with DFG into 1706 bytes in 17.730957 ms.

Speculation failure in decompress#EDGKb2:[0x7ff00ea3c6e0->0x7ff00ea30920->0x11605a970, DFGFunctionCall, 324] with executeCounter = 0.000000/3869.000000, -1000, reoptimizationRetryCounter = 1, optimizationDelayCounter = 5, osrExitCounter = 0
    GPRs at time of exit: rax:0xffff0000fffffffe rdx:0xffff000000000002 rcx:0x36ba8c01ae9f rbx:0x7ff00bf00c18 rdi:0x1 rsi:0xe r8:0x7ff00bf00c18 r9:0x10b00db00 r10:0xeab3070d r12:0x7ff00eb01000 r13:0x5
    FPRs at time of exit: xmm0:3fd77d0d45000000:0.367008 xmm1:4000000000000000:2.000000 xmm2:4330000000000000:4503599627370496.000000 xmm3:3fb8f6859999999a:0.097512 xmm4:40310628cbd1244a:17.024060 xmm5:ffffffff:0.000000

    operationTearOffActivation @ exec 0x7fff5bc3a270 cb 0x7ff00ea30920 | 0xa | isOptimizing JIT NO

[Mark Lam]

The offending code is in the OSR entry thunk.  Dissecting the thunk now ...

[Filip Pizlo]

This bug is awesome!  The way we reason about flushing fails to cope with infinite loops - either ones that existed in the original code or ones that get created by way of CFG simplification.

The best solution is probably to get CPS rethreading to inject flushes.  This means that we handle flushes in the following way in the different forms:

LoadStore: the bytecode is the ground truth of what is flushed.  You can't trust Flushes.  But, you know that anything that was flushed will be live-at-head in all of the right places.

ThreadedCPS: Flushes are at any "terminals" blocks - that is, blocks that have no forward successors.

SSA: no more Flushes!  This is already in effect after https://bugs.webkit.org/show_bug.cgi?id=130440.

[Filip Pizlo]

Created attachment 227248 [details]
bunch of test cases

[Filip Pizlo]

It's interesting how challenging this is!  The whole point is that we're trying to insert Flush's at any point where execution could "end", which basically needs to include the end of any basic block that has no forward successors.

But boy is that hard.

Once we're in DFG SSA, it's easy, because by then, we don't have Flush's.

But in CPS, it's kind of a poop show.

- I cannot insert flushes in all of the right places prior to optimization unless I was conservative and inserted them in a *lot* of places, since at that point I don't know who might end up without forward successors.  A looping branch might lose its forward successor due to an optimization, as the attached test cases illustrate.

- I cannot insert all of the flushes after parsing because then I don't know exactly where it's OK to insert them.  For example, we won't know when a SetLocal is an ImmediateSet and thus doesn't need a Flush.

I'm starting to think that the bytecode parser ought to insert flushes "as if" we were returning at any backwards branch in addition to the places where it already does it.

[Filip Pizlo]

Also found another bug, and I do intend to fix them as part of this patch, and hopefully have test cases too: InlineCallFrame::capturedVars is wrong for inlineDepth >= 2.

[Filip Pizlo]

Created attachment 227254 [details]
the wrong approach

This tries to make it so that CPSRethreading blows away all Flush's and then inserts them by using bytecode as the ground truth.  But I don't believe this will work.

[Filip Pizlo]

(In reply to comment #7)
> Also found another bug, and I do intend to fix them as part of this patch, and hopefully have test cases too: InlineCallFrame::capturedVars is wrong for inlineDepth >= 2.

Nope, that wasn't a bug.  That was just me misreading code.

[Filip Pizlo]

Created attachment 227305 [details]
hopefully the right approach

[WebKit Commit Bot]

Attachment 227305 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:783:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 227313 [details]
the patch

[WebKit Commit Bot]

Attachment 227313 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:783:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Babak Shafiei]

<rdar://problem/16332337>

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/165995