The default bug view has changed. See this FAQ.

[VC8] gfx/2d/Factory.cpp compilation fails since bug 732985 landed

RESOLVED FIXED in Firefox 15

Status

()

Core
Graphics
P1
blocker
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: sgautherie, Assigned: m_kato)

Tracking

({regression})

Trunk
mozilla15
x86
Windows Server 2003
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox15+ fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1336654250.1336656302.26708.gz&fulltext=1
WINNT 5.2 comm-central-trunk build on 2012/05/10 05:50:50
{
Factory.cpp

e:\builds\slave\comm-cen-trunk-w32\build\mozilla\gfx\2d\Rect.h(91) : warning C4244: 'argument' : conversion from 'int32_t' to 'mozilla::gfx::Float', possible loss of data

D:\msvs8\VC\INCLUDE\typeinfo(139) : warning C4275: non dll-interface class 'stdext::exception' used as base for dll-interface class 'std::bad_cast'

D:\msvs8\VC\INCLUDE\typeinfo(160) : warning C4275: non dll-interface class 'stdext::exception' used as base for dll-interface class 'std::bad_typeid'

D:\msvs8\VC\INCLUDE\intrin.h(944) : error C2733: second C linkage of overloaded function '_interlockedbittestandset' not allowed
        D:\msvs8\VC\INCLUDE\intrin.h(944) : see declaration of '_interlockedbittestandset'

D:\msvs8\VC\INCLUDE\intrin.h(945) : error C2733: second C linkage of overloaded function '_interlockedbittestandreset' not allowed
        D:\msvs8\VC\INCLUDE\intrin.h(945) : see declaration of '_interlockedbittestandreset'

e:/builds/slave/comm-cen-trunk-w32/build/mozilla/gfx/2d/Factory.cpp(251) : warning C4065: switch statement contains 'default' but no 'case' labels
}

https://tbpl.mozilla.org/?onlyunstarred=1&rev=b7b6565d12a0
https://hg.mozilla.org/mozilla-central/rev/b7b6565d12a0

As a guess, I assume this could be a "VC8 vs VC10" issue...
(Reporter)

Updated

5 years ago
Summary: [SeaMonkey] gfx/2d/Factory.cpp compilation fails after (m-i -> m-c) changeset b7b6565d12a0, (related to VC8 !?) → [SeaMonkey, Windows] gfx/2d/Factory.cpp compilation fails after (m-i -> m-c) changeset b7b6565d12a0, (related to VC8 !?)
Sorry, I have no idea, I just performed the merge...

Comment 2

5 years ago
Perhaps bustage from Bug 732985
(Reporter)

Updated

5 years ago
Blocks: 732985
(Reporter)

Comment 3

5 years ago
(In reply to Serge Gautherie (:sgautherie) from comment #0)
> As a guess, I assume this could be a "VC8 vs VC10" issue...

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=fe8e10215e91

Fwiw, these SeaMonkey VC10 TB-Try builds succeeded.
(In reply to Serge Gautherie (:sgautherie) from comment #3)
> (In reply to Serge Gautherie (:sgautherie) from comment #0)
> > As a guess, I assume this could be a "VC8 vs VC10" issue...
> 
> https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=fe8e10215e91
> 
> Fwiw, these SeaMonkey VC10 TB-Try builds succeeded.

In that case, I'd LOVE if people could still use MSVC8 locally if this fix isn't too hard for that.

Bas, is there an easy way to fix this, or do I need to bite the bullet and get my sorry bum to deploy the "build on MSVC2010" changes that are not fully written/tested yet for SeaMonkey machines?
(Reporter)

Comment 5

5 years ago
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=f61b5e6b3915

These SeaMonkey VC8 TB-Try builds failed: confirming compiler issue.

***

(In reply to Justin Wood (:Callek) from comment #4)
> In that case, I'd LOVE if people could still use MSVC8 locally if this fix
> isn't too hard for that.

Ftr, VC8+ is still supported, though MoCo use VC10 for official builds now.
tracking-firefox15: --- → ?
Summary: [SeaMonkey, Windows] gfx/2d/Factory.cpp compilation fails after (m-i -> m-c) changeset b7b6565d12a0, (related to VC8 !?) → [VC8 (SeaMonkey)] gfx/2d/Factory.cpp compilation fails since bug 732985 landed
(Assignee)

Comment 6

5 years ago
MSVC2005 with SDK 7.1 cannot include intrin.h well.

Workaround is
- Don't include intrin.h if MSVC2005 (or 2008?)
- Use inline assembler for cpuid
(Assignee)

Comment 7

5 years ago
Created attachment 623933 [details] [diff] [review]
fix
(Assignee)

Comment 8

5 years ago
Comment on attachment 623933 [details] [diff] [review]
fix

- community is still building for MACOSX PPC.  So don't use XP_MACOSX for SSE2 check.  We should use GCC macro "__SSE2__"
- Add workaround for MSVC2005
Attachment #623933 - Flags: review?(bas.schouten)
This is also busting xulrunner m-c nightlies on win32.
(In reply to Makoto Kato from comment #8)
> Comment on attachment 623933 [details] [diff] [review]
> fix
> 
> - community is still building for MACOSX PPC.  So don't use XP_MACOSX for
> SSE2 check.  We should use GCC macro "__SSE2__"
> - Add workaround for MSVC2005

Doesn't GCC only define __SSE2__ whenc ompiling with -msse2?
(In reply to Bas Schouten (:bas) from comment #10)
> (In reply to Makoto Kato from comment #8)
> > Comment on attachment 623933 [details] [diff] [review]
> > fix
> > 
> > - community is still building for MACOSX PPC.  So don't use XP_MACOSX for
> > SSE2 check.  We should use GCC macro "__SSE2__"
> > - Add workaround for MSVC2005
> 
> Doesn't GCC only define __SSE2__ whenc ompiling with -msse2?

To clarify, it's said that -msse2 is the default on OSX? But I don't really see that in the build logs.
On OS X, that is the default; gcc is equivalent to calling gcc -msse2 -fpmath=sse2 IIRC.
(Assignee)

Comment 13

5 years ago
(In reply to Bas Schouten (:bas) from comment #11)
> (In reply to Bas Schouten (:bas) from comment #10)
> > (In reply to Makoto Kato from comment #8)
> > > Comment on attachment 623933 [details] [diff] [review]
> > > fix
> > > 
> > > - community is still building for MACOSX PPC.  So don't use XP_MACOSX for
> > > SSE2 check.  We should use GCC macro "__SSE2__"
> > > - Add workaround for MSVC2005
> > 
> > Doesn't GCC only define __SSE2__ whenc ompiling with -msse2?
> 
> To clarify, it's said that -msse2 is the default on OSX? But I don't really
> see that in the build logs.

Yes. gcc on XCode uses -msse2 by default.  So it always define __SSE2__.  Of course, x86-64 gcc (OSX, Linux and etc) sets __SSE2__ even if no -msse2.
Comment on attachment 623933 [details] [diff] [review]
fix

Review of attachment 623933 [details] [diff] [review]:
-----------------------------------------------------------------

Approving this, I should reiterate that this -will- disable SSE2 for < MSVC10. Once we switch to Azure-for-content for D2D users very large images will be -slow- for < MSVC10 users.
Attachment #623933 - Flags: review?(bas.schouten) → review+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/55b4de9a4f53
I don't think that's too bad. Our official builds are VC10. Having the ability to compile the code with VC8 is important, even if the builds are suboptimal in perf. If someone is actually shipping code based on VC8, they can do the work to fix that if it's desired.

Updated

5 years ago
tracking-firefox15: ? → +
https://hg.mozilla.org/mozilla-central/rev/55b4de9a4f53
Assignee: nobody → m_kato
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(Assignee)

Comment 18

5 years ago
(In reply to Ted Mielczarek [:ted] from comment #16)
> I don't think that's too bad. Our official builds are VC10. Having the
> ability to compile the code with VC8 is important, even if the builds are
> suboptimal in perf. If someone is actually shipping code based on VC8, they
> can do the work to fix that if it's desired.

ImageScalingSSE2.cpp cannot compile on VC8 due to _mm_castsi128_ps.  So I think we should turn off SSE2 on ImageScaling to keep building using VC8 and file bug 756010.

I believe that VC8 should be still target compiler for seamonkey and community, but it has some limitations due to old.  Of course, if anyone fixes it, I am happy.
This is a VC8 bug; its intrin.h contains the wrong declarations for some of the functions. The bug was fixed in VC9. The fact that you're seeing the error means that you're already including the Platform SDK header that contains the correct declarations for those functions. I don't know whether the SDK header contains all the intrinsic declarations though.
(Reporter)

Updated

5 years ago
Blocks: 756010
(Reporter)

Updated

5 years ago
Summary: [VC8 (SeaMonkey)] gfx/2d/Factory.cpp compilation fails since bug 732985 landed → [VC8] gfx/2d/Factory.cpp compilation fails since bug 732985 landed
(Reporter)

Updated

5 years ago
Depends on: 756108
(Reporter)

Comment 20

5 years ago
Comment on attachment 623933 [details] [diff] [review]
fix

Review of attachment 623933 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Factory.cpp
@@ +96,5 @@
>  #else
>  
> +#if defined(_MSC_VER) && _MSC_VER >= 1600 && (defined(_M_IX86) || defined(_M_AMD64))
> +// MSVC 2005 or later supports __cpuid as intrin.
> +// But it does't work on MSVC 2005 with SDK 7.1 (Bug 753772)

I think this should either use "1500" or have a comment about MSVC 2008 case too.
(Assignee)

Comment 21

5 years ago
marking fixed for status-firefox15
status-firefox15: --- → fixed
Depends on: 839257
Depends on: 1266292
You need to log in before you can comment on or make changes to this bug.