525463 - AIX Port Seamonkey 2.0: macro redefinition in mailnews/base/src/nsMsgAccountManager.cpp

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Uli Link (:ul-mcamafia)]

User-Agent:       Mozilla/5.0 (X11; U; AIX 5.1; en-US; rv:1.8.1.23) Gecko/20091019 SeaMonkey/1.1.18
Build Identifier: Mozilla/5.0 (X11; U; AIX 5.1; en-US; rv:1.8.1.23) Gecko/20091019 SeaMonkey/1.1.18

xlC_r -o nsMsgAccountManager.o -c  -DMOZ_LDAP_XPCOM -DMOZILLA_INTERNAL_API -DMOZ
_SUITE=1 -DOSTYPE=\"AIX5.1\" -DOSARCH=AIX -DHAVE_MOVEMAIL  -I/home/ulink/Src/com
m-1.9.1/mailnews/base/src -I. -I../../../mozilla/dist/include/xpcom -I../../../m
ozilla/dist/include/alerts -I../../../mozilla/dist/include/string -I../../../moz
illa/dist/include/necko -I../../../mozilla/dist/include/dom -I../../../mozilla/d
ist/include/appshell -I../../../mozilla/dist/include/toolkitcomps -I../../../moz
illa/dist/include/appcomps -I../../../mozilla/dist/include/uconv -I../../../mozi
lla/dist/include/intl -I../../../mozilla/dist/include/htmlparser -I../../../mozi
lla/dist/include/widget -I../../../mozilla/dist/include/docshell -I../../../mozi
lla/dist/include/rdf -I../../../mozilla/dist/include/gfx -I../../../mozilla/dist
/include/thebes -I../../../mozilla/dist/include/layout -I../../../mozilla/dist/i
nclude/content -I../../../mozilla/dist/include/mailnews -I../../../mozilla/dist/
include/locale -I../../../mozilla/dist/include/unicharutil -I../../../mozilla/di
st/include/msgbaseutil -I../../../mozilla/dist/include/webshell -I../../../mozil
la/dist/include/txmgr -I../../../mozilla/dist/include/msgcompose -I../../../mozi
lla/dist/include/msgdb -I../../../mozilla/dist/include/uriloader -I../../../mozi
lla/dist/include/pref -I../../../mozilla/dist/include/msglocal -I../../../mozill
a/dist/include/msgimap -I../../../mozilla/dist/include/mork -I../../../mozilla/d
ist/include/msgnews -I../../../mozilla/dist/include/addrbook -I../../../mozilla/
dist/include/mime -I../../../mozilla/dist/include/mimetype -I../../../mozilla/di
st/include/windowwatcher -I../../../mozilla/dist/include/webbrwsr -I../../../moz
illa/dist/include/exthandler -I../../../mozilla/dist/include/xulapp -I../../../m
ozilla/dist/include/caps -I../../../mozilla/dist/include/xpconnect -I../../../mo
zilla/dist/include/js -I../../../mozilla/dist/include/mozldap -I../../../mozilla
/dist/include   -I../../../mozilla/dist/include/msgbase `/home/ulink/Src/AIX-51/
sm2/mozilla/dist/bin/nspr-config --prefix=/home/ulink/Src/AIX-51/sm2/mozilla/dis
t --includedir=/home/ulink/Src/AIX-51/sm2/mozilla/dist/include/nspr --cflags`   
   -qflag=w:w      -DNDEBUG -DTRIMMED -O2 -qmaxmem=16384 -qalias=noansi   -DMOZI
LLA_1_9_1_BRANCH=1 -DMOZILLA_VERSION=\"1.9.1.4\" -DMOZILLA_VERSION_U=1.9.1.4 -DA
IX=1 -DHAVE_SYS_INTTYPES_H=1 -DNSCAP_DISABLE_DEBUG_PTR_TYPES=1 -DD_INO=d_ino -DS
TDC_HEADERS=1 -DHAVE_ST_BLKSIZE=1 -DHAVE_SIGINFO_T=1 -DHAVE_INT16_T=1 -DHAVE_INT
32_T=1 -DHAVE_INT64_T=1 -DHAVE_INT64=1 -DHAVE_UINT=1 -DHAVE_UINT_T=1 -DHAVE_UINT
16_T=1 -DHAVE_DIRENT_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_NL_TYPES_H=1
 -DHAVE_MALLOC_H=1 -DHAVE_X11_XKBLIB_H=1 -DHAVE_SYS_STATVFS_H=1 -DHAVE_SYS_STATF
S_H=1 -DHAVE_LIBC_R=1 -DHAVE_LIBM=1 -DHAVE_LIBDL=1 -DHAVE_LIBC_R=1 -DFUNCPROTO=1
5 -DHAVE_XSHM=1 -D_REENTRANT=1 -DHAVE_RANDOM=1 -DHAVE_STRERROR=1 -DHAVE_LCHOWN=1
 -DHAVE_FCHMOD=1 -DHAVE_SNPRINTF=1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DHAVE_STAT64=
1 -DHAVE_LSTAT64=1 -DHAVE_TRUNCATE64=1 -DHAVE_FLOCKFILE=1 -DHAVE_LOCALTIME_R=1 -
DHAVE_STRTOK_R=1 -DHAVE_RES_NINIT=1 -DHAVE_LANGINFO_CODESET=1 -DHAVE_I18N_LC_MES
SAGES=1 -DMOZ_SUITE=1 -DMOZ_BUILD_APP=suite -DMOZ_X11=1 -DMOZ_WIDGET_GTK2=1 -DMO
Z_DISTRIBUTION_ID=\"org.mozilla\" -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DMOZ_
XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_NO_XPCOM
_OBSOLETE=1 -DMOZ_XTF=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_PLACES=1 -DMOZ_FEEDS=
1 -DMOZ_STORAGE=1 -DMOZ_HELP_VIEWER=1 -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\".mozilla\
" -DMOZ_STATIC_MAIL_BUILD=1 -DHAVE_INTTYPES_H=1 -DMOZ_XUL=1 -DMOZ_PROFILELOCKING
=1 -DMOZ_RDF=1 -DMOZ_MORK=1 -DMOZ_MORKREADER=1 -DMOZ_DLL_SUFFIX=\".so\" -DXP_UNI
X=1 -DUNIX_ASYNC_DNS=1  -D_COMM_CONFIG_H_ -DMOZILLA_CLIENT /home/ulink/Src/comm-
1.9.1/mailnews/base/src/nsMsgAccountManager.cpp
"../../../mozilla/dist/include/xpcom/nsCRT.h", line 295.9: 1540-0848 (S) The mac
ro name "FILE_ILLEGAL_CHARACTERS" is already defined with a different definition
.
"../../../mozilla/dist/include/msgbase/msgCore.h", line 231.11: 1540-0425 (I) "F
ILE_ILLEGAL_CHARACTERS" is defined on line 231 of "../../../mozilla/dist/include
/msgbase/msgCore.h".
gmake[4]: *** [nsMsgAccountManager.o] Error 1
gmake[4]: Leaving directory `/home/ulink/Src/AIX-51/sm2/mailnews/base/src'
gmake[3]: *** [src_libs] Error 2
gmake[3]: Leaving directory `/home/ulink/Src/AIX-51/sm2/mailnews/base'
gmake[2]: *** [libs_tier_app] Error 2
gmake[2]: Leaving directory `/home/ulink/Src/AIX-51/sm2'
gmake[1]: *** [tier_app] Error 2
gmake[1]: Leaving directory `/home/ulink/Src/AIX-51/sm2'
gmake: *** [default] Error 2


Reproducible: Always

Steps to Reproduce:
1. Compile SEAMONKEY 2.0 on AIX using IBM compiler

Actual Results:  
Build broken

Expected Results:  
Successfull compilation

Comment 1

[Uli Link (:ul-mcamafia)]

The macros FILE_ILLEGAL_CHARACTERS and FILE_PATH_SEPARATOR are defined in mozilla/xpcom/ds/nsCRT.h

the local header msgCore.h is included first, so the the macros aren't defined for the preprocessor yet, so clashing later.

Solution 1: #include "nsCRT.h" after #include "nsCRTGlue.h" in msgCore.h and remove the redundant macro definitions.
nsCRT.h will be added in all units using msgCore.h then

Solution 2: move the #include "msgCore.h" in nsMsgAccountManager.cpp to the end of the header includes so that the #ifndef FILE_PATH_SEPARATOR will do what it is intended for. Affects only the units nsMsgAccountManager.cpp and nsMsgFilterService.cpp

Comment 2

[Uli Link (:ul-mcamafia)]

Also with Thunderbird 3.0 from release tarball

Comment 3

[Uli Link (:ul-mcamafia)]

Attachment: Avoids macro redifinition

This resolves compilation error on AIX and should not alter the semantics at all.

Comment 4

[David :Bienvenu]

Comment on attachment 419819 [details] [diff] [review]
Avoids macro redifinition

If you include "nsCRT.h" as you have, will the code inside the #ifndef's ever be compiled? I guess we're preparing for building with frozen linkages...

nsCRT.h uses the define  OS_FILE_ILLEGAL_CHARACTERS,in the way we're using FILE_ILLEGAL_CHARACTERS, which means we may have been  allowing control characters in file names prior to your patch, possibly, but won't with your patch - that seems like a semantic change, though probably in a good way.

I'm tempted to suggest that we just include "nsCRT.h" and remove the override #defines, since they're a bit broken, as happens when you start cloning code.

Comment 5

[David :Bienvenu]

cc'ing Prasad in case he has more insight into this...

Comment 6

[Prasad Sunkari [:prasad]]

(In reply to comment #4)
> (From update of attachment 419819 [details] [diff] [review])
> If you include "nsCRT.h" as you have, will the code inside the #ifndef's ever
> be compiled? I guess we're preparing for building with frozen linkages...

As we prepare for the frozen linkages, we may want to avoid including nsCRT.h

> 
> nsCRT.h uses the define  OS_FILE_ILLEGAL_CHARACTERS,in the way we're using
> FILE_ILLEGAL_CHARACTERS, which means we may have been  allowing control
> characters in file names prior to your patch, possibly, but won't with your
> patch - that seems like a semantic change, though probably in a good way.
> 
> I'm tempted to suggest that we just include "nsCRT.h" and remove the override
> #defines, since they're a bit broken, as happens when you start cloning code.

Since "nsCRT.h" anyway includes "nsCRTGlue.h", we should probably consider moving these declarations to "nsCRTGlue.h".  That way users of either headers would be able to use these declarations.

Comment 7

[Uli Link (:ul-mcamafia)]

(In reply to comment #6)
> 
> Since "nsCRT.h" anyway includes "nsCRTGlue.h", we should probably consider
> moving these declarations to "nsCRTGlue.h".  That way users of either headers
> would be able to use these declarations.

I suggest creating a new bug in XPCOM for moving these declarations from nsCRT.h to nsCRTGlue.h and this bug blocked by the new bug.
When the new bug is fixed or in advance, I will rework the patch to 
 - not include nsCRT.h (as without my patch)
 - remove the duplicated code

Sometimes it's good to use a very strict compiler, which stops at redefinitions.
But I cannot build/test on trunk, because trunk does not like the IBM compiler ;-)
I'm happy to build 1.9.1 branch now and can surely provide the branch patches.
And it's time to get 1.9.2 branch build on AIX.

Comment 8

[Uli Link (:ul-mcamafia)]

For not forgetting:
https://bugzilla.mozilla.org/show_bug.cgi?id=471760#c1

Comment 9

[David :Bienvenu]

Comment on attachment 419819 [details] [diff] [review]
Avoids macro redifinition

minusing based on new plan...

Comment 10

[Uli Link (:ul-mcamafia)]

Attachment: avoids macro redefinition w/o including nsCRT.h

Comment 11

[David :Bienvenu]

Does this really help? msgCore.h is included before nsCRT.h, and nsCRT.h doesn't have similar #ifdef protection, so I still get a bunch of warnings about redefining the illegal chars.

Comment 12

[Uli Link (:ul-mcamafia)]

https://bugzilla.mozilla.org/attachment.cgi?id=425726 needs the patch from 
https://bugzilla.mozilla.org/show_bug.cgi?id=537967

Two bugs because spanning different products/modules.

Comment 13

[[e-mail]]

Does the compiler complain if the redeclaration is for the identical string? We could easily update the macro to match nsCRT.h for now.

Comment 14

[Uli Link (:ul-mcamafia)]

No macro redefinition possible without #undef before.
Even with exactly same strings.

Comment 15

[[e-mail]]

I guess we should now look into removing the definitions from msgCore.h (at least for those branches on which bug 537967 lands).

Comment 16

[Uli Link (:ul-mcamafia)]

(In reply to comment #15)
> I guess we should now look into removing the definitions from msgCore.h (at
> least for those branches on which bug 537967 lands).

It has landed on m-c and I will request branch approval also for 1.9.1 and since TB 3.1 will use 1.9.2 also there.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> Does this really help?

I wouldn't think so.

(In reply to comment #15)
> I guess we should now look into removing the definitions from msgCore.h (at
> least for those branches on which bug 537967 lands).

Indeed. In fact, this bug is a duplicate of bug 467015, isn't it?

Uli, could you fix it that way, either here or in that older bug?

Comment 18

[Uli Link (:ul-mcamafia)]

I suggest removing all the macros defined in nsCRTGlue.h from msgCore.h,
will create a new patch later.

Comment 19

[Uli Link (:ul-mcamafia)]

Attachment: removing duplicate definitions from nsCRTGlue.h [Checkin: Comment 24]

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 430996 [details] [diff] [review]
removing duplicate definitions from nsCRTGlue.h
[Checkin: Comment 24]


Yes, that should be what we want :-)

Comment 21

[Uli Link (:ul-mcamafia)]

The nsCRTGlue.h patch has landed on all needed branches.

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/2e523d1e398b
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/03ebb7b90df8

Comment 22

[[e-mail]]

Comment on attachment 430996 [details] [diff] [review]
removing duplicate definitions from nsCRTGlue.h
[Checkin: Comment 24]

I assume you want this on the comm-1.9.1 branch too.

Comment 23

[Uli Link (:ul-mcamafia)]

(In reply to comment #22)
> (From update of attachment 430996 [details] [diff] [review])
> I assume you want this on the comm-1.9.1 branch too.

Yes plz and thx :-)

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 430996 [details] [diff] [review]
removing duplicate definitions from nsCRTGlue.h
[Checkin: Comment 24]


http://hg.mozilla.org/comm-central/rev/bbb5625de160

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 430996 [details] [diff] [review]
removing duplicate definitions from nsCRTGlue.h
[Checkin: Comment 24]

a=Standard8.

However, please leave the landing to me as I need to co-ordinate picking up latest gecko on our TB 3.0 tinderboxes (some of which are currently tracking the relbranch).

Comment 26

[Mark Banner (:standard8)]

Checked into 1.9.1: http://hg.mozilla.org/releases/comm-1.9.1/rev/05a3f1a48963

Comment 27

[Mark Banner (:standard8)]

Verified fixed on 3.0 as it hasn't broken the builds, also visually verified what landed in m-1.9.1 was the same as what was removed from comm-1.9.1