598151 - Unconditional branches can confuse X87 stack depth diagnostics

Status: RESOLVED FIXED

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

Description

[William Maddox]

The following 'lirasm' example produces an assertion failure on non-SSE IA-32 platforms:

        ptr = allocp 8

        k = immd 1.0
        std k ptr 0

        a = immd 1.0
        b = immd 2.5
        c = addd a b
        std c ptr 0

        x = ldd ptr 0
        z = immd 0.0
        p = eqd x z
        jt p skip       

        j done

skip:   std c ptr 0

done:   res = ldd ptr 0
        retd res


wmaddox-macpro:nanojit-central wmaddox$ ./build/bin/lirasm bug.in 
Assertion failure: (_allocator.active[FST0] && _fpuStkDepth == -1) || (!_allocator.active[FST0] && _fpuStkDepth == 0) (../nanojit/Assembler.cpp:366)
Bus error

Comment 1

[William Maddox]

Attachment: Clean up regstate merging at unconditional branches

The consistency check for x87 stack depth vs. the allocation state of FST0 is applied blindly during a reverse scan of the LIR instructins without due concern for reachability.  The only reason this ever worked is that we were emitting spurious unreachable fpu stack pops following unconditional jumps.  This patch avoids emitting these unreachable instructions, and alters the stack depth accounting to ignore unreachable code following such jumps.

Comment 2

[Rick Reitmaier]

Comment on attachment 476923 [details] [diff] [review]
Clean up regstate merging at unconditional branches

The patch looks good, but I'd like to see the two pieces in 2 different patches, hence the '-'.  That is the replacement of intersect/union with loadRegisterState vs. the fpu depth changes.

Changing the register semantics may look correct as proposed, but its a delicate surgery that warrants a careful, independent review.

Comment 3

[William Maddox]

Attachment: Account for non-fallthrough of unconditional jump in x87 stack depth tracking

This patch just fixes the stack depth tracking bug without attempting to streamline the regstate merge for the degenerate unconditional jump case.

Comment 4

[Rick Reitmaier]

Comment on attachment 476974 [details] [diff] [review]
Account for non-fallthrough of unconditional jump in x87 stack depth tracking

nit: you could consolidate the ifdef logic to the bottom of the if () else.

Comment 5

[William Maddox]

Pushed to NJ:

http://hg.mozilla.org/projects/nanojit-central/rev/4becc719d20e

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/31f98b23ebfa

Comment 7

[William Maddox]

http://hg.mozilla.org/tamarin-redux/rev/aac5833237df

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/31f98b23ebfa