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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: fix inline asm

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

try at https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=d425c72891c7

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

and now on top of a working tree:

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=b1205a98406b

Comment 3

[Benjamin Smedberg]

Comment on attachment 558217 [details] [diff] [review]
fix inline asm

This doesn't mess up stack walking across xptcall invocations in breakpad, does it?

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(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.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: 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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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...

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Just the bits for dropping old gcc

With this in the rest of the patch should be easier to read.

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Try with only the gcc bits:

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=e5c5410b56ac

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: 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

Comment 12

[Justin Wood (:Callek)]

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.

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/dad4626cde58

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Only the cleanup patches went in so far.

Comment 15

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: 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.

Comment 16

[Ed Morley [:emorley]]

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