Fix inline asm so that we can drop a use of -fno-omit-frame-pointer

RESOLVED FIXED in mozilla9

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

Trunk
mozilla9
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 558217 [details] [diff] [review]
fix inline asm
Attachment #558217 - Flags: review?(khuey)
try at https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=d425c72891c7
and now on top of a working tree:

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=b1205a98406b
Attachment #558217 - Flags: review?(khuey) → review?(benjamin)
Component: Build Config → XPCOM
QA Contact: build-config → xpcom
Comment on attachment 558217 [details] [diff] [review]
fix inline asm

This doesn't mess up stack walking across xptcall invocations in breakpad, does it?
Attachment #558217 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Comment on attachment 558217 [details] [diff] [review]
> fix inline asm
> 
> This doesn't mess up stack walking across xptcall invocations in breakpad,
> does it?

It should not. Both the calls to this functions and the calls it makes are not affected, it is just the computation of the address of the function to call that no longer needs a stable esp.
Created attachment 559366 [details] [diff] [review]
new patch

After the previous patch failed on an opt build on the 10.5 bots I decided to take a deeper look at the problem.

It looks like xptcall has been patched up too much. The failing inline asm on 32 bit darwin in particular is very brittle. For example

* it has values live across calls, but the compiler can assign them to caller saved registers.
* registers used as temporaries are not marked as clobber.

I tried to solve these, but the inline assemble has so many inputs and outputs that one with all the constraints written down causes the compiler to runs out of registers with -fPIC -fno-omit-frame-pointer since it has no way of knowing where in the inline asm the values are live.

Given that the inline asm was already bigger than the rest of the function, I decided to just write the full thing in assembly. Fortunately, the version used by linux had already made this decision, so this patch

* Modifies the linux version so that both calls are aligned. And the produced can be used by the OS X assembler.
* Removes KEEP_STACK_16_BYTE_ALIGNED, since it is cheap to do it always and the linux implementation was already doing.
* Removes very old code like MOZ_PRESERVE_PIC and support for gcc 2.7.
* Define MOZ_NEED_LEADING_UNDERSCOR on OS X
* Move users of xptcinvoke_unixish_x86.cpp to xptcinvoke_gcc_x86_unix.cpp

A try run is at:

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=0164148e1713
Attachment #558217 - Attachment is obsolete: true
Attachment #559366 - Flags: review?(bzbarsky)
Attachment #559366 - Flags: feedback?(benjamin)
I'm not sure I can competently review this, at least in a short timeframe.  I'd need to learn more about calling conventions and ABI stuff first...

I'm also not sure whom else to suggest.  Benjamin, any ideas?
(In reply to Boris Zbarsky (:bz) from comment #6)
> I'm not sure I can competently review this, at least in a short timeframe. 
> I'd need to learn more about calling conventions and ABI stuff first...
> 
> I'm also not sure whom else to suggest.  Benjamin, any ideas?

Yes, sorry for the big patch. I can split up some of the cleanup only bits if you want. The changes to xptcinvoke_gcc_x86_unix.cpp can also go before another patch that switches away users of xptcinvoke_unixish_x86.cpp.
That part was actually ok (in that I assume I can ignore all the code being removed, etc).  My real problems are not having a good idea about what the calling conventions involved are and what ASM will do the right thing for us without a bit of reading up on things...
Created attachment 559479 [details] [diff] [review]
Just the bits for dropping old gcc

With this in the rest of the patch should be easier to read.
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #559479 - Flags: review?(benjamin)
Try with only the gcc bits:

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=e5c5410b56ac
Attachment #559479 - Flags: review?(benjamin) → review+
Attachment #559366 - Flags: review?(bzbarsky)
Attachment #559366 - Flags: feedback?(benjamin)
Created attachment 559635 [details] [diff] [review]
next cleanup round

This patch drops MOZ_PRESERVE_PIC which is not used since 2002, CFRONT_STYLE_THIS_ADJUST which was used only for really old systems like the first Freebsd 5.

With those out, the only thing left in xptc_platforms_unixish_x86.h is to set KEEP_STACK_16_BYTE_ALIGNED for QNX, since that is not supported and KEEP_STACK_16_BYTE_ALIGNED can be set from Makefile.in if needed, drop xptc_platforms_unixish_x86.h completely.

Try at:

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=42d93231b700
Attachment #559635 - Flags: review?(benjamin)
http://hg.mozilla.org/mozilla-central/rev/b08c94bd7f2e

I still see an open review request, so not sure what is going on here, I usually like one-landing per bug, so we can track this stuff better. Resolve if its supposed to be.
Attachment #559635 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/dad4626cde58
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Only the cleanup patches went in so far.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 560429 [details] [diff] [review]
final patch

Try at
https://hg.mozilla.org/try/raw-rev/c5fb00530583

What is left in this patch is:

* Modifies the linux version so that both calls are aligned and can be used by the OS X assembler.
* Removes KEEP_STACK_16_BYTE_ALIGNED, since it is cheap to do it always and the linux implementation was already doing.
* Define MOZ_NEED_LEADING_UNDERSCOR on OS X
* Move users of xptcinvoke_unixish_x86.cpp to xptcinvoke_gcc_x86_unix.cpp (and the stubs file too)

I can split this in two patches, but I don't think it would make review a lot easier. Let me know if you think it would.
Attachment #559366 - Attachment is obsolete: true
Attachment #560429 - Flags: review?(benjamin)
Attachment #560429 - Flags: review?(benjamin) → review+

Updated

6 years ago
Status: REOPENED → ASSIGNED
Whiteboard: [only cleanup patches landed, leave open for rest]
Oh, this isn't the cleanup patch but the last one.

Please in the future can you add notes to the whiteboard to make things easier for people merging. The use of the per patch checkin? and checkin+ flags might make things clearer too :-)

Also, when landing on inbound, please post the inbound URL and make the other changes as recommended by the recently updated https://wiki.mozilla.org/Inbound_Sheriff_Duty

Thanks :-)

https://hg.mozilla.org/mozilla-central/rev/11adc26523a4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [only cleanup patches landed, leave open for rest]
You need to log in before you can comment on or make changes to this bug.