54564 - under gcc, make L"..." do the right thing for us

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, defect, P3)

Description

[Scott Collins]

It turns out that if we use a recent enough version of the compiler, we can use 
the option |-fshort-wchar| on the command line.  This would be good, because it 
would give us the no-copying/no-conversion implementation of |NS_LITERAL_STRING|.  
We should write an autoconf test for this option, and another autoconf test 
(perhaps used only in the absence of this option?) to set |HAVE_CPP_2BYTE_WCHART|
.  Doug Turner expressed interest in this; so I'm assigning it to him initially.  
If you're not up for this, Doug, just assign it right back to me and I'll do it.  
Chris Seawood, Chris Waterson, or I (in decreasing order of expertise) can teach 
you about autoconf tests if you don't already have the knowledge you need.

Comment 1

[Scott Collins]

Thanks to shaver for discovering this switch, by the way.

Comment 2

[Scott Collins]

Oops.  Sorry, I misspelled the exact name of the important |#define|.  It should
read |HAVE_CPP_2BYTE_WCHAR_T|.

Comment 3

[Doug Turner (:dougt)]

Attachment: checks for wchar len and short-wchar compiler option

Comment 4

[Doug Turner (:dougt)]

Tested on linux with gcc 2.95.2.  Have not tested this with a gcc that supports
the -fshort-wchar option.  

Comment 5

[Doug Turner (:dougt)]

Attachment: patch 2 - it actually now works when gcc support short-wchar.

Comment 6

[Scott Collins]

I think there's a typo in your |AC_TRY_RUN| hunk, you'll need to say

  if (sizeof(wchar_t)...
                  ^^

Comment 7

[Scott Collins]

I'll also test this with new and old gcc under Yellow Dog Linux on my Mac

Comment 8

[Doug Turner (:dougt)]

I have check a patch in under the flag: MOZ_ENABLE_SHORTWCHAR.

I will remove this flag and make the test default once I figure out why a build
compiled with this option segfaults.

Comment 9

[Doug Turner (:dougt)]

I am thinking that this option is not ready yet.

when HAVE_CPP_2BYTE_WCHAR_T is enabled, this crashes:

int main(int argc, char** argv)
{
  NS_ConvertASCIItoUCS2 string1("String One");            
  NS_ConvertASCIItoUCS2 string2("String Two");
  string2.AssignWithConversion("- AssignedWithConversion");
  string1.Append(string2);
  return 0;
}

I can't get a stack trace.

Comment 10

[Doug Turner (:dougt)]

Chris, assigning this bug to you.  The mozilla build scripts does support this 
feature, but the compiler is generating some bad code.  I have commented out 
this check in configure for now.  If you have someone work on gcc stuff, please 
consider this work as well.

Comment 11

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

Why does the autoconf test test for sizeof(wchar) rather than sizeof(wchar_t)?
 Also, would it be better to do the test with the option as well, to make sure
it works?

Comment 12

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

Er... ignore the first comment.  I see it's fixen in the checked-in version.

Comment 13

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

The current code in configure.in only adds -fshort-wchar to CFLAGS, but not
CXXFLAGS, so only C files are compiled with the option.  However, the code
assumes wchar_t is 2-byte because of the #define.

Comment 14

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

Attachment: configure.in patch I tried

Comment 15

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

Actually, it seems to work.  I forgot to clobber at one point, so I thought it
didn't...

Comment 16

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

Attachment: same patch, but with the MOZ_ENABLE_SHORTWCHAR check removed

Comment 17

[Chris Waterson]

I think for `-fshort-wchar' it is ok to assume that cc & c++ will accept the
same arguments. It's gcc specific, anyway! r=waterson

Comment 18

[Chris Waterson]

I'd like to experiment with this re: generated binary size. What version of gcc
supports `-fshort-wchar'?

Comment 19

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

I'm not sure what versions support -fshort-wchar, but it works on my gcc "2.96"
(the RH7 version).

For which patch was the r=?

Comment 20

[Chris Waterson]

r= for the most recent patch, `12/17/00 18:19 same patch, but with the
MOZ_ENABLE_SORTWCHAR check removed'. FWIW, the gcc trunk does *not* support
`-fshort-wchar'. (But, we can't compile with that compiler yet for other reasons.)

Comment 21

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

What did the config.log say for the trunk gcc?  That -fshort-wchar wasn't
supported, or some other error?

Comment 22

[Chris Waterson]

$ gcc -fshort-wchar foo.c
cc1: Invalid option `-fshort-wchar'

Comment 23

[Chris Waterson]

Hrm. Doing `find . -type f | xargs grep -n short-wchar' turned up nothing on
gcc-2_95_2_1-branch, either (there are no tags in the gcc repository related to
gcc-2.96 AFAICT). I am beginning to suspect this was included as some special
one-off patches in the RH7.0 distro.

dbaron, could you install the gcc-2.96 SRPM and grep through the patches they've
applied for `short-wchar'? (For some reason, my version of `rpm' will not allow
me to install any RH7.0 [S]RPMs. Including the RH7.0 `rpm' RPM.)

Comment 24

[Brendan Eich [:brendan]]

Cc'ing blizzard; we need that gcc option!

/be

Comment 25

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

In the gcc RPM from redhat, it's part of the main tar file and not the patches
(based on `grep short-wchar *.patch`).  The entry in the change log describing
it is listed as:

---
Thu Sep 23 13:40:02 1999  Jason Merrill  <[e-mail]>

        * toplev.c (documented_lang_options): Add -fshort-wchar.
        * c-decl.c (c_decode_option): Likewise.
        (init_decl_processing): If -fshort-wchar, use 'short unsigned int'
        for wchar_t.
        * c-common.c, c-lex.c: Get WCHAR_TYPE_SIZE from wchar_type_node.
        * gcc.c (default_compilers): If -fshort-wchar,
        override __WCHAR_TYPE__.
        * tm.texi (C Dialect Options): Add -fshort-wchar.
---

Comment 26

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

waterson:  For RPM, see
http://www.redhat.com/support/errata/RHEA-2000-051-01.html

Comment 27

[Chris Waterson]

I must be smokin' something. The `-fshort-wchar' stuff is indeed in the gcc 
trunk.

Comment 28

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

However, -fshort-wchar doesn't seem to be supported in the gcc versions included
in RedHat 6.x distributions.  I thought (based on some comment...) that it might
have been included as an additional patch in those RPMs before it was in the gcc
distribution, but that doesn't seem to be the case.  I tried the gcc's on real
redhat 6.0 and 6.1 machines, and neither supported -fshort-wchar.  I then
upgraded the egcs, egcs-c++, libstdc++, and cpp packages on the 6.0 machine to
the 6.2 versions, and -fshort-wchar still didn't work.

Comment 29

[Brendan Eich [:brendan]]

Looks good to me, but cls has final say.  I'm not the default sr=-giver for
configure.in changes.

/be

Comment 30

[cls]

Sorry about the delay. sr=cls on attach 20852

Comment 31

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

jag reports that -fshort-wchar does not work on gcc 2.95.2.  So it seems to
require 2.96+.  2.95.2 must have been off a branch...

Comment 32

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

Based (mostly) on looking at tinderbox logs after checking in the fix this
morning, I have found that:

sizeof(wchar_t) == 4
====================
gcc 2.95.2 (monkeypox)
gcc 2.91.66 (numerous tinderbox machines)
gcc 2.8.1 (maleficia)
gcc 2.7.2.3 (speedracer)
HPUX B.11.00 (genoa)
IRIX 6.x (cement, mason)
SunWS 5.0 (nebiros)
DEC C V5.9-011 on Digital UNIX V4.0 (Rev. 1229) (ice.fas.harvard.edu)

sizeof(wchar_t) == 2
====================
VACPP (OS/2)
gcc 2.9 on BeOS

supports -fshort-wchar
======================
gcc "2.96" from RH7 (my own machine)

I filed http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=22786 on the
warnings given by gcc with -fshort-wchar.  I haven't tested to see if it's still
a problem in the gcc trunk, so I haven't filed a gcc bug.

So, marking as fixed, although we still have the issue that the compiler we're
using for releases doesn't support -fshort-wchar.  Hopefully gcc3 will be out
soon, and will be good...

Comment 33

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

See bug 63857 for an interesting problem this change caused.

Comment 34

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

Additional information from tinderbox logs:

sizeof(wchar_t) == 2
====================
AIX xlC

Comment 35

[Jon Granrose]

verified.