Closed
Bug 1110570
Opened 10 years ago
Closed 10 years ago
avx instruction detection incorrect on CPUs with avx support but when windows bootloader has xsavedisable=1
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
VERIFIED
FIXED
mozilla37
People
(Reporter: markh, Assigned: bbouvier)
References
()
Details
Attachments
(2 files, 1 obsolete file)
Nightly is regularly crashing with an invalid instruction exception. Slightly edited transcript from #developers where glandium helped me get to the root: (12:07:25 PM) glandium: markh: what does the disassembly view say the instruction is? (12:08:57 PM) markh: glandium: 1C040BE6 vaddsd xmm0,xmm1,xmm0 (12:09:48 PM) glandium: markh: sounds like the jit is trying to use avx instructions and your cpu doesn't support them (12:11:44 PM) markh: the crash report says I've got "GenuineIntel family 6 model 58 stepping 9 | 8" - it's an i7-3770 (12:11:48 PM) glandium: markh: can you check the value of js::jit::CPUInfo::avxPresent and avxEnabled? (12:13:56 PM) glandium: markh: interestingly, the i7-3770 is supposed to support avx... (12:16:24 PM) markh: glandium: so yeah, it sounds like you aren't surprised to find both those vars are true (12:17:01 PM) glandium: markh: yeah, so now the question is why a valid avx instruction doesn't run on your cpu that supports avx (12:22:52 PM) markh: hrm - random googling led me to "bcdedit" to show the boot manager config (12:23:05 PM) markh: and xsavedisable is true, which *looks* like it disables that (12:24:36 PM) markh: glandium: see http://msdn.microsoft.com/en-us/library/windows/hardware/ff542202%28v=vs.85%29.aspx - it talks about "XSAVE functionality" (12:24:44 PM) markh: is that related? (12:25:25 PM) glandium: markh: http://support.microsoft.com/kb/2568088 suggests it is and sure enough, toggling xsavedisable to 0 and rebooting caused the problem to stop. I've no idea how it ended up being disabled (but maybe the vmware installation toggled it - not clear). Regardless, it sounds like our avx detection code somehow needs to determine if the windows bootloader has disabled that functionality.
Comment 1•10 years ago
|
||
We apparently just need to check one more bit (which happens to be the one for XSAVE) https://software.intel.com/en-us/blogs/2011/04/14/is-avx-enabled/
Comment 2•10 years ago
|
||
Actually that code is also checking something in the extended control register.
Comment 3•10 years ago
|
||
Thanks for the report. This is probably a regression from bug 1065339.
Blocks: 1065339
Flags: needinfo?(sunfish)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8535605 -
Flags: review?(sunfish)
Assignee | ||
Comment 5•10 years ago
|
||
/r/1425 - Bug 1110570: AVX instructions should be emitted only if XSAVE is enabled by the OS; Pull down this commit: hg pull review -r 9008ada71bfc33283578197e0257b30533c650b2
Comment 6•10 years ago
|
||
https://reviewboard.mozilla.org/r/1423/#review859 ::: js/src/jit/shared/Assembler-x86-shared.cpp (Diff revision 1) > + avxPresent = (flagsECX & AVXBit) && (flagsECX & XSAVEBit) && avxEnabled; nice!
Updated•10 years ago
|
Attachment #8535605 -
Flags: review?(sunfish) → review+
Comment 7•10 years ago
|
||
https://reviewboard.mozilla.org/r/1423/#review861 Ship It!
Comment 8•10 years ago
|
||
https://reviewboard.mozilla.org/r/1423/#review863 ::: js/src/jit/shared/Assembler-x86-shared.cpp (Diff revision 1) > + avxPresent = (flagsECX & AVXBit) && (flagsECX & XSAVEBit) && avxEnabled; lgtm too
Updated•10 years ago
|
Flags: needinfo?(sunfish)
Assignee | ||
Comment 9•10 years ago
|
||
Doh! mozext was supposed to post the inbound link here directly, but it didn't... https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9daf256c68
Assignee: nobody → benj
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•10 years ago
|
||
Mark and/or Mike, does it fix the issue?
Flags: needinfo?(mhammond)
Flags: needinfo?(mh+mozilla)
Comment 11•10 years ago
|
||
Shouldn't we also add the "check the AVX registers restore at context switch" from the link in comment 1?
Flags: needinfo?(mh+mozilla)
https://hg.mozilla.org/mozilla-central/rev/9e9daf256c68
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 13•10 years ago
|
||
Ah, thanks for pointing that out. Attached is a patch which adds code to check the extended control register for OS support. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=191c4d34f182
Attachment #8535952 -
Flags: review?(benj)
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8535952 [details] [diff] [review] xgetbv.patch Review of attachment 8535952 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/jit/shared/Assembler-x86-shared.cpp @@ +152,5 @@ > + size_t xcr0EAX = 0; > +#if defined(_XCR_XFEATURE_ENABLED_MASK) > + xcr0EAX = _xgetbv(_XCR_XFEATURE_ENABLED_MASK); > +#elif defined(__GNUC__) > + asm(".byte 0x0f, 0x01, 0xd0" : "=a"(xcr0EAX) : "c"(0) : "%edx"); can you add a comment that 0x0f 0x01 0xd0 is the encoding for xgetbv and that it stores its results in eax::edx? @@ +231,5 @@ > + size_t xcr0EAX = ReadXGETBV(); > + static const int xcr0SSEBit = 1 << 1; > + static const int xcr0AVXBit = 1 << 2; > + if (!(xcr0EAX & xcr0SSEBit) || !(xcr0EAX & xcr0AVXBit)) > + avxPresent = false; avxPresent = (xcr0EAX & xcr0SSEBit) && (xcr0EAX & xcr0AVXBit); would allow you to get rid of the |if| statement, but not a big deal so up to you
Attachment #8535952 -
Flags: review?(benj) → review+
Reporter | ||
Comment 17•10 years ago
|
||
I just set the xsavedisable flag back to 1 and re-created the problem on my nightly channel (which hadn't yet been updated with the fix). I then tried with a local build that *does* have the fix and was unable to re-create the problem. So marking verified - thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(mhammond)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8535605 -
Attachment is obsolete: true
Attachment #8618902 -
Flags: review+
Assignee | ||
Comment 19•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•