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


(Core :: JavaScript Engine: JIT, defect)

Windows 7
Not set





(Reporter: markh, Assigned: bbouvier)





(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 - it talks about "XSAVE functionality"
(12:24:44 PM) markh: is that related?
(12:25:25 PM) glandium: markh: 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.
We apparently just need to check one more bit (which happens to be the one for XSAVE)
Actually that code is also checking something in the extended control register.
Thanks for the report. This is probably a regression from bug 1065339.
Blocks: 1065339
Flags: needinfo?(sunfish)
Attachment #8535605 - Flags: review?(sunfish)
/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

::: js/src/jit/shared/Assembler-x86-shared.cpp
(Diff revision 1)
> +    avxPresent = (flagsECX & AVXBit) && (flagsECX & XSAVEBit) && avxEnabled;

Attachment #8535605 - Flags: review?(sunfish) → review+

::: js/src/jit/shared/Assembler-x86-shared.cpp
(Diff revision 1)
> +    avxPresent = (flagsECX & AVXBit) && (flagsECX & XSAVEBit) && avxEnabled;

lgtm too
Flags: needinfo?(sunfish)
Doh! mozext was supposed to post the inbound link here directly, but it didn't...
Assignee: nobody → benj
Mark and/or Mike, does it fix the issue?
Flags: needinfo?(mhammond)
Flags: needinfo?(mh+mozilla)
Shouldn't we also add the "check the AVX registers restore at context switch" from the link in comment 1?
Flags: needinfo?(mh+mozilla)
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attached patch xgetbv.patchSplinter Review
Ah, thanks for pointing that out. Attached is a patch which adds code to check the extended control register for OS support.
Attachment #8535952 - Flags: review?(benj)
Comment on attachment 8535952 [details] [diff] [review]

Review of attachment 8535952 [details] [diff] [review]:


::: js/src/jit/shared/Assembler-x86-shared.cpp
@@ +152,5 @@
> +    size_t xcr0EAX = 0;
> +    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+
Depends on: 1111715
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!
Flags: needinfo?(mhammond)
Attachment #8535605 - Attachment is obsolete: true
Attachment #8618902 - Flags: review+
You need to log in before you can comment on or make changes to this bug.