Closed Bug 218629 Opened 19 years ago Closed 8 years ago

XPTC_InvokeByIndex, x86 Linux, gcc, and stack alignment

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: tenthumbs, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Newer gcc versions go to a lot of trouble to preserve stack alignment.
XPTC_InvokeByIndex trashes this. GCC 3.3 defaults to 128-bit alignment.

To save time XPTC_InvokeByIndex doesn't calculate how much space it
needs but assumes the worst case of 8*paramCount. It also passes the
"that" variable so it reserves 8*n+4 stack space.

A big problem is that both 32- and 64-bit parameters are allowed,
apparently in any order. The code tightly packs the parameters so any
previous alignment is lost. To fix this would require redesigning the
xptcall interface. That's hard so let's ignore that problem here.

The question, then, is whether mozilla should try to align the stack
and, if so, how. There are three options:

1) Align the new parameter space (8*n) and let "that" be just 32-bit aligned.
The current code has a comment showing how this can be done. This leaves
the stack mis-aligned but may help with 64-bit parameters if there
actually are any.

2) Align 8*n+4. This is guaranteed to mis-align the parameter space but
maybe that's ok. I'll attach a patch for this.

3) Do nothing. We know this works. :-)

The hardest part may be to divine what a future gcc version will do.

Comments, anyone?
Attached patch v1 patch (obsolete) — Splinter Review
A test patch for discussion.
Attached patch v2 patchSplinter Review
A better patch for testing. You can try out the three strategies. It would be
nice if someone who knows how to time mozilla could see if any of this actually
makes a difference.
Attachment #131064 - Attachment is obsolete: true
Assignee: dougt → nobody
QA Contact: scc → xpcom
Looks like this has been fixed:

http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptcall/md/unix/xptcinvoke_gcc_x86_unix.cpp#67
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.