Last Comment Bug 753772 - [VC8] gfx/2d/Factory.cpp compilation fails since bug 732985 landed
: [VC8] gfx/2d/Factory.cpp compilation fails since bug 732985 landed
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows Server 2003
: P1 blocker (vote)
: mozilla15
Assigned To: Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
:
Mentors:
Depends on: 756108 839257 1266292
Blocks: 732985 756010
  Show dependency treegraph
 
Reported: 2012-05-10 07:29 PDT by Serge Gautherie (:sgautherie)
Modified: 2016-04-20 21:36 PDT (History)
10 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
fix (1.53 KB, patch)
2012-05-14 21:45 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
bas: review+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2012-05-10 07:29:45 PDT
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...
Comment 1 Ed Morley [:emorley] 2012-05-10 07:54:15 PDT
Sorry, I have no idea, I just performed the merge...
Comment 2 Philip Chee 2012-05-10 08:55:39 PDT
Perhaps bustage from Bug 732985
Comment 3 Serge Gautherie (:sgautherie) 2012-05-10 12:55:32 PDT
(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.
Comment 4 Justin Wood (:Callek) 2012-05-10 12:59:18 PDT
(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?
Comment 5 Serge Gautherie (:sgautherie) 2012-05-10 19:06:26 PDT
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.
Comment 6 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-05-14 21:44:24 PDT
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
Comment 7 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-05-14 21:45:04 PDT
Created attachment 623933 [details] [diff] [review]
fix
Comment 8 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-05-14 21:47:52 PDT
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
Comment 9 Nick Thomas [:nthomas] 2012-05-15 00:43:09 PDT
This is also busting xulrunner m-c nightlies on win32.
Comment 10 Bas Schouten (:bas.schouten) 2012-05-15 04:16:30 PDT
(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?
Comment 11 Bas Schouten (:bas.schouten) 2012-05-15 08:05:56 PDT
(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.
Comment 12 Joe Drew (not getting mail) 2012-05-15 10:05:41 PDT
On OS X, that is the default; gcc is equivalent to calling gcc -msse2 -fpmath=sse2 IIRC.
Comment 13 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-05-15 19:07:45 PDT
(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 14 Bas Schouten (:bas.schouten) 2012-05-15 19:51:52 PDT
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.
Comment 15 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-05-16 01:47:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/55b4de9a4f53
Comment 16 Ted Mielczarek [:ted.mielczarek] 2012-05-16 09:50:39 PDT
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.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-05-16 19:30:41 PDT
https://hg.mozilla.org/mozilla-central/rev/55b4de9a4f53
Comment 18 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-05-16 23:40:35 PDT
(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.
Comment 19 neil@parkwaycc.co.uk 2012-05-17 00:43:47 PDT
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.
Comment 20 Serge Gautherie (:sgautherie) 2012-05-17 09:16:41 PDT
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.
Comment 21 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-06-14 22:58:16 PDT
marking fixed for status-firefox15

Note You need to log in before you can comment on or make changes to this bug.