Closed Bug 567135 Opened 11 years ago Closed 10 years ago
Need CFI for xptcstubs
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.
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.
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
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.
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)
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)
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?
We certainly get debug info sections when we build with -g with the Android toolchain, which contains CFI.
Nathan, what happens if you simply use the .cfi directives in the ARM assembly?
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.
The .s file generated by compiling the newly-patched xptcstubs_arm.cpp.
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.
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.
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.
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.
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 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.
(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 on attachment 568471 [details] [diff] [review] add CFI directives Waiting for a response to comment 16.
(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 on attachment 568471 [details] [diff] [review] add CFI directives Flagging Jim for review after addressing questions.
(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.
Let's get this landed!
Assignee: nfroyd → nobody
Component: XPConnect → XPCOM
QA Contact: xpconnect → xpcom
Assignee: nobody → nfroyd
Target Milestone: --- → mozilla12
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.