206811 - xp_iconv in nsNativeCharsetutils.cpp should use UTF-16 if available

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jungshik Shin]

xp_iconv in nsNativeCharsetUtils.cpp uses UCS-2, but *PRUnichar is not a string
of UCS2's but UTF-16 string. [1] Therefore, we should try UTF-16 before trying
UCS-2. 
At least, glibc, Solaris, HP/UX 10/11 and Tru64 [2] use 'UTF-16' for the native
endian UTF-16. I haven't yet figured out what AIX and IRIX do about it. [3]

[1] 
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsNativeCharsetUtils.cpp#177
[2]
http://docs.sun.com/db/doc/806-5584/6jej8rb0u?a=view
http://h30097.www3.hp.com/docs/porting/tru64-to-hpux/CHP18NXX.HTM

[3]http://publibn.boulder.ibm.com/doc_link/en_US/a_doc_lib/aixbman/admnconc/convert.htm

Comment 1

[Jungshik Shin]

Attachment: a patch

Comment 2

[Darin Fisher]

Comment on attachment 124018 [details] [diff] [review]
a patch

>+static const char *UTF_16_NAMES[] = {
>+    "UTF-16",     
>     "UCS-2",
>     "UCS2",
>     "UCS_2",

this patch makes good sense to me, but do we need to worry about
other variants of UTF-16 .. such as UTF16, UTF_16, utf-16, utf16, 
and utf_16?  i know it almost seems rediculous, but i can't really
say for sure.

also, how about replacing all instances of "ucs2" with "utf16"
e.g., ucs2_to_isolatin1 -> utf16_to_isolatin1

Comment 3

[Jungshik Shin]

As for replacing all instances of "ucs2" with "utf16", I'll do.

As for UTF-16 variants, I'm not sure. It seems like every platform that supports
UTF-16 uses "UTF-16". However, I guess it doesn't hurt to try other variants
because it's done only once at the init, right? 

Comment 4

[Jungshik Shin]

darin, 
Can you also comment on my posting 
'news://news.mozilla.org:119/bajml3$[e-mail]'? I thought I was
ringing a false alram, but I just realized that Linux does not use iconv(3) but
uses wcrtomb and mbrtowc assuming that wchar_t is UTF-16 string, of which I'm
not sure.

Comment 5

[Darin Fisher]

actually, i'm not sure why we'd want to ever prefer wcrtomb/mbrtowc over iconv
on linux.  i know that under OSX, nl_langinfo does not seem to exist, but the
nsNativeCharsetUtils methods aren't invoked until OSX IIRC.

Comment 6

[Ulrich Drepper]

On Linux it is *never* true that wchar_t == UCS2 or UTF-16.  With the
-fshort-wchar option one can make gcc define wchar_t as a two-byte value but
this doesn't magically the runtime.  In fact, when using this option and the
normal runtime for wide character handling you guarantee certain death for your
program.

Everybody who uses this option must be aware of this.  And s/he must be aware of
the fact that the so created fake wchar_t type violates ISO C.  UTF-16 is no
valid encoding for wchar_t and UCS2 is obviously insufficient.

If somebody insists on using -fshort-wchar s/he must make sure to never use any
of the wide character interfaces in the runtime.  It is possible to use iconv()
but even then the pseudo encoding name "WCHAR_T" must not be used.  It is then
necessary to explicitly use "UTF-16" or so.

Comment 7

[Jungshik Shin]

Attachment: a new patch (including switching to iconv under Linux)

Darin and I decided to switch over to iconv for Linux after discussing the
issue off-line (newsgroups/mail) as well as here with Ulrich and Bruno.

Comment 8

[Jungshik Shin]

Before leaving for the vacation, Darin wrote to me the following:
<quote>
jshin: i am going to be away on vacation for the next 2 weeks, in which
case i most likely will not get a chance to make this change in time for
1.4 final.  if you think we should try to make this change for 1.4 final
(sounds to me like we should), then you ought to get in touch with
[e-mail] to discuss it with them.  be sure to run some
performance tests (start up time is probably going to be most sensitive)
so they can be aware of all the implications of this change.  thanks!
</quote>

I agree with him that we should fix this both in trunk and 1.4f branch because
1.4f branch is likely to be a long-lived one. I measured the start-up time with
and without the patch (attachment 124092 [details] [diff] [review]) and I found that the one with the
patch takes slightly shorter than otherwise. (0.095 vs 0.097 on 10-time average.
the std. deviation is virtually zero unless I wait long enough between runs).

Comment 9

[Alec Flett]

Comment on attachment 124092 [details] [diff] [review]
a new patch (including switching to iconv under Linux)

looks good. sr=alecf

Comment 10

[Jungshik Shin]

Setting the target to 1.4final. Frank, can you review? 

Comment 11

[Jungshik Shin]

Comment on attachment 124092 [details] [diff] [review]
a new patch (including switching to iconv under Linux)

asking smontagu for r.

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 124092 [details] [diff] [review]
a new patch (including switching to iconv under Linux)

I can't say I know enough about iconv to review this properly, but since (from
comment 7) it looks like people who do know have signed off on the approach, I
am happy to give r=smontagu

Comment 13

[Benjamin Smedberg]

This appears to have increased codesize by 3.4k on various linux platforms. Was
this expected; are we statically linking the iconv libraries?

Comment 14

[Jungshik Shin]

> This appears to have increased codesize by 3.4k on various linux platforms.

Thank you for alerting me about the codesize I missed. 

I realize that USE_ICONV has more code than USE_STDCONV. 

 We can cut down the increase by half by NOT defining
ENABLE_UTF8_FALLBACK_SUPPORT. For sure, iconv(3) in glibc doesn't need that
which is for iconv(3) on Solaris. On Solaris, iconv(3) has to go through UTF-8
for most combinations of to/from codesets. 

Removing 'inline' from xp_iconv_reset helped me very little (100bytes) while
removing 'inline' from xp_iconv_open and xp_iconv didn't change the size at all
(they are too complex to be inlined with gcc 3.x with -O1). 

I can squeeze out additional ~300 bytes removing unnecessary array elements. The
final numbers are:

 - old       : 898056 bytes
 - current   : 902504 bytes   : +4448
 - w/ patch  : 899528 bytes   : +1472

Comment 15

[Jungshik Shin]

Attachment: a patch to reduce the size

This patch doesn't look pretty because it's littered with '#if
!defined(__linux)'. Anyway, with this patch, I cut down the code size increase
(libxpcom.so on ix86) to 1.2kB.

Comment 16

[Jungshik Shin]

I forgot to mention that I removed 'inline' in xp_iconv_reset, which saved me
only about 190 bytes. Because I removed the function call overhead on Linux in
other places (plain iconv_open() is used in place of xp_iconv_open), I don't
think there's any slow down in the startup. If it's a worry, perhaps we can just
put back inline for 190 byte size increase (on ix86, gcc 3.x, -O1...) 

BTW, I assumed that Linux binary is always compiled AND run with glibc 2.x or
GNU libiconv, which is very  unlikely to break and which enabled me to use
'UTF-16' and 'ISO-8859-1' directly in iconv_open(). Benjamin, I guess you're
concerned with embedded devices. Is my assumption valid there? 

Comment 17

[Benjamin Smedberg]

jshin, I am not actually an embedder. I was just made very conscious of codesize
issues while working with alecf, and looking a tbox it seemed a small change had
large codesize implications. If that was unexpected, I just wanted to bring it
to your attention. I don't have any particular need/desire for a fix.

Comment 18

[Alec Flett]

But I'd like to see this fix go in if in fact the extra code isn't needed on
linux... however, is there any way to detect at runtime what systems do and
don't need it, as opposed to choosing linux vs. not linux? i.e. is there some
kind of test we can add to configure.in to detect this case?

Comment 19

[Jungshik Shin]

> is there any way to detect at runtime what systems do and
> don't need it, as opposed to choosing linux vs. not linux? i.e. is 
> there some kind of test we can add to configure.in to detect this 
> case?

There may be. Adding seawood to CC.

We might be able to detect the codeset names for UTF-16, UTF-8, and ISO-8859-1
with some tweaking in configure.in. Then, we can reduce the codesize on all
Unix(-like) platforms. When I wrote about this possibility for another program,
some people had a reservation about it because what's detected at the
compile-time is not always the case at the run-time because iconv(3) is
available in a separate standalone libiconv and people often install it on
non-Linux systems if they're not satisfied with the stock version. 

On Linux (barring possible exceptions on embedded devices unknown to me. adding
dougt for this), the possibility is almost zero because there's no reason to
replace the stock iconv(3) with something else. It just occurrs to me that some
embedded devices (Linux) use a lightweight C library other than glibc. I'm not
sure whether it lacks iconv(3) altogether, in which case we don't have to worry
because 'HAVE_ICONV' is set to false. A tricky case is vanilla iconv(3) that
doesn't support UTF-16.  

BTW, Ulrich, is there any known glibc (configuration option) that doesn't
support UTF-16 but supports UCS-2? This question is also relevant to native
uconv (http://lxr.mozilla.org/mozilla/source/intl/uconv/native/).

If all these are regarded as too much headache for about 800bytes, I think we
can just go with #ifdef'ing out 'UTF-8 fallback' code for Linux. That'll save us
about 2.4kB (out of 4.5kB introduced by my check-in) instead of 3.2kB. 

Comment 20

[Boris Zbarsky [:bzbarsky]]

I think this checkin caused blocker bug 208809

Comment 21

[Ulrich Drepper]

> BTW, Ulrich, is there any known glibc (configuration option) that doesn't
> support UTF-16 but supports UCS-2?

UCS-2 handling is built in libc, UTF-16 is a module which can be removed.

Comment 22

[Darin Fisher]

>If all these are regarded as too much headache for about 800bytes, I think we
>can just go with #ifdef'ing out 'UTF-8 fallback' code for Linux. That'll save us
>about 2.4kB (out of 4.5kB introduced by my check-in) instead of 3.2kB. 

yeah, something like that is probably sufficient.

as for the other tweaks, since you have done them... maybe worth making the
changes, but in general there are bigger fish to fry ;-)

i think ulrich's comment suggests that we should fallback on UCS-2 if no UTF-16
converter exists.

Comment 23

[Jungshik Shin]

Attachment: a new patch to fix make it work on RH 6.2

This is the first of two patches. Here I'm explictly checking whether we need
byte swapping or not and byte-swap if necessary. I had to cast away constness,
which can be dangerous. I also disabled UTF8FALLBACK on linux but had to put
back the dummy conversion for BOM check because glibc 2.2.x also puts BOM for
UTF-16 and it's also necessary to check the endianness of UTF-16. With this,
the size reduction compared with the trunk is 1.3kB.

Comment 24

[Jungshik Shin]

Attachment: patch #2 to fix bug 208809 and to cut down the size

Here I'm following Ulrich's suggestion in bug 208809. Instead of using UTF-16
(which is 'native' endian on recent glibc but is BE in old glibc regardless of
the native endianness), UTF-16LE/UTF-16BE are used depending on the endianness.
One problem with this is that UTF-16LE/BE are not supported by glibc 2.1.x (at
least they're not in glibc 2.1.3 on RH 6.2 ). As a fallback, I'm using
'UNICODELITTLE' and 'UNICODEBIG' that are present both in glibc 2.1.x and
recent glibc.

The size reduction is about 2.8kB. Although we can't support nob-BMP on Linux
with old glibc with this patch, I think this patch is better than attachment
125292 [details] [diff] [review]. There're a couple of reasons. Firstly, I don't feel comfortable casting
away const (I guess it can crash Mozilla). And, it's smaler and faster (no need
to byte-swap). With attachment 125292 [details] [diff] [review], we might  be byteswapping twice, in
glibc and in our code. 

BTW, I blocked the dummy conversion to get rid of 'BOM"  on linux (because we
now explicitly specify the endianness), but that saves us only about 100bytes.
To be safe, I'd rather keep that code in.

Comment 25

[Ulrich Drepper]

> Instead of using UTF-16
> (which is 'native' endian on recent glibc but is BE in old glibc regardless of
> the native endianness),

That's wrong.  If you use UTF-16 and you provide no BOM big endian is used.  But
it is not correct to use UTF-16 without BOM.  So using the explicit names is
*always* correct, regardless of platform.

Comment 26

[Jungshik Shin]

> That's wrong.  If you use UTF-16 and you provide no BOM big endian is used. 
But it is not correct to use UTF-16 without BOM.  

  Let's put aside difference in interpreting UTF-16 and focus on how different
versions of glibc behave.  I didn't think about converting from UTF-16. 

1. In recent glibc, when converting to 'UTF-16',  the output is prepended with
BOM to indicate the native endian in the first invocation and the *native*
endian is used.

2.  When converting from UTF-16, the absence of BOM is regarded as indicating BE
regardless of the native endianness. 

3.  in old glibc, BOM was not prepended to the output and BE (not the native
endian) is used everywhere when converting to UTF-16.

#1 is what I observed and #3 was inferred from bz's test result in bug 208809. 

As for #2, is that what you meant? My test with glibc 2.2.93 on ix86 tells me a
different story. With or without BOM, "\x20\x21" is interpreted as U+2120 by
iconv when converting _from_ "UTF-16". That is, both '\xff\xfe\x20\x21' and
'\x20\x21' gave me the same result (interpreted as LE). Has there been a change
recently (2.3.x)?  

> So using the explicit names is *always* correct, regardless of 
> platform.

 No doubt about that. As I wrote, one problem with that is that UTF-16LE and
UTF-16BE are not available in old glibc. Anyway, using UNICODELITTLE and
UNICODEBIG as fallback at the cost of losing non-BMP support is reasonable.

Comment 27

[tenthumbs]

Glibc has /usr/include/byteswap.h which has optimized swapping 
functions. You should use them where available.

Comment 28

[Jungshik Shin]

Comment on attachment 125292 [details] [diff] [review]
a new patch to fix make it work on RH 6.2

This is not well tested nor safe. (crashes on bz's RH 6.2). Let's go with the
other one.

Comment 29

[Jungshik Shin]

> using UNICODELITTLE and UNICODEBIG as fallback at the cost of 
> losing non-BMP support in old glibc is reasonable.

Darin (Simon, and Alec) , what do you think? I think attachment 125293 [details] [diff] [review] (except
for not so pretty '__linux') is a reasonable compromise. If you agree, pls give
r/sr to the patch so that I can remove the blocker (bug 208809).  

Comment 30

[Jungshik Shin]

I've done some tests (direct and indirect) and found that UTF-16LE and UTF-16BE
work under glibc 2.2.x, Solaris 8 or newer, and Tru64 (Compaq - HP) V5. Under
Tru64 (both V4 and V5), the meaning of UTF-16 is dependent on the environment
variable ICONV_BYTEORDER, which is yet another reason to use UTF-16LE (because
alpha is LE). By default, 'UTF-16' in Tru64 is interpreted as LE (platform
endian). Unfortunately, Tru64 V4 doesn't support 'UTF-16LE' so that it'd break
Mozilla if ICONV_BYTEORDER is set to 'big-endian', but there's not much we can
do other than providing a shell script wrapper. Solaris 7 and IRIX 6 (both BE.
MIPS can be run as LE as well, but SGI machines are BE) interpret UTF-16 as BE
and emit UTF-16BE for 'UTF-16'. 
As mentioned earlier, for glibc 2.1.x, we need to use UNICODELITTLE
and UNICODEBIG because 'UTF-16' is always interpreted as BE by glibc 2.1.x and
UTF-16LE/UTF-16BE are not supported. 

Based on these and the fact that we have used UCS-2 and variants so far (AIX,
HP/UX and other Unix), I think we can use the following for  
UTF_16_NAMES[]

+#if defined(IS_LITTLE_ENDIAN)
+    "UTF-16LE",
+#if defined(__linux)
+    "UNICODELITTLE",
+#endif
+    "UCS-2LE",
+#else
+    "UTF-16BE",
+#if defined(__linux)
+    "UNICODEBIG",
+#endif
+    "UCS-2BE",
+#endif
     "UTF-16",
     "UCS-2",
     "UCS2",

Comment 31

[Darin Fisher]

jshin: that sounds reasonable to me.

Comment 32

[Jungshik Shin]

Attachment: a new patch doing what I wrote

Darin, thanks. Here's the patch implementing that. Can I get review? Alec and
Simon, please feel free to review :-)

This is basically the same as attachment 125293 [details] [diff] [review] except that UTF16_NAMES[] is
changed as I wrote and I kept the block removing BOM in the first invocation of
iconv(3) for linux (that is, I got rid of '#if !defined(__linux)' around that
code.).

Comment 33

[Jungshik Shin]

Just in case we need to refer to them later..Let me add a note that I used 
attachment 125288 [details] (to bug  208809) and attachment   125485 [details] (to bug 209048) ( 
for Solaris that doesn't like 4 NULL's in iconv, need to do what Mozilla  does 
in nsNativeCharsetUtils.cpp along with 'const char **' change) to try iconv(3) 
on a few platforms.  

Comment 34

[Jungshik Shin]

Comment on attachment 125560 [details] [diff] [review]
a new patch doing what I wrote


>+     * We also use it to determine whether or not 'UTF-16'/'UCS-2' is 
>+     * interpreted as native endian. If it's interpreted as BE on LE,
>+     * we need byte-swapping.

The above comment should have not been included.  I got rid of it in my tree.

I've been waiting for bug 209048 to be resolved. Now that it can be fixed by a
Solaris patch, I'm asking for r/sr.

Comment 35

[Darin Fisher]

Comment on attachment 125560 [details] [diff] [review]
a new patch doing what I wrote

>Index: xpcom/io/nsNativeCharsetUtils.cpp

>+// just in case... but we  know for sure that iconv(3) in glibc for Linux

nit:		    "but we know for"

>+// doesn't need this.
>+#if !defined(__linux)

is there a GLIBC specified #define that we should test instead?


>-// PRUnichar[] is NOT a UCS-2 array BUT for UTF-16 string. Therefore, we
>-// have to use UTF-16 with iconv(3) on platforms where it's supported.
>-// We could list 'UTF-16' name variants, but all platforms known (to me) to 
>-// support UTF-16 in iconv(3) uses 'UTF-16'. Let me know (jshin) if there's an
>-// exception. (bug 206811)
>+/* 


>+ * PRUnichar[] is NOT a UCS-2 array BUT for a UTF-16 string. Therefore, we

nit: PRUnichar[] is NOT a UCS-2 array BUT a UTF-16 array.

>+ * have to use UTF-16 with iconv(3) on platforms where it's supported.
>+ * However, the way UTF-16 and UCS-2 are  interpreted varies across platforms 

nit: However, the way UTF-16 and UCS-2 are interpreted varies across platforms 


>+ * and  implementations of iconv(3). On Tru64, it also depends on the environment
>+ * variable.  To avoid the trouble arising  from byte-swapping 

nit: variable.	To avoid the trouble arising from byte-swapping 


> static const char *UTF_16_NAMES[] = {
>+#if defined(IS_LITTLE_ENDIAN)
>+    "UTF-16LE",
>+#if defined(__linux)
>+    "UNICODELITTLE",
>+#endif
>+    "UCS-2LE",
...
>     "UTF-16",
>     "UCS-2",

so it's never the case that we want to try "UTF-16" before "UCS-2LE"?


>+     * the first use of the iconv converter. The same is the case 
>+     * case of glibc 2.2.9x and Tru64 V5 (see bug 208809)

nit: "case case"


>+     * when 'UTF-16' is used. However, we use 'UTF-16LE/BE' in both
>+     * cases, instead  so that we should be safe. But just in case...

nit:   * cases, instead so that we should be safe. But just in case...

>      * This dummy conversion gets rid of the BOMs and fixes bugid 153562.

nit: s/bugid/bug/

Comment 36

[Jungshik Shin]

> so it's never the case that we want to try "UTF-16" before "UCS-2LE"?

I guess what's in 125560 is not 100% correct, but safer than the following.    

--------------
static const char *UTF_16_NAMES[] = {
#if defined(IS_LITTLE_ENDIAN)
    "UTF-16LE",
#if defined(__linux)
    "UNICODELITTLE",
#endif
#else
    "UTF-16BE",
#if defined(__linux)  // or if defined(__GLIBC__)
    "UNICODEBIG",
#endif
#endif
    "UTF-16",
#if defined(IS_LITTLE_ENDIAN)
    "UCS-2LE",
#else
    "UCS-2BE",
#endif
    "UCS-2",
---------------

In reality, I think there's no difference unless there's an iconv implementation
that works the way glibc 2.1.x does. That is, we're in trouble if the
implementation interprets UTF-16 as UTF-16BE on a LE machine  but does not
recognize 'UTF-16LE'. Do you know of any implementation that does? 

Anyway, which one do you like?  

>>+#if !defined(__linux)

>is there a GLIBC specified #define that we should test instead?

  Perhaps '__GLIBC__' or '__GNU_LIBRARY__' (deprecated)? 

Comment 37

[Darin Fisher]

>In reality, I think there's no difference unless there's an iconv 
>implementation that works the way glibc 2.1.x does. That is, we're in trouble 
>if the implementation interprets UTF-16 as UTF-16BE on a LE machine  but does 
>not recognize 'UTF-16LE'. Do you know of any implementation that does? 

no, i don't know of any such implementations...


>Anyway, which one do you like?  

perhaps your first way was better since it'd be better... i defer to your best
judgement.  we can of course always fix it if we discover a problem on some
platform ;-)


>>>+#if !defined(__linux)
>
>>is there a GLIBC specified #define that we should test instead?
>
>  Perhaps '__GLIBC__' or '__GNU_LIBRARY__' (deprecated)? 

yeah, let's use __GLIBC__ instead since it's possible someone on another OS
might be using glibc as well.

Comment 38

[Jungshik Shin]

Attachment: a new patch with nits taken care of and __linux replaced by __GLIBC__

I decided to do the same as in attachment 125560 [details] [diff] [review]. So in terms of the actual
code, this is the same as attachment 125560 [details] [diff] [review] except that __GLIBC__ is used
instead of __linux.

Comment 39

[Darin Fisher]

Comment on attachment 127768 [details] [diff] [review]
a new patch with nits taken care of and  __linux replaced by __GLIBC__

sr=darin

Comment 40

[Jungshik Shin]

Ping :-) Simon, can I get r? Or shall I ask someone else? 

Comment 41

[Simon Montagu :smontagu]

Probably better ask someone else since, as I said before in comment 12, I really
know almost nothing about iconv.

Comment 42

[Jungshik Shin]

Comment on attachment 127768 [details] [diff] [review]
a new patch with nits taken care of and  __linux replaced by __GLIBC__

Ulrich, can you review? You're the authority on iconv :-)

Comment 43

[Ulrich Drepper]

Comment on attachment 127768 [details] [diff] [review]
a new patch with nits taken care of and  __linux replaced by __GLIBC__

The patch looks fine.  glibc will never use the BOM if the endianness is
explicitly requested but it probably doesn't hurt.

If the style is OK I see no problems.
r=drepper

Comment 44

[Jungshik Shin]

Comment on attachment 127768 [details] [diff] [review]
a new patch with nits taken care of and  __linux replaced by __GLIBC__

Fix checked into the trunk. Thank you all.

Now I'm asking for a1.4 as  Darin and I discussed (see comment #8). 

This patch (along with attachment 124092 [details] [diff] [review]) is to make nsNativeCharsetUtils on
Unix(including Linux) be UTF-16-safe. Some problems in attachment 124092 [details] [diff] [review] have
been identified and addressed in this patch so that I believe the risk is
pretty low. Linux with both glibc 2.2.x or later and old glibc 2.1.x were
tested. Solaris 8/9(Solaris 7 needs either a iconv patch from Sun or GNU iconv,
which was already relase-noted) is fine. Other Unix should be all right because
nsNativeCharsetUtils.cpp (with or without this patch) has a few  'defense
measures' against various possibilities. I also tested 'iconv(3)' APIs on Tru64
and HP/UX 11 and it works as is assummed here.

Considering that 1.4 would be a long lived one(like 1.0), I guess we can't just
assume that nobody would use non-BMP characters in filenames and other places.

On Linux, there was a little worry about the start-up time, but there's
virtually no difference (or slightly faster with this patch than without).

Given all these, I think it's safe to land this combined with attachment 124092 [details] [diff] [review]
in 1.4branch

Comment 45

[Asa Dotzler [:asa]]

Comment on attachment 127768 [details] [diff] [review]
a new patch with nits taken care of and  __linux replaced by __GLIBC__

We don't have many testing resources and we're short on time. This doesn't
appear to be a high-profile problem for users so it's not 1.4 material.

Comment 46

[Jungshik Shin]

sorry it's late. marking as fixed.