Need CFI for xptcstubs_arm

RESOLVED FIXED in mozilla12

Status

()

Core
XPCOM
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: ted, Assigned: froydnj)

Tracking

Trunk
mozilla12
ARM
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

7 years ago
Assignee: nobody → jim

Comment 1

7 years ago
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.
(Reporter)

Comment 2

7 years ago
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

7 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
Assignee: jimb → nfroyd
Status: NEW → ASSIGNED
Attachment #568084 - Flags: review?(benjamin)
(Reporter)

Comment 5

6 years ago
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.
Attachment #568084 - Flags: review?(benjamin) → review?(jimb)
(Assignee)

Comment 6

6 years ago
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?
(Reporter)

Comment 7

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

Comment 8

6 years ago
Nathan, what happens if you simply use the .cfi directives in the ARM assembly?
(Assignee)

Comment 9

6 years ago
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.
Attachment #568084 - Attachment is obsolete: true
Attachment #568471 - Flags: review?(jimb)
Attachment #568084 - Flags: review?(jimb)
(Assignee)

Comment 10

6 years ago
Created attachment 568473 [details]
xptcstubs_arm.s file

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

Comment 11

6 years ago
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.
(Reporter)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
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.
(Reporter)

Comment 15

6 years ago
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

6 years ago
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

6 years ago
(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

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

Waiting for a response to comment 16.
Attachment #568471 - Flags: review?(jimb)
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

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

Flagging Jim for review after addressing questions.
Attachment #568471 - Flags: review?(jimb)

Comment 21

6 years ago
(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.

Updated

6 years ago
Attachment #568471 - Flags: review?(jimb) → review+

Comment 22

6 years ago
Let's get this landed!
Assignee: nfroyd → nobody
Component: XPConnect → XPCOM
QA Contact: xpconnect → xpcom
(Assignee)

Updated

5 years ago
Assignee: nobody → nfroyd
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/559c270ad020
Keywords: checkin-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/559c270ad020
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.