Closed Bug 609028 Opened 9 years ago Closed 9 years ago

nanojit: Register allocator assumes control-flow continues past non-returning function

Categories

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

defect

Tracking

(Not tracked)

RESOLVED WONTFIX
Q2 11 - Wasabi

People

(Reporter: rreitmai, Assigned: rreitmai)

References

Details

If we have block A that terminates with a non-returning function, followed by block B, the register allocator will assume control flow falls into block B.

Normally this is not an issue, since the client producing LIR would not make the same assumption, otherwise they'd have incorrect code and block B will have some other incoming edge.

But the allocator incorrectly assumes there is an incoming edge from A and this currently leads to spurious non-reachable code being generated.
Blocks: 607110
Depends on: 607816
(In reply to comment #0)

> But the allocator incorrectly assumes there is an incoming edge from A and this
> currently leads to spurious non-reachable code being generated.

Is this spurious nonreachable code causing the checkForResourceLeaks() assert in bug 607110?  One would think that the assert would happen, or not, regardless of whether the function can return, or not, since the assembler doesn't know the function can't return.

Can you explain with a simple code sample why the assert is wrong and why teaching the assembler about nonreturning functions will fix it?  I'm just not understanding the root problem.
(In reply to comment #1)
> (In reply to comment #0)
> 
> > But the allocator incorrectly assumes there is an incoming edge from A and this
> > currently leads to spurious non-reachable code being generated.
> 
> Is this spurious nonreachable code causing the checkForResourceLeaks() assert
> in bug 607110?  

yes.

> Can you explain with a simple code sample why the assert is wrong and why
> teaching the assembler about nonreturning functions will fix it? 

Will work on putting a lirasm based test cases that covers this situation, since our test suite doesn't catch it and it will illustrate clearly what is going on .
Here's an example (lots of add clutter to force the register allocator to use caller saved regs on x86).  This is a re-creation of a CodegenLIR production, wherein the call is to a non-returning function.

This code will properly assert in debug builds, since l3 control is assumed to fall into l4.  Placing a 'j l5' after the call fixes the issue, but another solution would be to place this flow-control knowledge into LIR, as proposed in the bug.


    ptr = allocp 32
    a = immi 1
    b = immi 2
    c = immi 3
    d = immi 4
    s = immi 65

l1: sti s ptr 0
    j l3

l2: oa = addi a a
    ob = addi b b
    oc = addi c c
    od = addi d d
    j l4

l3: calli puts cdecl ptr

l4: n1 = addi s  oa
    n2 = addi s  ob
    n3 = addi s  oc
    n4 = addi s  od
    sti n1 ptr 0
    sti n2 ptr 1
    sti n3 ptr 2
    sti n4 ptr 3
l5: rn = immi 0
    reti rn
Bug 614510 highlights another example of this issue.
Duplicate of this bug: 614510
Running the brightspot swfs with -Dverifyonly:
$ avmshell_sd -Dverifyonly avmglue.abc <brightspot tests>.swf

there were 97 failed assertions out of 33k swfs.  The assertion was:
Assertion failure: frame entry 4 wasn't freed|: _entries[i] == __null (/Users/build/buildbot/tamarin-redux/mac-intel-10_5/repo/nanojit/Assembler.cpp:2335)

For example as3/as3_231110/as3/dump2/as3/6120.swf triggers the assertion.  In bug https://bugzilla.mozilla.org/show_bug.cgi?id=625458 I am working on running debug-debugger builds with brightspot.
see bug 620860 for another potential example.
Assignee: nobody → rreitmai
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug+
Priority: -- → P1
See Also: → 609028
Target Milestone: --- → Q2 11 - Wasabi
Priority: P1 → P3
Rick, this appears to be a long-standing bug.  Is there a reason for making this a P1 fix for Wasabi?  Appears to be an issue that we want to address but isn't a must-fix for Wasabi.  Pls advise.
Flags: flashplayer-qrb+ → flashplayer-qrb?
Depends on: Andre
Flags: flashplayer-qrb? → flashplayer-qrb+
Its important and we didn't want to lose track of it until we completely understood the issue, as the asserts are merely a symptom of something deeper.

We are converging on a solution, but I'm not sure this warrants a P1 for wasabi.
Fixed directly in tamarin (see bug 607816).  Did not modify nanojit.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
No longer blocks: 607110
No longer depends on: Andre
You need to log in before you can comment on or make changes to this bug.