559278 - wchar_t/char16_t problems when building against a SDK that was not built with the same flags

Status: RESOLVED WORKSFORME

(Toolkit Graveyard :: XULRunner, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Patch

(Not sure the component is the right one)

When building a SDK, there are some checks being done that influence how the build is done regarding compiler flags. One of these is the test for char16_t, followed by the test for wchar_t, then -fshort-wchar.
When the build occurs, one of these is picked, and depending on which one it is, the HAVE_CPP_2BYTE_WCHAR_T and HAVE_CPP_CHAR16_T variables are set or not, which then are put in xpcom-config.h.
This file is part of the SDK, and is included when building against the SDK.

When building third party applications using the SDK, the build flags are not necessarily corresponding to what was being used when building the SDK. The version of gcc could also be different and as such have different defaults and/or support for things (for example, char16_t is new to gcc 4.4).

In such cases, the build fails because what is harcoded in xpcom-config.h doesn't correspond to the current build environment.

Some additional checks at build time would be nice, to allow to fallback to what is supported at build time.

The attached patch only takes care of gcc, and does the following:
- if building with gcc < 4.4 or with gcc 4.4 without c++0x support, and HAVE_CPP_CHAR16_T is set, unset it.
- if building with gcc >= 4.4 with c++0x support, and HAVE_CPP_CHAR16_T is unset, set it.
- if building with gcc without -fshort-wchar, and HAVE_CPP_2BYTE_WCHAR_T is set, unset it.
- if building with gcc with -fshort-wchar, and HAVE_CPP_2BYTE_WCHAR_T is unset, set it.
All these cases issue a warning about the difference between the build environment and the SDK.

Finally, as the fallback for literal strings using conversions is not recommended, I also added a warning to indicate that it is recommended to use a 2-byte wchar_t or char16_t.

Improvements to the warning wordings are welcome.

Comment 1

[Mike Hommey [:glandium]]

Comment on attachment 438956 [details] [diff] [review]
Patch

Requestion review to both Darin and Benjamin as it is somehow across xpcom strings and xpcom base

Comment 2

[Mike Hommey [:glandium]]

I'm wondering if the warnings wouldn't be better in nsStringAPI.h and nsLiteralString.h, actually, so that only the builds actually requiring wchar_t or char16_t get them...

Comment 3

[Benjamin Smedberg]

Comment on attachment 438956 [details] [diff] [review]
Patch

I think we want a PR_STATIC_ASSERT here.

Comment 4

[Mike Hommey [:glandium]]

here where ?

Comment 5

[Benjamin Smedberg]

In nscore.h, where you currently use the #warning, we should just #error. I think at least until we have u"" support we should just require that you use the same -fshort-wchar option as the SDK.

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> In nscore.h, where you currently use the #warning, we should just #error. I
> think at least until we have u"" support we should just require that you use
> the same -fshort-wchar option as the SDK.

Third party applications are unfortunately not ready for this, starting with m-c's configure script. It won't try to get the same flags as the SDK was built with. Applications such as galeon or kazehakase will do a check for -fshort-wchar on their own, and use it when it works.

On the other hand, except on win32, wchar_t itself is not used, and only the L"" construct is used, which is just safe to use even when the SDK was built with char16_t, and the other way round, u"" is safe to use even when the SDK was built with L"".

So I do think warning is good enough, though I'm unsure, as I said in comment 2, that code that don't use nsStringAPI.h or nsLiteralString.h should be bothered with these warnings.

Comment 7

[Benjamin Smedberg]

Don't we include -fshort-wchar in the .pc file for the SDK? In any case, I was thinking the check should be:

#ifdef HAVE_CPP_CHAR16_T
PR_STATIC_ASSERT(sizeof(L' ') == 16);
#endif

Which would force the third-party apps to use -fshort-wchar for now.

Comment 8

[Mike Hommey [:glandium]]

(In reply to comment #7)
> Don't we include -fshort-wchar in the .pc file for the SDK? In any case, I was
> thinking the check should be:
> 
> #ifdef HAVE_CPP_CHAR16_T
> PR_STATIC_ASSERT(sizeof(L' ') == 16);
> #endif
> 
> Which would force the third-party apps to use -fshort-wchar for now.

I actually want to switch off -fshort-wchar for several reasons, one of which being that it is dangerous with third party applications (IIRC, we had problems with an application linking to a library that *does* use wchar_t as part of its API, and as such the result was the impossibility to link both to that library (which wasn't build with -fshort-wchar) and libxul), and another one being that it doesn't work on ARM. (It has actually been off on debian builds for 2 years already). OTOH, I want to use -std=gnu++0x when possible, and that isn't exposed in the .pc file, and I'm not convinced it should.

Comment 9

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

Comment on attachment 438956 [details] [diff] [review]
Patch

I'm inclined to agree with Benjamin that we want a PR_STATIC_ASSERT here.  That probably depends on either (a) getting the definition of PR_STATIC_ASSERT moved into a sensible file (i.e., updating to an NSPR with the fix for bug 551782) or (b) just expanding it manually for these one or two cases.

The #ifdefs you're adding to nscore.h also don't seem particularly future-proof; at some point char16_t won't be experimental anymore.

I'd be OK with ifdefs that check for whether __SIZEOF_WCHAR_T_ or an equivalent for whether char16_t is defined, and adjust the relevant macros with #warnings.

Comment 10

[Mike Hommey [:glandium]]

(In reply to comment #9)
> The #ifdefs you're adding to nscore.h also don't seem particularly
> future-proof; at some point char16_t won't be experimental anymore.

I'd love for them to be future-proof, but afaict the macro name for when it's not experimental anymore hasn't been defined.
 
> I'd be OK with ifdefs that check for whether __SIZEOF_WCHAR_T_ or an equivalent
> for whether char16_t is defined, and adjust the relevant macros with #warnings.

There is unfortunately not such a macro for char16_t. Or to be more precise, there is one:
#define __CHAR16_TYPE__ short unsigned int
... but it's always defined.

Anyways, I'm not sure nscore.h is the right location for that anymore, especially considering some C files in the mozilla codebase (indirectly) include it, and as such won't use the -std=gnu++0x flag (pldhash.c and gtk2xtbin.c). OTOH, nsStringAPI and nsLiteralString use C++ constructs, so it would probably be safer to move the checks there.

I'm still not convinced erroring out is a good thing to do for the char16_t/wchar_t mismatch, as it really doesn't matter for NS_LITERAL_STRINGs.

BUT, when the SDK was built with either of those, maybe the ascii conversion fallback could be considered an error.

Anyways, I have applied the current patch to the latest Debian package (though i do agree it's not a final form) and could gather data about third party (free/open source) software. I will try to push changes to these.

Comment 11

[Mike Hommey [:glandium]]

This is obsoleted by bug 904985.