225340 - Unaligned accesses in DoCharsetConversion

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Jim Brown]

User-Agent:       Mozilla/5.0 (X11; U; OSF1 alpha; en-US; rv:1.6b) Gecko/20031111
Build Identifier: Mozilla/5.0 (X11; U; OSF1 alpha; en-US; rv:1.6b) Gecko/20031111

DoCharsetConversion() generates unaligned accesses since 031110 (incl. 031111).
Trunk build on 031109 did not show this problem.
Page displays correctly, just a lot of noise on the console and a performance hit.


Reproducible: Always

Steps to Reproduce:
1. visit http://www.macnn.com/ (for example)
2. observe unaligned access messages on console




Partial traceback (no line numbers):

Thread stopped on Unaligned Access

(ladebug) where
>0  0x3ffbfb38190 in DoCharsetConversion(...) in /usr/local/mozilla-1.5/componen
ts/libi18n.so
#1  0x3ffbfb37ad4 in Convert(...) in /usr/local/mozilla-1.5/components/libi18n.s
o
#2  0x3ffbf55faf0 in Convert(...) in /usr/local/mozilla-1.5/components/libgfx_gt
k.so
#3  0x3ffbf55fce8 in GetWidth(...) in /usr/local/mozilla-1.5/components/libgfx_g
tk.so
#4  0x3ffbf563724 in GetTextDimensions(...) in /usr/local/mozilla-1.5/components
/libgfx_gtk.so
#5  0x3ffbf543b78 in GetTextDimensions(...) in /usr/local/mozilla-1.5/components
/libgfx_gtk.so

I will followup with additional information after I make a debug build.

Comment 1

[Simon Montagu :smontagu]

jshin, could this have been caused by any of your recent checkins in
intl/unicharutil?

Comment 2

[Jungshik Shin]

Yes, probably that was caused by my fix for bug 221024.[1] Actually, this is not
new but has been present ever since the extended CCMap was introduced. 

The macro 'CCMAP_HAS_CHAR_EXT' examines the first two PRUnichar's(actually -2nd
and -1st element) of a compressed charmap to see if the ccmap is extended (i.e.
covering non-BMP chararcters) and how long it is if it does. [2]
It works fine on 32bit machines. However, on 64bit machine, it leads to
unaligned accesses (I think). A possible patch is to modifiy CCMAP_HAS_CHAR_EXT
and the code used to generate extended CCMaps so that one machine word (on a
64bit machine, that would be two 32bit integers instead of two PRUnichar's) at
the beginning of extended CCMaps is assigned for encoding those two pieces of
information. 

BTW, it's a little strange that the unaligned access was triggered on a seeminly
pure ASCII page. Not even some punctuations (not in ISO-8859-1 but in
Windows-1252 so that they're not covered by ISO8859-1 X11 core fonts) seem to be
used. Ahah, there's an Euro sign. Jim, do you have any font with Euro coverage
(e.g ISO8859-15 font or UTF-8 font)? Probably, you don't. If you did, you
wouldn't have hit this bug. Installing ISO8859-15 fonts would enable you to
avoid this problem if my theory is correct. Nonetheless, we have to fix this
(for 64bit/RISC cleanness)


[1]http://lxr.mozilla.org/mozilla/source/intl/unicharutil/src/nsSaveAsCharset.cpp#271

[2]
http://lxr.mozilla.org/mozilla/source/intl/unicharutil/util/nsCompressedCharMap.h#225

Comment 3

[Jungshik Shin]

s/UTF-8 font/iso10646-1 font/

Comment 4

[Jim Brown]

I only use (have?) ISO8859-1 fonts.

Comment 5

[Jungshik Shin]

If you add support for 'Euro'-locales (e.g. de_DE@euro) or UTF-8 locales, I
guess iso8859-15 and other fonts will be installed.

Anyway, folks, which way do you think is better?

 A. Use two PRUint32's instead of two PRUint16's for 'meta' information about 
    extended CCMaps on all architectures (this will results in unaligned access 
    errors on  128bit machines in 
    the  future. Or now if Mozilla is ported to 128bit game machines by  
    Sony?/Nintendo? :-))

 B. Use two PRUint32's only on 64bit machines with #ifdef. 

Comment 6

[Jungshik Shin]

It turned out that 64bit-cleanness was already built in. So, this bug was not
present until my check-in of the patch for bug 221024. I'm gonna make a patch soon.
 

Comment 7

[Jungshik Shin]

Attachment: patch

On 64bit platforms, use two PRUint32's to store extra pieces of information for
extended. ccmaps.

Comment 8

[Jungshik Shin]

Comment on attachment 135581 [details] [diff] [review]
patch

asking for r/sr.

Comment 9

[Simon Montagu :smontagu]

Comment on attachment 135581 [details] [diff] [review]
patch

r=smontagu

Comment 10

[rbs]

Comment on attachment 135581 [details] [diff] [review]
patch

sr=rbs

Comment 11

[Jungshik Shin]

Comment on attachment 135581 [details] [diff] [review]
patch

Asking for 1.6b approval. (rbs forgot to mark 'sr' in the attachment but he did
in comment)

This fix is to avoid the unaligned memory access on 64bit architecture.

This change does not affect any 32bit platforms because 64bit part of the code
is brought in with #ifdef. 

All of our first tier targets are on 32bit (although all three of them can now
run on 64bit machine. Can Win XP ?) so that the risk should be very low. 

Also, note that I verified 32bit pre-composed Cmap generated by the perl script
(ccmapbin.pl that is NOT a part of the build but is included in the tree for
the future reference and improvement) for 32bit machines are identical to those
generated by the previous version of the script. This ensures us that 32bit
machines are not affected at all.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 135581 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.6 beta.

Comment 13

[Jungshik Shin]

fix checked in a few days ago. 

Comment 14

[Jungshik Shin]

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