Add utilities for processing Linux core dump files.

Add utilities for processing Linux core dump files.

This patch is part of a bigger patch that helps merging the breakpad code
with the modified version in Chromium OS.

Specifically, this patch makes the following changes:
1. Add an ElfCoreDump class for processing Linux core dump files, which will
   later be used to implement the core dump to minidump conversion.
2. Add a CrashGenerator class for generating a crash with a core dump file
   for testing the functionalities of ElfCoreDump.
3. Move some utility functions for reading/writing files to file_utils.h.

BUG=455
TEST=Tested the following:
1. Build on 32-bit and 64-bit Linux with gcc 4.4.3 and gcc 4.6.
2. Build on Mac OS X 10.6.8 with gcc 4.2 and clang 3.0 (with latest gmock).
3. All unit tests pass.

[Lei Zhang (chromium)]

http://breakpad.appspot.com/337001/diff/1/5
File src/common/linux/elf_core_dump.h (right):

http://breakpad.appspot.com/337001/diff/1/5#newcode51
Line 51: class ElfCoreDump {
Some of the Breakpad tools like md2core only works on the architecture it's
built on. i.e. 32-bit md2core only works with 32-bit .dmp files. Whereas other
tools like dump_syms works with 32 or 64-bit files whether the dump_syms binary
is 32 or 64-bit. (see
http://code.google.com/p/google-breakpad/source/detail?r=677)

I can't see how this will be used aside from the unit test. If it's going to be
called from a tool like dump_syms, it would be nice if we don't regress that
tool's bitness-independence.

http://breakpad.appspot.com/337001/diff/1/11
File src/common/tests/file_utils.cc (right):

http://breakpad.appspot.com/337001/diff/1/11#newcode43
Line 43: bool ReadFile(const char* path, void* buffer, ssize_t* buffer_size) {
Have you considered using the Chromium src/base/file_utils_posix.cc version of
ReadFile/WriteFile? At first glance, it looks like the Chromium version checks
the return value of close() and handles partial writes.

[Ben Chan]

http://breakpad.appspot.com/337001/diff/1/5
File src/common/linux/elf_core_dump.h (right):

http://breakpad.appspot.com/337001/diff/1/5#newcode51
Line 51: class ElfCoreDump {
On 2012/01/03 23:45:09, thestig wrote:
> Some of the Breakpad tools like md2core only works on the architecture it's
> built on. i.e. 32-bit md2core only works with 32-bit .dmp files. Whereas other
> tools like dump_syms works with 32 or 64-bit files whether the dump_syms
binary
> is 32 or 64-bit. (see
> http://code.google.com/p/google-breakpad/source/detail?r=677)
> 
> I can't see how this will be used aside from the unit test. If it's going to
be
> called from a tool like dump_syms, it would be nice if we don't regress that
> tool's bitness-independence.

For now, it's only used by core2md in chromium-os where we assume no 32/64-bit
combinations. But if we need to support 32/64 combinations, we can easily turn
this class into a template class with 32 and 64-bit instantiations.

http://breakpad.appspot.com/337001/diff/1/11
File src/common/tests/file_utils.cc (right):

http://breakpad.appspot.com/337001/diff/1/11#newcode43
Line 43: bool ReadFile(const char* path, void* buffer, ssize_t* buffer_size) {
On 2012/01/03 23:45:09, thestig wrote:
> Have you considered using the Chromium src/base/file_utils_posix.cc version of
> ReadFile/WriteFile? At first glance, it looks like the Chromium version checks
> the return value of close() and handles partial writes.

Done. It now handles close errors and partial writes.

[kmixter]

http://breakpad.appspot.com/337001/diff/1/5
File src/common/linux/elf_core_dump.h (right):

http://breakpad.appspot.com/337001/diff/1/5#newcode51
Line 51: class ElfCoreDump {
On 2012/01/04 00:16:07, Ben Chan wrote:
> On 2012/01/03 23:45:09, thestig wrote:
> > Some of the Breakpad tools like md2core only works on the architecture it's
> > built on. i.e. 32-bit md2core only works with 32-bit .dmp files. Whereas
other
> > tools like dump_syms works with 32 or 64-bit files whether the dump_syms
> binary
> > is 32 or 64-bit. (see
> > http://code.google.com/p/google-breakpad/source/detail?r=677)
> > 
> > I can't see how this will be used aside from the unit test. If it's going to
> be
> > called from a tool like dump_syms, it would be nice if we don't regress that
> > tool's bitness-independence.
> 
> For now, it's only used by core2md in chromium-os where we assume no 32/64-bit
> combinations. But if we need to support 32/64 combinations, we can easily turn
> this class into a template class with 32 and 64-bit instantiations.

+1.  I think it will be a change that involves a lot of lines of code to change
to support 32 or 64b dynamically but not too conceptually hard.  I'd suggest
having the 64b sizes always be stored and the 32b sizes promoted up to them upon
reading.  Note that we'd also need to handle endianness differences (and
optionally swizzle bits in addition to promoting sizes).  It's not clear that
that complexity is going to be necessary, so I'd agree that it's worth punting
on that for now, though making sure core2md properly complains if it cannot
handle the ELF file due to bit-size or endian mismatch.

http://breakpad.appspot.com/337001/diff/1/11
File src/common/tests/file_utils.cc (right):

http://breakpad.appspot.com/337001/diff/1/11#newcode1
Line 1: // Copyright (c) 2011, Google Inc.
nit: might as well update to 2012.

http://breakpad.appspot.com/337001/diff/13/5004
File src/common/linux/elf_core_dump.h (right):

http://breakpad.appspot.com/337001/diff/13/5004#newcode51
Line 51: class ElfCoreDump {
nit: Usually we try to name classes with nouns describing what they do, like
ElfCoreDumpReader or ElfCoreReader (or just CoreReader) unless the class is just
storing data (like Note is for instance).

http://breakpad.appspot.com/337001/diff/13/5004#newcode69
Line 69: class Note {
This interface feels like a combination of a list and iterator (with the Next
function and the FirstNote function).  Would be nice to use a more common
pattern by creating a separate NoteList and iterating over it.

CoreReader::NoteList::Iterator i = core_reader.note_list.begin();
while (i != core_reader.note_list.end()) {
  i->GetType() ...;
}

That's probably overkill for how often this is used and the operator
overloading.  What about calling this NoteIterator and making Next update in
place?  I'm guessing you don't need to have multiple of these Note objects alive
at once, but may be wrong.

http://breakpad.appspot.com/337001/diff/13/5004#newcode82
Line 82: const Nhdr* Header() const;
nit: Usually accessors start with Get here and below (specifically functions
start with verbs).

http://breakpad.appspot.com/337001/diff/13/5004#newcode114
Line 114: void SetContent(const MemoryRange& content);
I assume this is just for testing purposes.  Should mention that in the comment.

http://breakpad.appspot.com/337001/diff/13/5004#newcode130
Line 130: const Phdr* ProgramHeaderOfType(Word type) const;
mention that this gets the *first* phdr of the given type - in the name ideally.

http://breakpad.appspot.com/337001/diff/13/5007
File src/common/linux/tests/crash_generator.cc (right):

http://breakpad.appspot.com/337001/diff/13/5007#newcode137
Line 137: bool CrashGenerator::CreateChildCrash(unsigned num_threads, unsigned
crash_thread,
80 chars

http://breakpad.appspot.com/337001/diff/13/5007#newcode175
Line 175: pthread_attr_setdetachstate(&thread_attributes,
comment on why this is necessary?

[Ben Chan]

http://breakpad.appspot.com/337001/diff/13/5004
File src/common/linux/elf_core_dump.h (right):

http://breakpad.appspot.com/337001/diff/13/5004#newcode51
Line 51: class ElfCoreDump {
On 2012/01/05 02:43:55, kmixter wrote:
> nit: Usually we try to name classes with nouns describing what they do, like
> ElfCoreDumpReader or ElfCoreReader (or just CoreReader) unless the class is
just
> storing data (like Note is for instance).

I was considering both ElfCoreDump and ElfCoreDump::Note as data structure-like
classes as they simply wrap/contain a MemoryRange object and provide convenience
functions to access MemoryRange.   There will be a reader/dumper that actually
maps a file and interprets the file content by wrapping it using ElfCoreDump.

http://breakpad.appspot.com/337001/diff/13/5004#newcode69
Line 69: class Note {
On 2012/01/05 02:43:55, kmixter wrote:
> This interface feels like a combination of a list and iterator (with the Next
> function and the FirstNote function).  Would be nice to use a more common
> pattern by creating a separate NoteList and iterating over it.
> 
> CoreReader::NoteList::Iterator i = core_reader.note_list.begin();
> while (i != core_reader.note_list.end()) {
>   i->GetType() ...;
> }
> 
> That's probably overkill for how often this is used and the operator
> overloading.  What about calling this NoteIterator and making Next update in
> place?  I'm guessing you don't need to have multiple of these Note objects
alive
> at once, but may be wrong.

I thought about modeling it as a C++ InputIterator but that seems a bit overkill
for our purpose.

http://breakpad.appspot.com/337001/diff/13/5004#newcode82
Line 82: const Nhdr* Header() const;
On 2012/01/05 02:43:55, kmixter wrote:
> nit: Usually accessors start with Get here and below (specifically functions
> start with verbs).

Done.

I once felt that the Get prefix seems redundant as most of the methods are
getters. Perhaps the lower_case_getter() style (for member variables) may be
better. Anyway, I changed all getters to GetXXX().

http://breakpad.appspot.com/337001/diff/13/5004#newcode114
Line 114: void SetContent(const MemoryRange& content);
On 2012/01/05 02:43:55, kmixter wrote:
> I assume this is just for testing purposes.  Should mention that in the
comment.

It's used to assign the content when the ElfCoreDump is default constructed.

http://breakpad.appspot.com/337001/diff/13/5004#newcode130
Line 130: const Phdr* ProgramHeaderOfType(Word type) const;
On 2012/01/05 02:43:55, kmixter wrote:
> mention that this gets the *first* phdr of the given type - in the name
ideally.

Done. Renamed the method to GetFirstProgramHeaderOfType. Also updated the
comment.

http://breakpad.appspot.com/337001/diff/13/5007
File src/common/linux/tests/crash_generator.cc (right):

http://breakpad.appspot.com/337001/diff/13/5007#newcode137
Line 137: bool CrashGenerator::CreateChildCrash(unsigned num_threads, unsigned
crash_thread,
On 2012/01/05 02:43:55, kmixter wrote:
> 80 chars

Done.

http://breakpad.appspot.com/337001/diff/13/5007#newcode175
Line 175: pthread_attr_setdetachstate(&thread_attributes,
On 2012/01/05 02:43:55, kmixter wrote:
> comment on why this is necessary?

Done.

[Ben Chan]

http://breakpad.appspot.com/337001/diff/10001/11007
File src/common/linux/tests/crash_generator.cc (right):

http://breakpad.appspot.com/337001/diff/10001/11007#newcode102
Line 102: return reinterpret_cast<pid_t*>(shared_memory_) + index;
This change is just to avoid cpplint warning.

[kmixter]

LGTM