248509 - NT4 "illegal instruction" SSE2 jpeg crash [@ dct_8x8_inv_16s ]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bernard Alleysson]

Regression from bug 125762

Even if the CPU supports SSE2, NT4 doesn't support it natively, and any attempt
to use SSE2 will result in an "illegal instruction" crash.
For NT4 and service pack < 5, the "Intel® Streaming SIMD Extensions Driver" from
has to be installed:
http://support.intel.com/support/processors/pentiumiii/sb/CS-007573.htm
For NT4 and service pack >= 5, Intel says SSE2 is supported but users report it
still broken.

A solution for Mozilla trunk would be to add OS SSE2 detection, code is
available here:
http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&selm=ZZmc7.3239%24NW6.1508767%40news1.sttln1.wa.home.com&rnum=11
It looks like it is based on MS compiler extension, so Mozilla SSE2 should be
disabled for other compilers.

An item should be added to the release notes of Mozilla 1.7 stating that Service
Pack 5 is needed for NT4, or the Intel SIMD driver must be installed.

Comment 1

[Bernard Alleysson]

requesting blocking flags because it is a blocker bug for anyone running NT4
with Sevice pack < 5 or without Intel SIMD driver

Comment 2

[Michael Moy]

This code is only enabled for MSVC++ at the moment so other compilers and
operating systems are not an issue at this time. Anyone know how SIMD works
on Windows 95 and Windows 98?

Comment 3

[Michael Moy]

The call to determine the os should be

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/getversionex.asp

or

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/getversion.asp

With Microsoft preferring the former.

Codes are explained at
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/osversioninfo_str.asp

Comment 4

[Michael Moy]

I'm testing the sample code that I grabbed from the MSDN to test the ability to 
print out the OS name and version number in jdapimin.c. If that works, then I'll
put the conditionals in to block out NT and maybe 95/98.

Comment 5

[Michael Moy]

The sample code worked (it correctly identified my Windows XP Home SP1 OS). So
the approach of controlling the SSE2 code via Operating System and Version number
would be easy to do.

BTW, one may wonder why someone is running Windows NT on a Pentium 4. I think 
that some companies and maybe some individuals bought Windows NT on a machine 
and then upgraded the machine or replaced it withouth an OS and then installed
their old Windows NT kit on it to save a few bucks on the Operating System.

At any rate, the other approach of TRY/CATCH can be done too. I don't like the
idea of code that determines capabilities by trying to throw an exception and 
then checking for the results and haven't seen a any of that in my travels
through the code base. It has the advantage of finding the machines where it
works even though the OS and OS version says it shouldn't though.

At any rate, we have two options now and it merely remains that we have some
discussion to pick one.
exception

Comment 6

[Michael Moy]

Testing the TRY/Exception approach. I'll need someone with an NT machine with a
Pentium 4 at SP4 without the Intel driver.

Comment 7

[Michael Moy]

The TRY/EXCEPT code works; at least on the working side. I need someone to test
the non-working side. Let me know if you can get someone from the TalkBack
crashes. I'll put up a patch for review tonight with the TRY/EXCEPT tonight.

Comment 8

[Michael Moy]

Attachment: Add check for SSE2 instruction capability

This code adds a check to the hardware SSE2 checker to ensure that the
operating
system supports SSE2 instructions. In Windows NT 4.0 Service Pack 4.0 and
lower,
SIMD registers aren't saved on a context switch unless special Intel Drivers
are
installed. We do a check to disable SSE2 JPEG processing if we can't execute an

SSE2 instruction in our check; even if the CPU supports it.

Comment 9

[Michael Moy]

Take a look at the patch and let me know what you think. I'll build a release
1.7 build with it in the meantime. Let me know which FireFox branch builds would
be useful too.

Comment 10

[Bernard Alleysson]

looks good but the except block be simplified
tabs and indentation are a mess in this file but looks like the local function
needs 4 spaces tabs
#include <windows.h> should be #ifdef HAVE_SSE2, not HAVE_MMX,
I'm editing the patch

Comment 11

[Bernard Alleysson]

Attachment: nits fixed (I couldn't edit the previous attachment)

Comment 12

[Bernard Alleysson]

In theory, #include "windows.h" should be #include <windows.h>, this is not
important. As for the firefox builds, it is needed for the AVIARY branch, once
it gets checked in for 1.7.1. Just for sanity, the code could be tested when the
processor doesn't support SSE2 (ex Athlon) by changing the condition to "(if
(sse2availabble == 2) then TRY/EXCEPT" into the debugger.

Comment 13

[Michael Moy]

There was a little fuzz in the patch (extra space in one line) but I updated the
source and it's building 1.7 now.

Some Athlons support SSE2. I can do the negative testing on a PIII.

I'll put out a request for a P4 with NT 4.0 SP4. I have a feeling that no one
will reply on MozillaZine as NT is more of a corporate platform.

Do you have folks lined up to do the reviews?

Comment 14

[Michael Moy]

(In reply to comment #10)
> looks good but the except block be simplified
> tabs and indentation are a mess in this file but looks like the local function
> needs 4 spaces tabs
> #include <windows.h> should be #ifdef HAVE_SSE2, not HAVE_MMX,
> I'm editing the patch

HAVE_MMX and HAVE_SSE2 mean the same thing. Here's the code in jmorecfg.h:
These should probably be replaced with HAVE_WIN32_SIMD. GCC for Linux on
Pentiums could be HAVE_GCC_SIMD and Windows 64 (AMD) could be HAVE_WIN64_SIMD.
GCC's inline assembly syntax is different from MSVC++ for Windows 32 and
inline assembly isn't supported at all in the Windows 64 DDK (intrinsics
are supported and there's a conversion tool from MSVC++ inline). I haven't
played around with Whidbey (Visual Studio 2005) enough to determine what it
supports.

#if defined(XP_WIN32) && defined(_M_IX86) && !defined(__GNUC__)
#define HAVE_MMX_INTEL_MNEMONICS 
#define HAVE_SSE2_INTEL_MNEMONICS 
#endif

Comment 15

[Michael Moy]

Build finished and ran fine on my SSE2/XP Home system. Will package it up and put
it on pryan.org for those that need it. Will try a FireFox build with the altered
patch for the PIII.

Comment 16

[Olivier Cahagne]

*** Bug 248479 has been marked as a duplicate of this bug. ***

Comment 17

[Michael Moy]

There's a Release 1.7 build with this patch at:

http://www.pryan.org/mozilla/seamonkey/mmoy/Mozilla-Release-1.7-O1-G6-Bug-248509-fix.exe

I requested that the reporter of the problem on Windows NT Service Pack 6 to try it out in the other bug that was just marked as a duplicate of this one.

I have the sse2available==2 build of FireFox done but don't have time to test it this morning. Will try tonight.

Comment 18

[Michael Moy]

Fix verified in http://bugzilla.mozilla.org/show_bug.cgi?id=248479

Comment 19

[Bernard Alleysson]

(In reply to comment #13)
> Do you have folks lined up to do the reviews?
No, but asking for reviews wouldn't hurt

Comment 20

[Bernard Alleysson]

Comment on attachment 151656 [details] [diff] [review]
nits fixed (I couldn't edit the previous attachment)

asking review for this simple patch (user reports WFM)

Comment 21

[Michael Moy]

Just a note to say that the workaround build has had 42 downloads in July so
far so it appears that there are a few folks out there with this problem. I
didn't check the stats for June.

Comment 22

[Michael Moy]

Release 1.7.1 build is up. [e-mail] wants to turn off sse2 processing so
I think that this patch is on hold. I'll keep doing builds on request for this
patch though.

http://www.pryan.org/mozilla/seamonkey/mmoy/Mozilla-Release-1.7.1-O1-G6-Bug-248509-fix.exe

Comment 23

[Michael Moy]

moox told me that the SSE2 code is still in the Aviary branch so this problem should be fixed if the code SSE2 code is going to remain on for FireFox 1.0.

If the SSE2 code is going to be left in, then I'd suggest that the patch get
reviewed and checked in.

So yes, this is blocking AVIARY.

Comment 24

[chris hofmann]

tor, can you review?

Comment 25

[tor]

Comment on attachment 151656 [details] [diff] [review]
nits fixed (I couldn't edit the previous attachment)

r=tor for branch/aviary

Comment 26

[chris hofmann]

vlad, I wonder if you could help out in getting this landed on the aviary branch?

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Landed on aviary.

It builds fine on linux, but I wonder -- why does linux not complain about that
"#include <windows.h>"?  Should that (and the later test that was added) be
protected by some windows-build goop?

Comment 28

[tor]

HAVE_SSE2_INTEL_MNEMONICS is only set on windows+msvc builds.

Comment 29

[Bernard Alleysson]

*** Bug 248479 has been marked as a duplicate of this bug. ***

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 151656 [details] [diff] [review]
nits fixed (I couldn't edit the previous attachment)

This needs to land on the 1.7 branch also.

Comment 31

[Bernard Alleysson]

Could someone (tor, roc, vladimir ?) land this on 1.7.x branch ? Thanks.

Comment 32

[timeless]

stupid question, what about trunk?

Comment 33

[Bernard Alleysson]

It's ok for the trunk, no risk but no effect because SSE2 was disabled there
(bug 247437) 

Comment 34

[timeless]

Comment on attachment 151656 [details] [diff] [review]
nits fixed (I couldn't edit the previous attachment)

mozilla/jpeg/jdapimin.c 	3.7.2.1

Comment 35

[Wayne Mery (:wsmwk)]

blocking bug 125762 per comment 0
blocked by bug 247437 per comment 33 (unless SSE2 was reenabled on trunk)

Comment 36

[Ryan VanderMeulen [:RyanVM]]

NT4 isn't supported on the trunk anymore. INVALID?

Comment 37

[timeless]

since we fixed this on 1.7, it'd be better to change the version to that and mark as fixed.

Comment 38

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yes, marking this FIXED.