Allow passing info about known memory mappings to MinidumpWriter and ExceptionHandler

This patch adds an API to ExceptionHandler to add information about known
regions of memory. This is useful for when you have known memory mappings
that show up as anonymous memory maps in /proc/<pid>/maps, so LinuxDumper
doesn't know how to handle them.

For our Firefox Android builds, we have a custom library loader that maps
our shared libs out the APK into anonymous memory, which totally foils
LinuxDumper. Using this API we're able to feed the names and debug IDs of
our modules through ExceptionHandler and we get usable minidumps.

[Lei Zhang (chromium)]

I need to look at linux_dumper.cc for just a bit longer. The comments below are
mostly small nits.

http://breakpad.appspot.com/242001/diff/1/2
File src/client/linux/handler/exception_handler.cc (right):

http://breakpad.appspot.com/242001/diff/1/2#newcode501
Line 501: std::pair<MappingInfo, u_int8_t[sizeof(MDGUID)]> mapping;
MappingEntry

http://breakpad.appspot.com/242001/diff/1/3
File src/client/linux/handler/exception_handler.h (right):

http://breakpad.appspot.com/242001/diff/1/3#newcode254
Line 254: MappingList mapping_info_;
s/mapping_info_/mapping_list/ ?

http://breakpad.appspot.com/242001/diff/1/4
File src/client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/242001/diff/1/4#newcode602
Line 602: char module_identifier_buffer[37];
see comment in other unit test.

http://breakpad.appspot.com/242001/diff/1/8
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/242001/diff/1/8#newcode842
Line 842: size_t filename_len = mapping.name + filepath_len - filename_ptr;
|filename_len| lost its constness?

http://breakpad.appspot.com/242001/diff/1/8#newcode1284
Line 1284: const MappingList& mapping_info_;
Can we rename this to mapping_list_? mapping_info_ makes me think it's a
MappingInfo.

http://breakpad.appspot.com/242001/diff/1/9
File src/client/linux/minidump_writer/minidump_writer.h (right):

http://breakpad.appspot.com/242001/diff/1/9#newcode34
Line 34: #include <utility>
nit: c++ headers after c headers.

http://breakpad.appspot.com/242001/diff/1/10
File src/client/linux/minidump_writer/minidump_writer_unittest.cc (right):

http://breakpad.appspot.com/242001/diff/1/10#newcode97
Line 97: char module_identifier_buffer[37];
Can you make 37 a constant and mention it's sizeof(MDGUID) * 2 + 4 for dashes +
1 for null terminator? This value is used again on line 189.

http://breakpad.appspot.com/242001/diff/1/10#newcode147
Line 147: std::pair<MappingInfo, u_int8_t[sizeof(MDGUID)]> mapping;
nit: Isn't this a MappingEntry?

http://breakpad.appspot.com/242001/diff/1/10#newcode206
Line 206: printf("tempfile: %s\n", tempfile);
leftover debugging data?

http://breakpad.appspot.com/242001/diff/1/10#newcode253
Line 253: std::pair<MappingInfo, u_int8_t[sizeof(MDGUID)]> mapping;
nit: Isn't this a MappingEntry?

[Lei Zhang (chromium)]

http://breakpad.appspot.com/242001/diff/1/5
File src/client/linux/minidump_writer/linux_dumper.cc (left):

http://breakpad.appspot.com/242001/diff/1/5#oldcode243
Line 243: mapping->name[filename_len - sizeof(kDeletedSuffix) + 1] = '\0';
You can't remove this. Sorry, I didn't add more comments to explain what's going
on:

On Linux, in the case where the running binary has been deleted, we have
/path/to/binary, which shows up as "/path/to/binary (deleted)" in the mapping.

We can't read /path/to/binary nor "/path/to/binary (deleted)", since they don't
exist. Instead, we need to  read /proc/pid/exe to generate the file identifier.
However, we have to write /path/to/binary in the mapping name.

In the modified code, you save the mapping name as /proc/pid/exe, so we can read
it and generate the identifier, but we'll also end up writing /proc/pid/exe to
the dump file, which is not what we want.

[Ted Mielczarek]

http://breakpad.appspot.com/242001/diff/1/5
File src/client/linux/minidump_writer/linux_dumper.cc (left):

http://breakpad.appspot.com/242001/diff/1/5#oldcode243
Line 243: mapping->name[filename_len - sizeof(kDeletedSuffix) + 1] = '\0';
On 2010/12/09 21:17:23, thestig wrote:
> You can't remove this. Sorry, I didn't add more comments to explain what's
going
> on:
> 
> On Linux, in the case where the running binary has been deleted, we have
> /path/to/binary, which shows up as "/path/to/binary (deleted)" in the mapping.
> 
> We can't read /path/to/binary nor "/path/to/binary (deleted)", since they
don't
> exist. Instead, we need to  read /proc/pid/exe to generate the file
identifier.
> However, we have to write /path/to/binary in the mapping name.
> 
> In the modified code, you save the mapping name as /proc/pid/exe, so we can
read
> it and generate the identifier, but we'll also end up writing /proc/pid/exe to
> the dump file, which is not what we want.

Ah, okay. I was going to write a unit test for this code since I was touching
it, and there wasn't one landed with it, but I forgot. I'll have to rethink
this, then. I might be able to do what I want without refactoring this method.

[Ted Mielczarek]

Okay, I reworked the code slightly to not break this, and I wrote a unit test
for the deleted binary behavior.

[Lei Zhang (chromium)]

LGTM with minor nit below.

http://breakpad.appspot.com/242001/diff/12001/13008
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/242001/diff/12001/13008#newcode823
Line 823: // a file ID from the mapping. mapping_id can be -1 if this mapping
(I can't find this in the style guide, but...) many files in Breakpad have the
convention of putting '|' around variable names in comments. Ex. |mapping_id|.
Can we stick with that convention?