177150 - Some mozilla-config.h defines require additional cflags for embeddors

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: Packaging, defect)

Description

[Tommi Komulainen]

When using gcc3 to build mozilla, the configure script generates
mozilla-config.h which includes '#define HAVE_CPP_2BYTE_WCHAR_T 1'.  This seems
to affect how mozilla interprets strings.

Now, for the wchar_t to be 2 bytes wide, gcc3 seems to require that the
-fshort-wchar parameter is provided.  Without the flag the width is 4 bytes. 
The configure script in mozilla passes the parameter to the compiler and thus
mozilla works.

However, when embedding mozilla, and including the mozilla-config.h file, the
define gets set, but the required compiler parameter isn't used, because there
is no way of knowing it is required.  As a result, the strings in the embedding
application consists of 4 bytes wide characters, but when passed to mozilla it
interprets them as if they were only 2 bytes wide.  It's subtle enough as there
is no danger of crashing, but string comparisons begin to fail.

Adding the -fshort-wchar flag, if required, in mozilla-nspr.pc should solve the
problem at least for those using pkg-config.

Comment 1

[hacker formerly known as [e-mail]]

We definitely have too many ways for embeddors to get flags to build against us.
 I don't see how this is a problem with mozilla-config.h specifically as you
can't pass build flags via a header.  You must use mozilla-config.h in
conjunction with either the mozilla-config script or the pkgconfig scripts...not
that either one passes in ABI changing cflags right now.

Comment 2

[Chris Nalls]

Marking topembed- as per topembed triage. It would be good to do this work, but
not essential.

Comment 3

[Braden]

I don't think a pkgconfig metafile should assume gcc. IOW, I don't think it
should be adding any non-POSIX options.

Comment 4

[hacker formerly known as [e-mail]]

Mass reassign to new default build assignee

Comment 5

[(default assignee for unassigned bugs. does not receive bugmail)]

Mass reassign of Build/Config bugs to Leaf.

Comment 6

[Chase Phillips]

Mass reassign of open bugs for [e-mail] to [e-mail].

Comment 7

[cls]

*** Bug 323340 has been marked as a duplicate of this bug. ***

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Wow, this is the first time I've ever heard of this mozilla-config script... How are you supposed to use it?

Is this something that's really only important for embedding (as opposed to XULRunner apps)? This script isn't built in my xulrunner tree, it doesn't look like...

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

I guess these days you should use pkg-config instead...

Comment 10

[:Mook]

Attachment: Compile-time check for sizeof(wchar_t)

If HAVE_CPP_2BYTE_WCHAR_T is defined, define a dummy struct with a bitfield that is invalid when sizeof(wchar_t) is not two.  (No, this does not solve the bug, but it does help when dumb people like me trip over it when using the Gecko SDK)

Tested on trunk, SUSE's gcc-4.10 and MSVC 7.1.  The struct needs a name; an anonymous struct definition that doesn't declare a variable kills GCC. ("abstract declarator ‘<anonymous struct>’ used as declaration")

I do wish there were better ways to make this check...

Comment 11

[Benjamin Smedberg]

Comment on attachment 247758 [details] [diff] [review]
Compile-time check for sizeof(wchar_t)

dbaron should look this over as well

Comment 12

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

Comment on attachment 247758 [details] [diff] [review]
Compile-time check for sizeof(wchar_t)

The concept looks fine, but you can simplify it to just typedef-ing an int array like the JS_STATIC_ASSERT macro from http://lxr.mozilla.org/seamonkey/source/js/src/jsutil.h#70 

Somebody should also really copy this macro into either nscore.h or nsDebug.h as NS_STATIC_ASSERTION, but it's still not ideal for use in header files because of namespace pollution.  (I have a slight preference for nsDebug.h over nscore.h, but only very slight.  Not sure what bsmedberg thinks.)

Comment 13

[:Mook]

Attachment: Use PR_STATIC_ASSERT

... Not sure if #including prerror.h is okay due to the namespace pollution.  If preferred, I can just typedef an int array like comment 16 said to.

Sorry for forgetting about this... :(

Comment 14

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

Comment on attachment 260921 [details] [diff] [review]
Use PR_STATIC_ASSERT

sr=dbaron  If you're there, you may also want to assert that PRUnichar(-1) > PRUnichar(0) (outside the ifdefs).

PR_STATIC_ASSERT doesn't affect binary size, right?  Would you mind checking?  Also, should this assert also go in the equivalent Mozilla-internal file?

Comment 15

[:Mook]

Attachment: PR_STATIC_ASSERT, reflecting comment 14

- Changed to use prlog.h instead (see bug 375985 comment 14 )
- Added assertion that PRUnichar is unsigned
- PR_STATIC_ASSERT should have no effect on codesize (see bug 381236 comment 17 )

Carrying forward dbaron's sr+, checking if bsmedberg wants to look at the changes from that again.  If I'm being too careful, tell me :)

For internal strings, checking that things make sense should probably in configure instead (that's what it's there for, right?).  Which, I believe, is why we have HAVE_CPP_2BYTE_WCHAR_T to begin with...

Comment 16

[Kenneth Herron]

"PR_STATIC_ASSERT, reflecting comment 14" patch checked in. Clearing checkin-needed status.