Bug 96286 – Another SIGILL in JavaScriptCore on a Geode processor

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

[Daniel Drake]

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)

[Daniel Drake]

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]

(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]

(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]

Created attachment 166363 [details]
Patch

[Daniel Drake]

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 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 on attachment 166363 [details]
Patch

Oops, I meant r-

[Daniel Drake]

Created attachment 166502 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166502 [details]
Patch

Clearing flags on attachment: 166502

Committed r130076: <http://trac.webkit.org/changeset/130076>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.