Allow MinidumpGenerator to dump processes of different CPU architecture

This patch allows MinidumpGenerator to dump processes that are of a different
CPU architecture than the current process. This allows for dumping i386
processes from x86-64 processes running on the same system (and vice versa).

For Firefox 4, we're shipping a universal 32/64 binary, and running some
32-bit plugins out-of-process when running a 64-bit Firefox. We need 
these changes in order for crash reporting to work on the plugin processes.

This patch wound up being a lot larger than I expected, mostly due to all
the changes necessary in dynamic_images and having to mess with the
nlist implementation. Most of it is more refactoring than anything else,
just taking code that used to assume 32 or 64-bit mach structures based
on the current architecture and changing it to be templated and choosing
the structures based on the architecture of the process being dumped.

The CrossArchitectureDump test passes on my 10.6 system for both
x86-64 and i386 (if I flip the project settings to use the 10.6 SDK). I
don't have access to a 10.5 machine locally, so I don't know if it would
actually work on a 64-bit capable machine running 10.5, so I decided to
punt on it.

I don't have access to PPC hardware either, so I can't run the tests on
PPC. They fail under Rosetta, but they failed under Rosetta before my patch
anyway.

[Mark Mentovai]

I just read the first few files.

After reading this, I’m on the fence about having you fix all of the existing
problems in the file now. I’ll leave it up to your judgment.

http://breakpad.appspot.com/241001/diff/1/3
File src/client/mac/handler/breakpad_nlist_64.cc (right):

http://breakpad.appspot.com/241001/diff/1/3#newcode115
Line 115: struct MachBits <nlist32> {
The template parameters should be snuggled up against what precedes them. Here
and on line 121, write:

template<>
struct MachBits<nlist##> {

http://breakpad.appspot.com/241001/diff/1/3#newcode142
Line 142: int fd, n;
Don’t declare things until you use them. Existing and throughout.

http://breakpad.appspot.com/241001/diff/1/3#newcode148
Line 148: (void)close(fd);
We don’t really need the (void) cast.

http://breakpad.appspot.com/241001/diff/1/3#newcode149
Line 149: return (n);
And you don’t need to parenthesize a return value. Same up on line 146.

http://breakpad.appspot.com/241001/diff/1/3#newcode175
Line 175: const uint32_t magic = MachBits<nlist_type>::magic;
Gimme a blank line before, to separate the typedefs from the other stuff.

http://breakpad.appspot.com/241001/diff/1/3#newcode177
Line 177: register nlist_type *p, *q;
Don’t declare stuff until you use it.

http://breakpad.appspot.com/241001/diff/1/4
File src/client/mac/handler/breakpad_nlist_64.h (right):

http://breakpad.appspot.com/241001/diff/1/4#newcode39
Line 39: breakpad_nlist(const char *name,
Do you want to just expose this as breakpad_nlist<nlist_type>?

http://breakpad.appspot.com/241001/diff/1/5
File src/client/mac/handler/dynamic_images.cc (right):

http://breakpad.appspot.com/241001/diff/1/5#newcode215
Line 215: typedef typename MachBits::mach_segment_command_type
mach_segment_command_type;
80

http://breakpad.appspot.com/241001/diff/1/5#newcode218
Line 218: bool foundDylibIDCommand = false;
foundTextSection and foundDylibIDCommand should move down closer to where you
need them—the loop.

http://breakpad.appspot.com/241001/diff/1/5#newcode226
Line 226: reinterpret_cast<const struct load_command *>(header + 1);
Continuation lines get a 4-space indent. 262 too.

http://breakpad.appspot.com/241001/diff/1/5#newcode240
Line 240: image.slide_ = (uintptr_t)image.GetLoadAddress() -
(uintptr_t)seg->vmaddr;
80

http://breakpad.appspot.com/241001/diff/1/5#newcode307
Line 307: : task_(task),
4-space indento n this line and make the following lines of the initializer list
line up.

http://breakpad.appspot.com/241001/diff/1/5#newcode319
Line 319: nlist_type l[8];
l is kind of a crappy name.

http://breakpad.appspot.com/241001/diff/1/5#newcode320
Line 320: nlist_type &list = l[0];
Same thing about declaring closer to point of use.

http://breakpad.appspot.com/241001/diff/1/5#newcode322
Line 322: memset(l, 0, sizeof(l));
You could have used an {} initializer list to zero this out when you delcared
it.

http://breakpad.appspot.com/241001/diff/1/5#newcode358
Line 358: kern_return_t kr;
...

http://breakpad.appspot.com/241001/diff/1/5#newcode366
Line 366: (ReadTaskMemory(images.task_,
Fix the continuation indent.

http://breakpad.appspot.com/241001/diff/1/5#newcode433
Line 433: free(file_path);
Having memory managed C-style in this way is a little scary. Can we get some
kind of scoper in here to own stuff?

http://breakpad.appspot.com/241001/diff/1/5#newcode441
Line 441: sort(images.image_list_.begin(), images.image_list_.end() );
images.image_list_.end());

[Ted Mielczarek]

On 2010/12/09 02:40:28, Mark Mentovai wrote:
> I just read the first few files.
> 
> After reading this, I’m on the fence about having you fix all of the existing
> problems in the file now. I’ll leave it up to your judgment.

Yeah, there are a lot of ugly bits in the existing code. The breakpad_nlist_64
bits were all originally C code
(http://www.opensource.apple.com/darwinsource/10.5.2/Libc-498/gen/nlist.c) that
Neal lightly ported, and some of the other code is just weird. I was worried
about trying to clean everything up since this patch wound up being so large
anyway, but now that I've got it working I can go back and clean things up a
bit.

http://breakpad.appspot.com/241001/diff/1/4
File src/client/mac/handler/breakpad_nlist_64.h (right):

http://breakpad.appspot.com/241001/diff/1/4#newcode39
Line 39: breakpad_nlist(const char *name,
On 2010/12/09 02:40:29, Mark Mentovai wrote:
> Do you want to just expose this as breakpad_nlist<nlist_type>?

I'd have to inline them into the header then, which is why I avoided it. Stupid
template rules. :-/

http://breakpad.appspot.com/241001/diff/1/5
File src/client/mac/handler/dynamic_images.cc (right):

http://breakpad.appspot.com/241001/diff/1/5#newcode433
Line 433: free(file_path);
On 2010/12/09 02:40:29, Mark Mentovai wrote:
> Having memory managed C-style in this way is a little scary. Can we get some
> kind of scoper in here to own stuff?

Yeah, ReadTaskString has a crappy API. I might just change it to take a
vector<char>& to return the result in.

http://breakpad.appspot.com/241001/diff/1/5#newcode441
Line 441: sort(images.image_list_.begin(), images.image_list_.end() );
On 2010/12/09 02:40:29, Mark Mentovai wrote:
> images.image_list_.end());

Hrmph. I fixed a few other instances of this, but missed this one.

[Ted Mielczarek]

Ok, this should address all of your existing review comments. I rearranged quite
a bit of breakpad_nlist_64.cc to move variable declarations right before their
first usage and other things. I also changed the ReadTask{Memory,String} APIs to
use std::{vector,string} respectively, instead of directly allocating heap
memory, which makes using them slightly cleaner.

[Mark Mentovai]

LGTM otherwise

Sorry that this is mostly a style review. It’s large and monotonous.

http://breakpad.appspot.com/241001/diff/8001/9002
File src/client/mac/handler/breakpad_nlist_64.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9002#newcode147
Line 147: return (-1);
The () around the return expression is gratuitous. Get rid of it.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode170
Line 170: int
Return type goes on the next line.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode201
Line 201: unsigned  arch_offset = 0;
Too many spaces between the type and the variable name.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode205
Line 205: /* Get our host info */
/* Get host info */

http://breakpad.appspot.com/241001/diff/8001/9002#newcode218
Line 218: lseek(fd, 0, SEEK_SET);
Check the return value?

http://breakpad.appspot.com/241001/diff/8001/9002#newcode228
Line 228: (struct fat_arch *)malloc(fh.nfat_arch * sizeof(struct fat_arch));
Two more spaces needed. This is a continuation line.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode272
Line 272: lseek(fd, arch_offset, SEEK_SET);
Check the return value. What if the file had been truncated somewhere along the
line, and the seek fell short?

http://breakpad.appspot.com/241001/diff/8001/9002#newcode278
Line 278: off_t sa;             /* symbol address */
That’s a lot of space before the comments.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode282
Line 282: lseek(fd, arch_offset, SEEK_SET);
Check.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode289
Line 289: (struct load_command *)malloc(mh.sizeofcmds);
Continuation line, 2 more spaces.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode300
Line 300: // nealsid:iterate through all load commands, looking for
Should this be TODO(nealsid):?

http://breakpad.appspot.com/241001/diff/8001/9002#newcode335
Line 335: else {
Use the } else { cuddly style.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode341
Line 341: lseek(fd, sa, SEEK_SET);
Check this one too?

http://breakpad.appspot.com/241001/diff/8001/9002#newcode357
Line 357: long savpos = lseek(fd, 0, SEEK_CUR);
And this.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode366
Line 366: lseek(fd, ss+q->n_un.n_strx, SEEK_SET);
And this.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode367
Line 367: read(fd, nambuf, maxlen+1);
And this? (This looks like the only unchecked read.)

http://breakpad.appspot.com/241001/diff/8001/9002#newcode371
Line 371: symbolNames[p-list][0];
This will fit on the preceding line.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode378
Line 378: goto cont;
This is kind of ugly. Is there a cleaner way? (381 too.)

http://breakpad.appspot.com/241001/diff/8001/9002#newcode395
Line 395: lseek(fd, savpos, SEEK_SET);
And this?

http://breakpad.appspot.com/241001/diff/8001/9003
File src/client/mac/handler/breakpad_nlist_64.h (right):

http://breakpad.appspot.com/241001/diff/8001/9003#newcode38
Line 38: int
Move the return type to the next line, please. Same at 43-44 (becoming 42!)

http://breakpad.appspot.com/241001/diff/8001/9004
File src/client/mac/handler/dynamic_images.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9004#newcode142
Line 142: != KERN_SUCCESS)
!= on the preceding line reads a little better.

http://breakpad.appspot.com/241001/diff/8001/9004#newcode143
Line 143: return string("");
Is the "" necessary? string() is an empty string and may be slightly less work.

http://breakpad.appspot.com/241001/diff/8001/9004#newcode228
Line 228: bool foundTextSection = false;
found_text_section.

http://breakpad.appspot.com/241001/diff/8001/9004#newcode229
Line 229: bool foundDylibIDCommand = false;
found_dylib_id_command.

http://breakpad.appspot.com/241001/diff/8001/9004#newcode241
Line 241: if (seg->fileoff == 0  &&  seg->filesize != 0) {
Too much whitespace around the &&.

http://breakpad.appspot.com/241001/diff/8001/9004#newcode407
Line 407: sizeof(mach_header_type) + header->sizeofcmds;
More space.

http://breakpad.appspot.com/241001/diff/8001/9006
File src/client/mac/handler/minidump_generator.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9006#newcode320
Line 320: stack_memory) != KERN_SUCCESS)
You need {} here because you’ve got so many lines.

http://breakpad.appspot.com/241001/diff/8001/9006#newcode344
Line 344: break;
Break not needed after return.

http://breakpad.appspot.com/241001/diff/8001/9006#newcode362
Line 362: break;
And again.

http://breakpad.appspot.com/241001/diff/8001/9006#newcode380
Line 380: break;
And a third time.

http://breakpad.appspot.com/241001/diff/8001/9006#newcode393
Line 393: reinterpret_cast<ppc_thread_state_t *>(state);
Indent. Also on 401, 409, 417, 427…

http://breakpad.appspot.com/241001/diff/8001/9006#newcode490
Line 490: reinterpret_cast<ppc_thread_state64_t *>(state);
490, 550, 559, …

http://breakpad.appspot.com/241001/diff/8001/9006#newcode840
Line 840: return false;
{}

http://breakpad.appspot.com/241001/diff/8001/9007
File src/client/mac/handler/minidump_generator.h (right):

http://breakpad.appspot.com/241001/diff/8001/9007#newcode50
Line 50: const u_int64_t TOP_OF_THREAD0_STACK_64BIT = 0x00007fff5fbff000LL;
Yuck to hardcoding these. (I said the same thing when this was written many
years ago.)

http://breakpad.appspot.com/241001/diff/8001/9008
File src/client/mac/tests/minidump_generator_test.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9008#newcode38
Line 38: #if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
The stuff below seems to use 10_6?

http://breakpad.appspot.com/241001/diff/8001/9008#newcode279
Line 279: {
{ goes on the preceding line.

http://breakpad.appspot.com/241001/diff/8001/9009
File src/client/mac/tests/minidump_generator_test_helper.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9009#newcode43
Line 43: {
{ on the preceding line.

[Ted Mielczarek]

Thanks for the review. That's okay, it's a large and monotonous patch. Certainly
wound up being larger than I anticipated.

http://breakpad.appspot.com/241001/diff/8001/9002
File src/client/mac/handler/breakpad_nlist_64.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9002#newcode300
Line 300: // nealsid:iterate through all load commands, looking for
On 2010/12/15 18:53:24, Mark Mentovai wrote:
> Should this be TODO(nealsid):?

No, I think he was just annotating what he added at the time? I can just remove
the nealsid bit.

http://breakpad.appspot.com/241001/diff/8001/9002#newcode378
Line 378: goto cont;
On 2010/12/15 18:53:24, Mark Mentovai wrote:
> This is kind of ugly. Is there a cleaner way? (381 too.)

I don't know, I honestly haven't taken the time to fully understand this
function, so I'm worried about making more than simple mechanical changes to it
in the name of style. Changing the control flow is worrisome to me, so I'd
prefer to avoid it, even if it is truly ugly.

http://breakpad.appspot.com/241001/diff/8001/9004
File src/client/mac/handler/dynamic_images.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9004#newcode143
Line 143: return string("");
On 2010/12/15 18:53:24, Mark Mentovai wrote:
> Is the "" necessary? string() is an empty string and may be slightly less
work.

Probably not. I think I might have had a bare string literal there at one point
and decided the explicit constructor was better.

http://breakpad.appspot.com/241001/diff/8001/9006
File src/client/mac/handler/minidump_generator.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9006#newcode393
Line 393: reinterpret_cast<ppc_thread_state_t *>(state);
On 2010/12/15 18:53:24, Mark Mentovai wrote:
> Indent. Also on 401, 409, 417, 427…

Clearly I need to fix my Emacs settings. Sorry about that.

http://breakpad.appspot.com/241001/diff/8001/9007
File src/client/mac/handler/minidump_generator.h (right):

http://breakpad.appspot.com/241001/diff/8001/9007#newcode50
Line 50: const u_int64_t TOP_OF_THREAD0_STACK_64BIT = 0x00007fff5fbff000LL;
On 2010/12/15 18:53:24, Mark Mentovai wrote:
> Yuck to hardcoding these. (I said the same thing when this was written many
> years ago.)

I believe this is http://code.google.com/p/google-breakpad/issues/detail?id=247
.

http://breakpad.appspot.com/241001/diff/8001/9008
File src/client/mac/tests/minidump_generator_test.cc (right):

http://breakpad.appspot.com/241001/diff/8001/9008#newcode38
Line 38: #if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
On 2010/12/15 18:53:24, Mark Mentovai wrote:
> The stuff below seems to use 10_6?

Oops.