Last Comment Bug 684659 - Fix inline asm so that we can drop a use of -fno-omit-frame-pointer
: Fix inline asm so that we can drop a use of -fno-omit-frame-pointer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla9
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-04 21:17 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-09-17 02:13 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix inline asm (3.10 KB, patch)
2011-09-04 21:17 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
benjamin: review+
Details | Diff | Splinter Review
new patch (57.22 KB, patch)
2011-09-08 19:29 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review
Just the bits for dropping old gcc (46.20 KB, patch)
2011-09-09 08:48 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
benjamin: review+
Details | Diff | Splinter Review
next cleanup round (14.66 KB, patch)
2011-09-09 17:27 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
benjamin: review+
Details | Diff | Splinter Review
final patch (15.90 KB, patch)
2011-09-15 12:10 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
benjamin: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-04 21:17:48 PDT
Created attachment 558217 [details] [diff] [review]
fix inline asm
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-04 21:18:19 PDT
try at https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=d425c72891c7
Comment 2 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-04 22:31:17 PDT
and now on top of a working tree:

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=b1205a98406b
Comment 3 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-09-07 10:47:56 PDT
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) 2011-09-07 12:36:38 PDT
(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) 2011-09-08 19:29:17 PDT
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
Comment 6 Boris Zbarsky [:bz] 2011-09-08 19:36:30 PDT
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) 2011-09-08 20:02:51 PDT
(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 [:bz] 2011-09-08 20:05:08 PDT
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) 2011-09-09 08:48:44 PDT
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.
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-09 08:50:29 PDT
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) 2011-09-09 17:27:52 PDT
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
Comment 12 Justin Wood (:Callek) 2011-09-09 22:49:07 PDT
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 14 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-15 11:20:09 PDT
Only the cleanup patches went in so far.
Comment 15 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-09-15 12:10:19 PDT
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.
Comment 16 Ed Morley [:emorley] 2011-09-17 02:13:59 PDT
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

Note You need to log in before you can comment on or make changes to this bug.