Breakpad Mac symbol dumper: Unify with Linux dumper; support DWARF CFI.

This patch rewrites the Mac symbol dumper to use the same set of classes
the Linux dumper does for reading debugging information from various
sources, consolidating them into a single table, and writing that out as a
Breakpad symbol file.

In the process, it also adds support for dumping DWARF call frame
information and .eh_frame exception-handling information as Breakpad 'STACK
CFI' records. This allows the Breakpad processor to generate stack traces
from code compiled with -fomit-frame-pointer.

The patch also replaces the DumpSymbols Objective C++ class with
google_breakpad::DumpSymbols, a plain C++ class. The code still uses some
Objective C++ to use the Foundation facilities for dealing with file names
in a file-system-independent fashion, and for examining the contents of
.dSYM bundles.

Since the code has been entirely rewritten, I have changed the author
lines.

[jimb]

Here's the big one.

[jimb]

By the way, I think that this patch fixes the following Breakpad issues:

Issue 289:  	 DWARF support in dump_syms on Mac OS X crashes for 64-bit binaries
Issue 317:  	 dump_syms needs 64-bit darwin support, especially for Mac OS X
10.6
Issue 322:  	 dump_syms crashes on PPC
Issue 329:  	 Mac dump_syms crashes when built with gcc-4.2 at -O1, -O2, or -O3
Issue 337:  	 Random crashes of dump_syms on Mac

[jimb]

Add mark as reviewer.

[Mark Mentovai]

Longer, more interesting change. I’ll get to it later.

[jimb]

Adding Ted as reviewer.

[Mark Mentovai]

http://breakpad.appspot.com/96001/diff/1/2
File src/common/mac/dump_syms.h (right):

http://breakpad.appspot.com/96001/diff/1/2#newcode1
Line 1: // -*- mode: C++ -*-
Stupid consistency nit C++ (uppercase) in this file, but c++ (lowercase) in the
.mm. Just to let you know I’m reading carefully…

http://breakpad.appspot.com/96001/diff/1/2#newcode43
Line 43: #include <mach-o/loader.h>
Sort per guide. “C system headers” doesn’t distinguish between POSIX headers and
other system headers.

http://breakpad.appspot.com/96001/diff/1/2#newcode66
Line 66: if (object_files_) free(object_files_);
Conditional unnecessary.

http://breakpad.appspot.com/96001/diff/1/2#newcode69
Line 69: // Prepare to read debugging information from FILENAME. FILENAME may be
Same as what I said in the other review about documenting arguments |this_way|
instead of BY_YELLING. Throughout.

http://breakpad.appspot.com/96001/diff/1/2#newcode94
Line 94: const struct fat_arch *AvailableArchitectures(size_t *size) {
count is better than size.

http://breakpad.appspot.com/96001/diff/1/2#newcode112
Line 112: // Read debugging information from DWARF_SEGMENT, which was taken from
There is no dwarf_segment or reader here, fix the comment (or the arguments?)
READER shows up again in the comment for ReadCFI.

http://breakpad.appspot.com/96001/diff/1/2#newcode131
Line 131: NSString *filename_;
filename_ is a bit of a misnomer if this can be a non-leaf bundle directory.
path_ or pathname_ might be better…

http://breakpad.appspot.com/96001/diff/1/2#newcode137
Line 137: NSString *resource_filename_;
resource_filename_ makes me think of resource forks and other things, maybe
macho_filename_ is a better name for this. If you’re concerned about that being
not strictly correct for fat files, I hear filename_ might be available given
the preceding comment.

http://breakpad.appspot.com/96001/diff/1/2#newcode149
Line 149: size_t object_files_size_;
object_files_count_ is better than object_files_size_.

http://breakpad.appspot.com/96001/diff/1/3
File src/common/mac/dump_syms.mm (right):

http://breakpad.appspot.com/96001/diff/1/3#newcode76
Line 76: const char *message = strerror(errno);
This isn’t right. There’s no guarantee that errno will be set if the previous
line fails. +[NSFileManager defaultManager] may have been nil for its own
(pretty fatal) reasons, or -[NSFileManager fileExistsAtPath:] may have returned
NO for a reason other than something that made a system call via libc.

http://breakpad.appspot.com/96001/diff/1/3#newcode77
Line 77: std::cerr << "error opening object file: "
Misleading - this isn’t about an error opening.

http://breakpad.appspot.com/96001/diff/1/3#newcode99
Line 99: // pathForResource:ofType:inDirectory likes.
Yeah, isn’t this frustrating? They really should have stuffed the right name
into Info.plist.

http://breakpad.appspot.com/96001/diff/1/3#newcode142
Line 142: if (! contents_) {
No space between unary operator and its argument.

http://breakpad.appspot.com/96001/diff/1/3#newcode143
Line 143: const char *message = strerror(errno);
There’s that pesky errno again. I’m afraid that there really isn’t a good way to
provide better error information. :(

http://breakpad.appspot.com/96001/diff/1/3#newcode158
Line 158: return false;
{}

http://breakpad.appspot.com/96001/diff/1/3#newcode191
Line 191: FileID file_id([resource_filename_ fileSystemRepresentation]);
Uh-oh, this goes back through the old Mach-O reader stuff? Would it be too much
to integrate the LC_UUID stuff into your nice new Mach-O reader and use it here?

http://breakpad.appspot.com/96001/diff/1/3#newcode227
Line 227: dwarf2reader::ByteReader *byte_reader_;
Weak?

http://breakpad.appspot.com/96001/diff/1/3#newcode294
Line 294: case CPU_TYPE_X86:
This doesn’t support PPC and PPC64? PPC at least is still relevant for some
users.

http://breakpad.appspot.com/96001/diff/1/3#newcode362
Line 362: google_breakpad::Module *module_;
Weak?

http://breakpad.appspot.com/96001/diff/1/3#newcode376
Line 376: // If there is a problem reading this, don't treat it as a fatal
error.
When might there be a problem reading this?

Same question at line 388.

http://breakpad.appspot.com/96001/diff/1/3#newcode397
Line 397: const ByteBuffer &strings) {
This function is so easy!

http://breakpad.appspot.com/96001/diff/1/3#newcode425
Line 425: << "object file contains more than one" << std::endl
Considering that resource_filename_ is of unknown length, I wouldn’t have even
bothered with the newlines interspersed throughout.

[jimb]

Thanks for the review!

http://breakpad.appspot.com/96001/diff/1/2
File src/common/mac/dump_syms.h (right):

http://breakpad.appspot.com/96001/diff/1/2#newcode1
Line 1: // -*- mode: C++ -*-
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Stupid consistency nit C++ (uppercase) in this file, but c++ (lowercase) in
the
> .mm. Just to let you know I’m reading carefully…

"My, Mark certainly is reading carefully!"  Okay.

http://breakpad.appspot.com/96001/diff/1/2#newcode43
Line 43: #include <mach-o/loader.h>
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Sort per guide. “C system headers” doesn’t distinguish between POSIX headers
and
> other system headers.

Okay.

http://breakpad.appspot.com/96001/diff/1/2#newcode66
Line 66: if (object_files_) free(object_files_);
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Conditional unnecessary.

Okay.

http://breakpad.appspot.com/96001/diff/1/2#newcode69
Line 69: // Prepare to read debugging information from FILENAME. FILENAME may be
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Same as what I said in the other review about documenting arguments |this_way|
> instead of BY_YELLING. Throughout.

IT IS STANDARD GNU PRACTICE TO YELL.  Whenever possible.

(Okay.)

http://breakpad.appspot.com/96001/diff/1/2#newcode94
Line 94: const struct fat_arch *AvailableArchitectures(size_t *size) {
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> count is better than size.

Okay.

http://breakpad.appspot.com/96001/diff/1/2#newcode112
Line 112: // Read debugging information from DWARF_SEGMENT, which was taken from
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> There is no dwarf_segment or reader here, fix the comment (or the arguments?)
> READER shows up again in the comment for ReadCFI.

Oh, right.  I already had the SectionMap, which is closer to what
dwarf2reader::CompilationUnit wants, anyway. And ReadDwarf and ReadCFI each make
their own ByteReader.

http://breakpad.appspot.com/96001/diff/1/2#newcode131
Line 131: NSString *filename_;
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> filename_ is a bit of a misnomer if this can be a non-leaf bundle directory.
> path_ or pathname_ might be better…

Hum.  input_pathname_?

http://breakpad.appspot.com/96001/diff/1/2#newcode137
Line 137: NSString *resource_filename_;
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> resource_filename_ makes me think of resource forks and other things, maybe
> macho_filename_ is a better name for this. If you’re concerned about that
being
> not strictly correct for fat files, I hear filename_ might be available given
> the preceding comment.

How about object_filename_?

http://breakpad.appspot.com/96001/diff/1/2#newcode149
Line 149: size_t object_files_size_;
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> object_files_count_ is better than object_files_size_.

Okay.

http://breakpad.appspot.com/96001/diff/1/3
File src/common/mac/dump_syms.mm (right):

http://breakpad.appspot.com/96001/diff/1/3#newcode76
Line 76: const char *message = strerror(errno);
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> This isn’t right. There’s no guarantee that errno will be set if the previous
> line fails. +[NSFileManager defaultManager] may have been nil for its own
> (pretty fatal) reasons, or -[NSFileManager fileExistsAtPath:] may have
returned
> NO for a reason other than something that made a system call via libc.

The code is spastic.  It should just say, "file does not exist".

http://breakpad.appspot.com/96001/diff/1/3#newcode99
Line 99: // pathForResource:ofType:inDirectory likes.
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Yeah, isn’t this frustrating? They really should have stuffed the right name
> into Info.plist.

Actually, I purloined all this code from the original .mm, so I'm just taking
the previous author's word for it.  (And the loop got reshaped, too, so I think
this comment is the biggest piece of code that survived.)

http://breakpad.appspot.com/96001/diff/1/3#newcode142
Line 142: if (! contents_) {
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> No space between unary operator and its argument.

Okay.

http://breakpad.appspot.com/96001/diff/1/3#newcode143
Line 143: const char *message = strerror(errno);
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> There’s that pesky errno again. I’m afraid that there really isn’t a good way
to
> provide better error information. :(

Should I use dataWithContentsOfFile:options:error: instead?

http://breakpad.appspot.com/96001/diff/1/3#newcode158
Line 158: return false;
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> {}

Okay.

http://breakpad.appspot.com/96001/diff/1/3#newcode191
Line 191: FileID file_id([resource_filename_ fileSystemRepresentation]);
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Uh-oh, this goes back through the old Mach-O reader stuff? Would it be too
much
> to integrate the LC_UUID stuff into your nice new Mach-O reader and use it
here?

I was afraid you'd ask about that. :)  I don't think it would be too much work;
can we leave it for a cleanup patch?

http://breakpad.appspot.com/96001/diff/1/3#newcode227
Line 227: dwarf2reader::ByteReader *byte_reader_;
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Weak?

Yes.

http://breakpad.appspot.com/96001/diff/1/3#newcode294
Line 294: case CPU_TYPE_X86:
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> This doesn’t support PPC and PPC64? PPC at least is still relevant for some
> users.

I haven't done the stack walker work to support CFI on PPC.  But note that this
is not treated as a fatal error; they still get all the debugging information
they used to.

http://breakpad.appspot.com/96001/diff/1/3#newcode362
Line 362: google_breakpad::Module *module_;
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Weak?

Yes.

http://breakpad.appspot.com/96001/diff/1/3#newcode376
Line 376: // If there is a problem reading this, don't treat it as a fatal
error.
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> When might there be a problem reading this?
> 
> Same question at line 388.

Unsupported architecture, poorly formatted debugging info?

http://breakpad.appspot.com/96001/diff/1/3#newcode397
Line 397: const ByteBuffer &strings) {
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> This function is so easy!

Yeah!!  All the actual work needed to make StabsReader understand Mac stabs was
in prior patches.

http://breakpad.appspot.com/96001/diff/1/3#newcode425
Line 425: << "object file contains more than one" << std::endl
On 2010/06/03 20:24:01, Mark Mentovai wrote:
> Considering that resource_filename_ is of unknown length, I wouldn’t have even
> bothered with the newlines interspersed throughout.

Okay.

[Mark Mentovai]

http://breakpad.appspot.com/96001/diff/1/2
File src/common/mac/dump_syms.h (right):

http://breakpad.appspot.com/96001/diff/1/2#newcode131
Line 131: NSString *filename_;
jimb wrote:
> Hum.  input_pathname_?

Sure.

http://breakpad.appspot.com/96001/diff/1/2#newcode137
Line 137: NSString *resource_filename_;
jimb wrote:
> How about object_filename_?

Sure.

http://breakpad.appspot.com/96001/diff/1/3
File src/common/mac/dump_syms.mm (right):

http://breakpad.appspot.com/96001/diff/1/3#newcode143
Line 143: const char *message = strerror(errno);
jimb wrote:
> Should I use dataWithContentsOfFile:options:error: instead?

Yeah, you could. I actually missed this on the first read, I thought you were
using -dataWithContentsOfMappedFile:. I like mmap, and it’s quite possible that
huge chunks of these files won’t be needed at all. The big comment above
certainly makes it seem like using mmap might be scary—I actually don’t know Mac
OS X’ behavior here offhand although it would be easy enough to check—but do we
really expect someone else to be writing to the file behind our backs? And if
they are, what of it? If someone’s doing that, there’s just as much a race to
modify the file and leave it in an inconsistent state before we’ve opened it.

Rebuttal?

http://breakpad.appspot.com/96001/diff/1/3#newcode191
Line 191: FileID file_id([resource_filename_ fileSystemRepresentation]);
jimb wrote:
> I was afraid you'd ask about that. :)  I don't think it would be too much
work;
> can we leave it for a cleanup patch?

Please. It should be really simple, and as you probably know, I tend to respond
much more quickly to small patches than big (but interesting) behemoths like
this one. (But I’m not in the middle of trying to stabilize and release software
right now, so my response times shouldn’t be as bad regardless.)

http://breakpad.appspot.com/96001/diff/1/3#newcode294
Line 294: case CPU_TYPE_X86:
jimb wrote:
> I haven't done the stack walker work to support CFI on PPC.  But note that
this
> is not treated as a fatal error; they still get all the debugging information
> they used to.

True. Don’t worry about PPC, then.

http://breakpad.appspot.com/96001/diff/1/3#newcode376
Line 376: // If there is a problem reading this, don't treat it as a fatal
error.
jimb wrote:
> Unsupported architecture, poorly formatted debugging info?

“Unsupported architecture” makes sense given the last comment. Poorly-formatted
debugging info does seem like something we’d want to treat fatally, though.

[jimb]

http://breakpad.appspot.com/96001/diff/1/3
File src/common/mac/dump_syms.mm (right):

http://breakpad.appspot.com/96001/diff/1/3#newcode143
Line 143: const char *message = strerror(errno);
On 2010/06/03 21:26:28, Mark Mentovai wrote:
> jimb wrote:
> > Should I use dataWithContentsOfFile:options:error: instead?
> 
> Yeah, you could. I actually missed this on the first read, I thought you were
> using -dataWithContentsOfMappedFile:. I like mmap, and it’s quite possible
that
> huge chunks of these files won’t be needed at all. The big comment above
> certainly makes it seem like using mmap might be scary—I actually don’t know
Mac
> OS X’ behavior here offhand although it would be easy enough to check—but do
we
> really expect someone else to be writing to the file behind our backs? And if
> they are, what of it? If someone’s doing that, there’s just as much a race to
> modify the file and leave it in an inconsistent state before we’ve opened it.
> 
> Rebuttal?

I like mmap too, but I'm worried that other processes manipulating the file
would cause me *segfaults* --- is this the case?  By avoiding mmap, we only have
to check system call return codes, cope with early EOFs and misformatted data
--- all of which I think we've got in hand.

http://breakpad.appspot.com/96001/diff/1/3#newcode376
Line 376: // If there is a problem reading this, don't treat it as a fatal
error.
On 2010/06/03 21:26:28, Mark Mentovai wrote:
> jimb wrote:
> > Unsupported architecture, poorly formatted debugging info?
> 
> “Unsupported architecture” makes sense given the last comment.
Poorly-formatted
> debugging info does seem like something we’d want to treat fatally, though.

Hum.  Yeah, 'false' doesn't give us a lot of information, does it.

http://breakpad.appspot.com/96001/diff/1/3#newcode397
Line 397: const ByteBuffer &strings) {
Just trying to follow this line of thought through, here are the factors that
make the DWARF case so much hairier:

- DWARF lives in a lot of sections, and thus needs a section map, not simply two
sections.

- DWARF uses a separate ByteReader object to handle endianness and address size,
instead of just taking arguments.

- dwarf2reader::CompilationUnit has a pretty simple-minded handler interface, so
we use a separate layer, DIEDispatcher, to provide more organized parsing.

- A DWARF parser handles only one CU at a time, so instead of keeping file-wide
information internally, the caller needs to create and pass a FileContext.

- For testing, DwarfCUToModule and DumperLineToModule are separate classes,
instead of having DwarfCUToModule just deal with line number information
internally.

- StabsReader uses the same class as both a handler and an error/warning
reporter; DwarfCUToModule uses a separate class,
DwarfCUToModule::WarningReporter.

So the greater complexity of the DWARF format isn't really the largest factor;
it has more to do with interface choices. I'd say that the decoupling for
testability needn't have as much impact on clients as it does here.

[Mark Mentovai]

http://breakpad.appspot.com/96001/diff/1/3
File src/common/mac/dump_syms.mm (right):

http://breakpad.appspot.com/96001/diff/1/3#newcode143
Line 143: const char *message = strerror(errno);
jimb wrote:
> I like mmap too, but I'm worried that other processes manipulating the file
> would cause me *segfaults* --- is this the case?  By avoiding mmap, we only
have
> to check system call return codes, cope with early EOFs and misformatted data
> --- all of which I think we've got in hand.

We could do a few tests and read some kernel source to be sure, but it’s late in
the day. You can leave it as-is, since it’s no worse than what this program used
to do.

[Ted Mielczarek]

This landed:
http://code.google.com/p/google-breakpad/source/detail?r=614