Bug 191184 – REGRESSION(r237547): Test failures on 32-bit JSC since the JIT was disabled

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 353687 [details]
Patch

[Guillaume Emont]

Comment on attachment 353687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353687&action=review

> LayoutTests/js/script-tests/regress-139548.js:1
> -// FIXME: unskip when this is solved
> -// https://bugs.webkit.org/show_bug.cgi?id=191163
> -//@ skip if $architecture == "mips" or $architecture == "arm"
> +//@ skip if not $jitTests

FWIW, I think all the occurrences of this make sense, + then we won't have to manually unskip tests when we reactivate the JIT. The reason I did not do that in the first place is that I did not know whether other platforms were faster and wouldn't need the skipping, so I tried to minimize the skipping. But if it's established that it's needed for all platforms when JIT is disabled, then I think that's how we should do it.

> JSTests/microbenchmarks/array-push-1.js:1
> +//@ skip if not $jitTests

All the tests that were not previously skipped on arm or mips do manage to run within the timeout limit on the mips and arm bots, (except stress/sampling-profiler-richards.js which I forgot to skip on mips), so I think it could make sense to *not* skip them on these platforms, which would give us more test coverage of these platforms (currently on cloop) for the time being.

> JSTests/stress/op_lshift-ConstVar.js:1
> -// FIXME: unskip when this is solved
> -// https://bugs.webkit.org/show_bug.cgi?id=191163
> -//@ skip if $architecture == "arm"
> +//@ skip if not $jitTests

Similarly, the tests that I skipped only on arm run within the timeout limit on mips. Though arguably we could lower the timeout limit on our mips bot.

[Saam Barati]

Comment on attachment 353687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353687&action=review

r=me

> Source/JavaScriptCore/ChangeLog:15
> +        REGRESSION(r237547): Exception handlers should be aware of wide opcodes when JIT is disabled

Remove this entry from the changelog since it’s for a different bug.

> Source/JavaScriptCore/jit/JITExceptions.cpp:76
> +        catchRoutine = catchPCForInterpreter->isWide()

I think this already landed?

[Tadeu Zagallo]

Created attachment 353843 [details]
Patch

[Tadeu Zagallo]

Comment on attachment 353687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353687&action=review

>> Source/JavaScriptCore/jit/JITExceptions.cpp:76
>> +        catchRoutine = catchPCForInterpreter->isWide()
> 
> I think this already landed?

Oops, I had an early commit in the tree and didn't notice. I removed it now.

>> JSTests/microbenchmarks/array-push-1.js:1
>> +//@ skip if not $jitTests
> 
> All the tests that were not previously skipped on arm or mips do manage to run within the timeout limit on the mips and arm bots, (except stress/sampling-profiler-richards.js which I forgot to skip on mips), so I think it could make sense to *not* skip them on these platforms, which would give us more test coverage of these platforms (currently on cloop) for the time being.

Sounds good, I changed all of this to only skip on x86. My rational was that most of these tests don't seem to do a whole lot of testing if the JIT is disabled, but I'm happy to un-skip them.

>> JSTests/stress/op_lshift-ConstVar.js:1
>> +//@ skip if not $jitTests
> 
> Similarly, the tests that I skipped only on arm run within the timeout limit on mips. Though arguably we could lower the timeout limit on our mips bot.

I've changed those to skip only on arm || x86. I've also added back the FIXME comments.

[Tadeu Zagallo]

Created attachment 354066 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 354066 [details]
Patch for landing

Clearing flags on attachment: 354066

Committed r237919: <https://trac.webkit.org/changeset/237919>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45868793>