567135 - Need CFI for xptcstubs_arm

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(not currently active) Ted Mielczarek]

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]

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

[(not currently active) Ted Mielczarek]

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]

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

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

[(not currently active) Ted Mielczarek]

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

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

[(not currently active) Ted Mielczarek]

We certainly get debug info sections when we build with -g with the Android toolchain, which contains CFI.

Comment 8

[Jim Blandy :jimb]

Nathan, what happens if you simply use the .cfi directives in the ARM assembly?

Comment 9

[Nathan Froyd [:froydnj]]

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

Attachment: xptcstubs_arm.s file

The .s file generated by compiling the newly-patched xptcstubs_arm.cpp.

Comment 11

[Nathan Froyd [:froydnj]]

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

[(not currently active) Ted Mielczarek]

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

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

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

[(not currently active) Ted Mielczarek]

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]

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]

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

Comment on attachment 568471 [details] [diff] [review]
add CFI directives

Waiting for a response to comment 16.

Comment 19

[Nathan Froyd [:froydnj]]

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

Comment on attachment 568471 [details] [diff] [review]
add CFI directives

Flagging Jim for review after addressing questions.

Comment 21

[Jim Blandy :jimb]

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

Let's get this landed!

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/559c270ad020

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/559c270ad020