201574 - Many Unaligned access messages when reading XUL.mfasl

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Jim Brown]

User-Agent:       Mozilla/5.0 (X11; U; OSF1 alpha; en-US; rv:1.4a) Gecko/20030409
Build Identifier: Mozilla/5.0 (X11; U; OSF1 alpha; en-US; rv:1.4a) Gecko/20030409

When reading the 'XUL.mfasl' file (on Tru64 V5.1B, cc, cxx) mozilla gets more
than 34600 'Unaligned access' messages on the console.
If I delete 'XUL.mfasl' then no messages are output and a new XUL.mfasl file is
created.
The next time I run mozilla I get all the unaligned access messages again.
This incorrect behavior happens every time an XUL.mfasl file is present.


Reproducible: Always

Steps to Reproduce:
1. remove any XUL.mfasl file
2. run mozilla once - starts clean, no messages
3. run mozilla second time - many unaligned access messages
4. remove XUL.mfasl
5. mozilla will start clean one time

Actual Results:  
many unaligned access messages on the console when XUL.mfasl is present.


Expected Results:  
no messages.


Here is traceback from mozilla while it is in the code section generating these
console messages.

(ladebug) status
#1 PC==0x12004c7d8 in int main(int, char**) "nsAppRunner.cpp":1517 { break }
Disabled
#2 PC==0x3ffbfe21f14 in nsresult WriteSegmentToString(class nsIInputStream*,
void*, const char*, PRUint32, PRUint32, PRUint32*) "nsBinaryStream.cpp":533
{ break }

...
Unaligned access pid=120740 <mozilla-bin> va=0x14030e47d pc=0x3ffbfe21f14
ra=0x3ffbfe21f2c inst=0xa0a50000
Unaligned access pid=120740 <mozilla-bin> va=0x14030e481 pc=0x3ffbfe21f14
ra=0x3ffbfe21f2c inst=0xa0a50000
...

(interrupt application while this is happening and enable a breakpoint
 at the problem PC location)

(ladebug) enable 2

(ladebug) c
[2] stopped at [nsresult WriteSegmentToString(class nsIInputStream*, void*,
const char*, PRUint32, PRUint32, PRUint32*):533 0x3ffbfe21f14]
    533         outString->Append(PRUnichar(NS_SWAP16(unicodeSegment[i])));

(ladebug) where
>0  0x3ffbfe21f14 in WriteSegmentToString(aStream=Info: symbol aStream is
defined but not allocated (optimized away)
<no value>, aClosure=0x11fffb658, aFromSegment=0x14030eb7f="",
aToOffset=Info: symbol aToOffset is defined but not allocated (optimized
away)
<no value>, aCount=26, aWriteCount=0x11fffb5f0) "nsBinaryStream.cpp":533
#1  0x3ffbfe216c4 in ReadSegmentForwardingThunk(aStream=Info: symbol aStream
is defined but not allocated (optimized away)
<no value>, aClosure=0x11fffb658, aFromSegment=0x14030eb7f="",
aToOffset=494, aCount=26, aWriteCount=0x11fffb5f0) "nsBinaryStream.cpp":312
#2  0x3ffbf6e9c50 in
((nsBufferedInputStream*)0x140313840)->nsBufferedInputStream::ReadSegments(write
r=0x3ffbfe216b0, closure=0x11fffb608, count=26, result=0x11fffb668)
"nsBufferedStreams.cpp":304
#3  0x3ffbfe21764 in
((nsBinaryInputStream*)0x11fffb760)->nsBinaryInputStream::ReadSegments(writer=0x
11fffb658, closure=0x14030eb7f, count=26, _retval=0x11fffb668)
"nsBinaryStream.cpp":324
#4  0x3ffbfe22074 in
((nsBinaryInputStream*)0x14031a300)->nsBinaryInputStream::ReadString(aString=Inf
o: no allocation applies for symbol aString at the current PC
<no value>) "nsBinaryStream.cpp":567
#5  0x3ffbec16420 in
((nsXULPrototypeDocument*)0x140630080)->nsXULPrototypeDocument::Read(aStream=0x1
4031a300) "nsXULPrototypeDocument.cpp":377
#6  0x3ffbec0c300 in
((nsXULPrototypeCache*)0x14016b000)->nsXULPrototypeCache::GetPrototype(aURI=0x14
0316d80, _result=0x1404d2a30) "nsXULPrototypeCache.cpp":271
#7  0x3ffbebfbf80 in
((nsXULDocument*)0x1404d2600)->nsXULDocument::ResumeWalk()
"nsXULDocument.cpp":3145
#8  0x3ffbdd78298 in ((nsDocumentOpenInfo*)Info: no allocation applies for
symbol this at the current PC
<bad value>)->nsDocumentOpenInfo::OnStopRequest(request=0x140320be0,
aCtxt=0x0, aStatus=0) "nsURILoader.cpp":255
#9  0x3000904e834 in nsCachedChromeChannel::HandleStopLoadEvent(aEvent=Info:
no allocation applies for symbol aEvent at the current PC
<no value>) "nsChromeProtocolHandler.cpp":470
#10 0x3ffbfe8f7e8 in PL_HandleEvent(self=0x14049ad20) "plevent.c":659
#11 0x3ffbfe8f5c8 in PL_ProcessPendingEvents(self=0x14007f340)
"plevent.c":592
#12 0x3ffbfe941c8 in
((nsEventQueueImpl*)0x14007ef30)->nsEventQueueImpl::ProcessPendingEvents()
"nsEventQueue.cpp":387
#13 0x3ffbee00ea4 in event_processor_callback(data=0x11fffb760,
source=536852056, condition=5371915135) "nsAppShell.cpp":194
#14 0x3ffbee006f0 in our_gdk_io_invoke(source=Info: no allocation applies
for symbol source at the current PC
<no value>, condition=Info: no allocation applies for symbol condition at
the current PC
<no value>, data=Info: no allocation applies for symbol data at the current
PC
<no value>) "nsAppShell.cpp":72
#15 0x30002815520 in UnknownProcedure3FromFile9(...) in
/usr/local/lib/libglib-1.2.so
#16 0x300028177b4 in UnknownProcedure12FromFile11(...) in
/usr/local/lib/libglib-1.2.so
#17 0x3000281800c in UnknownProcedure13FromFile11(...) in
/usr/local/lib/libglib-1.2.so
#18 0x30002818240 in g_main_run(...) in /usr/local/lib/libglib-1.2.so
#19 0x300010d7760 in gtk_main(...) in /usr/local/lib/libgtk-1.2.so
#20 0x3ffbee01834 in ((nsAppShell*)0x1401de570)->nsAppShell::Run()
"nsAppShell.cpp":334
#21 0x3ffbd747670 in
((nsAppShellService*)0x11fffb760)->nsAppShellService::Run()
"nsAppShellService.cpp":479
#22 0x12004bb98 in main1(argc=1, argv=0x11fffc018, nativeApp=Info: no
allocation applies for symbol nativeApp at the current PC
<no value>) "nsAppRunner.cpp":1271
#23 0x12004ca04 in main(argc=1, argv=0x11fffc018) "nsAppRunner.cpp":1642
#24 0x1200372e8 in __start(...) in ./mozilla-bin

Comment 1

[hacker formerly known as [e-mail]]

What source tree are you building with?  I thought the fastload corruption
problem had been fixed post 1.3.

Comment 2

[Jim Brown]

I am building from the mozilla-source-1.4a.tar.gz tarball.
I don't think this is fastload corruption - I think this is an Alpha architecture 
issue.  OSF1 produces this message whenever a word or longword 
access to a lesser alignment (e.g. byte aligned address) happens.

Comment 3

[Jim Brown]

I am pretty sure the unaligned accesses are being caused by the size-of and
alignment-of the binary data being read from the fast load file.
mozilla will (eventually) read in the fast load file and work (no corruption).
This problem is solely in the way mozilla is reading the data from the fast load
file.

When the fast load file is written are the data fields 'packed' into it from
their native structures?

Comment 4

[Jim Brown]

WriteSegmentToString in nsBinaryStream.cpp line No. 533
  outString->Append(PRUnichar(NS_SWAP16(unicodeSegment[i])));
uses macro NS_SWAP16 on the unicode string character unicodeSegment[i].

For little endian machines (Alpha) this macro (and also NS_SWAP32)
assume their argument is a 16-bit (32-bit) [aligned] integer.
unicodeSegment is just aFromSegment and is NOT word aligned (0x14030182f).

Either aFromSegment (or unicodeSegment) needs to be copied to be word aligned
or a different macro should be used here which can handle byte aligned string data.

Comment 5

[Jim Brown]

dbm/include/mcom_db.h has a macro - P_16_COPY(a,b) which would work.
Something like this instead of NS_SWAP16 would solve the problem.
Perhaps something like this in xpcom/io/nsBinaryStream.cpp) for little endian
case (define own version of P_16_COPY?):

  PRUnichar unicodeChar;
...
  for (PRUint32 i = 0; i < segmentLength; i++) {
    P_16_COPY(unicodeSegment[i], unicodeChar;);
    outString->Append(unicodeChar;);
  }

Comment 6

[Jim Brown]

I think this is a better way to fix this (and stay with the NS_SWAP16 macro).
This has to be done in two places in xpcom/io/nsBinaryStream.cpp: lines 194 and 533.

for the lines near 194:
  PRUnichar unicodeChar;
...
  for (PRUint32 i = 0; i < length; i++) {
    unicodeChar = aString[i];
    copy[i] = NS_SWAP16(unicodeChar);
  }

for the lines near 533:
  PRUnichar unicodeChar;
...
  for (PRUint32 i = 0; i < segmentLength; i++) {
    unicodeChar = unicodeSegment[i];
    outString->Append(NS_SWAP16(unicodeChar));
  }

I have made changes like this to my source and will see what happens.

Comment 7

[Jim Brown]

Attachment: v1.0, resolves problem

It seems fast enough on my test system (i.e., not noticeably slow).

Comment 8

[Darin Fisher]

Comment on attachment 120234 [details] [diff] [review]
v1.0, resolves problem

>+++ xpcom/io/nsBinaryStream.cpp	2003-04-11 16:44:03.000000000 -0400

>+    // NS_SWAPxx macros expect aligned data
>+    for (PRUint32 i = 0; i < length; i++) {
>+        memcpy(&unicodeChar, &aString[i], sizeof(PRUnichar));
>+        copy[i] = NS_SWAP16(unicodeChar);
>+    }

is memcpy really required?  why not simple assignment... or does that
also result in unaligned memory access warnings?

    unicodeChar = aString[i];
    copy[i] = NS_SWAP16(unicodeChar);

Comment 9

[Jim Brown]

> is memcpy really required?  why not simple assignment...

This generates the same warnings -- I tried this first.
It probably fails because the underlying type of PRUnichar is really some kind
of int rather than a wide character (the compiler is still treating it as an int
for alignment purposes).

The other way to fix this would be a macro or inline function which would use
the address of the input 'character' to access the two bytes and either return
them as a PRUnichar value or go ahead and switch them so the NS_SWAP16 is
unnecessary.
memcpy was simpler to test that these other possibilities.

Comment 10

[Darin Fisher]

some more thoughts about this bug after talking with waldemar...

if we are seeing an unaligned memory access for a PRUnichar array, then it means
the array is aligned on an odd byte boundary.  if so, it means a performance hit
on all machines.  some machines, notably intel, are very forgiving of this, and
simply don't warn you about the slowdown.  this bug is probably a blessing in
disguise.  i'm going to add some assertions to my linux build and try to repro
this misalignment.  we should try to ensure that the input array is aligned
correctly.

Comment 11

[Darin Fisher]

yup, sure enough WriteSegmentToString gets called with aFromSegment equal to an
odd valued address!!  the patch in this bug is just a workaround to hide the
real problem.

Comment 12

[Darin Fisher]

changing hardware:os to all and adding perf keyword.

Comment 13

[Darin Fisher]

Attachment: v2 patch

revised patch to do the following:

1- copy input string to destination buffer first
2- then byte swap destination buffer
3- shouldn't need to worry about byte-alignment of WriteWStringZ input since it
should already be properly aligned.  i added an assertion just in case though.

this patch does away with all the nsAString::Append calls in the ReadSegments
callback.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 120253 [details] [diff] [review]
v2 patch

Whaddya think of my patch for the same prob (or did I miss some cases?) in
alecf's bug 184204 ?

/be

Comment 15

[Darin Fisher]

brendan: no, i think the two patches are functionally equivalent.  however, this
patch is simpler and more efficient because of the fact that it does not have to
deal with single calls to Append.  by calling SetLength up front, i can call
BeginWriting to get direct access to the output buffer.  as a result, i can 
memcpy directly to the output buffer, and then byte swap if necessary.  (also,
the Truncate call before SetLength might be unnecessary.)

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 120253 [details] [diff] [review]
v2 patch

Darin, I like.	Can you dup the other bug against this one?

>+    NS_ASSERTION((PRUint32(aString) & 0x1 == 0), "aString not properly aligned");

Nit: gratuitous over-parenthesization of the outer expression passed to
NS_ASSERTION.

Non-nit (bug!): & has *lower* precedence than == in C (old botch in the
language), you must parenthesize the & expression here.

>+    for (PRUint32 i = 0; i < segmentLength; ++i, ++cursor)
>+        *cursor = (PRUnichar) NS_SWAP16(*cursor);

For faster loop control, count i down from segmentLength to 0.

Even better (smaller code, just as fast), compute a fencepost for cursor and
loop till cursor reaches that limit pointer.

>     aString.Truncate();
>+    aString.SetLength(length);

So yeah, is the Truncate needed?

I'll gladly sr= a patch that addresses these points.

/be

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Darin Fisher]

Attachment: v2.1 patch

thanks brendan!  here's the revised patch.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 120323 [details] [diff] [review]
v2.1 patch


>+    NS_ASSERTION((PRUint32(aString) & 0x1) == 0, "aString not properly aligned");

One final nit: use PRPtrdiff (or PRUptrdiff, doesn't matter) instead of
PRUint32 instead to avoid loss-of-precision or similar warnings on
64-bit-pointer platforms?

sr=[e-mail].

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 120323 [details] [diff] [review]
v2.1 patch

>+    const PRUnichar *end = cursor + segmentLength;
>+    for (; cursor != end; ++cursor)
>+        *cursor = (PRUnichar) NS_SWAP16(*cursor);

Uber-nit, because if aCount or the variable it induces, segmentLength, were
negative (in a PRUint32), memcpy and other things would have blown up already:
use cursor < end to promulgate "number-line style", whereby if some other loop,
with no prior memcpy to blow up on a negative aCount, imitates that style, the
loop doesn't iterate for a very long time.

/be

Comment 21

[Darin Fisher]

Attachment: v2.2 patch

fixed up brendan's nits, and fixed a bug (forgot to update mWriteCursor at the
end of WriteSegmentToString, which would have been very bad if a string spanned
more than one segment!)

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 120399 [details] [diff] [review]
v2.2 patch

Oops, sorry for missing that failure to update mWriteCursor.  Thanks for
catching that!

/be

Comment 23

[Alec Flett]

Comment on attachment 120399 [details] [diff] [review]
v2.2 patch

this looks good. sr=alecf

Comment 24

[Darin Fisher]

fixed-on-trunk