1096651 - Wrong CPU features detection on some x86 CPUs

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Mike Hommey [:glandium]]

I got a report from a Debian user that was getting JIT illegal instruction crashes on a very old CPU since he upgraded from esr24 to esr31.

After some investigation, it turns out the JIT thinks his CPU has support for SSE3, which it doesn't. It doesn't even have SSE. I suspect his CPU's cpuid reports something that doesn't quite match the current spec. My guess is that this would be worked around by making SSE3 only detected if SSE2 is detected as well.

I'm waiting for the exact values cpuid is returning on his CPU. /proc/cpuinfo reads:
processor       : 0
vendor_id       : CyrixInstead
cpu family      : 6
model           : 2
model name      : M II 3x Core/Bus Clock
stepping        : 4
cpu MHz         : 225.016
fdiv_bug        : no
hlt_bug         : no
f00f_bug        : no
coma_bug        : no
fpu             : yes
fpu_exception   : yes
cpuid level     : 1
wp              : yes
flags           : fpu de tsc msr cx8 pge cmov mmx cyrix_arr
bogomips        : 450.03
clflush size    : 32
cache_alignment : 32
address sizes   : 32 bits physical, 32 bits virtual
power management:

Comment 1

[Mike Hommey [:glandium]]

Attachment: Avoid using random bits when determining SSE3/SSE4 availability for the JIT

Comment 2

[Mike Hommey [:glandium]]

Attachment: Backport for esr31

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

As a heads up, :sunfish also has a patch for this on try: https://hg.mozilla.org/try/rev/100c6b5c69e4

I don't know whether it also solves the problem reported here, since it's just based on the description in comment #0.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #3)
> As a heads up, :sunfish also has a patch for this on try:
> https://hg.mozilla.org/try/rev/100c6b5c69e4
> 
> I don't know whether it also solves the problem reported here, since it's
> just based on the description in comment #0.

That patch would still misidentify avx support, since the root problem is that ecx is containing random bits.

Comment 5

[Mike Hommey [:glandium]]

Ah no, it would work because of the && maxSSEVersion >= SSE. Clobbering ecx is easier, though.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8520997 [details] [diff] [review]
Avoid using random bits when determining SSE3/SSE4 availability for the JIT

Nice job tracking that down.

Comment 7

[Dan Gohman [:sunfish]]

Cool; this is a better patch than mine.

Comment 8

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78e1b06d8c79

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8520997 [details] [diff] [review]
Avoid using random bits when determining SSE3/SSE4 availability for the JIT

Approval Request Comment
[User impact if declined]: Crashes with "illegal instruction" on machines using old CPUs.
[Risks and why]: None. The only change is to reset a CPU register before calling the cpuid instruction, which doesn't use that register as input but as output. The problem being fixed is that some CPU's cpuid instruction doesn't set that register at all, so we get whatever the value of the register was before calling cpuid. With this patch, the value is fixed and doesn't have the side effect to enable features that the CPU won't support, like SSE3. If it builds, it can't break anything.
[String/UUID change made/needed]: None

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8521011 [details] [diff] [review]
Backport for esr31

[Approval Request Comment]
See beta/aurora approval request. There is an additional risk with this patch, though, in that the patch is different. The worst that can happen is that it fails to build because of a typo. Specifically, I'm not entirely sure there shouldn't be a ; in the first patch hunk.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78e1b06d8c79

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/c450f3f34316

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8520997 [details] [diff] [review]
Avoid using random bits when determining SSE3/SSE4 availability for the JIT

Beta+
Aurora+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/016db9a29ac4
https://hg.mozilla.org/releases/mozilla-beta/rev/3e5cb63660bd

Comment 15

[Kamil Jozwiak [:kjozwiak]]

I'm going to mark this as qe‑verify- as this issue needs a specific CPU that only supports Streaming SIMD Extensions 2 (SSE2) which unfortunately I don't have :/ I tried searching to see if I could some how emulate a SSE2 CPU via VMware Fusion but it seems like fusion doesn't emulate CPU's.. (you need the higher end enterprise software for that capability)

Mike, if there's any other way I can test this to see if it's been fixed, please let me know and I'll gladly take another look! Perhaps we can get the original reporter to try it on his Debian machine and see if the issue is fixed for him?

Comment 16

[Mike Hommey [:glandium]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #15)
> I'm going to mark this as qe‑verify- as this issue needs a specific CPU that
> only supports Streaming SIMD Extensions 2 (SSE2) which unfortunately I don't
> have :/ I tried searching to see if I could some how emulate a SSE2 CPU via
> VMware Fusion but it seems like fusion doesn't emulate CPU's.. (you need the
> higher end enterprise software for that capability)

Worse than that, you need a specific CPU that doesn't set ecx with its cpuid instruction.

> Mike, if there's any other way I can test this to see if it's been fixed,
> please let me know and I'll gladly take another look! Perhaps we can get the
> original reporter to try it on his Debian machine and see if the issue is
> fixed for him?

I got confirmation that this fixed it for him.

Comment 17

[Kamil Jozwiak [:kjozwiak]]

> I got confirmation that this fixed it for him.

Awesome, thanks!

Comment 18

[Martin Thomson [:mt:]]

Why wasn't the same fix applied to the windows branch of the conditional?  Just asking so that I can determine how to apply this to bug 1263495.

Comment 19

[Martin Thomson [:mt:]]

Sorry, my bad, I'm not reading this right, the JS_CODEGEN_X64 branch doesn't have the fix, but that's still gcc.

Comment 20

[Mike Hommey [:glandium]]

Presumably, x64 CPUs are well-behaved.