464750 - Breakpad should use DWARF CFI for stack unwinding

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Jim Blandy :jimb]

We'd like to be able to build with -fomit-frame-pointer on x86, because this frees up a register in functions that have fixed stack frame sizes.  However, breakpad can't generate backtraces through functions compiled with -fomit-frame-pointer.

DWARF CFI (Call Frame Information) is instruction-by-instruction annotation of the machine code indicating how to unwind the stack.  We should be able to convert DWARF CFI to the format breakpad uses for this stuff (the STACK WIN records, I guess).

Comment 1

[(not currently active) Ted Mielczarek]

a) Gonna need to get this upstream
b) There's no DWARF support at all yet on Linux (but maybe you don't care?)
c) You volunteering? :)

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> a) Gonna need to get this upstream

That happens, right? I mean, we can assume we'll get breakpad patches upstream in general -- I hope.

> b) There's no DWARF support at all yet on Linux (but maybe you don't care?)

We would use this on Mac even if Linux lagged (like that never happens! :-P)

> c) You volunteering? :)

I hope Jim is, but it's up to him and mgmt.

/be

Comment 3

[Robert Sayre]

Is there a way to add this to a try server build to see what the upside is here?

Comment 4

[(not currently active) Ted Mielczarek]

You should be able to test the vomit-frame-pointer performance impact without caring about the breakpad symbols. And yeah we can easily upstream stuff, I'm just opposed to taking local patches was my point.

Comment 5

[Robert Sayre]

Yeah, I'm asking if the try servers will clobber for us.

Comment 6

[Jim Blandy :jimb]

Work upstream, definitely.

Comment 7

[Jim Blandy :jimb]

Changeset a8fc7f522e6e is in the Try server; it uses -fomit-frame-pointer throughout (or at least, widely; I don't know that I caught every case).

Comment 8

[Graydon Hoare :graydon]

Fwiw, I'm reasonably familiar with DWARF CFI stuff too and would probably "enjoy" writing the part of the symbol-dumper required. Assuming we want it and Jim didn't feel inclined.

Comment 9

[Jim Blandy :jimb]

I'm pretty sure we do want it, because it's a significant speed increase; you drop three instructions and free up a register in most functions.

I'd enjoy doing this, too.  I think whichever one of us has free bandwidth first should take it.

Comment 10

[(not currently active) Ted Mielczarek]

I'm available for moral support and guidance into the breakpad code, as always.

Comment 11

[Jim Blandy :jimb]

I've started work on this.  Status quo, at start of project:
- Linux is stabs-only.
- Breakpad DWARF code is Mac-only.
- Breakpad DWARF code maps PCs to functions and source locations, but has no CFI parsing.

So it seems like the agenda is:

* Make the DWARF reader code shareable between Mac and Linux.  At the moment, the DWARF code is in a mac subdirectory, and only used on Mac.

* Lightly refactor Linux code to separate STABS parsing from Breakpad symbol file writing.  This separation almost exists already.

* Get the Linux dump_syms program to write pc-to-func/source mapping data based on DWARF.

* Add CFI parsing code to the DWARF reader, and code to convert it to Breakpad's format and write it out.

Comment 12

[Jim Blandy :jimb]

Status: I've posted a bunch of cleanup patches to breakpad (some of which speed up symbol dumping on Linux roughly 20x).  I'm currently adapting the Linux code to use a representation for the debugging information that it can share with the DWARF reader, so that we can read both STABS and DWARF information from an executable, and dump a single breakpad-format file.

Comment 13

[Jim Blandy :jimb]

Status here:
http://groups.google.com/group/google-breakpad-dev/browse_thread/thread/f7a2a6c6bd8442f0

Comment 14

[Jim Blandy :jimb]

I've posted a 16-patch series to the breakpad list for review.
http://groups.google.com/group/google-breakpad-dev

Comment 15

[Jim Blandy :jimb]

Over the weekend, I got the DWARF-and-STABS-on-Linux reader working.  This is an additional ten-patch series.  It's not thoroughly tested, but it produces plausible-looking output.  There are no special flags to pass: it just aggregates whatever debugging information it finds and writes it all out as a single breakpad symbol file.

Note that this does not handle Call Frame Information yet, so we still can't use -fomit-frame-pointer.  But it does bring us to the point where we can begin work on that.

What this does let us do now (well, once it has been reviewed and baked) is stop using STABS on Linux.  Of course, except for CFI, the breakpad symbol format is even less expressive than STABS, so switching from STABS to DWARF won't improve our crash dump reporting in any material way.  But at least we can stop passing funny compiler flags.

Full DWARF-and-STABS-on-Linux patch series at:
http://hg.mozilla.org/users/jblandy_mozilla.com/breakpad-mq/

Of the original patch series mentioned in comment 14:

- All but the last two --- the most substantial --- have been reviewed, revised, and landed in the Google breakpad repository.

- Google's Neal Sidhwaney has reviewed the last two; I've made the revisions he suggested, except for one naming suggestion still being discussed.

- Neal pointed out the lack of unit tests for the new code; rectifying that will take a few days.

Comment 16

[(not currently active) Ted Mielczarek]

I think I noticed you fixing some weird stabs bugs along the way, and I know we picked up a few fixes for free when we switched from stabs to dwarf on OS X, things like inline handling and some weird missing filenames, off the top of my head.

Comment 17

[(not currently active) Ted Mielczarek]

That being said, I don't see any real pressing reason to sync with the upstream Breakpad code until you get your CFI work done.

Comment 18

[Jim Blandy :jimb]

(In reply to comment #16)
> I think I noticed you fixing some weird stabs bugs along the way

Yes, there were three bug fixes.  I was doing before/after comparisons as I worked, so whenever I'd do something right that the old code got wrong, I'd just fix it in the old code to have a minimally disturbed basis for comparison. 

Probably the most important bug fix gave us mangled names with some STABS junk attached for certain functions.  I can backport that if you think it would make sense.

(In reply to comment #17)
> That being said, I don't see any real pressing reason to sync with the upstream
> Breakpad code until you get your CFI work done.

I don't see one either.

Comment 19

[Jim Blandy :jimb]

The original sixteen-patch series (Linux dumper restructuring, no DWARF) is now fully committed to Google's breakpad SVN repo.

Comment 20

[Jim Blandy :jimb]

I'm going to break down the remaining work on this bug into separate blocking bugs.

Comment 21

[Jim Blandy :jimb]

Okay.  The "show dependency graph" function might be useful on this one. :)