Bug 96286

Summary: Another SIGILL in JavaScriptCore on a Geode processor
Product: WebKit Reporter: Daniel Drake <dsd>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cgarcia, fpizlo, mrobinson, oliver, webkit.review.bot, wingo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 112239    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Daniel Drake
Reported 2012-09-10 10:20:24 PDT
An issue similar to bug #82496 has re-emerged in WebKit-1.9.x. Testing webkitgtk3-1.9.91 on AMD Geode LX (inside OLPC XO-1 laptop), loading Google crashes with SIGILL. Looks like it is unhappy with the mulsd instruction. Program received signal SIGILL, Illegal instruction. 0xb37743cf in llint_op_mul () from /lib/libjavascriptcoregtk-3.0.so.0 (gdb) bt #0 0xb37743cf in llint_op_mul () from /lib/libjavascriptcoregtk-3.0.so.0 #1 0xadfef088 in ?? () #2 0xb370e009 in JSC::Interpreter::execute () from /lib/libjavascriptcoregtk-3.0.so.0 #3 0xb37fffaf in JSC::evaluate () from /lib/libjavascriptcoregtk-3.0.so.0 #4 0xb1259b0c in ?? () #5 0x00000000 in ?? () (gdb) x/20i $pc-12 0xb37743c3 <llint_op_mul+182>: movd %ecx,%mm7 0xb37743c6 <llint_op_mul+185>: psllq $0x20,%xmm7 0xb37743cb <llint_op_mul+190>: por %xmm7,%xmm0 => 0xb37743cf <llint_op_mul+194>: mulsd %xmm1,%xmm0 0xb37743d3 <llint_op_mul+198>: movsd %xmm0,(%edi,%edx,8) 0xb37743d8 <llint_op_mul+203>: add $0x14,%esi 0xb37743db <llint_op_mul+206>: jmp *(%esi) 0xb37743dd <llint_op_mul+208>: mov 0x4(%esi),%ecx 0xb37743e0 <llint_op_mul+211>: cmp $0xfffffff9,%ebx 0xb37743e3 <llint_op_mul+214>: ja 0xb3774408 <llint_op_mul+251> 0xb37743e5 <llint_op_mul+216>: cvtsi2sd %eax,%xmm0 0xb37743e9 <llint_op_mul+220>: movd %edx,%xmm1 0xb37743ed <llint_op_mul+224>: movd %ebx,%xmm7 0xb37743f1 <llint_op_mul+228>: psllq $0x20,%xmm7 0xb37743f6 <llint_op_mul+233>: por %xmm7,%xmm1 0xb37743fa <llint_op_mul+237>: mulsd %xmm1,%xmm0 0xb37743fe <llint_op_mul+241>: movsd %xmm0,(%edi,%ecx,8) 0xb3774403 <llint_op_mul+246>: add $0x14,%esi 0xb3774406 <llint_op_mul+249>: jmp *(%esi) 0xb3774408 <llint_op_mul+251>: mov %edi,(%esp)
Attachments
Patch (1.45 KB, patch)
2012-09-29 08:05 PDT, Daniel Drake 		no flags
DetailsFormatted DiffDiff
Patch (1.43 KB, patch)
2012-10-01 10:38 PDT, Daniel Drake 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Daniel Drake
Comment 1 2012-09-27 15:38:26 PDT
Oliver, you rescued us last time, can you help us here? :) I have checked the basics from bug #82496: isSSE2Present() still returns false on this platform (cpuid feature bits does not suggest that SSE2 is supported), DFG canCompileOpcodes() still calls down to MacroAssembler::supportsFloatingPoint() which correctly says "no". Searching through the source for e.g. mulsd, I see two potential places where mulsd instructions might be generated which aren't directly/obviously protected by the above checks. They are: 1. ./Source/JavaScriptCore/assembler/X86Assembler.h (mulsd_mr) ? 2. ./Source/JavaScriptCore/offlineasm/x86.rb I guess offlineasm has a likelihood of being the culprit, given that this is a regression over webkitgtk-1.8 and offlineasm/LLint seems to be a new feature. CCing Filip Pizlo who seems to be involved in this project. Filip, sorry to bother you, would you mind checking if llint/offlineasm checks that the x86 CPU supports SSE2 instructions before executing them? Or point us to a better candidate to ask?
Oliver Hunt
Comment 2 2012-09-27 16:13:15 PDT
(In reply to comment #1) > Oliver, you rescued us last time, can you help us here? :) > > I have checked the basics from bug #82496: isSSE2Present() still returns false on this platform (cpuid feature bits does not suggest that SSE2 is supported), DFG canCompileOpcodes() still calls down to MacroAssembler::supportsFloatingPoint() which correctly says "no". > > Searching through the source for e.g. mulsd, I see two potential places where mulsd instructions might be generated which aren't directly/obviously protected by the above checks. They are: > 1. ./Source/JavaScriptCore/assembler/X86Assembler.h (mulsd_mr) ? > 2. ./Source/JavaScriptCore/offlineasm/x86.rb > > I guess offlineasm has a likelihood of being the culprit, given that this is a regression over webkitgtk-1.8 and offlineasm/LLint seems to be a new feature. CCing Filip Pizlo who seems to be involved in this project. Filip, sorry to bother you, would you mind checking if llint/offlineasm checks that the x86 CPU supports SSE2 instructions before executing them? Or point us to a better candidate to ask? Unfortunately the only solution for this is for the gtk port to disable the LLInt -- supporting non-SSE2 x86 chips in LLInt would simply be too expensive (in terms of both implementation and/or runtime).
Filip Pizlo
Comment 3 2012-09-28 12:31:00 PDT
(In reply to comment #1) > Oliver, you rescued us last time, can you help us here? :) > > I have checked the basics from bug #82496: isSSE2Present() still returns false on this platform (cpuid feature bits does not suggest that SSE2 is supported), DFG canCompileOpcodes() still calls down to MacroAssembler::supportsFloatingPoint() which correctly says "no". > > Searching through the source for e.g. mulsd, I see two potential places where mulsd instructions might be generated which aren't directly/obviously protected by the above checks. They are: > 1. ./Source/JavaScriptCore/assembler/X86Assembler.h (mulsd_mr) ? > 2. ./Source/JavaScriptCore/offlineasm/x86.rb > > I guess offlineasm has a likelihood of being the culprit, given that this is a regression over webkitgtk-1.8 and offlineasm/LLint seems to be a new feature. CCing Filip Pizlo who seems to be involved in this project. Filip, sorry to bother you, would you mind checking if llint/offlineasm checks that the x86 CPU supports SSE2 instructions before executing them? Or point us to a better candidate to ask? No, it does no such checks. I think you have three approaches to fixing this: 1) Switch GTK to either LLInt cloop-only or JIT-only. 2) Add support in the offlineasm x86 backend to compile floating point primitives to x87. Then have a build flag indicating that you intend to target no-SSE configurations, and in those configurations, have offlineasm use x87 instead of SSE2. 3) Use run-time checks in LLInt: a) Transform that hasSSE2() run-time check into something that sets a flag on JSGlobalData. Rename it to hasFloatingPoint(). We will never support x87 floating point unless you implement it, so a chip that doesn't have SSE2 amounts to a chip that doesn't have floating point. b) Add a MAY_NOT_HAVE_FLOATING_POINT flag somewhere, and plumb it through to the LLInt. This is similar to the flag you'll need for (2) above. 3) If MAY_NOT_HAVE_FLOATING_POINT is true, then the LLInt should do an extra check prior at the top of floating point paths checking if JSGlobalData::hasFloatingPoint is false; if it's false then jump straight to the slow path. Those are your options.
Daniel Drake
Comment 4 2012-09-29 08:05:33 PDT
Created attachment 166363 [details] Patch
Daniel Drake
Comment 5 2012-09-29 08:10:22 PDT
Thanks for your help/input. I do like the #3 approach, but I don't have enough time to implement it at the moment, so I went for a variant of #1 based on an idea from Martin Robinson: we disable LLint if the build target doesn't support SSE2 instructions. The compiler knows about the availability of SSE2 support and automatically defines __SSE2__ accordingly: http://lists.webkit.org/pipermail/webkit-gtk/2012-September/001250.html Also, I think anyone who is really serious about performance on x86-type platforms these days will be using 64 bit builds. This consideration doesn't apply there; x86_64 always has SSE2 available. So I don't feel too bad about taking this "shortcut" at least for now.
Filip Pizlo
Comment 6 2012-09-29 14:39:01 PDT
Comment on attachment 166363 [details] Patch The usual style we use for disabling something is to use #define ENABLE_LLINT 0 And put it before the logic that enables it. You'll notice that the logic that enables it checks if the macro is already defined, and if so, does nothing. I think this is less fragile, and should be easier to grok since we already do this in other places.
Filip Pizlo
Comment 7 2012-09-29 14:39:51 PDT
Comment on attachment 166363 [details] Patch Oops, I meant r-
Daniel Drake
Comment 8 2012-10-01 10:38:12 PDT
Created attachment 166502 [details] Patch
WebKit Review Bot
Comment 9 2012-10-01 14:00:42 PDT
Comment on attachment 166502 [details] Patch Clearing flags on attachment: 166502 Committed r130076: <http://trac.webkit.org/changeset/130076>
WebKit Review Bot
Comment 10 2012-10-01 14:00:49 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 11 2012-10-01 14:32:55 PDT
Comment on attachment 166502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166502&action=review > Source/WTF/wtf/Platform.h:904 > +/* On the GTK+ port we take an extra precaution for LLINT support: > + * We disable it on x86 builds if the build target doesn't support SSE2 > + * instructions (LLINT requires SSE2 on this platform). */ Other comments in this file are using spaces to indent subsequent lines of comments, not * with space. > Source/WTF/wtf/Platform.h:906 > +#if !defined(ENABLE_LLINT) && PLATFORM(GTK) && CPU(X86) && COMPILER(GCC) \ > + && !defined(__SSE2__) WebKit coding style would put this all on a single line. I don’t think this line breaking should be done here.
Note You need to log in before you can comment on or make changes to this bug.