Add functionality to read the .gnu_debuglink section and load symbols from...

Add functionality to read the .gnu_debuglink section and load symbols from a
debug ELF file.

Committed: http://code.google.com/p/google-breakpad/source/detail?r=624

[Lei Zhang (chromium)]

To test:
gcc -g -m32 foo.c -o foo.orig
strip --only-keep-debug -o foo.dbg foo.orig
strip --strip-debug -o foo foo.orig
objcopy --add-gnu-debuglink=foo.dbg foo

Also tested with Ubuntu Lucid's libc and libc-dbg packages:
http://packages.ubuntu.com/lucid/i386/libc6/download
http://packages.ubuntu.com/lucid/i386/libc6-dbg/download

http://breakpad.appspot.com/126001/diff/2001/3001
File src/common/linux/dump_symbols.cc (right):

http://breakpad.appspot.com/126001/diff/2001/3001#newcode102
Line 102: // Wrapper class to make sure mapped regions are unmapped.
note: This is not a cut+paste. I added MmapWrapper::set() and changed the
constructor.

[jimb]

Thanks very much for taking this on.

http://breakpad.appspot.com/126001/diff/2001/3001
File src/common/linux/dump_symbols.cc (right):

http://breakpad.appspot.com/126001/diff/2001/3001#newcode82
Line 82: }
Let's put this whole constructor on one line.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode107
Line 107: }
Let's put the closing curly on the same line as the open.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode226
Line 226: explicit DumperLineToModule(dwarf2reader::ByteReader *byte_reader)
Thanks.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode435
Line 435: fprintf(stderr, "Failed to open debug ELF file '%s': %s\n",
I think this error message should also include the name of the file containing
the .gnu_debuglink section whose referent we couldn't open.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode458
Line 458: }
Let's bring that closing brace up a line. The style guide seems to like vertical
compactness.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode468
Line 468: assert(false);
If the file contains bogus information (like the same section with bits in both
files), this isn't an internal error in dump_syms; it's bad data.  This should
be an error, not an assert.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode481
Line 481: fprintf(stderr,
Could this error message include the filenames of the two files it's complaining
about?

http://breakpad.appspot.com/126001/diff/2001/3001#newcode509
Line 509: bool read_gnu_debug_link_;  // True if LoadSymbols() should read the
Couldn't we just add a parameter to LoadSymbols indicating whether it should
look for the debug link?

http://breakpad.appspot.com/126001/diff/2001/3001#newcode618
Line 618: fprintf(stderr, ".gnu_debuglink section found, "
This message should include the filename.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode622
Line 622: fprintf(stderr, "%s: no .gnu_debuglink section\n", obj_file.c_str());
Can we make this closer to being a proper sentence?  "file has no..."?

http://breakpad.appspot.com/126001/diff/2001/3001#newcode728
Line 728: // Sanity checks to make sure everything match up.
"matches"

http://breakpad.appspot.com/126001/diff/2001/3001#newcode737
Line 737: "%s with ELF match architecture %s\n",
Extra "match".

[Lei Zhang (chromium)]

http://breakpad.appspot.com/126001/diff/2001/3001
File src/common/linux/dump_symbols.cc (right):

http://breakpad.appspot.com/126001/diff/2001/3001#newcode82
Line 82: }
On 2010/07/13 20:59:29, jimb wrote:
> Let's put this whole constructor on one line.

Done.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode107
Line 107: }
On 2010/07/13 20:59:29, jimb wrote:
> Let's put the closing curly on the same line as the open.

Done.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode435
Line 435: fprintf(stderr, "Failed to open debug ELF file '%s': %s\n",
On 2010/07/13 20:59:29, jimb wrote:
> I think this error message should also include the name of the file containing
> the .gnu_debuglink section whose referent we couldn't open.

Done.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode458
Line 458: }
On 2010/07/13 20:59:29, jimb wrote:
> Let's bring that closing brace up a line. The style guide seems to like
vertical
> compactness.

Done.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode468
Line 468: assert(false);
On 2010/07/13 20:59:29, jimb wrote:
> If the file contains bogus information (like the same section with bits in
both
> files), this isn't an internal error in dump_syms; it's bad data.  This should
> be an error, not an assert.

Assert() removed.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode481
Line 481: fprintf(stderr,
On 2010/07/13 20:59:29, jimb wrote:
> Could this error message include the filenames of the two files it's
complaining
> about?

Done.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode509
Line 509: bool read_gnu_debug_link_;  // True if LoadSymbols() should read the
On 2010/07/13 20:59:29, jimb wrote:
> Couldn't we just add a parameter to LoadSymbols indicating whether it should
> look for the debug link?

At one point I was passing too many arguments to LoadSymbols(), so I put them
all inside a LoadSymbolInfo instead.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode618
Line 618: fprintf(stderr, ".gnu_debuglink section found, "
On 2010/07/13 20:59:29, jimb wrote:
> This message should include the filename.

Done.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode622
Line 622: fprintf(stderr, "%s: no .gnu_debuglink section\n", obj_file.c_str());
On 2010/07/13 20:59:29, jimb wrote:
> Can we make this closer to being a proper sentence?  "file has no..."?

Done.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode728
Line 728: // Sanity checks to make sure everything match up.
On 2010/07/13 20:59:29, jimb wrote:
> "matches"

Done.

http://breakpad.appspot.com/126001/diff/2001/3001#newcode737
Line 737: "%s with ELF match architecture %s\n",
On 2010/07/13 20:59:29, jimb wrote:
> Extra "match".

Done.

[jimb]

LGTM, although I still think it would be better if read_gnu_debug_link were a
separate parameter; see comments.

http://breakpad.appspot.com/126001/diff/2001/3001
File src/common/linux/dump_symbols.cc (right):

http://breakpad.appspot.com/126001/diff/2001/3001#newcode509
Line 509: bool read_gnu_debug_link_;  // True if LoadSymbols() should read the
On 2010/07/14 01:34:00, thestig wrote:
> At one point I was passing too many arguments to LoadSymbols(), so I put them
> all inside a LoadSymbolInfo instead.

Right, but this parameter is different in each call.  For each call, you've
replaced a parameter with a call to set_read_gnu_debug_link, which is a
regression, to my eyes.

[Lei Zhang (chromium)]

On 2010/07/15 03:52:00, jimb wrote:
> LGTM, although I still think it would be better if read_gnu_debug_link were a
> separate parameter; see comments.
> 
> http://breakpad.appspot.com/126001/diff/2001/3001
> File src/common/linux/dump_symbols.cc (right):
> 
> http://breakpad.appspot.com/126001/diff/2001/3001#newcode509
> Line 509: bool read_gnu_debug_link_;  // True if LoadSymbols() should read the
> On 2010/07/14 01:34:00, thestig wrote:
> > At one point I was passing too many arguments to LoadSymbols(), so I put
them
> > all inside a LoadSymbolInfo instead.
> 
> Right, but this parameter is different in each call.  For each call, you've
> replaced a parameter with a call to set_read_gnu_debug_link, which is a
> regression, to my eyes.

Fair enough. Fixed and committed as r624. Thanks for the review!