Breakpad Mac symbol dumper: Add new Mach-O reader class.

This patch adds files defining new classes in the google_breakpad::Mach_O
namespace for parsing fat binaries and Mach-O files. These are used in the
new dumper to handle STABS debugging information, DWARF call frame
information, and .eh_frame exception handling stack walking information.

These new classes are independent of endianness and word size, and
therefore can be used on binaries of all the relevant architectures: x86,
x86_64, ppc, and ARM.

The patch adds a complete set of unit tests for the new classes.

[jimb]

Here's the right version of this patch to review.  Sorry for the confusion.

[jimb]

Add mark as reviewer.

[Mark Mentovai]

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

[jimb]

Adding Ted as reviewer.

[jimb]

Patch set 2 has only comment changes.

[jimb]

On 2010/05/20 19:29:33, jimb wrote:
> Patch set 2 has only comment changes.

Actually, that's a lie.  It also contains changes to header usage and whitespace
requested in other reviews.

[Mark Mentovai]

OK, let’s get started here. I like what I’ve read so far.

http://breakpad.appspot.com/93001/diff/6001/7001
File src/common/mac/macho_reader.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7001#newcode38
Line 38: #include <stdlib.h>
C system headers before any C++ headers. Put this below <assert.h>.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

http://breakpad.appspot.com/93001/diff/6001/7001#newcode40
Line 40: #include "common/mac/macho_reader.h"
The header for a specific implementation file should come before any other
headers, including system headers. This helps find header files that don’t
#include or forward declare everything that they need to work properly.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode43
Line 43: namespace Mach_O {
namespace mach_o {

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

http://breakpad.appspot.com/93001/diff/6001/7001#newcode60
Line 60: std::cerr << filename_ << ": file is neither a fat binary file"
Google style disallows streams except for logging.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Most of the std::cerr stuff would be written as fprintf(stderr, ...). It seems
that you’ve also used streams for other stuff too, though. Would it be a lot of
work to unstream them? If so, I might be uncharacteristically flexible on this
point this time.

http://breakpad.appspot.com/93001/diff/6001/7002
File src/common/mac/macho_reader.h (right):

http://breakpad.appspot.com/93001/diff/6001/7002#newcode36
Line 36: #ifndef MAC_COMMON_MACHO_READER_H_
Recommend BREAKPAD_COMMON_MAC_MACHO_READER_H_.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=The__d...

Don’t forget the comment at the end.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode39
Line 39: extern "C" {
Not needed. It’s the standard library’s job to define everything properly,
giving everything the proper linkage. These headers are perfectly safe to
#include from C++ code without an extern "C" guard on Mac OS X and on any other
reasonable system; they already wrap the necessary parts (function declarations)
in extern "C" when __cplusplus is #defined.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode48
Line 48: #include <mach-o/loader.h>
These ought to go with the C system headers.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode54
Line 54: namespace Mach_O {
mach_o (as in the other file).

http://breakpad.appspot.com/93001/diff/6001/7002#newcode56
Line 56: using std::map;
Strictly speaking, style only permits using-declarations in functions, methods,
or classes in .h files. I consider it an omission that namespaces are omitted
from this list, and I think your usage here is fine. I’ll try to remember to get
the style guide revised on this point.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

http://breakpad.appspot.com/93001/diff/6001/7002#newcode62
Line 62: // they appear, which makes them a bit easier to use with ByteCursors.
Good idea.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode71
Line 71: // 32-bit/64-bit PowerPC binaries. When applied to a (non-fat) Mach-O
file,
How is PowerPC relevant here? Any more relevant than x86 or x86_64, that is?

http://breakpad.appspot.com/93001/diff/6001/7002#newcode97
Line 97: string filename_;
I see no reason for this data member to be protected, make it private. The only
subclass I’m aware of (after a quick scan) is the one used for testing, and that
doesn’t make use of filename_ at all.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

http://breakpad.appspot.com/93001/diff/6001/7002#newcode105
Line 105: // Read the SIZE bytes at BUFFER as a fat binary file. On success,
Nit: |size|, |buffer| reads a little more easily than SIZE and BUFFER, and since
the case-sensitive names actually are lowercase…

http://breakpad.appspot.com/93001/diff/6001/7002#newcode128
Line 128: // utilities that work with fat binaries.
I like your documentation. Very thorough.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode143
Line 143: uint32_t magic_;
Type Magic?

http://breakpad.appspot.com/93001/diff/6001/7002#newcode143
Line 143: uint32_t magic_;
This is the only field that’s not explicitly initialized and that won’t
automatically initialize itself. Do you want to provide magic_(0) in the
constructor? (I haven’t looked at the implementation yet to see whether it might
be used uninitialized.)

http://breakpad.appspot.com/93001/diff/6001/7002#newcode158
Line 158: bool bits_64;
For better packing, maybe you should put this last, and move the ByteBuffers up
here. Same with Section.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode174
Line 174: uint32_t maxprot;
vm_prot_t for compatibility with segment_command and segment_command_64?
(vm_prot_t is int, so signed, as opposed to uint32_t.)

http://breakpad.appspot.com/93001/diff/6001/7002#newcode197
Line 197: string section_name, segment_name;
It’s exceedingly rare for there to be more than one declaration on a line,
especially in structs (and classes). I almost missed that segment_name was here
and was about to say “but wait, the segment name is a property of the section
too!” Could you put segment_name on its own line?

http://breakpad.appspot.com/93001/diff/6001/7002#newcode206
Line 206: uint32_t alignment;
Perhaps you should call this |align| for compatibility with section and
section_64.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode219
Line 219: typedef map<std::string, Section> SectionMap;
std:: unnecessary given your using-declaration.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode258
Line 258: // commands, but the I'th load command, of type TYPE, extends beyond
|type| and again on line 260, 264, 269, 273, and 277, as before.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode284
Line 284: virtual bool MisplacedSymbolTable();
I like the idea that these are “misplaced.” :)

http://breakpad.appspot.com/93001/diff/6001/7002#newcode287
Line 287: string filename_;
As above, I think this can be private.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode294
Line 294: public:
nit: one more space of indent on this guy.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode297
Line 297: // Called to report that the segment's section list contains SECTION.
|section|, then again with |command| on line 321, |segment| on 331, …

http://breakpad.appspot.com/93001/diff/6001/7002#newcode393
Line 393: // Populate MAP with a map of the sections in SEGMENT, from section
As I read the comment, I wonder if |section_map| (note: neither MAP nor |map|)
will be cleared by this method or not.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode413
Line 413: bool big_endian_;
And I wonder if any of the rest of the fields need to be initialized.

[Mark Mentovai]

I’m really enjoying reading this. Nice work.

http://breakpad.appspot.com/93001/diff/6001/7001
File src/common/mac/macho_reader.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7001#newcode61
Line 61: << " nor a Mach-O object file" << std::endl;
Probably nicer to keep this a single string literal (broken up with
"file"(newline)" nor" if needed).

http://breakpad.appspot.com/93001/diff/6001/7001#newcode71
Line 71: if (object_files_)
free(NULL) is safe, so this isn’t strictly necessary.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode85
Line 85: if (cursor >> magic_) {
(As mentioned in the last batch of comments…)

Your use of streams for ByteCursor is OK. I’d still encourage not using
std::cerr.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode89
Line 89: if (! (cursor >> object_file_count)) {
When you read from a system-defined struct into a field with a name that doesn’t
match the system’s name, it’s a good idea to put a comment. Here, you could say
“// nfat_arch”. Then, when someone comes along to maintain the code in the
future, and they look for where FatReader deals with nfat_arch (the known name
from the system’s fat_header struct), they’ll zero in on exactly the right spot
in the code.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode89
Line 89: if (! (cursor >> object_file_count)) {
No space between a unary operator and its argument.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...

No big deal, but it happens a few times, search for “! ”.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode107
Line 107: if (object_files_size_ == 1)
You can get away without this check and just use the realloc case each time.
realloc(NULL, size) is equivalent to malloc(size).

http://breakpad.appspot.com/93001/diff/6001/7001#newcode108
Line 108: object_files_ = (struct fat_arch *) malloc(sizeof(*object_files_));
But why don’t you just do this outside the loop?

object_files_ = (struct fat_arch *)malloc(sizeof(*object_files_) *
object_file_count);

http://breakpad.appspot.com/93001/diff/6001/7001#newcode109
Line 109: else
Readability bugaboo: when the conditional expression or conditioned block uses
more than one line, even if it’s a single statement, use {braces}. People’s eyes
are better at operating on lines, not semicolons. It helps conceptualize the
control flow.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode114
Line 114: struct fat_arch *objfile = &object_files_[object_files_size_ - 1];
Why not just i for the subscript instead of |object_files_size_ - 1|?

I don’t understand the separate tracking of i and object_files_size_ in this
loop. Is it because you expect a truncated file to still be usable somehow? It
seems (at this point) that you don’t handle that anyway (you’ll return false),
and I don’t think it’s worth trying to support. I won’t really object if you
want to support it (or sorta-partially support it), though. It just doesn’t seem
terribly useful: nobody’s actually going to want to do anything with a damaged
file.

It seems to me that you could do away with object_file_count altogether and read
the count right into object_files_size_ (which should be renamed
object_files_count_).

http://breakpad.appspot.com/93001/diff/6001/7001#newcode130
Line 130: reporter_->TooShort();
Is it worth having a distinct MisplacedObjectFile method to distinguish this
error from the “file is too short for the fat_header + nfat_arch * fat_arch”
case that the other TooShort calls indicate? The Reader::Reporter class breaks
out various forms of “too short” in this way.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode142
Line 142: uint32_t cpu_type, cpu_subtype;
You can avoid the local temporaries by moving the malloc up here.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode149
Line 149: // Fake a plausible alignment value.
12 is always correct for the 32- and 64-bit variants of x86 and ppc. Leave a
pointer to get_align in
http://www.opensource.apple.com/source/cctools/cctools-773/misc/lipo.c as an
aide to anyone who might want to handle other CPU types.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode156
Line 156: object_files_->align = 12;
// 2^12 = 4096

http://breakpad.appspot.com/93001/diff/6001/7001#newcode197
Line 197: << "the contents of load command #" << i << ","
Looks like you could have put the : with the next string literal instead of
having it separate. Same with the comma on this line and the next line. Conserve
string literals!

Same with several other of these Reporter methods.

(But grumble about std::cerr…)

http://breakpad.appspot.com/93001/diff/6001/7001#newcode237
Line 237: bool Reader::Read(const uint8_t *buffer, size_t size) {
Can the caller provide this method with the expected cputype and cpusubtype? If
this is a fat file, it would be good to validate the expected values from the
fat header against the values retrieved from the architecture-specific object
file. If it’s not a fat file, cputype and cpusubtype will come from right here
anyway.

I’d also validate cputype against the magic number: x86 should be MH_CIGAM, ppc
should be MH_MAGIC, the 64-bit variant of either should be _64.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode260
Line 260: uint32_t flags, reserved;
If you’re going to declare these as you’ve done, I’d leave the ; off at the end
of the previous line and the uint32_t off on this line. That makes it easier to
see that this is just a batch of like declarations.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode271
Line 271: .PointTo(&load_commands_.end, 0);
Cool.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode276
Line 276: // What's that thing Ruby programmers like to say --- "DRY"?  What is
that?
So then why didn’t you just use the members to begin with?

http://breakpad.appspot.com/93001/diff/6001/7001#newcode289
Line 289: while (index < load_command_count_) {
Coulda been a for loop, the body seems very appropriate for that.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode316
Line 316: segment.bits_64 = (type == LC_SEGMENT_64);
Is it worth validating that 32-bit files only have LC_SEGMENT and 64-bit files
only have LC_SEGMENT_64?

http://breakpad.appspot.com/93001/diff/6001/7001#newcode330
Line 330: if (!reporter_->LoadCommandTooShort(index, type))
LoadCommandTooShort always returns false. A subclass could override that, but
does that ever happen? Should it (and some of the other Reporter methods) just
return void and let these always be treated as a failure?

http://breakpad.appspot.com/93001/diff/6001/7001#newcode367
Line 367: // How big are the entries in the symbol table?
Reference “struct nlist” and “struct nlist_64” from <mach-o/nlist.h> in the
comment, or use sizeof(those) instead of hard-coding 12 and 16.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode377
Line 377: ByteBuffer entries(buffer_.start + symoff, symbols_size);
symbols is a better name than strings, same for the declaration of SymtabCommand
in the header.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode399
Line 399: class Reader::SegmentFinder: public LoadCommandHandler {
Stupid style nit: space before the : in “: public”. It seems kind of stupid, but
I actually search for subclasses by searching for “ : public B”.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode422
Line 422: // Where we should store the segment if found.
Say weak.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode432
Line 432: return finder.found();
Cool.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode505
Line 505: // The map under construction.
Weak.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode511
Line 511: section_map->clear();
Ah, there’s the answer to my question from yesterday. (Still, it ought to be
documented in the header.)

http://breakpad.appspot.com/93001/diff/6001/7002
File src/common/mac/macho_reader.h (right):

http://breakpad.appspot.com/93001/diff/6001/7002#newcode1
Line 1: // -*- mode: C++ -*-
Mode line here but not in the .cc file. Want one there too?

http://breakpad.appspot.com/93001/diff/6001/7002#newcode129
Line 129: struct fat_arch *object_files(size_t *size) const {
|size| and |object_files_size_| make it sound like these are concerned with the
size (in bytes) of the object files. |count| and |object_files_count_| may be
better.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode135
Line 135: // We use this to report problems parsing the file's contents.
It’s intended that this is weak as opposed to owned by the FatReader, right? If
so, say so.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode300
Line 300: virtual bool operator()(const Section &section) = 0;
There isn’t a compelling reason to use operator() here. Can you make this a
proper named method

http://breakpad.appspot.com/93001/diff/6001/7002#newcode338
Line 338: virtual bool SymtabCommand(const ByteBuffer &entries,
symbols is a better name than strings, I think.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode405
Line 405: // We use this to report problems parsing the file's contents.
Weak?

http://breakpad.appspot.com/93001/diff/6001/7002#newcode419
Line 419: cpu_type_t cpu_type_;
You should put comments on each of these identifying the field names used by the
system’s mach_header and mach_header_64 structs. cputype, cpusubtype, filetype,
ncmds, sizeofcmds. The same goes for the FatReader class, and the segname and
sectname fields in the Section struct. I made a similar request in the .cc file,
to help someone approaching the code for the first time to quickly locate where
a field is handled based on the known name used by the system headers.

[Mark Mentovai]

I didn’t read the test file as carefully, other than to poke into a bunch of the
TEST_Fs to see that they were doing good things. They are.

http://breakpad.appspot.com/93001/diff/6001/7003
File src/common/mac/macho_reader_unittest.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7003#newcode211
Line 211: .Append(10 * 5 * 4, '*');  // dummy architecture entries
5 and 4 are magic. Can you use sizeof(fat_arch) from <mach-o/fat.h>? Again on
line 227.

[Mark Mentovai]

http://breakpad.appspot.com/93001/diff/6001/7006
File src/tools/mac/dump_syms/macho_dump.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7006#newcode35
Line 35: extern "C" {
Unnecessary.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode41
Line 41: #include "unistd.h"
<unistd.h>, not "unistd.h>

http://breakpad.appspot.com/93001/diff/6001/7006#newcode44
Line 44: #include <errno.h>
C system header belongs in the batch above.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode48
Line 48: #include <string.h>
Me too.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode55
Line 55: using std::cerr;
Prefer fprintf(stderr, …) and printf(…) from <stdio.h> to these.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode66
Line 66: string program_name;
static or anonymous namespace?

http://breakpad.appspot.com/93001/diff/6001/7006#newcode68
Line 68: int check_syscall(int result, const char *operation, const char
*filename) {
static or anonymous namespace?

http://breakpad.appspot.com/93001/diff/6001/7006#newcode78
Line 78: class DumpSection: public Mach_O::Reader::SectionHandler {
anonymous namespace?

http://breakpad.appspot.com/93001/diff/6001/7006#newcode124
Line 124: program_name = argv[0];
program_name = basename(argv[0]), basename is from <libgen.h>.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode132
Line 132: const char *filename = argv[i];
Refactor the loop body into its own function that accepts a const char *filename
argument.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode133
Line 133: int fd = check_syscall(open(argv[i], O_RDONLY), "opening", filename);
Never closed (fd leak). You can close immediately after mmap.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode137
Line 137: void *mapping = mmap(NULL, attributes.st_size, PROT_READ,
Never unmapped (memory leak).

I wouldn’t care about either of these in this test program if you weren’t inside
a loop.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode146
Line 146: exit(1);
{}

[jimb]

Thanks very much for the review!

http://breakpad.appspot.com/93001/diff/6001/7001
File src/common/mac/macho_reader.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7001#newcode40
Line 40: #include "common/mac/macho_reader.h"
I've done that elsewhere; just dropped the ball here.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode43
Line 43: namespace Mach_O {
Okay.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode60
Line 60: std::cerr << filename_ << ": file is neither a fat binary file"
I don't remember anything in this patch series that actually uses any iostream
features, so changing it all to <stdio.h> is fine with me.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode71
Line 71: if (object_files_)
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> free(NULL) is safe, so this isn’t strictly necessary.

So it is.  The 'if' should go.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode85
Line 85: if (cursor >> magic_) {
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Your use of streams for ByteCursor is OK. I’d still encourage not using
std::cerr.

Well, this >> is a custom operator on a ByteCursor.  I'm pretty fond of it, so
I'd like to keep it.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode89
Line 89: if (! (cursor >> object_file_count)) {
Good point.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode89
Line 89: if (! (cursor >> object_file_count)) {
Sorry --- old GNU style habits die hard.  Mozillians tag me for this all the
time too.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode107
Line 107: if (object_files_size_ == 1)
So it does.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode108
Line 108: object_files_ = (struct fat_arch *) malloc(sizeof(*object_files_));
Actually, that's a good idea.  I had originally written this using vectors using
push_back, and didn't notice the opportunity when I converted it to a C array.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode109
Line 109: else
Okay.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode114
Line 114: struct fat_arch *objfile = &object_files_[object_files_size_ - 1];
It doesn't make any sense to me either.  I'm going to blame it on the myopic
vector->array conversion.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode130
Line 130: reporter_->TooShort();
Sure, that would allow the unit tests to be a little more precise, too.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode142
Line 142: uint32_t cpu_type, cpu_subtype;
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> You can avoid the local temporaries by moving the malloc up here.

I think I had enums at some point, so I couldn't trust the size.  Now that I've
ditched them, what you suggest is clearly the way to go.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode149
Line 149: // Fake a plausible alignment value.
Okay.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode197
Line 197: << "the contents of load command #" << i << ","
Agree this should use <stdio.h>.  Re: the colon: I think you're thinking about
this from the point of view of the compiled code, not the reader.  A reader
wants to see the colon with the word it's attached to.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode237
Line 237: bool Reader::Read(const uint8_t *buffer, size_t size) {
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Can the caller provide this method with the expected cputype and cpusubtype?
If
> this is a fat file, it would be good to validate the expected values from the
> fat header against the values retrieved from the architecture-specific object
> file.

I see your point.  Conceptually, though, mach_o::Reader could be used directly,
in which case one wouldn't know the cpu type/subtype yet.  Perhaps a new
overloading of this method that accepts the expected type/subtype, calls this
version, and then does the consistency check?

> I’d also validate cputype against the magic number: x86 should be MH_CIGAM,
ppc
> should be MH_MAGIC, the 64-bit variant of either should be _64.

Sure.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode260
Line 260: uint32_t flags, reserved;
Okay.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode276
Line 276: // What's that thing Ruby programmers like to say --- "DRY"?  What is
that?
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> So then why didn’t you just use the members to begin with?

Now, I think that would work.  As I say, in a prior version these were all enums
(believe it or not, I was originally trying to make this completely
cross-platform, so that you could build the Mac dumper on Linux or whatever, so
I had enum definitions... bad overreach), so I couldn't trust the size.  Now,
assuming these members are using the right typedefs, reading directly into them
would be fine.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode289
Line 289: while (index < load_command_count_) {
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Coulda been a for loop, the body seems very appropriate for that.

Definitely.  I think I had this loop driven by the cursor, not the command
count; another myopic conversion.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode316
Line 316: segment.bits_64 = (type == LC_SEGMENT_64);
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Is it worth validating that 32-bit files only have LC_SEGMENT and 64-bit files
> only have LC_SEGMENT_64?

Is that true?  *waggles eyebrows suggestively*

http://breakpad.appspot.com/93001/diff/6001/7001#newcode330
Line 330: if (!reporter_->LoadCommandTooShort(index, type))
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> LoadCommandTooShort always returns false. A subclass could override that, but
> does that ever happen? Should it (and some of the other Reporter methods) just
> return void and let these always be treated as a failure?

There are no subclasses of the reporters, except for unit testing. The whole
idea of allowing some reporter methods to return true to allow the process to
continue doesn't really get used in the dumper. It may be better to just let
someone else add that in if it's ever needed.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode367
Line 367: // How big are the entries in the symbol table?
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Reference “struct nlist” and “struct nlist_64” from <mach-o/nlist.h> in the
> comment, or use sizeof(those) instead of hard-coding 12 and 16.

I think I'd rather go with the comment.  I feel uncomfortable using sizeof since
we want the size on the target machine, not on our machine.  I know the
structures are defined with exact-width types, but you can still have alignment
vary from platform to platform.  It's picky, I know.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode377
Line 377: ByteBuffer entries(buffer_.start + symoff, symbols_size);
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> symbols is a better name than strings, same for the declaration of
SymtabCommand
> in the header.

I guess think of a "symbol" as comprising a fixed-width "entry" and a
variable-length "string".  How about 'names' instead of 'strings'?

http://breakpad.appspot.com/93001/diff/6001/7001#newcode399
Line 399: class Reader::SegmentFinder: public LoadCommandHandler {
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Stupid style nit: space before the : in “: public”. It seems kind of stupid,
but
> I actually search for subclasses by searching for “ : public B”.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode422
Line 422: // Where we should store the segment if found.
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Say weak.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode505
Line 505: // The map under construction.
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Weak.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode511
Line 511: section_map->clear();
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Ah, there’s the answer to my question from yesterday. (Still, it ought to be
> documented in the header.)

Okay.

http://breakpad.appspot.com/93001/diff/6001/7002
File src/common/mac/macho_reader.h (right):

http://breakpad.appspot.com/93001/diff/6001/7002#newcode1
Line 1: // -*- mode: C++ -*-
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Mode line here but not in the .cc file. Want one there too?

They're only needed in header files, as Emacs can infer the mode from the
filename elsewhere.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode36
Line 36: #ifndef MAC_COMMON_MACHO_READER_H_
Okay.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode39
Line 39: extern "C" {
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> Not needed. It’s the standard library’s job to define everything properly,
> giving everything the proper linkage.

Of course. I don't know why I did that.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode48
Line 48: #include <mach-o/loader.h>
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> These ought to go with the C system headers.

D'oh.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode54
Line 54: namespace Mach_O {
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> mach_o (as in the other file).

Okay.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode71
Line 71: // 32-bit/64-bit PowerPC binaries. When applied to a (non-fat) Mach-O
file,
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> How is PowerPC relevant here? Any more relevant than x86 or x86_64, that is?

It seems like the OS X documentation talks a lot in terms these two specific use
cases, even though fat files are of course completely general. I just wanted to
match the mindset of the documentation. The whole dependent clause can go, as
far as I'm concerned; no reason to go along with people creating asymmetries
where there are none.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode97
Line 97: string filename_;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> I see no reason for this data member to be protected, make it private. The
only
> subclass I’m aware of (after a quick scan) is the one used for testing, and
that
> doesn’t make use of filename_ at all.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

Sure.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode105
Line 105: // Read the SIZE bytes at BUFFER as a fat binary file. On success,
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> Nit: |size|, |buffer| reads a little more easily than SIZE and BUFFER, and
since
> the case-sensitive names actually are lowercase…

Capitalization is GNU practice, but if |pipes| are Google style, then we should
try to fit in.  (They're Mozilla style as well.)  This is going to be a lot of
changes, though; I use CAPS in pretty much every per-function comment.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode129
Line 129: struct fat_arch *object_files(size_t *size) const {
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> |size| and |object_files_size_| make it sound like these are concerned with
the
> size (in bytes) of the object files. |count| and |object_files_count_| may be
> better.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode135
Line 135: // We use this to report problems parsing the file's contents.
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> It’s intended that this is weak as opposed to owned by the FatReader, right?
If
> so, say so.

Yes, it's weak.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode143
Line 143: uint32_t magic_;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> Type Magic?

Yes.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode143
Line 143: uint32_t magic_;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> This is the only field that’s not explicitly initialized and that won’t
> automatically initialize itself. Do you want to provide magic_(0) in the
> constructor? (I haven’t looked at the implementation yet to see whether it
might
> be used uninitialized.)

I believe it can only be used uninitialized if the client ignores an error
return from Read. And initializing variables of this sort conceals bugs from
valgrind. Not sure how you feel about that vs. defensive programming...

Ideally, valgrind would have a less verbose way to say, "I am initializing this
memory with a value which correct code should never access".  You can do it, but
it's a pain.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode158
Line 158: bool bits_64;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> For better packing, maybe you should put this last, and move the ByteBuffers
up
> here. Same with Section.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode174
Line 174: uint32_t maxprot;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> vm_prot_t for compatibility with segment_command and segment_command_64?
> (vm_prot_t is int, so signed, as opposed to uint32_t.)

That wouldn't be wrong, but I believe I use the ByteCursor::>> operator to put
values in this field; if so, it sounds off-key to use a host-sized type (int)
when reading a target-sized value (vm_prot_t).  It's documented to be a 32-bit
field.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode197
Line 197: string section_name, segment_name;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> It’s exceedingly rare for there to be more than one declaration on a line,
> especially in structs (and classes). I almost missed that segment_name was
here
> and was about to say “but wait, the segment name is a property of the section
> too!” Could you put segment_name on its own line?

Sure.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode206
Line 206: uint32_t alignment;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> Perhaps you should call this |align| for compatibility with section and
> section_64.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode219
Line 219: typedef map<std::string, Section> SectionMap;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> std:: unnecessary given your using-declaration.

Sure.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode287
Line 287: string filename_;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> As above, I think this can be private.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode294
Line 294: public:
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> nit: one more space of indent on this guy.

D'oh.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode300
Line 300: virtual bool operator()(const Section &section) = 0;
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> There isn’t a compelling reason to use operator() here. Can you make this a
> proper named method

Okay.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode338
Line 338: virtual bool SymtabCommand(const ByteBuffer &entries,
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> symbols is a better name than strings, I think.

(Not sure which comment you'll read first...) I think of a "symbol" as
comprising a fixed-size "entry" and a variable-length "string".  But "strings"
could be "names"; would that be better?

http://breakpad.appspot.com/93001/diff/6001/7002#newcode393
Line 393: // Populate MAP with a map of the sections in SEGMENT, from section
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> As I read the comment, I wonder if |section_map| (note: neither MAP nor |map|)
> will be cleared by this method or not.

Yes, this comment should specify that it is cleared.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode405
Line 405: // We use this to report problems parsing the file's contents.
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> Weak?

Yes.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode413
Line 413: bool big_endian_;
On 2010/06/03 01:31:18, Mark Mentovai wrote:
> And I wonder if any of the rest of the fields need to be initialized.

See prior comment about more accurate valgrind checks.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode419
Line 419: cpu_type_t cpu_type_;
On 2010/06/03 19:18:55, Mark Mentovai wrote:
> You should put comments on each of these identifying the field names used by
the
> system’s mach_header and mach_header_64 structs. cputype, cpusubtype,
filetype,
> ncmds, sizeofcmds. The same goes for the FatReader class, and the segname and
> sectname fields in the Section struct. I made a similar request in the .cc
file,
> to help someone approaching the code for the first time to quickly locate
where
> a field is handled based on the known name used by the system headers.

Okay.  I just found the official OS X names really obscure, but I guess this is
its own form of obscurity.

http://breakpad.appspot.com/93001/diff/6001/7003
File src/common/mac/macho_reader_unittest.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7003#newcode211
Line 211: .Append(10 * 5 * 4, '*');  // dummy architecture entries
On 2010/06/03 19:27:55, Mark Mentovai wrote:
> 5 and 4 are magic. Can you use sizeof(fat_arch) from <mach-o/fat.h>? Again on
> line 227.

Using sizeof to talk about the size of a target structure smells bad, even when
it gives the right answer. I could make this a manifest constant.

http://breakpad.appspot.com/93001/diff/6001/7006
File src/tools/mac/dump_syms/macho_dump.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7006#newcode35
Line 35: extern "C" {
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> Unnecessary.

Actually, is this really unnecessary for headers like <fcntl.h> and
<mach-o/arch.h>, which are not required by the C++ standard to work? I thought
only those mentioned in D.5 could be used without extern "C".

http://breakpad.appspot.com/93001/diff/6001/7006#newcode41
Line 41: #include "unistd.h"
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> <unistd.h>, not "unistd.h>

D'oh.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode44
Line 44: #include <errno.h>
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> C system header belongs in the batch above.

D'oh.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode48
Line 48: #include <string.h>
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> Me too.

D'oh.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode55
Line 55: using std::cerr;
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> Prefer fprintf(stderr, …) and printf(…) from <stdio.h> to these.

Understood.  Don't know why I did it that way.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode66
Line 66: string program_name;
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> static or anonymous namespace?

If we need an anonymous namespace for things below, then I guess we might as
well go with that here.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode124
Line 124: program_name = argv[0];
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> program_name = basename(argv[0]), basename is from <libgen.h>.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode132
Line 132: const char *filename = argv[i];
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> Refactor the loop body into its own function that accepts a const char
*filename
> argument.

Okay.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode133
Line 133: int fd = check_syscall(open(argv[i], O_RDONLY), "opening", filename);
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> Never closed (fd leak). You can close immediately after mmap.

Oops.  Thanks.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode137
Line 137: void *mapping = mmap(NULL, attributes.st_size, PROT_READ,
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> Never unmapped (memory leak).
> 
> I wouldn’t care about either of these in this test program if you weren’t
inside
> a loop.

Oops.

http://breakpad.appspot.com/93001/diff/6001/7006#newcode146
Line 146: exit(1);
On 2010/06/03 19:38:44, Mark Mentovai wrote:
> {}

Okay.

[Mark Mentovai]

http://breakpad.appspot.com/93001/diff/6001/7001
File src/common/mac/macho_reader.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7001#newcode85
Line 85: if (cursor >> magic_) {
jimb wrote:
> Well, this >> is a custom operator on a ByteCursor.  I'm pretty fond of it, so
> I'd like to keep it.

Yup, I agree. Keep ByteCursor as-is. I like it too.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode197
Line 197: << "the contents of load command #" << i << ","
jimb wrote:
> Agree this should use <stdio.h>.  Re: the colon: I think you're thinking about
> this from the point of view of the compiled code, not the reader.  A reader
> wants to see the colon with the word it's attached to.

You’re right, I’m thinking of object code.

If you use a string literal that continues from line to line (as you probably
would do when you convert to <stdio.h>), then you get to keep the best of all
worlds.

"this is all "
"a single const char*."

nice.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode237
Line 237: bool Reader::Read(const uint8_t *buffer, size_t size) {
jimb wrote:
> I see your point.  Conceptually, though, mach_o::Reader could be used
directly,
> in which case one wouldn't know the cpu type/subtype yet.  Perhaps a new
> overloading of this method that accepts the expected type/subtype, calls this
> version, and then does the consistency check?

cpu_type_t 0 is not defined, and cpu_type_t -1 is CPU_TYPE_ANY. You could do
this without the override by doing the consistency check only if the expected
value is not CPU_TYPE_ANY. How’s that?

http://breakpad.appspot.com/93001/diff/6001/7001#newcode276
Line 276: // What's that thing Ruby programmers like to say --- "DRY"?  What is
that?
jimb wrote:
> (believe it or not, I was originally trying to make this completely
> cross-platform, so that you could build the Mac dumper on Linux or whatever,
so
> I had enum definitions... bad overreach), so I couldn't trust the size.  Now,
> assuming these members are using the right typedefs, reading directly into
them
> would be fine.

I was thinking about this as I read this. A better way to make this work on
other platforms would be to provide skeleton versions of some of those mach-o
headers. As it stands, it doesn’t really matter much, because the driver program
is still an .mm. (It would be OK by me if you wanted to redo it to avoid
Foundation and other Mac-specific bits, but it’s also fine as-is. ’Course if you
rewrote it in terms of straight libc, I wouldn’t be able to find fault with your
use of errno.)

http://breakpad.appspot.com/93001/diff/6001/7001#newcode330
Line 330: if (!reporter_->LoadCommandTooShort(index, type))
jimb wrote:
> There are no subclasses of the reporters, except for unit testing. The whole
> idea of allowing some reporter methods to return true to allow the process to
> continue doesn't really get used in the dumper. It may be better to just let
> someone else add that in if it's ever needed.

Yup, let’s keep it simpler, make ’em return void, and get rid of these few
tests.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode367
Line 367: // How big are the entries in the symbol table?
jimb wrote:
> I think I'd rather go with the comment.  I feel uncomfortable using sizeof
since
> we want the size on the target machine, not on our machine.  I know the
> structures are defined with exact-width types, but you can still have
alignment
> vary from platform to platform.  It's picky, I know.

I recognized your aversion to sizeof throughout and can’t really fault you for
it. This sounds fine.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode377
Line 377: ByteBuffer entries(buffer_.start + symoff, symbols_size);
jimb wrote:
> I guess think of a "symbol" as comprising a fixed-width "entry" and a
> variable-length "string".  How about 'names' instead of 'strings'?

“name” only makes sense if you understand what “nlist” means. Fortunately, we
both do. Go for it. :)

http://breakpad.appspot.com/93001/diff/6001/7002
File src/common/mac/macho_reader.h (right):

http://breakpad.appspot.com/93001/diff/6001/7002#newcode143
Line 143: uint32_t magic_;
jimb wrote:
> I believe it can only be used uninitialized if the client ignores an error
> return from Read. And initializing variables of this sort conceals bugs from
> valgrind. Not sure how you feel about that vs. defensive programming...
> 
> Ideally, valgrind would have a less verbose way to say, "I am initializing
this
> memory with a value which correct code should never access".  You can do it,
but
> it's a pain.

It’s fine as-is. I made the comment before I looked at the implementation -
there were already enough other things I wanted to remember to follow up on that
I just stuck this concern in a comment so you (or I) could look later instead of
interrupting myself to chase down yet another bogey.

I like valgrind too!

http://breakpad.appspot.com/93001/diff/6001/7002#newcode174
Line 174: uint32_t maxprot;
jimb wrote:
> That wouldn't be wrong, but I believe I use the ByteCursor::>> operator to put
> values in this field; if so, it sounds off-key to use a host-sized type (int)
> when reading a target-sized value (vm_prot_t).  It's documented to be a 32-bit
> field.

True. That’s fine, then.

http://breakpad.appspot.com/93001/diff/6001/7002#newcode419
Line 419: cpu_type_t cpu_type_;
jimb wrote:
> Okay.  I just found the official OS X names really obscure, but I guess this
is
> its own form of obscurity.

I certainly like your names better. Keep those, just put the system’s names into
comments for cross-referencing.

http://breakpad.appspot.com/93001/diff/6001/7003
File src/common/mac/macho_reader_unittest.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7003#newcode211
Line 211: .Append(10 * 5 * 4, '*');  // dummy architecture entries
jimb wrote:
> Using sizeof to talk about the size of a target structure smells bad, even
when
> it gives the right answer. I could make this a manifest constant.

If sizeof(fat_arch) differs on various Apple targets, then someone’s in deep
doo-doo. ’Course, it’s possible that this might run on a non-Apple with its own
weird rules. On the other hand, I wouldn’t have known what 5 * 4 meant or
whether it was right unless I was simultaneously consulting <mach-o/fat.h> while
reading your patch (which I was, of course). But I doubt future readers will
have a copy of that header fresh in their minds when they’re trying to figure
out what this test does.

http://breakpad.appspot.com/93001/diff/6001/7006
File src/tools/mac/dump_syms/macho_dump.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7006#newcode35
Line 35: extern "C" {
jimb wrote:
> Actually, is this really unnecessary for headers like <fcntl.h> and
> <mach-o/arch.h>, which are not required by the C++ standard to work? I thought
> only those mentioned in D.5 could be used without extern "C".

Very good point! I think we can agree that no reasonable system is going to
break this, and that no new system is going to be so obstinate as to break all
of the world’s existing code. If you want to keep the “extern” on the headers
not listed in D.5, you should at least provide a comment, like

extern "C" {  // Headers not listed in D.5.

I’d still err on the no-extern side of things, but you can do it this way if you
prefer.

[jimb]

http://breakpad.appspot.com/93001/diff/6001/7001
File src/common/mac/macho_reader.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7001#newcode197
Line 197: << "the contents of load command #" << i << ","
On 2010/06/03 21:16:21, Mark Mentovai wrote:
> jimb wrote:
> > Agree this should use <stdio.h>.  Re: the colon: I think you're thinking
about
> > this from the point of view of the compiled code, not the reader.  A reader
> > wants to see the colon with the word it's attached to.
> 
> You’re right, I’m thinking of object code.
> 
> If you use a string literal that continues from line to line (as you probably
> would do when you convert to <stdio.h>), then you get to keep the best of all
> worlds.
> 
> "this is all "
> "a single const char*."
> 
> nice.

Works for me.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode237
Line 237: bool Reader::Read(const uint8_t *buffer, size_t size) {
On 2010/06/03 21:16:21, Mark Mentovai wrote:
> jimb wrote:
> > I see your point.  Conceptually, though, mach_o::Reader could be used
> directly,
> > in which case one wouldn't know the cpu type/subtype yet.  Perhaps a new
> > overloading of this method that accepts the expected type/subtype, calls
this
> > version, and then does the consistency check?
> 
> cpu_type_t 0 is not defined, and cpu_type_t -1 is CPU_TYPE_ANY. You could do
> this without the override by doing the consistency check only if the expected
> value is not CPU_TYPE_ANY. How’s that?

Works for me.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode276
Line 276: // What's that thing Ruby programmers like to say --- "DRY"?  What is
that?
On 2010/06/03 21:16:21, Mark Mentovai wrote:
> I was thinking about this as I read this. A better way to make this work on
> other platforms would be to provide skeleton versions of some of those mach-o
> headers. As it stands, it doesn’t really matter much, because the driver
program
> is still an .mm. (It would be OK by me if you wanted to redo it to avoid
> Foundation and other Mac-specific bits, but it’s also fine as-is. ’Course if
you
> rewrote it in terms of straight libc, I wouldn’t be able to find fault with
your
> use of errno.)

I got to the point where I realized that I really had no idea what those bundle
management functions were doing, and didn't want to try a cross-platform
implementation of those, and gave up.

http://breakpad.appspot.com/93001/diff/6001/7001#newcode330
Line 330: if (!reporter_->LoadCommandTooShort(index, type))
On 2010/06/03 21:16:21, Mark Mentovai wrote:
> jimb wrote:
> > There are no subclasses of the reporters, except for unit testing. The whole
> > idea of allowing some reporter methods to return true to allow the process
to
> > continue doesn't really get used in the dumper. It may be better to just let
> > someone else add that in if it's ever needed.
> 
> Yup, let’s keep it simpler, make ’em return void, and get rid of these few
> tests.

Works for me.

http://breakpad.appspot.com/93001/diff/6001/7002
File src/common/mac/macho_reader.h (right):

http://breakpad.appspot.com/93001/diff/6001/7002#newcode419
Line 419: cpu_type_t cpu_type_;
On 2010/06/03 21:16:21, Mark Mentovai wrote:
> jimb wrote:
> > Okay.  I just found the official OS X names really obscure, but I guess this
> is
> > its own form of obscurity.
> 
> I certainly like your names better. Keep those, just put the system’s names
into
> comments for cross-referencing.

All right.

http://breakpad.appspot.com/93001/diff/6001/7003
File src/common/mac/macho_reader_unittest.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7003#newcode211
Line 211: .Append(10 * 5 * 4, '*');  // dummy architecture entries
On 2010/06/03 21:16:21, Mark Mentovai wrote:
> jimb wrote:
> > Using sizeof to talk about the size of a target structure smells bad, even
> when
> > it gives the right answer. I could make this a manifest constant.
> 
> If sizeof(fat_arch) differs on various Apple targets, then someone’s in deep
> doo-doo. ’Course, it’s possible that this might run on a non-Apple with its
own
> weird rules. On the other hand, I wouldn’t have known what 5 * 4 meant or
> whether it was right unless I was simultaneously consulting <mach-o/fat.h>
while
> reading your patch (which I was, of course). But I doubt future readers will
> have a copy of that header fresh in their minds when they’re trying to figure
> out what this test does.

Agreed that '5 * 4' is pretty horrible.

http://breakpad.appspot.com/93001/diff/6001/7006
File src/tools/mac/dump_syms/macho_dump.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7006#newcode35
Line 35: extern "C" {
On 2010/06/03 21:16:21, Mark Mentovai wrote:
> I’d still err on the no-extern side of things, but you can do it this way if
you
> prefer.

If it is de facto safe, then we should get rid of the extern "C" crud.  I wasn't
sure what the reality on the ground was.

[Mark Mentovai]

http://breakpad.appspot.com/93001/diff/6001/7001
File src/common/mac/macho_reader.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7001#newcode276
Line 276: // What's that thing Ruby programmers like to say --- "DRY"?  What is
that?
jimb wrote:
> I got to the point where I realized that I really had no idea what those
bundle
> management functions were doing, and didn't want to try a cross-platform
> implementation of those, and gave up.

“Is it a directory? Try adding Contents/Resources/DWARF/name. Does that exist?
Good!” It’s that simple. But like I said, I don’t mind this being Mac-only.
Certainly not for now. Certainly not given that there’d still need to be a
fallback header to provide compatible definitions on other platforms.

[jimb]

http://breakpad.appspot.com/93001/diff/6001/7003
File src/common/mac/macho_reader_unittest.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7003#newcode211
Line 211: .Append(10 * 5 * 4, '*');  // dummy architecture entries
I've put up a patch that fixes this in a nice way (and makes the tests shorter
and more readable) up here:

http://hg.mozilla.org/users/jblandy_mozilla.com/breakpad-mq/file/586ba9341429...

[Mark Mentovai]

http://breakpad.appspot.com/93001/diff/6001/7003
File src/common/mac/macho_reader_unittest.cc (right):

http://breakpad.appspot.com/93001/diff/6001/7003#newcode211
Line 211: .Append(10 * 5 * 4, '*');  // dummy architecture entries
jimb wrote:
> I've put up a patch that fixes this in a nice way (and makes the tests shorter
> and more readable) up here:
> 
>
http://hg.mozilla.org/users/jblandy_mozilla.com/breakpad-mq/file/586ba9341429...

I like that!

[Ted Mielczarek]

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