96286 2012-09-10 10:20:24 -0700 Another SIGILL in JavaScriptCore on a Geode processor 2013-04-05 09:08:09 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 112239 0 dsd webkit-unassigned benjamin cgarcia fpizlo mrobinson oliver webkit.review.bot wingo oldest_to_newest 716223 0 dsd 2012-09-10 10:20:24 -0700 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) 729935 1 dsd 2012-09-27 15:38:26 -0700 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? 729974 2 oliver 2012-09-27 16:13:15 -0700 (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). 730891 3 fpizlo 2012-09-28 12:31:00 -0700 (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. 731205 4 166363 dsd 2012-09-29 08:05:33 -0700 Created attachment 166363 Patch 731206 5 dsd 2012-09-29 08:10:22 -0700 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. 731254 6 166363 fpizlo 2012-09-29 14:39:01 -0700 Comment on attachment 166363 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. 731255 7 166363 fpizlo 2012-09-29 14:39:51 -0700 Comment on attachment 166363 Patch Oops, I meant r- 731791 8 166502 dsd 2012-10-01 10:38:12 -0700 Created attachment 166502 Patch 731958 9 166502 webkit.review.bot 2012-10-01 14:00:42 -0700 Comment on attachment 166502 Patch Clearing flags on attachment: 166502 Committed r130076: <http://trac.webkit.org/changeset/130076> 731959 10 webkit.review.bot 2012-10-01 14:00:49 -0700 All reviewed patches have been landed. Closing bug. 731994 11 166502 darin 2012-10-01 14:32:55 -0700 Comment on attachment 166502 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. 166363 2012-09-29 08:05:33 -0700 2012-10-01 10:38:07 -0700 Patch bug-96286-20120929110447.patch text/plain 1483 dsd SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxMjk5NzgpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTA5LTI5ICBEYW5pZWwgRHJha2UgIDxkc2RA bGFwdG9wLm9yZz4KKworICAgICAgICBBbm90aGVyIFNJR0lMTCBpbiBKYXZhU2NyaXB0Q29yZSBv biBhIEdlb2RlIHByb2Nlc3NvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9OTYyODYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBEaXNhYmxlIExMaW50IGZvciB0aGUgR1RLIGJ1aWxkIHdoZXJlIHRoZSBidWls ZCB0YXJnZXQgZG9lcyBub3QKKyAgICAgICAgc3VwcG9ydCBTU0UyIGluc3RydWN0aW9ucy4gUmVz dG9yZXMgc3VwcG9ydCBmb3Igbm9uLVNTRTIgcHJvY2Vzc29ycworICAgICAgICBzdWNoIGFzIHRo ZSBBTUQgR2VvZGUuCisKKyAgICAgICAgKiB3dGYvUGxhdGZvcm0uaDoKKwogMjAxMi0wOS0yOCAg QW5kZXJzIENhcmxzc29uICA8YW5kZXJzY2FAYXBwbGUuY29tPgogCiAgICAgICAgIFJlbW92ZSBK YXZhIGJyaWRnZQpJbmRleDogU291cmNlL1dURi93dGYvUGxhdGZvcm0uaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybS5oCShyZXZpc2lvbiAxMjk5NzYpCisrKyBTb3VyY2Uv V1RGL3d0Zi9QbGF0Zm9ybS5oCSh3b3JraW5nIGNvcHkpCkBAIC05MDksNiArOTA5LDE0IEBACiAj ZGVmaW5lIEVOQUJMRV9MTElOVCAxCiAjZW5kaWYKIAorLyogT24gdGhlIEdUSysgcG9ydCB3ZSB0 YWtlIG9uZSBleHRyYSBwcmVjYXV0aW9uIGZvciBMTElOVCBzdXBwb3J0OgorICogV2UgZG9uJ3Qg ZW5hYmxlIGl0IG9uIHg4NiBidWlsZHMgaWYgdGhlIGJ1aWxkIHRhcmdldCBkb2Vzbid0IHN1cHBv cnQKKyAqIFNTRTIgaW5zdHJ1Y3Rpb25zIChMTElOVCByZXF1aXJlcyBTU0UyIG9uIHRoaXMgcGxh dGZvcm0pLiAqLworI2lmIEVOQUJMRShMTElOVCkgJiYgUExBVEZPUk0oR1RLKSAmJiBDUFUoWDg2 KSAmJiBDT01QSUxFUihHQ0MpIFwKKyAgICAmJiAhZGVmaW5lZChfX1NTRTJfXykKKyN1bmRlZiBF TkFCTEVfTExJTlQKKyNlbmRpZgorCiAjaWYgIWRlZmluZWQoRU5BQkxFX0RGR19KSVQpICYmIEVO QUJMRShKSVQpICYmICFDT01QSUxFUihNU1ZDKQogLyogRW5hYmxlIHRoZSBERkcgSklUIG9uIFg4 NiBhbmQgWDg2XzY0LiAgT25seSB0ZXN0ZWQgb24gTWFjIGFuZCBHTlUvTGludXguICovCiAjaWYg KENQVShYODYpIHx8IENQVShYODZfNjQpKSAmJiAoUExBVEZPUk0oTUFDKSB8fCBPUyhMSU5VWCkp Cg== 166502 2012-10-01 10:38:12 -0700 2012-10-01 14:32:55 -0700 Patch bug-96286-20121001133725.patch text/plain 1465 dsd SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxMzAwNTUpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTEwLTAxICBEYW5pZWwgRHJha2UgIDxkc2RA bGFwdG9wLm9yZz4KKworICAgICAgICBBbm90aGVyIFNJR0lMTCBpbiBKYXZhU2NyaXB0Q29yZSBv biBhIEdlb2RlIHByb2Nlc3NvcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9OTYyODYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBEaXNhYmxlIExMaW50IGZvciB0aGUgR1RLIGJ1aWxkIHdoZXJlIHRoZSBidWls ZCB0YXJnZXQgZG9lcyBub3QKKyAgICAgICAgc3VwcG9ydCBTU0UyIGluc3RydWN0aW9ucy4gUmVz dG9yZXMgc3VwcG9ydCBmb3Igbm9uLVNTRTIgcHJvY2Vzc29ycworICAgICAgICBzdWNoIGFzIHRo ZSBBTUQgR2VvZGUuCisKKyAgICAgICAgKiB3dGYvUGxhdGZvcm0uaDoKKwogMjAxMi0xMC0wMSAg WXVyeSBTZW1pa2hhdHNreSAgPHl1cnlzQGNocm9taXVtLm9yZz4KIAogICAgICAgICBXZWIgSW5z cGVjdG9yOiBwcm92aWRlIG1lbW9yeSBpbnN0cnVtZW50YXRpb24gZm9yIEhhc2hNYXAKSW5kZXg6 IFNvdXJjZS9XVEYvd3RmL1BsYXRmb3JtLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dURi93dGYv UGxhdGZvcm0uaAkocmV2aXNpb24gMTMwMDU1KQorKysgU291cmNlL1dURi93dGYvUGxhdGZvcm0u aAkod29ya2luZyBjb3B5KQpAQCAtODk5LDYgKzg5OSwxNCBAQAogI2RlZmluZSBFTkFCTEVfRElT QVNTRU1CTEVSIDEKICNlbmRpZgogCisvKiBPbiB0aGUgR1RLKyBwb3J0IHdlIHRha2UgYW4gZXh0 cmEgcHJlY2F1dGlvbiBmb3IgTExJTlQgc3VwcG9ydDoKKyAqIFdlIGRpc2FibGUgaXQgb24geDg2 IGJ1aWxkcyBpZiB0aGUgYnVpbGQgdGFyZ2V0IGRvZXNuJ3Qgc3VwcG9ydCBTU0UyCisgKiBpbnN0 cnVjdGlvbnMgKExMSU5UIHJlcXVpcmVzIFNTRTIgb24gdGhpcyBwbGF0Zm9ybSkuICovCisjaWYg IWRlZmluZWQoRU5BQkxFX0xMSU5UKSAmJiBQTEFURk9STShHVEspICYmIENQVShYODYpICYmIENP TVBJTEVSKEdDQykgXAorICAgICYmICFkZWZpbmVkKF9fU1NFMl9fKQorI2RlZmluZSBFTkFCTEVf TExJTlQgMAorI2VuZGlmCisKIC8qIE9uIHNvbWUgb2YgdGhlIHBsYXRmb3JtcyB3aGVyZSB3ZSBo YXZlIGEgSklULCB3ZSB3YW50IHRvIGFsc28gaGF2ZSB0aGUgCiAgICBsb3ctbGV2ZWwgaW50ZXJw cmV0ZXIuICovCiAjaWYgIWRlZmluZWQoRU5BQkxFX0xMSU5UKSBcCg==