Closed Bug 414042 Opened 12 years ago Closed 12 years ago

MVSC71 build error in jdapimin.c attempting to include intrin.h

Categories

(Core :: ImageLib, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mook, Assigned: Mook)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 411379 introduced the use of __cpuid via #include <intrin.h>.  MSVC71 hasn't got that header, causing the build to break.

Random replacement that probably sucks:

#ifdef __cplusplus
extern "C" void __cpuid( int CPUInfo[4], int InfoType );
#endif /* __cplusplus */
void __cpuid( int CPUInfo[4], int InfoType )
{
  int my_eax, my_ebx, my_ecx, my_edx;
  __asm {
    mov eax, InfoType;
    cpuid
    mov my_eax, eax
    mov my_ebx, ebx
    mov my_ecx, ecx
    mov my_edx, edx
  }
  CPUInfo[0] = my_eax;
  CPUInfo[1] = my_ebx;
  CPUInfo[2] = my_ecx;
  CPUInfo[3] = my_edx;
}

(This fails on old 486s and earlier, where cpuid is an illegal instruction)
(yes that code probably sucks)
I'll put back in the old code. Why is FF 3.0 running on MSVC71?
Remove the use of CPUID and the include of intrin.h.
Assignee: nobody → mmoy
Status: NEW → ASSIGNED
Duplicate of this bug: 414072
Comment on attachment 299308 [details] [diff] [review]
Undo CPUID usage.

Setting r because us Songbird people like to get this in (but we're definitely installing shiny new vc8 buildbots too!)

Thanks, mmoy :)
Attachment #299308 - Flags: review?(pavlov)
As I said in Bug 414072 VC2005 is the "official" compiler for Trunk.   Does this change have any downsides mmoy?

Comment on attachment 299308 [details] [diff] [review]
Undo CPUID usage.

it would be better imho to just write cpuid() in asm and not revert all of this code.
Attachment #299308 - Flags: review?(pavlov) → review-
The 486 doesn't support the instruction which is apparently the reason for a good chunk of the asm code in the MMX routine. Does FF 3.0 support the 486?
Okay, this instead attempts to implement __cpuid manually when not using MSVC8 or greater.  Will cause SIGILL on 486 DX2 and older.  Does that matter, or should I copy that larger block to check EFLAGS?

This will be slower in the non-intrinsic path.

Is that check for >= vc8 the right one?  I do not have access to ICC to see if they have that header.
Attachment #299352 - Flags: review?(pavlov)
Comment on attachment 299352 [details] [diff] [review]
implement __cpuid as mentioned in comment 0

changing reviewer based on conversation with pavlov in IRC.

(he was also in favor of adding the eflags bit in)
Attachment #299352 - Flags: review?(pavlov) → review?(mmoy)
(In reply to comment #8)
> Will cause SIGILL on 486 DX2 and older.  Does that matter, or
> should I copy that larger block to check EFLAGS?

I'm totally the wrong person to comment on this, but per http://en.wikipedia.org/wiki/486_DX2 this chip was obsoleted by the pentium series, and we apparently do not support anything older than win2k, which itself has a pentium processor as the minimum CPU requirement (http://en.wikipedia.org/wiki/Windows_2000#System_Requirements) - so it seems to me that not supporting the older chip would be OK.
Comment on attachment 299352 [details] [diff] [review]
implement __cpuid as mentioned in comment 0

cancelling review, because
- _MSC_VER test is wrong (it's 1400, and not hex)
- not saving registers properly
- while I'm at it, might as well put the 486 detection back.
Attachment #299352 - Flags: review?(mmoy)
Sorry for the bugspam and slowness; now probably works right :)
According to dbradley, though, msvc8 doesn't generate any of the eflags checking (just a straight cpuid call).
Attachment #299352 - Attachment is obsolete: true
Attachment #299521 - Flags: review?(mmoy)
It looks okay to me with the fix of the #endif comment.

On the performance comment compared to intrinsics earlier, it's not an issue as this code is only called on the first decode.
Attached patch fixed commentSplinter Review
If this looks good to you, please mark r+, thanks! :)
Attachment #299521 - Attachment is obsolete: true
Attachment #305409 - Flags: review?(mmoy)
Attachment #299521 - Flags: review?(mmoy)
Attachment #305409 - Flags: review?(mmoy) → review+
Mook, can you please try to get this into 1.9? I believe you'll need approval, but it's your patch... :-)
Comment on attachment 305409 [details] [diff] [review]
fixed comment

Err, crap, totally forgot about this, sorry. (I haven't built in a while)

Should be pretty safe (preprocessed to not affect the shipping platforms at all).  The code is mostly copied from previously working code too.
Attachment #305409 - Flags: approval1.9?
Comment on attachment 305409 [details] [diff] [review]
fixed comment

a1.9+=damons
Attachment #305409 - Flags: approval1.9? → approval1.9+
Assignee: mmoy → mook.moz+mozbz
Status: ASSIGNED → NEW
Comment on attachment 305409 [details] [diff] [review]
fixed comment

stuart, can you rs= this since mmoy isn't a jpeg peer?
Attachment #305409 - Flags: superreview?(pavlov)
Attachment #305409 - Flags: superreview?(pavlov) → superreview+
Checked in attachment 305409 [details] [diff] [review]:

Checking in mozilla/jpeg/jdapimin.c;
/cvsroot/mozilla/jpeg/jdapimin.c,v  <--  jdapimin.c
new revision: 3.11; previous revision: 3.10
done
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.