Closed Bug 753772 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect, P1)

x86
Windows Server 2003
defect

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox15 + fixed

People

(Reporter: sgautherie, Assigned: m_kato)

References

Details

(Keywords: regression)

Attachments

(1 file)

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...
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...
Perhaps bustage from Bug 732985
Blocks: 732985
(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?
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.
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
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
Attached patch fixSplinter Review
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.
(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+
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.
https://hg.mozilla.org/mozilla-central/rev/55b4de9a4f53
Assignee: nobody → m_kato
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(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.
Blocks: 756010
Summary: [VC8 (SeaMonkey)] gfx/2d/Factory.cpp compilation fails since bug 732985 landed → [VC8] gfx/2d/Factory.cpp compilation fails since bug 732985 landed
Depends on: 756108
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.
marking fixed for status-firefox15
Depends on: 839257
Depends on: 1266292
You need to log in before you can comment on or make changes to this bug.