Closed Bug 224337 Opened 21 years ago Closed 21 years ago

Mozilla crashes viewing hectorplasmic.com

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: gonufer, Assigned: jshin1987)

References

(
URL
)

Details

(Keywords: crash)

Attachments

(2 files, 2 obsolete files)

stack trace from Forte compiled crash on Solaris
14.80 KB, text/plain
Details
update (basically the same) with all brendan's concerns addressed
169.43 KB, patch
brendan
: review+
brendan
: superreview+
dbaron
: approval1.6+
Details | Diff | Splinter Review 
On SunOS 5.9 SPARC platforms (at the very least) mozilla 1.4 branch, mozilla 1.5
branch, and a trunk build from 2003103020) all crash when
http://hectorplasmic.com/?Brainstorm is loaded.  The bus error occurs in
libgklayout.so.  The stacktrace does not have symbols so I will not include it here.

mozilla 1.3 does NOT crash.
wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031028
but on the page mentioned I see a scrollbar missing the slider, as it is not
needed. On another link from there I see some funny behaviour on scrolling:
http://hectorplasmic.com/?l_name=library
Heading LIBRARY: The L is scrolling separately,
The word library below the heading: LIB is scrolling separately.
There is a small column from the top to the bottom of the page, including these
examples, which isn´t synchronized with the rest of the line while scrolling.
After standstill, the chars are well positioned.
Severity: major → critical
Keywords: crash
Greg, if you can make a debug build and get the stack symbols that would be
much appreciated.  (WFM, Mozilla 1.4, 1.5 and trunk on Linux)
Assignee: general → other
Component: Browser-General → Layout
QA Contact: general → ian
This is the stack trace...  a bogus or misaligned pointer is dereferenced in
nsTextFrame::MeasureText.

current thread: t@1
  [1] nsTextFrame::MeasureText(0x9b2590, 0x8be670, 0xffbe81fc, 0xffbe7e1c, 0x9a6
4b0, 0xffbe7f70), at 0xfa5dac94 
  [2] nsTextFrame::Reflow(0x9b2590, 0x8be670, 0xffbe81a4, 0xffbe81fc, 0xffbe8d14
, 0x0), at 0xfa5dccbc 
  [3] nsLineLayout::ReflowFrame(0xffbe8eb4, 0x9b2590, 0xffbe8d14, 0xffbe8a9c, 0x
ffbe8378, 0xffbe8d14), at 0xfa4df42c 
  [4] nsFirstLetterFrame::Reflow(0x9b25d0, 0x8be670, 0xffbe8a9c, 0xffbe8af4, 0xf
fbe8d14, 0x0), at 0xfa3fdee4 
  [5] nsLineLayout::ReflowFrame(0xffbe8eb4, 0x9b25d0, 0xffbe8d14, 0x0, 0xffbe8d1
0, 0x0), at 0xfa4df42c 
  [6] nsBlockFrame::ReflowInlineFrame(0x9afa70, 0xffbe9cb8, 0xffbe8eb4, 0xffbe8d
d8, 0x9b25d0, 0xffbe8e03), at 0xfa3ab4cc 
  [7] nsBlockFrame::DoReflowInlineFrames(0x9afa70, 0xffbe9cb8, 0xffbe8eb4, 0xffb
e8ea8, 0xffbe97b4, 0xffbe9383), at 0xfa3aac88 
  [8] nsBlockFrame::DoReflowInlineFramesAuto(0x9afa70, 0xffbe9cb8, 0xffbe9370, 0
xffbe97b4, 0xffbe9383, 0x0), at 0xfa3aa63c 
  [9] nsBlockFrame::ReflowInlineFrames(0x9afa70, 0xffbe9cb8, 0xffbe94b4, 0xffbe9
7b4, 0x0, 0x0), at 0xfa3aa188 
  [10] nsBlockFrame::ReflowLine(0x9afa70, 0xffbe9cb8, 0xffbe974c, 0xffbe97b4, 0x
0, 0xfbd49a30), at 0xfa3a6540 
  [11] nsBlockFrame::ReflowDirtyLines(0x9afa70, 0xffbe9cb8, 0x8be670, 0x9afa70, 
0xffbea3d0, 0x0), at 0xfa3a4a14 
  [12] nsBlockFrame::Reflow(0x9afa70, 0x8be670, 0xffbea3d0, 0xffbea280, 0xffbea3
48, 0x0), at 0xfa39e8d8 
  [13] nsBlockReflowContext::ReflowBlock(0xffbea38c, 0xffbea35c, 0x0, 0xffbeadc4
, 0x1, 0xffbea338), at 0xfa3cb6dc 
  [14] nsBlockFrame::ReflowBlockFrame(0x9af938, 0xffbead60, 0xffbea5d8, 0xffbea8
5c, 0xfbd49a30, 0xfbd49a30), at 0xfa3a91c8 
  [15] nsBlockFrame::ReflowLine(0x9af938, 0xffbead60, 0xffbea7f4, 0xffbea85c, 0x
0, 0xfbd49a30), at 0xfa3a5c34 
  [16] nsBlockFrame::ReflowDirtyLines(0x9af938, 0xffbead60, 0x8be670, 0x9af938, 
0xffbeb478, 0x0), at 0xfa3a4a14 
  [17] nsBlockFrame::Reflow(0x9af938, 0x8be670, 0xffbeb478, 0xffbeb328, 0xffbeb3
f0, 0x0), at 0xfa39e8d8 
  [18] nsBlockReflowContext::ReflowBlock(0xffbeb434, 0xffbeb404, 0x1, 0xffbebe6c
, 0x0, 0xffbeb3e0), at 0xfa3cb6dc 
  [19] nsBlockFrame::ReflowBlockFrame(0x99fef8, 0xffbebe08, 0xffbeb680, 0xffbeb9
04, 0xfbd49a30, 0xfbd49a30), at 0xfa3a91c8 
  [20] nsBlockFrame::ReflowLine(0x99fef8, 0xffbebe08, 0xffbeb89c, 0xffbeb904, 0x
1, 0xfbd49a30), at 0xfa3a5c34 
  [21] nsBlockFrame::ReflowDirtyLines(0x99fef8, 0xffbebe08, 0x8be670, 0x99fef8, 
0xffbec520, 0x0), at 0xfa3a4a14 
  [22] nsBlockFrame::Reflow(0x99fef8, 0x8be670, 0xffbec520, 0xffbec3d0, 0xffbec4
98, 0x0), at 0xfa39e8d8 
  [23] nsBlockReflowContext::ReflowBlock(0xffbec4dc, 0xffbec4ac, 0x1, 0xffbecf14
, 0x1, 0xffbec488), at 0xfa3cb6dc 
  [24] nsBlockFrame::ReflowBlockFrame(0x99fd94, 0xffbeceb0, 0xffbec728, 0xffbec9
ac, 0xfbd49a30, 0xfbd49a30), at 0xfa3a91c8 
  [25] nsBlockFrame::ReflowLine(0x99fd94, 0xffbeceb0, 0xffbec944, 0xffbec9ac, 0x
1, 0xfbd49a30), at 0xfa3a5c34 
  [26] nsBlockFrame::ReflowDirtyLines(0x99fd94, 0xffbeceb0, 0x8be670, 0x99fd94, 
0xffbed2b0, 0x400000), at 0xfa3a4a14 
  [27] nsBlockFrame::Reflow(0x99fd94, 0x8be670, 0xffbed2b0, 0xffbed1d4, 0xffbed5
a8, 0x8be670), at 0xfa39e8d8 
=>[28] nsContainerFrame::ReflowChild(0x9936ac, 0x99fd94, 0x8be670, 0xffbed2b0, 0
xffbed1d4, 0x0), at 0xfa3f2cb8 
  [29] CanvasFrame::Reflow(0x9936ac, 0x8be670, 0xffbed55c, 0xffbed3d0, 0xffbed5a
8, 0x2), at 0xfa46ef60 
  [30] nsBoxToBlockAdaptor::Reflow(0x99fd04, 0xffbedcdc, 0x8be670, 0xffbed55c, 0
xffbedec4, 0xffbed5a8), at 0xfa893584 
  [31] nsBoxToBlockAdaptor::DoLayout(0x99fd04, 0xffbedcdc, 0xffbedcdc, 0xfbd49a3
0, 0x0, 0xffbed604), at 0xfa892a9c 
  [32] nsBox::Layout(0x99fd04, 0xffbedcdc, 0xffbed794, 0x0, 0x0, 0xffbedcdc), at
 0xfa868edc 
  [33] nsScrollBoxFrame::DoLayout(0x9939d8, 0xffbedcdc, 0xffbedcdc, 0xfbd49a30, 
0x0, 0xffbed804), at 0xfa8553f4 
  [34] nsBox::Layout(0x993a10, 0xffbedcdc, 0xffbedad0, 0x0, 0x0, 0xffbedcdc), at
 0xfa868edc 
  [35] nsContainerBox::LayoutChildAt(0xffbedcdc, 0x993a10, 0xffbedad0, 0xfbd49a3
0, 0x18f154c, 0x90), at 0xfa89bb44 
  [36] nsGfxScrollFrameInner::LayoutBox(0x998c38, 0xffbedcdc, 0x993a10, 0xffbeda
d0, 0x0, 0xfbd93a38), at 0xfa45851c 
  [37] nsGfxScrollFrameInner::Layout(0x998c38, 0xffbedcdc, 0xffbedb90, 0xfbd49a3
0, 0xfbd49a30, 0xfbd49a30), at 0xfa458b64 
  [38] nsGfxScrollFrame::DoLayout(0x9938bc, 0xffbedcdc, 0xffbedcdc, 0xfbd49a30, 
0x0, 0xffbedbdc), at 0xfa4585dc 
  [39] nsBox::Layout(0x9938f4, 0xffbedcdc, 0xffbedca4, 0x4519, 0x30fc, 0xffbee14
c), at 0xfa868edc 
  [40] nsBoxFrame::Reflow(0x9938bc, 0x8be670, 0xffbedf84, 0xffbedec4, 0xffbee14c
, 0xffbee14c), at 0xfa878e34 
  [41] nsGfxScrollFrame::Reflow(0x9938bc, 0x8be670, 0xffbedf84, 0xffbedec4, 0xff
bee14c, 0x8be670), at 0xfa456324 
  [42] nsContainerFrame::ReflowChild(0x9935a0, 0x9938bc, 0x8be670, 0xffbedf84, 0
xffbedec4, 0x0), at 0xfa3f2cb8 
  [43] ViewportFrame::Reflow(0x9935a0, 0x8be670, 0xffbee30c, 0xffbee150, 0xffbee
14c, 0xffbee208), at 0xfa5fcf94 
  [44] IncrementalReflow::Dispatch(0xffbee2b8, 0x8be670, 0xffbee30c, 0xffbee2fc,
 0x977858, 0xffbee2fc), at 0xfa548100 
  [45] PresShell::ProcessReflowCommands(0x8c05f0, 0x1, 0x0, 0xffbee3e0, 0x0, 0x1
de13), at 0xfa56de20 
  [46] ReflowEvent::HandleEvent(0x9a8f00, 0xfbd49a30, 0x0, 0x1916a0, 0x0, 0x1de1
1), at 0xfa57b08c 
  [47] HandlePLEvent(0x9a8f00, 0x9a8f00, 0x18a6a4, 0xfdef8ef8, 0x8c8368, 0x9a8f0
0), at 0xfa56d17c 
  [48] PL_HandleEvent(0x9a8f00, 0x1911b8, 0x0, 0x5, 0x5, 0x4), at 0xfdd73924 
  [49] PL_ProcessPendingEvents(0x18a6a0, 0x0, 0x0, 0x0, 0xfdef8ef8, 0x0), at 0xf
dd735c8 
  [50] nsEventQueueImpl::ProcessPendingEvents(0x1a9c50, 0xefb48, 0x0, 0xfbf1c1a8
, 0x0, 0x0), at 0xfdd78ce0 
  [51] event_processor_callback(data = 0x1a9c50, source = 6, condition = GDK_INP
UT_READ), line 187 in "nsAppShell.cpp"
  [52] our_gdk_io_invoke(source = 0x3a8820, condition = G_IO_IN, data = 0x3a9210
), line 72 in "nsAppShell.cpp"
  [53] g_io_unix_dispatch(0x180ce8, 0xffbee878, 0x3a9210, 0xfe6efea8, 0x23, 0xfe
6f0010), at 0xfe6c5358 
  [54] g_main_dispatch(0x0, 0xfe6efe10, 0xab4, 0xaac, 0xab4, 0xaac), at 0xfe6c91
5c 
  [55] g_main_iterate(0xab4, 0x1, 0x800, 0xabc, 0xab4, 0xaac), at 0xfe6c8dc0 
  [56] g_main_run(0x3a9220, 0x0, 0xfe6efe30, 0x0, 0x1a9c50, 0x1), at 0xfe6c76dc 
  [57] gtk_main(), line 524 in "gtkmain.c"
  [58] nsAppShell::Run(this = 0x19c2a0), line 327 in "nsAppShell.cpp"
  [59] nsAppShellService::Run(this = 0x294d00), line 483 in "nsAppShellService.c
pp"
  [60] main1(0x4, 0xffbeee34, 0x28ada0, 0x0, 0xffbeed98, 0x0), at 0x28f84 
  [61] main(0x4, 0xffbeee34, 0xffbeee48, 0xddc00, 0x0, 0x0), at 0x2ab3c 

*** This bug has been marked as a duplicate of 161826 ***
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → DUPLICATE
Does the crash go away if you set the environment variable
MOZILLA_GFX_DISABLE_FAST_MEASURE ?

If so, was "__i386" somehow defined when you were building?  See 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/gtk/nsFontMetricsGTK.cpp&rev=1.262#5594
My trunk builds still fails with that environment variable defined.  There is no
reason for __i386 to have been defined, I'll modify that file to #error if it is.
The code is being compiled as expected, no __i386 preprocessor tokens.  It
_does_ fail with MOZILLA_GFX_DISABLE_FAST_MEASURE in the environment.
*** Bug 161826 has been marked as a duplicate of this bug. ***
not sure that this is really a duplicate
Status: RESOLVED → REOPENED
Component: Layout → Layout: Fonts and Text
Depends on: 161826
Resolution: DUPLICATE → ---
*** Bug 225962 has been marked as a duplicate of this bug. ***
Just to add more mystery to this, i tried compiling with SOS8 and Forte 6.x
compiler on Sun and both will crash with the url mentioned, or even this

    http://www.csszengarden.com/?cssfile=/054/054.css&page=1

However, since i'm not a dbx man, i compiled with gcc 3.x and i cannot
for the life of me get either page to crash anymore. Not sure if this is
a compiler bug ? Greg, can you try a compile with gcc 3.x ? 
The original URL no longer crashes for me with a trunk built with Sun C++ 5.5
(maybe the site changed?) but the csszengarden URL from Comment 11 does.  I
recompiled the trunk with a pre-release of Sun C++ 5.6 and could not reproduce
the crash.  Sigh.

I'm not set up right now to compile with gcc.
Ok, i got fedup with this - i had enough buses arrive yesterday that i
decided to devote some time and do something about it.

Greg, you're right, this isn't a compiler bug. Somehow gcc was doing something
different not to trigger this. Anyhow i have fixed this (i believe). This
fix affects bug 161826 and others probably. Another example crashing URL is
the chatzilla page

    http://www.hacksrus.com/~ginda/chatzilla/

I'm attaching a new stack showing the error clearly. This is a result of the
compressed cmaps. I believe the cast in CCMAP_TO_ALU macro is wrong since the 
cmap's are arrays of 16bit pointers and we are attempting to read from a 32bit
address.

More details to follow in the next attachment and comment and patch diff.
All the mentioned sites in the bugs now don't crash the browser anymore.

Please someone who has the ability to review comment on the change
and checkin since this affects all the sparc users out there.

The regs from the stack (at the bottom of this attachment) shows that
we should really be doing a 'lduh' instead from the 32bit misaligned address
since %l2 + %l1 is not 32 bit aligned. I looked at the code and the compressed
cmaps are arrays of 16bit pointers, so this should really be cast correctly.
Attached patch Proposed patch (obsolete) — — Splinter Review 
And here is the diminutive patch that fixes the problem.
Note that Firebird is also affected, all the URL's mentioned
in this bug work now with the attached fix on Solaris on both
browsers (mozilla nd firebird).

Someone please sr and sr+
Just an update, i was slightly wrong in Comment 13 since ccmaps are arrays
of shorts (Pruint16) and not pointers, so i believe the fix is still correct.
However this seems to have probably uncovered another bug which is that the
font loading code seems to loop indefinetly reloading the same font over
and over again. I find it i add this path to my ~/.mozilla/*/*/prefs.js

user_pref("font.directory.truetype.2", "/usr/openwin/lib/X11/fonts/TrueType");

we end up reloading fonts over and over, e.g. when you set NS_FONT_DEBUG=1 
in your environment you can see this when you start up mozilla and the mail
client especially...

loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
loaded "Mono-symbol-symbol-fontspecific", size=12,
filename=/usr/openwin/lib/X11/fonts/TrueType/Symbol.ttf
....etc...

More work needs to be done.
OS: SunOS → Solaris
Comment on attachment 137105 [details] [diff] [review]
Proposed patch

If ccmaps are always PRUint16, why do we need ALU_TYPE, etc.?
Attachment #137105 - Flags: review?(jshin)
Assignee: other → font
Status: REOPENED → NEW
No longer depends on: 161826
Good question. I'm waiting for someone to explain it to me.
I think ALU_TYPE is only used for the alignment in the
nsCompressedCharMap class (see below).

You can see clearly that the compressed maps are a array of shorts in
the protected class, and thus should be indexed as such  

class nsCompressedCharMap {          
public:                    
  nsCompressedCharMap();
  ~nsCompressedCharMap();
                         
 ...stuff...
protected:
  union {
    PRUint16 mCCMap[CCMAP_MAX_LEN];
    ALU_TYPE used_for_align; // do not use; only here to cause
                             // alignment
  } u;
 ...stuff...


CCMaps are arrays of PRUnichar* only on the surface. When they're accessed,
they're accessed as arrays of ALUTYPE. That is, on 32bit machines, they're
accessed as 32 bit int while on 64bit machines, they're accesse as 64bit int.


// offset from base to alu
#define CCMAP_TO_ALU(m,c) \
          (*((ALU_TYPE*)(&((m)[CCMAP_TO_PAGE((m),(c))])) + CCMAP_ALU_INDEX(c)))

// test the bit
#define CCMAP_HAS_CHAR(m,c) (((CCMAP_TO_ALU(m,c))>>CCMAP_BIT_INDEX(c)) & 1)

See how CCMAP_BIT_INDEX is defined:

#define CCMAP_BIT_INDEX(c)          ((c) & PR_BITMASK(CCMAP_BITS_PER_ALU_LOG2))


Comment on attachment 137105 [details] [diff] [review]
Proposed patch

As I explained, this fix would not work at all. 

I'm pretty sure the symptom originally reported is not present any more because
I fixed it in bug 225340. I broke it in bug 205387
Attachment #137105 - Flags: review?(jshin) → review-

*** This bug has been marked as a duplicate of 225340 ***
Status: NEW → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → DUPLICATE
I realized that the crash reported here occurred where a BMP-only CCMap is
accessed so that it's not a dupe of bug 225340. I'm sorry for the confusion. The
fact that the crash cannot be reproducible any more in December (after being
discovered in October) and it has to do with the alignment made me jump to the
conclusion. Anyway, the patch proposed here is not right. It'd break Mozilla on
every platform (except for 16bit platforms that Mozilla doesn't support). 

The only explanation I can come up with  is that a binary compiled on 32bit
Sparc was tried on Sparc64 (or the other way around). Needless to say, this
doesn't explain why 1.3 and recent trunk builds are fine while 1.5 is not.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
The mozilla binary was compiled and executed on a 64bit sparc machine with a
compiler generating 32 bit binaries. You can see at the end of my comment 14 that
we are loading from a misaligned address.
Aha, that explains at least a part of the story. So, what is ALU_SIZE at the
compilation time? Is it 32 or 64? What's the size of 'int', 64 or 32? If they're
mixed up somewhere, misaligned accesses are bound to happen. See, for instance,
punct_marks.ccmap, in layout/html/base/src.

ALU_SIZE == PR_BITS_PER_LONG == sizeof(long) == 32

The misalignment is happening on 16bit boundaries.
Jungshik let me explain it more simply what i see is happening.

In the nsCompressedCharMap class you have a mCCmap array
of shorts (PRUint16) - i.e. aligned on 16bit boundary.
If you index this array, it will increment by 2 bytes
and so a 32it read from this address will give you a misalignment
error. Thus here

#define CCMAP_TO_ALU(m,c) \
	(*((ALU_TYPE*)(&((m)[CCMAP_TO_PAGE((m),(c))])) + CCMAP_ALU_INDEX(c)))

you index the 'm' array of shorts and are then using ALU_TYPE 
which is 32bit word aligned to load from the 16bit half word
aligned address. This produces the crash.

Irrespective of what you copy into the mCCmap array (32bit, 64bit, 16bit,
etc values), that macro access is incorrect. 

p.s. casting the macro to a half-word	does appear to work, and i
can't crash any of the URL's mentioned in this bug with the proposed fix.
Though, i'm not sure it is the coorect fix since i'm still trying to
get my head around the structure of the ccmap.

Please, see how CCMAP_TO_MID, CCMAP_ALU_INDEX, CCMAP_TO_PAGE, CCMAP_UPPER_INDEX,
and CCMAP_MID_INDEX are defined differently depending on ALU_SIZE before saying
the macro is incorrect. How would you explain that the macro have worked fine on
64bit Alpha as well as on 32bit/64bit PPC, and paRisc? All of them are as picky
about unaligned accesses as Sparc is.


This can only happen if Forte compiler puts const static PRUnichar[] at an
address unaligned (from the point of view of 32bit int). That is, as long as the
starting address of const static PRUnichar[] (in nsTextFrame) is aligned at
32bit int boundary, there should be no problem, which explains why there's no
problem when compiled with g++. Perhaps, it's not a compiler bug, but still I
wouldn't like such a  compiler very much.



gcc/g++ uses various tricks to allow unaligned memory access... but this results
in a huge slowdown in some cases. Actually it's one of the reasons why gcc/g++
compiled Mozillas are _magnitudes_ SLOWER than Sun Workshop builds.
Assuming my speculation is right, we have to figure out how to force Forte
compiler to start PRUnichar[] at an aligned address? Isn't there a compiler option.

As for gcc/g++, you got me wrong. Perhaps, it allows an unaligned access (so
does compilers for Tru64, but run-time linker? complains loudly at run-time if a
binary thus compiled is executed). However, that's totally besides the point. My
point is that Forte compiler seems to put PRUnichar[] at any 16bit boundary it
likes while gcc/g++ puts it always at 32bit address boundary (on 32bit machine).
In other words, various offsets calculated in macros I listed in the last
comment are written specifically to acess memory without misalignment so that
the only possible cause of misaligned access is that the array itself starts at
an unaligned address.



Jungshik Shin wrote:
> Assuming my speculation is right, we have to figure out how to force Forte
> compiler to start PRUnichar[] at an aligned address? Isn't there a compiler 
> option.

Sure, there is an option for force Sun Workshop to handle unaligned memory
accesses - but that will trigger a _DRAMATIC_ performance loss (even the
compiler docs have warnings about the quite drastic consequences).
That's not what jshin asked.  Is it possible to force a PRUnichar[] to be
aligned to a 32-bit or 64-bit boundary rather than just a 16-bit boundary?
Mitch, please use diff -u for patches.

The standard way to ensure alignment is to use a union.  If the PRUint16 array
jshin mentions in comment 29 needs to be aligned on a wider type boundary, union
it with a dummy member of the wider type.

/be
Want the right patch for 1.6.

/be
Flags: blocking1.6+
Are you sure that a |PRUnichar *| with 16bit alignment is triggering the problem
?
AFAIK (correct me if I am wrong) SPARC follows the "natural" alignment rules:
One byte for a char,
two bytes for a short,
four bytes for an integer etc.
If n is present, it must be a power of 2 specifying the strictest natural
alignment for any structure member.
The code in question is accessing memory from a PRUnichar[] as though it were an
array of 32-bit units.
Roland, please see
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsTextFrame.cpp#4550
for the array that is aligned, per its type (PRUint16 -- note not PRUnichar, but
the same size), on at most a 0 mod 2 boundary, but which may need to be more
strictly aligned, depending on ALU_TYPE.

A union here is ugly, unless C++ allows you to initialize the first member of a
union (C does not, if memory serves).  Jshin, dbaron?

/be
Brendan Eich wrote:
> Roland, please see
>
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsTextFrame.cpp#4550

Ouch... somehow I was still thinking about bug 161826
("nsTextFrame::MeasureText()'s fast text measuring codepath crashes on RISC
machines") and did not realise that this is another part of the
nsTextFrame-rocky-horrow-show... my fault... ;-(

If I recall it correctly a simple
#pragma align 64 (gPuncCharsCCMap)
should cure the problem for both 32bit and 64bit Sun Workshop builds (I am not
sure about the syntax... I didn't use the macro for a couple of years now...).
Ah, C++ allows first-union-member initialization:
http://66.102.7.104/search?q=cache:a9nMdeUaOoMJ:www.informit.com/isapi/guide~cplusplus/seq_id~154/guide/content.asp+C%2B%2B+union+initializer&hl=en&ie=UTF-8

So the idea would be to change nsTextFrame.cpp around line 4550 to be:

static const union {
    PRUint16 array[];
    ALU_TYPE align;
} gPuncCharsCCMap =
{
#include "punct_marks.ccmap"
};

But probably, you'd need to dimension the array member.  And is ALU_TYPE exposed
to clients of the CCMap code?

/be
We want a portable solution here.  Unless there's a pragma supported by all
standard C++ compilers, I think the union is the one true way.

/be
Wow, you guys are quick. I was writing more or less the same comment while your
comments arrived in my mailbox.

As for 'pragma' (Sun Workshop compiler specific), it's documented here:

http://docs.sun.com/db/doc/805-4952/6j4mdceh2?a=view#z400016f35be

Obviously, we want to go with a general solution brendan found because this can
be potentially an issue to other platform/compiler combinations.

Taking. There are a few other PRUint16[] arrays like this one. 

I'll make a patch (tomorrow) after playing with 'union' for a while. If we have
to dimension the array explicitly, I need to modify my perl script
(intl/unicharutil/tools/ccmapbin.pl) as well.
Assignee: font → jshin
Status: REOPENED → NEW
Component: Layout: Fonts and Text → Internationalization
A trivial test program shows that g++ at least requires you to dimension the
array explicitly.

/be
Thanks for testing, brendan. Then, we have two options:

1. modify my perl script to generate the whole thing (as shown in comment #38)
when passed the name of a variable('gPuncCharsCCMap' in this case and include
the generated file. In *cpp, we'd only have this

#include "punct_marks.ccmap"

2. modify the script to print out the number of PRUint16's and use the number to
dimension the array explicitly. We'd have 

static const union {
    PRUint16 array[128];
    ALU_TYPE align;
} gPuncCharsCCMap =
{
#include "punct_marks.ccmap"
};

I like #2  better than #1. What do you think?
Status: NEW → ASSIGNED
OS: Solaris → All
Hardware: Sun → All
#1 doesn't tell exactly what the variable name is, does it?

#2 is verbose in its default form, suggesting

#3 macro? e.g.

DEFINE_CCMAP(128, gPuncCharsCCMap) ?

[and its friend DEFINE_X_CMAP() could hide the reference to the 'raw' stuff]
#2 requires you to watch the number the tool puts out and copy it into the .cpp
file?  I don't like that so much, compared to a no-manual-transcription method
like #1, or a macro defined in a header.  But if you define the length of the
array as a macro, you'll need to #include its header -- may as well do #1 and
cut down on .cpp file complexity/noise.

/be
QA Contact: ian → nobody
*** Bug 228151 has been marked as a duplicate of this bug. ***
rbs' #3 combined with another header file defining the array size (brendan's
idea) would be best if C preprocessing were able to deal with '#include .....'
directive generated as the result of a macro expansion. Then, I'd be able to get
away with just a single line in *cpp

DEFINE_CCMAP(gPuncCharsCCMap, punct_marks) 

where the second parameter is the filename prefix (punct_marks.ccmap and
punct_marks.def). Unfortunately, it doesn't work. So, I'm left with two options :

A. (brendan's preference?)

#include "punct_marks.ccmap"

where punct_marks.ccmap hides away the variable name (gPuncCharsCCMap) as well
as details of the way it's defined. I'm a bit uncomfortable with hiding the
variable name as rbs is. 

B. (rbs' + brendan's ) 

  a. define the following macro in nsCompressedCharMap.h
  #define CCMAP_DEFINE(var) \
  static const union { \
    PRUint16 array[var ## Size]; \
    PRUint32 align; \
  } var ## Union

  b. have the script emit two files, one defining the size of the array and the
other with
     the actual pre-compiled ccmap

  c. In *ccp, use the following:

   #include "punct.def"
   CCMAP_DEFINE(gPunctMap) =
   {   
     #include "punct.ccmap"
   };  
   const PRUint16* gPunctMap = gPunctMapUnion.array;

With B, we'd have two more lines (for a BMP-only CCMap) and one more lines for
an extended CCMap than we currently have.

Thoughts? 

Why put the const var-named decl in the macro?

#define CCMAP_DEFINE(var)                                                     \
  static const union {                                                        \
    PRUint16 array[var ## Size];                                              \
    PRUint32 align;                                                           \
  } var ## Union;
  const PRUint16* var = var##Union.array

I guess we're stuck with a pair of #include files if we don't want to hand-copy
the array dimension somewhere.  Don't use .def as a suffix though, it is used on
some systems for a linker control file (Windows, OS/2 -- possibly archaic, but
still worth avoiding).

/be




I'll just use '.h' instead of '.def'.  As for the macro, we have to initialize
'union'  by '#includ'ing 'punct_marks.ccmap' file so that I don't see how yours
can be used. 

BTW, just in case somebody is wondering, I'm gonna use 'ALU_TYPE' (not PRUint32)
for 'align'. 
jshin: duh, I was dreaming that the big array initializer was a parameter of the
macro.  Why not?  Less magic coupling and redundancy at each macro call site.

I had shown ALU_TYPE align; in comment 38 -- must I check everything twice? ;-)

/be

Aha, that's great. I was too preoccupied with the idea of '#includ'ing files to
realize that I can #define the initializer. With the initializer being defined
as a macro, I don't have to #include two files. Just one would work. Thanks !
The following two lines will do.

#include "punct.ccmap"
CCMAP_DEFINE(gPunctMap)

> I had shown ALU_TYPE align; in comment 38 -- must I check everything twice? ;-)

  Surely, you don't have to :-)  I added the note just in case you (or someone
else) may notice the macro (in my comment #46) using PRUint32 instead of
ALU_TYPE (despite the fact that you gave the correct macro in your comment #38)
and bring that into my attention later.
Attached patch a patch (obsolete) — — Splinter Review 
I had to add a parameter for 'constness' because I can't use 'const static' in
nsFontMetricsGTK.cpp.  'type' should be either 'const' or NULL(empty). If some
compilers don't like 'DEFINE_CCMAP(var, )', we have to define two separate
macros for const and non-const cases. 


#define DEFINE_CCMAP(var, type) 	    \
type static union {			    \
  PRUint16 array[var ## SIZE];		    \
  ALU_TYPE align;			    \
} var ## Union =			    \
{					    \
  { var ## INITIALIZER }		    \
};					    \
type static PRUint16* var = var ## Union.array;


Another potential problem is a big initializer that is several thousand bytes
long after the macro expansion. g++ 3.x and VC++ 6 didn't complain, but they're
usually on the more tolerant side. 

Greg and Mitch, why don't you apply the patch and see whether Forte compiler
has any trouble?
Attachment #137105 - Attachment is obsolete: true
Just tried with Jungshik's fix on Solaris with the forte compilers as used
perviously and all the dodgy URL's on this bug that would previously crash
the browser on misalignment errors work just fine.
Big initializers and big macro expansions should not be a problem.  C compilers
can handle 'em, have had to for decades.

Jshin, can you request reviews quickly so we can get this in for 1.6?

/be
Comment on attachment 137343 [details] [diff] [review]
a patch

Thanks for the confirmation, Mitch and Brendan.

BTW, an empty macro argument should be OK (as long as there's the first
argument that is not empty), right? 

Asking for r/sr. I don't mind getting a1.6 as well :-)
Attachment #137343 - Flags: superreview?(brendan)
Attachment #137343 - Flags: review?(smontagu)
Comment on attachment 137343 [details] [diff] [review]
a patch


>+#define DEFINE_CCMAP(var, type)             \
>+type static union {                         \
>+  PRUint16 array[var ## SIZE];              \
>+  ALU_TYPE align;                           \
>+} var ## Union =                            \
>+{                                           \
>+  { var ## INITIALIZER }                    \
>+};                                          \
>+type static PRUint16* var = var ## Union.array;
>+
>+#define DEFINE_X_CCMAP(var, type)                       \
>+type  static union {                                    \
>+  PRUint16 array[var ## SIZE];                          \
>+  ALU_TYPE align;                                       \
>+} var ## Union =                                        \
>+{                                                       \
>+  { var ## INITIALIZER }                                \
>+};                                                      \
>+type  static PRUint16* var = var ## Union.array +  CCMAP_EXTRA; 

I always factor macros, so it seems better to me to have DEFINE_CCMAP take
(varname, typequal, extra) as params: varname is var above, typequal is type
(better name, const is not a type, it's a type qualifier in C/C++), and extra
is 0 or CCMAP_EXTRA.  If you do that, then you don't need more macros, but if
you want conveniences that eliminate the extra parameter, you could call the
base macro DEFINE_ANY_CCMAP and call it from DEFINE_CCMAP and DEFINE_X_CCMAP
one-liners.

_SIZE and _INITIALIZER seem like better suffixes, althought it really doesn't
matter. The resulting macro name will have interCapsALLCAPS name style the way
you do it, and interCaps_ALLCAPS with this suggestions.  Matter of taste.

>+DEFINE_CCMAP(gDoubleByteSpecialCharsCCMap, )

Check the C standard.  This should be ok, but if not, just pass /* nothing */
instead of an empty argument.  May want to do that to be safe.

> # When CCMap is accessed, (PRUint16 *) is casted to 

Nit: "cast to", not "casted to".

Looks great otherwise, thanks for fixing this!

/be
Attachment #137343 - Flags: superreview?(brendan) → superreview+
Thanks for review and comment. I addressed all your concerns (including '_SIZE'
vs 'SIZE' because I like the former, too). I now have the following macros.
Somehow, VC++ 6 doesn't like an empty parameter in the middle (the macro
expansion gets screwed up, to my surprise) so that typequal had to go last. At
macro call sites, '/* nothing */' is used. 

#define DEFINE_ANY_CCMAP(var, extra, typequal)              \
typequal static union {                                     \
  PRUint16 array[var ## _SIZE];                             \
  ALU_TYPE align;                                           \
} var ## Union =                                            \
{                                                           \
  { var ## _INITIALIZER }                                   \
};                                                          \
typequal static PRUint16* var = var ## Union.array + extra; 

#define DEFINE_CCMAP(var, typequal)   DEFINE_ANY_CCMAP(var, 0, typequal)
#define DEFINE_X_CCMAP(var, typequal) DEFINE_ANY_CCMAP(var, CCMAP_EXTRA, typequal)

BTW, I replaced 
   DEFINE_CCMAP(gIgnorableCCMapExt, const)
with
   DEFINE_X_CCMAP(gIgnorableCCMapExt, const) 
  
in nsSaveAsCharset.cpp

#define DEFINE_ANY_CCMAP(var, extra, typequal)              \
typequal static union {                                     \
  PRUint16 array[var ## _SIZE];                             \
  ALU_TYPE align;                                           \
} var ## Union =                                            \
{                                                           \
  { var ## _INITIALIZER }                                   \
};                                                          \
typequal static PRUint16* var = var ## Union.array + extra; 

You don't want that terminating ; quoted above -- sorry if I missed it when
reviewing.  It will break if the macro is called with a ; after it, resulting in
an empty top-level declaration.  That's not supported by all compilers, and it
is not standard C AFAIK.

/be
Comment on attachment 137343 [details] [diff] [review]
a patch

>Index: intl/unicharutil/tools/ccmapbin.pl

>@@ -86,14 +95,23 @@ $e_pg_offset = 32;  

>+$variable = $comments{'VARIABLE'} if (! defined($variable));
this could be written:
$variable = $comments{'VARIABLE'} unless defined($variable);

Subject for another bug, would you consider changing
sub print_preamble

so that it could collect a license from the file it's parsing or from the
commandline?

I don't like the idea of having a mozilla.org critter which always spits an
MPL/whatever license on its output.
We don't have MPL licenses at the top of every web page we render.
Similarly, gcc doesn't license every app it produces under GPL. GCC, imo, does
the right thing in this respect, and I request that you consider changing
ccmapbin.pl to behave similarly.  In short, I should be able to write a ccmap
input file and license it under say the BSDL and have the output from
ccmapbin.pl not be MPL licensed.
Comment on attachment 137343 [details] [diff] [review]
a patch

If all you need from me is moa imprimatur, then please take this as r=smontagu,
but unfortunately I don't really have the technical knowledge for a proper
review, nor the free time at the moment.
I addressed all of brendan's concerns (including the terminating colon) and am
uploading it here because it'd be easier (later) for code archaelogy.

smontagu, I guess that's sorta what I want (technical issues are well covered
by brendan, et al, I think)

timeless, I share your concern, but that's a separate bug (it's not only
ccmapbin.pl but also a few  other scripts/C programs that do that. I just
followed what's done in others). Why don't you file one and add me to the CC
list.
Attachment #137343 - Attachment is obsolete: true
Comment on attachment 137426 [details] [diff] [review]
update (basically the same)  with all brendan's concerns addressed

Asking for 1.6a.

This patch is an update to attachment 137343 [details] [diff] [review] for which I've obtained brendan's
sr and smontagu's implicit r. 

What's fixed: misaligned access on Sparc (and possibly other RISC
architectures) that leads to crashes/slow execution.

Affected platform: all, but RISC architectures are more likely to be affected
(for the better)

Patch tested on Sparc(Solaris) and ix86(Linux, Win32). 

No change in the logical flow of the code, but just a wrapping PRUin16[] with a
union to ensure the proper alignment.
Attachment #137426 - Flags: approval1.6?
Comment on attachment 137426 [details] [diff] [review]
update (basically the same)  with all brendan's concerns addressed

I couldn't find any drivers around, but this is needed for ports, so get it in.

/be
Attachment #137426 - Flags: superreview+
Attachment #137426 - Flags: review+
Oops, I found dbaron.  I'll let him a=.

/be
Comment on attachment 137426 [details] [diff] [review]
update (basically the same)  with all brendan's concerns addressed

a=dbaron  (Is there a good reason that some of them are not |const|?  Also
|static const| is probably preferred over |const static|, just for
consistency...)
Attachment #137426 - Flags: approval1.6? → approval1.6+
thanks all. fix checked into the trunk with 'static const' in place
of 'const static'. 
In nsFontMetricsGTK.cpp, they can't be 'static const' because it has
to be 'mapped' to a non-const member variable of nsFontGTK??. 
When nsFontMetricsGTK.cpp is revised the way nsFontMetricsXlib.cpp
was, we could lift that restriction. In case of nsFontMetricsWin.cpp,
my memory is not clear but there was a reason. I'll take another look.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Attachment #137343 - Flags: review?(smontagu)
Blocks: 235913
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: