510193 - TraceMonkey: testTableSwitch2 fails on ARM.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacob Bramley [:jbramley]]

Attachment: Make the jitstat check conditional so that it only applies for x86.

testTableSwitch2 fails on ARM, and has always failed since its introduction.

The test appears to have been added by bug 507156, which makes some changes to the x86-specific ::tableSwitch() function in jstracer.cpp. As far as I can see, the test is only valid for x86 as it has a slightly different tracing behaviour. I am not sure if this is really true, but _something_ is wrong anyway, as the test fails.

In addition, this should also affect other platforms too, so I've set this to platform "All" as there is no "All except x86" option :-)

----

The attached patch hides the jitstat check behind a check for an x86 platform. This may not be entirely what we want. For example, a more robust test would provide alternative jtistat checks for other platforms, but I don't know what is correct. On ARM, I see the following:

    recorderStarted: 1
    recorderAborted: 0 
    traceCompleted: 2 (as opposed to 3 on x86)
    sideExitIntoInterpreter: 3 (as opposed to 4 on x86)

If this is generally correct for all non-x86 platforms, I'll update the patch accordingly, but I don't know enough about how the tracer works to be sure of this.

----

Jason, I've flagged you for review as you wrote the test for bug 507156 and so you should know what I'm talking about :-) Feel free to redirect.

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 394262 [details] [diff] [review]
Make the jitstat check conditional so that it only applies for x86.

Ugh, this situation stinks.

Implementing LIR_xtbl for the other platforms wouldn't be hard, but you have to be able to test on each platform.

The `#ifdef NANOJIT_IA32` around that tableswitch code should probably say `#ifdef NANOJIT_HAS_LIR_XTBL` but we can worry about that later, when someone implements xtbl for some other platform.

...Which wouldn't be hard to do. Mainly testing.

Thanks for the patch.

Comment 2

[Jacob Bramley [:jbramley]]

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

Comment 3

[Jason Orendorff [:jorendorff]]

Oops, jitstats is debug-only. This is causing the tests to fail on all platforms instead of just non-x86 ones. :)

I'll backout. I guess we need a new approach.

Comment 4

[David Mandelin [:dmandelin]]

How about if we just disable the jistats check for that test for now.

When bug 505588 lands, we can make the Python script pass sys.platform to the JS, which can then run the right test. Alternatively, we can use the Makefile to control which tests get run via CL arguments to the Python script.

Comment 5

[Jason Orendorff [:jorendorff]]

Oops, already backed out.

http://hg.mozilla.org/tracemonkey/rev/837f328aafb7

But yeah, I should have just pushed a trivial fix, hang on...

Comment 6

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/653353855fff

There. r=shaver over IRC for the trivial fix:

>-if (jitstats.archIsIA32) {
>+if (hadJITstats && jitstats.archIsIA32) {

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/653353855fff

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/34f3deb4bd56