217009 - build issues with mingw gcc 3.3.1

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[cls]

I attempted to build SeaMonkey using mingw gcc 3.3.1 RC1 and the results were
not pretty.  The first problem I hit was that gcc was unable to auto-import
certain vtables when building.  

/c/root/obj-test/../mozilla/build/cygwin-wrapper g++ -mno-cygwin -shared
-Wl,--export-all-symbols -Wl,--out-implib -Wl,libxpcom_compat.dll.a -o
xpcom_compat.dll  nsFileSpec.o nsFileStream.o nsIFileStream.o nsFileSpecImpl.o
nsSpecialSystemDirectory.o dlldeps.o   ./module.res   -Wl,--whole-archive
../../dist/lib/libmozreg_s.a  -Wl,--no-whole-archive  -L../../dist/lib -lxpcom
-L../../dist/bin -L../../dist/lib -lnspr4 -lplc4 -lplds4 -lshell32 -lole32
-limagehlp -lm    
Info: resolving _IID_IPersistFile by linking to __imp__IID_IPersistFile
(auto-import)
Info: resolving __ZTV16nsQueryInterface by linking to
__imp___ZTV16nsQueryInterface (auto-import)
Info: resolving __ZTV28nsCreateInstanceByContractID by linking to
__imp___ZTV28nsCreateInstanceByContractID (auto-import)
Info: resolving __ZTV24nsASingleFragmentCString by linking to
__imp___ZTV24nsASingleFragmentCString (auto-import)
Info: resolving __ZTV10nsACString by linking to __imp___ZTV10nsACString
(auto-import)
Info: resolving __ZTV9nsHashKey by linking to __imp___ZTV9nsHashKey (auto-import)
nsFileSpec.o(.text$_ZN16nsQueryInterfaceC1EP11nsISupportsPj+0x16):nsFileSpec.cpp:
variable 'vtable for nsQueryInterface' can't be auto-imported. Please read the
documentation for ld's --enable-auto-import for details.
nsFileSpec.o(.text$_ZN28nsCreateInstanceByContractIDC1EPKcP11nsISupportsPj+0x16):nsFileSpec.cpp:
variable 'vtable for nsCreateInstanceByContractID' can't be auto-imported.
Please read the documentation for ld's --enable-auto-import for details.
nsFileSpec.o(.text$_ZN24nsASingleFragmentCStringC2Ev+0x16):nsFileSpec.cpp:
variable 'vtable for nsASingleFragmentCString' can't be auto-imported. Please
read the documentation for ld's --enable-auto-import for details.
nsFileSpec.o(.text$_ZN24nsASingleFragmentCStringD2Ev+0xb):nsFileSpec.cpp:
variable 'vtable for nsASingleFragmentCString' can't be auto-imported. Please
read the documentation for ld's --enable-auto-import for details.
nsFileSpec.o(.text$_ZN10nsACStringC2Ev+0x8):nsFileSpec.cpp: variable 'vtable for
nsACString' can't be auto-imported. Please read the documentation for ld's
--enable-auto-import for details.
nsFileSpec.o(.text$_ZN10nsACStringD2Ev+0xb):nsFileSpec.cpp: variable 'vtable for
nsACString' can't be auto-imported. Please read the documentation for ld's
--enable-auto-import for details.
nsSpecialSystemDirectory.o(.text$_ZN9nsHashKeyC2Ev+0xb):nsSpecialSystemDirectory.cpp:
variable 'vtable for nsHashKey' can't be auto-imported. Please read the
documentation for ld's --enable-auto-import for details.
en

After reading the suggested ld documentation, I added
-Wl,-enable-runtime-psuedo-reloc to DSO_LDOPTS and that allowed the build to get
further.  

The next set of problems were mainly casting issues & a less forgiving parser. 
There are a number of places where we incorrectly assume that PRUnichar ==
PRUint16.  That's true on most unix platforms but not on win32.  There are a
couple of cases where we have trailing commas in an enum and one case where the
a class function is declared using the class prefix.

Comment 1

[cls]

Attachment: v1.0

This patch allows the build to complete but it crashes on startup.  When
debugging, I ran across something strange.  Eventually, the code goes through
NS_NewURI which calls nsIOService::NewURI but gdb cannot access the memory for
the location of 'nsIOService::NewURI(nsACString const&, char const*, nsIURI*,
nsIURI**)'.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 130235 [details] [diff] [review]
v1.0

Most of these changes (excluding the compiler option, which I don't know
anything about) look good.  However, a few comments:

>Index: mailnews/imap/src/nsImapMailFolder.cpp
>@@ -294,7 +294,9 @@
>       // unfortunately we can't just say:
>       //          path += sep;
>       // here because of the way nsFileSpec concatenates
>-      nsAutoString str; str.AssignWithConversion(NS_STATIC_CAST(nsFilePath, path));
>+      nsAutoString str; 
>+      nsFilePath tmpPath(path);
>+      str.AssignWithConversion(tmpPath);
>       str += sep;
>       path = nsFilePath(str);
>     }

Does |str.AssignWithConversion(nsFilePath(path));| work?  If so, I'd prefer it
to the named temporary variable.

>Index: mailnews/news/src/nsNewsFolder.cpp

Same here.

>Index: netwerk/protocol/http/src/nsHttpNTLMAuth.cpp
>@@ -276,11 +276,11 @@
>         PRUnichar *buf = nsnull;
> 
>         if (user && pass && domain) {
>-            ident.User           = (PRUnichar *) user;
>+            ident.User           = (unsigned short*) user;
>             ident.UserLength     = nsCRT::strlen(user);
>-            ident.Domain         = (PRUnichar *) domain;
>+            ident.Domain         = (unsigned short*) domain;
>             ident.DomainLength   = nsCRT::strlen(domain);
>-            ident.Password       = (PRUnichar *) pass;
>+            ident.Password       = (unsigned short*) pass;
>             ident.PasswordLength = nsCRT::strlen(pass);
>             ident.Flags          = SEC_WINNT_AUTH_IDENTITY_UNICODE;
>             pIdent = &ident;

These should probably be casts to |WCHAR*| rather than |unsigned short*|. 
(That's what the examples on MSDN show, despite the documentation for
SEC_WINNT_AUTH_IDENTITY (not _W, but I don't know what the difference is) uses
|unsigned short*|.

>Index: uriloader/exthandler/win/nsOSHelperAppService.cpp
>@@ -41,7 +41,7 @@
> #define LOG(args) PR_LOG(mLog, PR_LOG_DEBUG, args)
> 
> // helper methods: forward declarations...
>-BYTE * GetValueBytes( HKEY hKey, const char *pValueName, DWORD *pLen=0);
>+static BYTE * GetValueBytes( HKEY hKey, const char *pValueName, DWORD *pLen=0);
> nsresult GetExtensionFrom4xRegistryInfo(const char * aMimeType, nsCString& aFileExtension);
> nsresult GetExtensionFromWindowsMimeDatabase(const char * aMimeType, nsCString& aFileExtension);
> 

You might need to remove the later occurrence of |static| in the definition --
I think that's the portable way of doing it, anyway.

>Index: webshell/tests/viewer/nsWinMain.cpp
>+#ifndef __GNUC__

Hmm?

>Index: widget/src/windows/nsWindow.cpp
>@@ -6590,6 +6590,7 @@
>     if (hIMC) {
>       int positioning = 0;
>       int offset = 0;
>+      PRInt32 i = 0;
> 
>       // calcurate positioning and offset
>       // char :            JCH1|JCH2|JCH3
>@@ -6598,7 +6599,7 @@
> 
>       // Note: hitText has been done, so no check of mIMECompCharPos
>       // and composing char maximum limit is necessary.
>-      for (PRInt32 i = 0; i < mIMECompUnicode->Length(); i++) {
>+      for (i = 0; i < mIMECompUnicode->Length(); i++) {

Maybe put the |PRInt32 i = 0| right before the loop?

>Index: xpcom/glue/nsCOMPtr.cpp
>Index: xpcom/glue/nsCOMPtr.h

Maybe move the comment too?

(I think this code needs to be fixed, though, since whenever this change causes
things to run, it probably causes them to leak as well.)

>Index: xpfe/bootstrap/nsAppRunner.cpp
>@@ -1684,7 +1684,7 @@
>   return TranslateReturnValue(mainResult);
> }
> 
>-#if defined( XP_WIN ) && defined( WIN32 )
>+#if defined( XP_WIN ) && defined( WIN32 ) && !defined(__GNUC__)
> // We need WinMain in order to not be a console app.  This function is
> // unused if we are a console application.
> int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR args, int)

Hmm?  Does this have anything to do with not being able to run?

>Index: xpfe/bootstrap/nsNativeAppSupportWin.cpp
>@@ -1062,7 +1062,7 @@
> PRBool
> nsNativeAppSupportWin::InitTopicStrings() {
>     for ( int i = 0; i < topicCount; i++ ) {
>-        if ( !( mTopics[ i ] = DdeCreateStringHandle( mInstance, topicNames[ i ], CP_WINANSI ) ) ) {
>+        if ( !( mTopics[ i ] = DdeCreateStringHandle( mInstance, (char*)topicNames[ i ], CP_WINANSI ) ) ) {
>             return PR_FALSE;
>         }
>     }

This should be NS_CONST_CAST to show why you're casting and to be somewhat less
dangerous.

Comment 3

[cls]

I think the auto-import problem may be a side-effect of using
-Wl,--export-all-symbols when linking non-component libraries.  The alternative
to using that flag is to properly decorate each to-be-exported function with the
NS_COM macro (dllimport/dllexport).  Apparently, just declaring a class with
NS_COM isn't good enough.  

Comment 4

[Brant Gurganus]

If it is of any use, I have built optimize -march=athlon-xp -mmmx -msse -msse2
-mfpmath=sse -m3dnow -O3 up to that point where it stops and moved the DLLs it
did build to my regular working Firebird directory and things still work and
seem to be faster too.  I haven't noticed any problems from doing that.

Comment 5

[cls]

> 
> These should probably be casts to |WCHAR*| rather than |unsigned short*|. 
> (That's what the examples on MSDN show, despite the documentation for
> SEC_WINNT_AUTH_IDENTITY (not _W, but I don't know what the difference is) uses
> |unsigned short*|.

Perhaps it's another mingw oddity but gcc complains about invalid conversion
from 'WCHAR*' to 'short unsigned int*' when I do that.  The w32api headers use
'unsigned short*' for those fields.

> 
>>Index: uriloader/exthandler/win/nsOSHelperAppService.cpp
>>@@ -41,7 +41,7 @@
>>#define LOG(args) PR_LOG(mLog, PR_LOG_DEBUG, args)
>>
>>// helper methods: forward declarations...
>>-BYTE * GetValueBytes( HKEY hKey, const char *pValueName, DWORD *pLen=0);
>>+static BYTE * GetValueBytes( HKEY hKey, const char *pValueName, DWORD *pLen=0);
>>nsresult GetExtensionFrom4xRegistryInfo(const char * aMimeType, nsCString&
aFileExtension);
>>nsresult GetExtensionFromWindowsMimeDatabase(const char * aMimeType,
nsCString& aFileExtension);
>>
> 
> 
> You might need to remove the later occurrence of |static| in the definition --
> I think that's the portable way of doing it, anyway.

If I remove the latter |static|, wouldn't that expose the function as global?


>>Index: xpfe/bootstrap/nsAppRunner.cpp
>>@@ -1684,7 +1684,7 @@
>>  return TranslateReturnValue(mainResult);
>>}
>>
>>-#if defined( XP_WIN ) && defined( WIN32 )
>>+#if defined( XP_WIN ) && defined( WIN32 ) && !defined(__GNUC__)
>>// We need WinMain in order to not be a console app.  This function is
>>// unused if we are a console application.
>>int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR args, int)
> 
> 
> Hmm?  Does this have anything to do with not being able to run?

No, I think the not being able to run was a side-effect of the __stdcall
problems we were having that were fixed by bug 203137.  With the current
changes, the build runs now.

> 
>>Index: webshell/tests/viewer/nsWinMain.cpp
>>+#ifndef __GNUC__
> 
> 
> Hmm?

Those bits are ifdef'd out because otherwise gcc complains about the call to main():

c:/root/mozilla/webshell/tests/viewer/nsWinMain.cpp:169: error ISO C++ forbids
calling '::main' from within program

c:/root/mozilla/webshell/tests/viewer/nsWinMain.cpp:169: error ISO C++ forbids
taking address of function '::main'

Comment 6

[cls]

Attachment: v1.1

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 131615 [details] [diff] [review]
v1.1

sr=dbaron, except for the nsCOMPtr changes.

Now that I'm looking at the nsCOMPtr changes for a second time, I realize that
that change will cause all the platforms where FACTOR_DESTRUCTOR isn't defined
to have to call the destructor even though it doesn't do anything, which will
slow them down.

What's the error that those changes are fixing?

(And maybe it's better to get the rest of the patch in while we're discussing
that...)

Comment 8

[cls]

> Now that I'm looking at the nsCOMPtr changes for a second time, I realize that
> that change will cause all the platforms where FACTOR_DESTRUCTOR isn't defined
> to have to call the destructor even though it doesn't do anything, which will
> slow them down.

Unless I'm mistaken, that's already the current behavior of that code.  The
patch just gets rid of the inline function.

> What's the error that those changes are fixing?

Unfortunately, I have no idea.  I've modified my system a bit since making the
original patch and I cannot reproduce that error now.  If I run into that
problem again, I'll post the error.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> Unless I'm mistaken, that's already the current behavior of that code.  The
> patch just gets rid of the inline function.

When the function is inline, the call can be optimized away since the compiler
knows it's empty.  When it's no longer inline, it can't be optimized away. 
That's the problem.

Comment 10

[cls]

The patch, minus the nsCOMPtr changes & with the configure.in changes moved to
rules.mk, has been checked in.

Comment 11

[Will Levine]

I just attempted to build with gcc 3.3.1 and I got an error in nsCOMPtr.h . Then
I applied the nscomptr parts of the attached patch and it built fine.  Here's
the error I got the first time:

Building deps for nsXPCOMObsolete.cpp
/cygdrive/c/mozilla/build/cygwin-wrapper g++ -mno-cygwin -o nsXPCOMObsolete.o -c
 -DOSTYPE=\"WINNT5.1\" -DOSARCH=\"WINNT\" -DHAVE_DEPENDENT_LIBS -I/cygdrive/c/mo
zilla/xpcom/obsolete/component/../  -I../../../dist/include/xpcom -I../../../dis
t/include/xpcom_obsolete -I../../../dist/include/string -I../../../dist/include/
libreg -I../../../dist/include/xpcom_compat_c -I../../../dist/include -I../../..
/dist/include/nspr    -I.       -fno-rtti -fno-exceptions -Wall -Wconversion -Wp
ointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -W
no-long-long -pedantic -mms-bitfields -pipe  -DNDEBUG -DTRIMMED -O2   -DX_DISPLA
Y_MISSING=1 -DHAVE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_
WIN32=1 -DHW_THREADS=1 -DWINVER=0x400 -DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1 -
DNO_X11=1 -D_X86_=1 -DD_INO=d_ino -DSTDC_HEADERS=1 -Duid_t=int -Dgid_t=int -DHAV
E_DIRENT_H=1 -DHAVE_GETOPT_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_MALLOC
_H=1 -DHAVE_LIBM=1 -DNO_X11=1 -DMMAP_MISSES_WRITES=1 -DHAVE_STRERROR=1 -DHAVE_SN
PRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DVA_COPY=va_copy -DHAVE_VA_COPY=1 -DMOZ
_DEFAULT_TOOLKIT=\"windows\" -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DMOZ_XPINSTALL=1 -
DMOZ_JSLOADER=1 -DMOZ_MATHML=1 -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\"Mozilla\" -DMOZ_
XUL=1 -DMOZ_PROFILESHARING=1 -DMOZ_PROFILELOCKING=1 -DMOZ_DLL_SUFFIX=\".dll\" -D
JS_THREADSAFE=1 -DNS_PRINT_PREVIEW=1 -DNS_PRINTING=1 -DMOZILLA_VERSION=\"1.6a\"
 -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /cygdrive/c/mozilla/xpcom/obsolete/compon
ent/nsXPCOMObsolete.cpp
In file included from ../../../dist/include/xpcom/nsComponentManagerUtils.h:28,
                 from ../../../dist/include/xpcom/nsIComponentManager.h:158,
                 from ../../../dist/include/xpcom/nsIServiceManagerObsolete.h:52
,
                 from ../../../dist/include/xpcom/nsIServiceManagerUtils.h:42,
                 from ../../../dist/include/xpcom/nsIServiceManager.h:167,
                 from ../../../dist/include/xpcom/nsDirectoryServiceUtils.h:27,
                 from ../../../dist/include/xpcom/nsIFile.h:819,
                 from ../../../dist/include/xpcom/nsILocalFile.h:10,
                 from ../../../dist/include/xpcom_obsolete/nsFileSpec.h:176,
                 from ../../../dist/include/xpcom_obsolete/nsIFileSpec.h:17,
                 from c:/mozilla/xpcom/obsolete/component/nsFileSpecImpl.h:42,
                 from c:/mozilla/xpcom/obsolete/component/nsXPCOMObsolete.cpp:41
:
../../../dist/include/xpcom/nsCOMPtr.h:408: error: function `
   nsCOMPtr_base::~nsCOMPtr_base()' definition is marked dllimport.
make[5]: *** [nsXPCOMObsolete.o] Error 1
make[5]: Leaving directory `/cygdrive/c/mozilla/xpcom/obsolete/component'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/c/mozilla/xpcom/obsolete'
make[3]: *** [tier_2] Error 2
make[3]: Leaving directory `/cygdrive/c/mozilla'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/cygdrive/c/mozilla'
make[1]: *** [build] Error 2
make[1]: Leaving directory `/cygdrive/c/mozilla'
make: *** [build] Error 2

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The patch in bug 220291 should fix the nsCOMPtr issue here as well.