Last Comment Bug 567135 - Need CFI for xptcstubs_arm
: Need CFI for xptcstubs_arm
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: ARM Linux
: -- normal (vote)
: mozilla12
Assigned To: Nathan Froyd [:froydnj]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-20 09:52 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2011-12-24 22:17 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
adding unwind directives for xptcstubs_arm.cpp (1.31 KB, patch)
2011-10-19 09:31 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
add CFI directives (2.66 KB, patch)
2011-10-20 12:31 PDT, Nathan Froyd [:froydnj]
jimb: review+
Details | Diff | Splinter Review
xptcstubs_arm.s file (262.64 KB, text/plain)
2011-10-20 12:32 PDT, Nathan Froyd [:froydnj]
no flags Details
xptcstubs_arm.o file (54.32 KB, application/octet-stream)
2011-10-20 12:34 PDT, Nathan Froyd [:froydnj]
no flags Details

Description Ted Mielczarek [:ted.mielczarek] 2010-05-20 09:52:38 PDT
This is a crash report from a trunk Fennec Maemo build:
http://crash-stats.mozilla.com/report/index/0ab93368-b95a-4717-86b3-2e7cb2100519

Note that the stack ends after PrepareAndDispatch, because that's called from an XPTC stub function, which are hand-coded ASM:
http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp#243

If we want our stacks on ARM to get past XPTcall, we'll need to add hand-coded CFI inside the ASM.
Comment 1 Jim Blandy :jimb 2010-05-20 10:12:19 PDT
Could you pick any reasonably big C++ file, compile for ARM with -save-temps added, and attach the .s file to the bug? I want some cfi directives to crib from; I trust the assembler's documentation not much.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2010-05-20 10:34:09 PDT
A bit too big to attach, I think, but:
http://people.mozilla.com/~tmielczarek/jsinterp.s.gz

is built from:
http://hg.mozilla.org/mozilla-central/file/39544b4cb578/js/src/jsinterp.cpp
Comment 3 Jim Blandy :jimb 2010-05-20 11:37:41 PDT
It looks like GCC 4.0 was released 2005-4, and the GAS CFI directive support has been there since 2003 (with the .eh_frame stuff being added later; but I don't need that).  So it should be safe to use the assembler CFI directives in that code.
Comment 4 Nathan Froyd [:froydnj] 2011-10-19 09:31:19 PDT
Created attachment 568084 [details] [diff] [review]
adding unwind directives for xptcstubs_arm.cpp

Here's a patch which I believe gets the job done.  We just need a few directives in the shared stub to let the unwinder know what we're doing there.  We don't need directives in the individual stubs because they don't do any unwind-relevant work.

ARM uses a custom unwind format, so DWARF directives are not used.  The directives are documented at http://sourceware.org/binutils/docs/as/ARM-Directives.html and a brief tutorial on their use can be found at http://sourceware.org/binutils/docs/as/ARM-Unwinding-Tutorial.html .  I believe said directives have been supported for some time, so I haven't added configury checks.

I haven't added directives for the ARM BSD targets.

Will push to try to make sure it's OK; it compiles fine with an arm-linux cross compiler, but there might be android funniness lurking.

Nominating bsmedberg for review since his name is prominent in hg log.  Taking bug from jimb; hopefully he does not mind.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2011-10-19 11:07:45 PDT
I'm not sure this will fix the problem. Breakpad's dump_syms doesn't know how to parse the ARM unwind tables, only CFI. (I know this because I have a patch to implement ARM unwind support, which is not going to land upstream in Breakpad.)

You could test by doing a local Android (or ARM) build, and then running dist/host/bin/dump_syms toolkit/library/libxul.so, looking for the FUNC line for one of the stub functions, and then looking for a STACK CFI line matching the same address.
Comment 6 Nathan Froyd [:froydnj] 2011-10-19 12:20:32 PDT
OK, then; that's a little weird.  I don't see what difference Breakpad's ignorance makes, since the toolchain won't generate CFI information for anything else in the binary.  (Checked with an arm-eabi and android's ndk (prebuilt) toolchain...but I could definitely be missing something.)

Anyway, it's easy enough to hack an ARM toolchain to generate DWARF instead.  Will stare at that for a bit and return with something that does CFI bits.  Is it worth keeping the ARM unwind table bits?
Comment 7 Ted Mielczarek [:ted.mielczarek] 2011-10-19 12:23:20 PDT
We certainly get debug info sections when we build with -g with the Android toolchain, which contains CFI.
Comment 8 Jim Blandy :jimb 2011-10-19 12:51:36 PDT
Nathan, what happens if you simply use the .cfi directives in the ARM assembly?
Comment 9 Nathan Froyd [:froydnj] 2011-10-20 12:31:21 PDT
Created attachment 568471 [details] [diff] [review]
add CFI directives

Here's another attempt.  It's a little noisy, mostly because SharedStub needed to be rewritten for proper conditional compilation.

I am explicitly not doing what bug 645111 did for the definition of the CFI macro.  __GCC_HAVE_DWARF2_CFI_ASM is very new (GCC 4.6+), so using it would lose on older toolchains.  Similarly, I'm not playing games with .cfi_sections; that's a rather new directive and Breakpad will happily fetch information out of .eh_frame.

Will upload examples of DWARF CFI generated by GCC for this file for comparison's sake.
Comment 10 Nathan Froyd [:froydnj] 2011-10-20 12:32:50 PDT
Created attachment 568473 [details]
xptcstubs_arm.s file

The .s file generated by compiling the newly-patched xptcstubs_arm.cpp.
Comment 11 Nathan Froyd [:froydnj] 2011-10-20 12:34:23 PDT
Created attachment 568476 [details]
xptcstubs_arm.o file

The .o file you get from compiling the previously-attached .s file.  .debug_frame and .eh_frame both contain the information generated by our .cfi_* directives, and there's unwind information for a couple of other functions for comparison's sake.
Comment 12 Ted Mielczarek [:ted.mielczarek] 2011-10-21 11:51:47 PDT
How did you get this to build at all with Apple's iOS toolchain? I have a patch on bug 681602, Apple as was choking on that file before that.
Comment 13 Nathan Froyd [:froydnj] 2011-10-21 11:56:01 PDT
I didn't.  I cargo-culted from your patch on that bug.  Jim had said bug 645111 needed the CFI macro games because Apple's x86oid assembler didn't support .cfi_* either.
Comment 14 Nathan Froyd [:froydnj] 2011-10-21 11:57:25 PDT
Of course, it would be interesting to know if the Apple ARM as supported the ARM unwind directives...or if people have a strong aversion to having them in the first place.
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-10-21 12:11:20 PDT
Ah. I am not a good person to ask, my assembly knowledge is pretty poor, despite mucking around in there like I own the place.
Comment 16 Jim Blandy :jimb 2011-10-21 16:47:58 PDT
Comment on attachment 568471 [details] [diff] [review]
add CFI directives

Review of attachment 568471 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp
@@ +67,5 @@
>  #define DOUBLEWORD_ALIGN(p) (p)
>  #endif
>  
> +// Apple's iOS toolchain is lame and does not support .cfi directives.
> +#ifdef __APPLE__

Why aren't you testing __GCC_HAVE_DWARF2_CFI_ASM here, instead of __APPLE__?

@@ +175,5 @@
> +         ".text\n"
> +         ".align 2\n"
> +         "SharedStub:\n"
> +         ".fnstart\n"
> +         CFI(".cfi_startproc\n")

I think you need an initial CFA rule here, perhaps ".cfi_def_cfa sp, 0"? The DWARF spec says that all rules are initially undefined, although ABIs can specify particular initial rules.
Comment 17 Jim Blandy :jimb 2011-10-21 16:50:55 PDT
(In reply to Jim Blandy :jimb from comment #16)
> > +// Apple's iOS toolchain is lame and does not support .cfi directives.
> > +#ifdef __APPLE__
> 
> Why aren't you testing __GCC_HAVE_DWARF2_CFI_ASM here, instead of __APPLE__?

I'm sorry --- I see your explanation of this above.

(In reply to Nathan Froyd (:froydnj) from comment #9)
> I am explicitly not doing what bug 645111 did for the definition of the CFI
> macro.  __GCC_HAVE_DWARF2_CFI_ASM is very new (GCC 4.6+), so using it would
> lose on older toolchains.  Similarly, I'm not playing games with
> .cfi_sections; that's a rather new directive and Breakpad will happily fetch
> information out of .eh_frame.

The compiler version that matters is the one we're building our shipping executables with. Are our production compilers too old for this preprocessor conditional?
Comment 18 Jim Blandy :jimb 2011-11-08 15:26:23 PST
Comment on attachment 568471 [details] [diff] [review]
add CFI directives

Waiting for a response to comment 16.
Comment 19 Nathan Froyd [:froydnj] 2011-11-08 17:16:00 PST
(In reply to Jim Blandy :jimb from comment #17)
> The compiler version that matters is the one we're building our shipping
> executables with. Are our production compilers too old for this preprocessor
> conditional?

Yes, 4.4.x is too old.

(In reply to Jim Blandy :jimb from comment #16)
> @@ +175,5 @@
> > +         ".text\n"
> > +         ".align 2\n"
> > +         "SharedStub:\n"
> > +         ".fnstart\n"
> > +         CFI(".cfi_startproc\n")
> 
> I think you need an initial CFA rule here, perhaps ".cfi_def_cfa sp, 0"? The
> DWARF spec says that all rules are initially undefined, although ABIs can
> specify particular initial rules.

The CIE for .debug_frame on ARM says "DW_CFA_def_cfa: r13 ofs 0", which I think is sufficient to address your concern.
Comment 20 Nathan Froyd [:froydnj] 2011-11-14 08:39:55 PST
Comment on attachment 568471 [details] [diff] [review]
add CFI directives

Flagging Jim for review after addressing questions.
Comment 21 Jim Blandy :jimb 2011-11-14 14:09:56 PST
(In reply to Nathan Froyd (:froydnj) from comment #19)
> The CIE for .debug_frame on ARM says "DW_CFA_def_cfa: r13 ofs 0", which I
> think is sufficient to address your concern.

Okay, that sounds good.

It seems odd to me to just emit .cfi directives unconditionally, as long as we're not on Apple; there should be some kind of GCC version test in there of some sort, it seems to me. But you're more familiar with the chronology than I am, so can't suggest something specific.
Comment 22 Jim Blandy :jimb 2011-11-30 11:14:47 PST
Let's get this landed!
Comment 24 Phil Ringnalda (:philor) 2011-12-24 22:17:09 PST
https://hg.mozilla.org/mozilla-central/rev/559c270ad020

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