Closed Bug 620860 Opened 9 years ago Closed 8 years ago

Assertion failure: frame entry 172 wasn't freed

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Q4 11 - Anza

People

(Reporter: edwsmith, Assigned: stejohns)

References

Details

Attachments

(4 files)

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.
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Future
Target Milestone: Future → flash9.0.x-Spicy
Target Milestone: flash9.0.x-Spicy → flash10.2.x-Spicy
Attachment #499221 - Attachment mime type: application/octet-stream → text/plain
Priority: -- → P1
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)
this bug is a possible duplicate of bug 614510; not marking it yet until it is investigated.
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
Moving from Spicy to Wasabi for investigation.
Flags: flashplayer-qrb+
Flags: flashplayer-bug+
Target Milestone: flash10.2.x-Spicy → flash10.x-Wasabi
Assignee: nobody → rreitmai
Most likely dup of bug 609028
Blocks: Andre
No longer blocks: Andre
Depends on: Andre
Depends on: 607816
Target Milestone: Q2 11 - Wasabi → Q3 11 - Serrano
Flags: flashplayer-injection-
Moving to Anza
Target Milestone: Q3 11 - Serrano → Q4 11 - Anza
This should already be fixed, it just need to be validated against the test case outlined in the bug desc.
Attached patch Bad sourceSplinter Review
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?
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?
Attached file 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.
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.
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.
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...
Attached patch PatchSplinter Review
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)
Review ping?
(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 on attachment 547586 [details] [diff] [review]
Bad source

yep, pretty bad source!  great test case.
Attachment #547586 - Flags: feedback?(wmaddox) → feedback+
Attachment #547586 - Flags: feedback?(edwsmith)
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+
> 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.
the main ones have charCodeAt in their name (under test/acceptance/jsmicro and asmicro), but looking for new outliers would be good too.
(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.
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 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+
pushed to TR: 6490:8de850f0d909
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
pushed to TR-Serrano: 6340:f3b64bc72d18
No longer depends on: Andre
Attachment #547786 - Flags: review?(wmaddox) → review+
You need to log in before you can comment on or make changes to this bug.