Bug 188476 – Disable JIT on IA-32 without SSE2

• Component: WebKitGTK
• Status: RESOLVED FIXED

I get Illegal instruction (core dumped) error message when JIT is enabled on IA-32 machines without SSE2. Reproduced on an Athlon XP. Also tested on Intel Celeron single core 2GHz (Northwood-128 architecture) which has SSE2 but on that it was OK, so the issue is SSE2.

Created attachment 346916 [details]
Patch

[Michael Catanzaro]

Comment on attachment 346916 [details]
Patch

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

> Source/cmake/OptionsGTK.cmake:-129
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ON)

Hm, I would rather keep this option PUBLIC. Can you change it to reuse ENABLE_JIT_DEFAULT, like this:

WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ${ENABLE_JIT_DEFAULT})

That will be fragile, since it would be broken if the variable is removed from WebKitFeatures.cmake, but I don't see a better option, so also add a warning comment in WebKitFeatures.cmake to indicate the variable is surprisingly used in OptionsGTK.cmake.

Then test that it actually works, of course.

> Source/cmake/WebKitFeatures.cmake:75
> -    if (WTF_CPU_ARM OR WTF_CPU_ARM64 OR WTF_CPU_MIPS OR WTF_CPU_X86_64 OR WTF_CPU_X86)
> +    if (WTF_CPU_ARM OR WTF_CPU_ARM64 OR WTF_CPU_MIPS OR WTF_CPU_X86_64 OR WTF_CPU_X86_SSE2)

This is never set in CMake, so you're disabling it for everyone. Look in the toplevel CMakeLists.txt where WTF_CPU_X86 is set. You'll need to add some test for SSE2 instruction set there to ensure it gets defined when appropriate.

Created attachment 346962 [details]
Patch

[Yusuke Suzuki]

Why not disabling JIT at runtime at runtime/Options.cpp?
Windows already does this.
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/Options.cpp#L400-L404

Created attachment 346968 [details]
Patch

(In reply to Yusuke Suzuki from comment #4)
> Why not disabling JIT at runtime at runtime/Options.cpp?
> Windows already does this.
> https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/
> Options.cpp#L400-L404

Thank you Yusuke, this is much simpler and more flexible, I tried it and it works on my Athlon XP.

[Yusuke Suzuki]

Comment on attachment 346968 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 346968 [details]
Patch

Clearing flags on attachment: 346968

Committed r234784: <https://trac.webkit.org/changeset/234784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

I think we need to remove OS(WINDOWS) from here:
https://github.com/WebKit/webkit/blob/89aab857234c36c97bc0696c7d5bc03b475b6c6b/Source/JavaScriptCore/runtime/Options.cpp#L52

Maybe now it works only because something else is including MacroAssemblerX86.h but normally MacroAssembler.h would include it. Maybe the headers are included because of the source files are unified.

This line would need MacroAssemblerX86.h to be included:
MacroAssemblerX86::supportsFloatingPoint()

What do you think?

[Michael Catanzaro]

Yes, please attach a follow-up patch.

[Michael Catanzaro]

Reopening

Created attachment 346984 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 346984 [details]
Patch

Clearing flags on attachment: 346984

Committed r234787: <https://trac.webkit.org/changeset/234787>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.