620860 - Assertion failure: frame entry 172 wasn't freed

Status: RESOLVED FIXED

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P1)

Description

[Edwin Smith]

Attachment: LIR listing of the problem code.

Found in:

swfcontents/y/Q/q/e/Y/YqQeYqtmpL2mtsnSJo2efZRkdzTX+Z3E2mCgrtuZgUI=

I don't know what the problem is yet, but I want to get the bug
entered anyway so we don't lose it.

Other than being some random assert, a few interesting things appear
in the LIR listing:

1. the function name is mx.utils::UIDUtil$/isUID(). So this is our
code, not some random thing off the web.

2. the LIR listing has calls to multiple variants of stringCharCodeAt()
with the same args:

   81: String_charCodeAtFU1 = calld.none #String_charCodeAtFU ( ldq10 ldi2 )

   131: String_charCodeAtIU1 = calli.none #String_charCodeAtIU ( ldq10 ldi2 )

There looks like quite a bit of branchy control flow, too.

Comment 1

[Edwin Smith]

As a data point, x86 fails with a different assertion:

Assertion failure: (_allocator.active[REGNUM(FST0)] && _fpuStkDepth == -1) || (!_allocator.active[REGNUM(FST0)] && _fpuStkDepth == 0) (/Users/edwin/vm/jitrefac/nanojit/Assembler.cpp:371)

Comment 2

[Edwin Smith]

this bug is a possible duplicate of bug 614510; not marking it yet until it is investigated.

Comment 3

[Rick Reitmaier]

I can't find file swfcontents/y/Q/q/e/Y/YqQeYqtmpL2mtsnSJo2efZRkdzTX+Z3E2mCgrtuZgUI=.

Can you run with ' -Dverbose=jit,verify,raw,regs ' before and after applying patches from bug 607816.

You should be able to see something similar to what's described in comment 3 in bug 614510 if this is indeed the same problem.

And btw the issues manifests as a 'frame entry wasn't freed' assertion, not an fpu assertion: 

  Eg. frame entry xxx wasn't freed : _entries[i] == __null

Comment 4

[Dan Smith]

Moving from Spicy to Wasabi for investigation.

Comment 5

[Rick Reitmaier]

Most likely dup of bug 609028

Comment 6

[Dan Smith]

Moving to Anza

Comment 7

[Rick Reitmaier]

This should already be fixed, it just need to be validated against the test case outlined in the bug desc.

Comment 8

[Steven Johnson]

Attachment: Bad source

Here's a simple script that will demo the problem: rebuild it and run using a Debug Debugger build. If you run 'normally' (jit or interp) it works; if you run with -Dnodebugger, it asserts, and if continued after the assert, incorrectly prints "false" instead of "true".

Comment 9

[Steven Johnson]

Comment on attachment 547586 [details] [diff] [review]
Bad source

Note also that a Release build will (incorrectly) print false unless you force -Dinterp.

Comment 10

[Steven Johnson]

Attachment: Even simpler source

Even simpler testcase that demonstrates the problem. Note that if you change "var c:Number" into "var c:uint", the problem goes away.

Comment 11

[Steven Johnson]

Digging further, it appears that the specialization in optimizeIntCmpWithNumberCall() is questionable in this case (though it's not clear whether/why it's causing the misbehavior)... we have already called charCodeAt(), and since the result is stored into an explicit Number type, we specialize to the "FU" variant returning float. However, in test vs literal '45', we note that it's an integer literal compared against charCodeAt result, and emit another call to the "IU" variant returning int. Yikes, certainly not a win to make redundant calls.

Comment 12

[Steven Johnson]

OK, so the problem is that the FU->IU specialization occurs in both branches, but in the 'else' branch, we reuse the same LIns... but that's only valid in the 'if' branch since that's the call site.

-- Note 1: it's a little disturbing that the LIns-recycling-code can emit stale stuff this way.
-- Note 2: even if it didn't recycle, we'd still have potentially suboptimal code since we'd be emitting two charCodeAt() calls where one would suffice.

Comment 13

[Steven Johnson]

Aha! The gotcha here is in specializeIntCall(); it attempts to avoid redundant calls by keeping a hashmap of specialized calls it's emitted, but this doesn't consider that they may have been emitted in code paths that are mutually exclusive...

Comment 14

[Steven Johnson]

Attachment: Patch

Here's a simplistic fix that seems to address the issue: clear the specialized-function map when we insert a label. This is suboptimal, in that we may well be inserting redundant calls in some cases, but it should eliminate incorrect usage.

It's not clear to me if there's an existing structure in the JIT that allows us to do a smarter job of hoisting potentially redundant pure calls in this way -- is there?

Comment 15

[Steven Johnson]

Review ping?

Comment 16

[William Maddox]

(In reply to comment #15)
> Review ping?

This is a bit conservative, as you noted: If one specialization occurs above a control-flow diamond, and another identical one below, we'll duplicate the call even though the first dominates the second.  Unfortunately, we don't have the machinery to do better than this, so I think the patch is a reasonable way to restore correctness.

Aside from this bug, the specializer has the weakness that we may have already been forced to emit an unspecialized call anyway by the time we come to a use that allows for a context-dependent specialization.  At runtime, we may end of calling the both unspecialized version and a specialization, when it might be
cheaper just to coerce the result of the unspecialized call.  This specializer
is really on shaky ground, and probably looked good on benchmarks only because the screw cases are rare in practice, or perhaps just in the few benchmarks measured.

I think it may be advisable to re-run the benchmarks upon which this optimization was originally justified, and make sure we're still seeing a net win.

Comment 17

[Edwin Smith]

Comment on attachment 547586 [details] [diff] [review]
Bad source

yep, pretty bad source!  great test case.

Comment 18

[Edwin Smith]

Comment on attachment 547786 [details] [diff] [review]
Patch

Patch looks find contingent on test cases and no serious regressions, as Bill mentioned.

Comment 19

[Steven Johnson]

> I think it may be advisable to re-run the benchmarks upon which this
> optimization was originally justified, and make sure we're still seeing a
> net win.

Do we know which benchmarks those were? This needs to land in Serrano, which is scheduled for ZBB by end of week, so there's some urgency here.

Comment 20

[Edwin Smith]

the main ones have charCodeAt in their name (under test/acceptance/jsmicro and asmicro), but looking for new outliers would be good too.

Comment 21

[Steven Johnson]

(In reply to comment #20)
> the main ones have charCodeAt in their name (under test/acceptance/jsmicro
> and asmicro), but looking for new outliers would be good too.

Good call. I'll go ahead and run before-and-after benchmarks on those, at least.

Comment 22

[Steven Johnson]

before-and-after on mac-x86 and mac-x64 don't show any red flags. I'm going to land it if/when William gives the R+.

Comment 23

[William Maddox]

Comment on attachment 547786 [details] [diff] [review]
Patch

My concerns were only with respect to possible performance regression.  From a correctness viewpoint, it looks sound.  R+

Comment 24

[Steven Johnson]

pushed to TR: 6490:8de850f0d909

Comment 25

[Steven Johnson]

pushed to TR-Serrano: 6340:f3b64bc72d18