Closed Bug 312929 Opened 19 years ago Closed 19 years ago

Firefox does not launch on latest Intel Mac OS X build (8F1099)

Categories

(Core :: XPCOM, defect)

PowerPC
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

(Keywords: fixed1.8)

Attachments

(2 files)

Firefox does not launch on latest Intel Mac OS X build (8F1099).
Attached file crash log
> Exception:  EXC_BAD_INSTRUCTION (0x0002)

Probably misaligned stack. From the seed notes for 8F1099:

"The 16-byte stack alignment requirement in the Intel ABI is now
aggressively enforced. Applications which crash with an illegal
instruction exception are probably misaligning the stack. This should
only affect assembly code; all code built with GCC should automatically
align the stack correctly."
We have hit this problem in my group at Apple, this sounds like some assembly
somewhere, NSPR? is not 16-byte aligned.

Someone in my group took a look at this stack and suggested that
frames 1 and 14 of Thead 0 looks like a good place to start looking.

I can give more details in email and/or hook people up with OS people,
send me mail.
Depends on: 297326
If anyone else who cares is as uneducated on this subject as I am, you can visit
http://tinyurl.com/965zr for an idea of what is going on.
bsmedberg: We use "xptcinvoke_unixish_x86.cpp" not "xptcinvoke_gcc_x86_unix.cpp".

McAfee: Almost assuredly the problem is in "xptcinvoke_unixish_x86.cpp". We are
short on people who know x86 assembly, and if you know someone who could take a
look that would be great. See 297326 for what we had to do for the first Mac OS
X for Intel builds.
Severity: major → critical
I have a few people to ask, just sent mail.
Firefox wasn't aligning the stack inside nsXPTCStubBase::Stub5.  I've attached diffs against mozilla TOT 
which do this.  With this change in place I can launch DeerPark and browse the web a bit.  I eventually 
hit a crash inside some plugin code that isn't due to a misaligned stack, so I'm guessing that's a 
separate problem in TOT.

For what it's worth, I'm not sure why nsXPTCStubBase::Stub##n has inline asm at all.  As far as I can tell, 
the function could be written like this:
  return PrepareAndDispatch(firstArg, n, &secondArg);

You can't quite do that today because the stubs are defined as taking no arguments in 
xptcstubsdecl.inc, but that's clearly wrong.  They obviously do take arguments, and if they were 
declared like that this code would be a lot more maintainable.
(In reply to comment #8)
> You can't quite do that today because the stubs are defined as taking no
arguments in 
> xptcstubsdecl.inc, but that's clearly wrong.  They obviously do take
arguments, and if they were 
> declared like that this code would be a lot more maintainable.

The problem is that they take variable numbers of arguments using normal (not
varargs) calling conventions, and the numbers and types are determined by
information contained in data structures that the callee has access to.
With Eric's patch, Firefox is up and running again. Thanks!
Attachment #200322 - Flags: superreview?(dbaron)
Attachment #200322 - Flags: review?(dougt)
Comment on attachment 200322 [details] [diff] [review]
Align the stack inside nsXPTCStubBase::Stub##n

>+#define ALIGN_STACK	\
>+    "andl $0xfffffff0, %%esp\n\t" \
>+    "subl $0x4, %%esp\n\t"

You could put an 

"addl $0x4, %%esp\n\t" \

in as the first line of this macro to avoid wasting 16 bytes in one of the 4
possible cases (although we probably never hit that one; I suspect there's no
room for variation and if we hit that one nobody would have filed the bug)

With or without that, sr=dbaron.
Attachment #200322 - Flags: superreview?(dbaron) → superreview+
Attachment #200322 - Flags: review?(dougt) → review+
Comment on attachment 200322 [details] [diff] [review]
Align the stack inside nsXPTCStubBase::Stub##n

Requesting 1.8 approval. Consider carefully - while we don't need this fix for
FF 1.5, we will need it on the Mozilla 1.8 branch within the next 2-3 months.
It is absolutely necessary in order to get FF running on Intel Macs.

So, we either take the risk now or in a later point release. Perhaps get some
guidance on the risk factor from dbaron.
Attachment #200322 - Flags: approval1.8rc1?
The diff to me looks like MACOSX/i386 only, we could test this with
other OS's that use the xptcstubs_unixish_x86.cpp file to build, hmm
looks like linux/i386 does?
Landed on trunk with dbaron's suggestion from comment #11 (it seems to work
fine). Forgot to give Eric credit in the cvs commit credit. Won't forget on
branch landing whenever that happens. Thanks Eric!
(In reply to comment #9)
> The problem is that they take variable numbers of arguments using normal (not
> varargs) calling conventions, and the numbers and types are determined by
> information contained in data structures that the callee has access to.

The weird thing about that, though, is that the stubs don't seem to pass those
extra arguments on to the function they're calling or use those extra arguments.
Maybe that's something specific to the x86 implementation -- I haven't looked
at any of the others -- but PrepareAndDispatch only takes three arguments.  If
we pass fewer than two arguments it'll be passed junk, and if we pass more than
that they'll get ignored.  Because we're calling PrepareAndDispatch rather than
jumping to it, it doesn't really have a way to access extra arguments unless it's
both written in assembly and doing some very nasty stuff.

Or maybe I'm missing something very important here. :)  Which wouldn't be
surprising considering that I hadn't looked at this code before last night....
Magic!

or...

The stub doesn't pass arguments to PrepareAndDispatch in accordance with the
standard calling convention, it passes a pointer to the base of the arguments in
memory.  The args argument that PrepareAndDispatch receives points to the
arguments that the stub received in its own stack frame.  methodIndex, the same
as the stub number, informs PrepareAndDispatch of the quantity and types of the
arguments.

Other architectures work similarly, although the calling conventions are of
course different.
*** Bug 313398 has been marked as a duplicate of this bug. ***
Attachment #200322 - Flags: approval1.8rc1? → approval1.8rc1+
Landed on branch. Thanks Eric!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: