Bug 188476

Summary: Disable JIT on IA-32 without SSE2
Product: WebKit Reporter: karogyoker2+webkit
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Blocker CC: berto, bugs-noreply, commit-queue, ews-watchlist, guijemont, karogyoker2+webkit, keith_miller, mark.lam, mcatanzaro, msaboff, saam, tpopela, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

karogyoker2+webkit
Reported 2018-08-10 11:50:17 PDT
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.
Attachments
Patch (3.35 KB, patch)
2018-08-10 12:07 PDT, karogyoker2+webkit 		no flags
DetailsFormatted DiffDiff
Patch (5.44 KB, patch)
2018-08-11 09:37 PDT, karogyoker2+webkit 		no flags
DetailsFormatted DiffDiff
Patch (1.34 KB, patch)
2018-08-11 13:14 PDT, karogyoker2+webkit 		no flags
DetailsFormatted DiffDiff
Patch (1.04 KB, patch)
2018-08-11 23:11 PDT, karogyoker2+webkit 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
karogyoker2+webkit
Comment 1 2018-08-10 12:07:26 PDT
Created attachment 346916 [details] Patch
Michael Catanzaro
Comment 2 2018-08-10 17:39:17 PDT
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.
karogyoker2+webkit
Comment 3 2018-08-11 09:37:53 PDT
Created attachment 346962 [details] Patch
Yusuke Suzuki
Comment 4 2018-08-11 10:17:45 PDT
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
karogyoker2+webkit
Comment 5 2018-08-11 13:14:23 PDT
Created attachment 346968 [details] Patch
karogyoker2+webkit
Comment 6 2018-08-11 13:17:19 PDT
(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 7 2018-08-11 13:34:53 PDT
Comment on attachment 346968 [details] Patch r=me
WebKit Commit Bot
Comment 8 2018-08-11 14:04:18 PDT
Comment on attachment 346968 [details] Patch Clearing flags on attachment: 346968 Committed r234784: <https://trac.webkit.org/changeset/234784>
WebKit Commit Bot
Comment 9 2018-08-11 14:04:20 PDT
All reviewed patches have been landed. Closing bug.
karogyoker2+webkit
Comment 10 2018-08-11 15:03:59 PDT
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
Comment 11 2018-08-11 15:26:28 PDT
Yes, please attach a follow-up patch.
Michael Catanzaro
Comment 12 2018-08-11 15:26:39 PDT
Reopening
karogyoker2+webkit
Comment 13 2018-08-11 23:11:13 PDT
Created attachment 346984 [details] Patch
WebKit Commit Bot
Comment 14 2018-08-12 09:36:58 PDT
Comment on attachment 346984 [details] Patch Clearing flags on attachment: 346984 Committed r234787: <https://trac.webkit.org/changeset/234787>
WebKit Commit Bot
Comment 15 2018-08-12 09:37:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.