Assertion failure: frame entry 172 wasn't freed

RESOLVED FIXED in Q4 11 - Anza

Status

P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: edwsmith, Assigned: stejohns)

Tracking

unspecified
Q4 11 - Anza
Bug Flags:
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug +

Details

Attachments

(4 attachments)

(Reporter)

Description

8 years ago
Created attachment 499221 [details]
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.
(Reporter)

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Future
(Reporter)

Updated

8 years ago
Target Milestone: Future → flash9.0.x-Spicy
(Reporter)

Updated

8 years ago
Target Milestone: flash9.0.x-Spicy → flash10.2.x-Spicy
(Reporter)

Updated

8 years ago
Attachment #499221 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Updated

8 years ago
Priority: -- → P1
(Reporter)

Comment 1

8 years ago
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)
(Reporter)

Comment 2

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

Comment 3

8 years ago
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

8 years ago
Moving from Spicy to Wasabi for investigation.
Flags: flashplayer-qrb+
Flags: flashplayer-bug+
Target Milestone: flash10.2.x-Spicy → flash10.x-Wasabi

Updated

8 years ago
Assignee: nobody → rreitmai

Comment 5

8 years ago
Most likely dup of bug 609028

Updated

8 years ago
Blocks: 641055

Updated

8 years ago
No longer blocks: 641055
Depends on: 641055

Updated

8 years ago
Depends on: 607816

Updated

8 years ago
Target Milestone: Q2 11 - Wasabi → Q3 11 - Serrano

Updated

8 years ago
Flags: flashplayer-injection-

Comment 6

8 years ago
Moving to Anza

Updated

8 years ago
Target Milestone: Q3 11 - Serrano → Q4 11 - Anza

Comment 7

8 years ago
This should already be fixed, it just need to be validated against the test case outlined in the bug desc.
(Assignee)

Comment 8

7 years ago
Created attachment 547586 [details] [diff] [review]
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".
Attachment #547586 - Flags: feedback?
(Assignee)

Comment 9

7 years ago
Comment on attachment 547586 [details] [diff] [review]
Bad source

Note also that a Release build will (incorrectly) print false unless you force -Dinterp.
Attachment #547586 - Flags: feedback?(wmaddox)
Attachment #547586 - Flags: feedback?(edwsmith)
Attachment #547586 - Flags: feedback?
(Assignee)

Comment 10

7 years ago
Created attachment 547735 [details]
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.
(Assignee)

Comment 11

7 years ago
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.
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
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...
(Assignee)

Comment 14

7 years ago
Created attachment 547786 [details] [diff] [review]
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?
Assignee: rreitmai → stejohns
Attachment #547786 - Flags: superreview?(edwsmith)
Attachment #547786 - Flags: review?(wmaddox)
(Assignee)

Comment 15

7 years ago
Review ping?

Comment 16

7 years ago
(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.
(Reporter)

Comment 17

7 years ago
Comment on attachment 547586 [details] [diff] [review]
Bad source

yep, pretty bad source!  great test case.
Attachment #547586 - Flags: feedback?(wmaddox) → feedback+
(Reporter)

Updated

7 years ago
Attachment #547586 - Flags: feedback?(edwsmith)
(Reporter)

Comment 18

7 years ago
Comment on attachment 547786 [details] [diff] [review]
Patch

Patch looks find contingent on test cases and no serious regressions, as Bill mentioned.
Attachment #547786 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Comment 19

7 years ago
> 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.
(Reporter)

Comment 20

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

Comment 21

7 years ago
(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.
(Assignee)

Comment 22

7 years ago
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

7 years ago
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+
(Assignee)

Comment 24

7 years ago
pushed to TR: 6490:8de850f0d909
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 25

7 years ago
pushed to TR-Serrano: 6340:f3b64bc72d18

Updated

7 years ago
No longer depends on: 641055

Updated

7 years ago
Attachment #547786 - Flags: review?(wmaddox) → review+
You need to log in before you can comment on or make changes to this bug.