609393 - resourceConsistencyCheck x87 stack assert can fire

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Steven Johnson]

Steps:
-- use tamarin-redux in a Flash Win32 Standalone Debug non-Debugger build
-- open http://help.adobe.com/en_US/flex/using/movies/SparkTextAreaControl.swf
-- get an assert, fpuStckChk == -2 while jitting flashx.textLayout.property::NumberOrPercentProperty/computeActualPropertyValue

Comment 1

[Steven Johnson]

Attachment: Verbose dump of verify/assembly of the function

Note that I inserted extra output for FPU_PSH and FPU_POP to show where push/pop occurs.

Comment 2

[Steven Johnson]

Observation: we always do fpu_push/fpu_pop around CALL() if the return type is "d", but this is really only correct if the calling convention returns in FST0; in at least one case here we are getting the return in xmm0, which shouldn't trigger the push/pop. (Not sure if this is causing the assert.)

Comment 3

[Rick Reitmaier]

Michelle is seeing the following fire:

NanoAssert((_allocator.active[FST0] && _fpuStkDepth == -1) ||
                   (!_allocator.active[FST0] && _fpuStkDepth == 0));
 
NanoAssertMsgf(_fpuStkDepth == 0,"_fpuStkDepth %d\n",_fpuStkDepth);
 

 against;  http://www.google.com/finance?q=NYSE:RIG

Comment 4

[Rick Reitmaier]

also reported http://watsonexp.corp.adobe.com/#bug=2801964

Comment 5

[Dan Smith]

Targeting for Serrano.   Fixing failing assertions is a priority so we can enable assertions during all Player testing.

Comment 6

[Rick Reitmaier]

Attachment: disable assert on sse2 machines

We believe the assert is firing due to bad accounting and that there is not an issue with the underlying code that the assert is attempting to protect.

This is not a given, so it probably doesn't make sense to completely defeat the assert.

So instead this patch restricts it to fire only on architectures where we are actually generating non-sse2 (i.e. fpu) instructions, then at least we're clearing out some of the false-positive noise.

Comment 7

[William Maddox]

Comment on attachment 533429 [details] [diff] [review]
disable assert on sse2 machines

Review of attachment 533429 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.  It is unlikely that any routine engineering or testing is being done on non-sse2 capable machines anymore.
Note that it would be wise to continue avmshell testing with the assertion in place, which can be done with the -nosse flag on the
avmshell command line.  Are we testing a -nosse configuration in our automated testing?

Comment 8

[Edwin Smith]

Note that we don't need old hardware to test this code, you just have to turn off sse2 support.  plus, sse2-capable x86-32 machines still use FST(0) for return values, and so this logic should be fixed asap.

Comment 9

[Rick Reitmaier]

Attachment: ver1

Analysis showed that the assert in resourceConsistencyCheck() was firing due to the code not resetting fpuStkDepth in the case of LIR_ret (see asm_jmp for a similar treatment).

Comment 10

[William Maddox]

Comment on attachment 534132 [details] [diff] [review]
ver1

Review of attachment 534132 [details] [diff] [review]:
-----------------------------------------------------------------

The substance of the fix is treating the unreachability of the code following a return in the same way as code following an unconditional branch, which is clearly needed and uncontroversial.  It's also nice to see some more platform-specific code migrate out of Assembler.cpp, with another #ifdef biting the dust.

Comment 11

[Rick Reitmaier]

Attachment: ver 2 - final patch which includes whitespace cleanup

what actually landed for posterity.

Comment 12

[Rick Reitmaier]

rreitmai http://hg.mozilla.org/projects/nanojit-central/rev/87a15611dc19

Comment 13

[Tamarin Bot]

changeset: 6333:2fec52cc4356
user:      Rick Reitmaier <[e-mail]>
summary:   Bug 609393 - resourceConsistencyCheck x87 stack assert can fire (r+wmaddox)

http://hg.mozilla.org/tamarin-redux/rev/2fec52cc4356

Comment 14

[Andre Kruetzfeldt]

Bill, can we close this bug? Removing Andre blocker.

Comment 15

[William Maddox]

The issue is resolved for Tamarin.  We keep nanojit bugs open until Mozilla has confirmed that fix has made it into Tracemonkey.  I've added Nick to the CC list.

Comment 16

[Nicholas Nethercote [inactive]]

Oh, I must have missed marking this one:

http://hg.mozilla.org/tracemonkey/rev/a163ff4b8bf1