85890 - Unaligned accesses

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[jim_nance]

When running under Tru64 Unix, I get a lot of unaligned access messages, which
means that something is accessing data through pointers which are not aligned.
A have poked around in the debugger an it is not obvious to me what pointer
is having problems.  They all happen on the following line though:

(ladebug) where 5
>0  0x3000442565c in
((nsRuleNode*)0x14055e070)->nsRuleNode::ComputeTextResetData(aStartData=0x0,
aTextData=& struct nsCSSText { ... }, aContext=0x14055e14c,
aHighestNode=0x14055e020, aInherited=0)
"/scratch/jlnance/mozilla/content/html/style/src/nsRuleNode.cpp":1725
#1  0x30004423528 in
((nsRuleNode*)0x140559350)->nsRuleNode::ComputeStyleData(aSID=<bad reference>,
aStartStruct=0x14055e000, aStartData=<bad reference>, aContext=0x0,
aHighestNode=0x1404ff800, aRuleDetail=& 1079370060, aInherited=1079370156)
"/scratch/jlnance/mozilla/content/html/style/src/nsRuleNode.cpp":1113
#2  0x30004422890 in ((nsRuleNode*)0x14055e070)->nsRuleNode::WalkRuleTree(aSID=&
eStyleStruct_TextReset, aContext=0x14055e0d8, aRuleData=0x11fffe4b0,
aSpecificData=0x11fffe538)
"/scratch/jlnance/mozilla/content/html/style/src/nsRuleNode.cpp":912
#3  0x30004421370 in
((nsRuleNode*)0x14055e070)->nsRuleNode::GetTextResetData(aContext=0x14055e0d8)
"/scratch/jlnance/mozilla/content/html/style/src/nsRuleNode.cpp":603
#4  0x3000442e858 in
((nsRuleNode*)0x14055e1ac)->nsRuleNode::GetStyleData(aSID=eStyleStruct_TextReset,
aContext=0x14055e000)
"/scratch/jlnance/mozilla/content/html/style/src/nsRuleNode.cpp":4518
(ladebug) w
   1720     // style context.
   1721     aContext->SetStyle(eStyleStruct_TextReset, *text);
   1722   else {
   1723     // We were fully specified and can therefore be cached right on the
rule node.
   1724     if (!aHighestNode->mStyleData.mResetData)
>  1725       aHighestNode->mStyleData.mResetData = new (mPresContext)
nsResetStyleData;
   1726     aHighestNode->mStyleData.mResetData->mTextData = text;
   1727     // Propagate the bit down.
   1728     PropagateInheritBit(NS_STYLE_INHERIT_TEXT_RESET, aHighestNode);
   1729   }

aHighestNode appears to be fine, so whatever is happening must be down in
operator new.

Comment 1

[jim_nance]

I found another one.  It looks like someone is handing out memory thats only
aligned on 4 byte boundaries.  I dont know who it is though:

Unaligned access pid=6916 <mozilla-bin> va=0x14055e1b4 pc=0x3ffbff59170
ra=0x300046cbdac inst=0xb4490000
Thread stopped on Unaligned Access
stopped at [nsString::nsString(void):73 0x3ffbff59174]
     73 nsString::nsString() {
(ladebug) p this
0x14055e1b4
(ladebug) where 5
>0  0x3ffbff59174 in ((nsString*)0x14055e1b4)->nsString::nsString()
"/scratch/jlnance/mozilla/string/obsolete/nsString2.cpp":73
#1  0x300046cbda8 in
((nsStyleDisplay*)0x14055e1ac)->nsStyleDisplay::nsStyleDisplay()
"/scratch/jlnance/mozilla/content/shared/src/nsStyleStruct.cpp":838
#2  0x30004426338 in
((nsRuleNode*)0x14055e070)->nsRuleNode::ComputeDisplayData(aStartDisplay=0x0,
aDisplayData=& struct nsCSSDisplay { ... }, aContext=0x14055e0d8,
aHighestNode=0x14055e070, aInherited=0)
"/scratch/jlnance/mozilla/content/html/style/src/nsRuleNode.cpp":1941
#3  0x30004423348 in
((nsRuleNode*)0x14055e1b4)->nsRuleNode::ComputeStyleData(aSID=& 1951626094,
aStartStruct=0x14055e000, aStartData=& struct nsCSSStruct { ... },
aContext=0x14055e070, aHighestNode=0x11fffe420, aRuleDetail=& 1079370156,
aInherited=-306656)
"/scratch/jlnance/mozilla/content/html/style/src/nsRuleNode.cpp":1068
#4  0x30004422890 in ((nsRuleNode*)0x14055e070)->nsRuleNode::WalkRuleTree(aSID=&
eStyleStruct_Display, aContext=0x14055e0d8, aRuleData=0x11fffe4c0,
aSpecificData=0x11fffe548)
"/scratch/jlnance/mozilla/content/html/style/src/nsRuleNode.cpp":912

Note that the this pointers of frame 0 & 1 are both aligned to 4 but not 8
bytes.  Pointers under Tru64 must be 8 byte aligned.

Comment 2

[jim_nance]

BTW, this problem was not present in the 0.9.1 release, so it has been
introduced since then.

Comment 3

[jim_nance]

I found the problem.  In the arena code in nsPresShell.cpp we are rounding to 4
byte boundaries.  If I round to 8 it fixes the problem.  If someone is really
worried about memory we could probably get away with rounding to sizeof(void*)
which would go to 8 on 64 bit platforms where this is likely to be a problem. 
However we round to 8 at other points in the code already, and I dont think this
is going to bloat memory usage.  I have a patch I will attach in a minute.  I
would like to get this in for the 0.9.2 branch.  Is that possible?

Comment 4

[jim_nance]

Attachment: Patch to fix unaligned memory problems

Comment 5

[Alec Flett]

does this effectively double the size of the space being used to store the
pointers in the arena? 
Seems like we should do a PR_ROUNDUP(aSize, sizeof(void*))

Comment 6

[jim_nance]

Yes, sizeof(void*) is probably better.  I was concerned about there being
plaforms which wanted doubles aligned on 8 byte boundaries, but I guess we will
worry about that when/if it becomes a problem.

I dont think bloat is a big problem unless you are allocating a lot of 4 byte
objects one at a time.  I do not think we do this.

Comment 7

[chris hofmann]

pierre is on sabatical and has been for all of 0.9.1...  wonder who the right
owner for this should be...   style sheet rule matching????

Comment 8

[Alec Flett]

hyatt wrote the frame arena stuff
I'll rs=alecf the sizeof(void*) change though

Comment 9

[jim_nance]

Attachment: Patch using sizeof(void*)

Comment 10

[Alec Flett]

sr=alecf

Comment 11

[Christopher Blizzard (:blizzard)]

This is marked as critical for 0.9.2.  What's the status of this?

Comment 12

[Marc Attinasi]

Pierre is still out - we need someone else to check this in. Let me know if you
want me to do it.

Comment 13

[Christopher Blizzard (:blizzard)]

Please do.  a=blizzard on behalf of drivers for 0.9.2

Comment 14

[Marc Attinasi]

Checked into branch... will get to trunk next.

Comment 15

[John Morrison]

Hmm. This was also being pursued as bug 84165 (I just realized). That bug 
has a different patch for this. Don't know if one is better than the other.

Comment 16

[Marc Attinasi]

OK, in the trunk bucket too now. Marking closed - 

thanks, Jim, for the patch!

Comment 17

[Marc Attinasi]

Uh, did I say FIXED?

Comment 18

[Alec Flett]

good catch john - I actually think the other patch is better..I'm not sure if
this one is going to work, after seeing the other patch, actually :)

Comment 19

[Jim Dunn]

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