Summary: | Another SIGILL in JavaScriptCore on a Geode processor | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Drake <dsd> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Daniel Drake
2012-09-10 10:20:24 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? (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). (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. Created attachment 166363 [details]
Patch
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. 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.
Comment on attachment 166363 [details]
Patch
Oops, I meant r-
Created attachment 166502 [details]
Patch
Comment on attachment 166502 [details] Patch Clearing flags on attachment: 166502 Committed r130076: <http://trac.webkit.org/changeset/130076> All reviewed patches have been landed. Closing bug. 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. |