Add some unit tests for Linux WriteSymbolFile

I promised this in http://breakpad.appspot.com/275001, so here it is. This wound
up taking me way longer than I wanted, and I still didn't write enough tests to
really get good coverage of WriteSymbolFile, but I'm kind of burned out on it
now, and it's more test coverage than we had previously. This patch introduces a
google_breakpad::synth_elf namespace with ELF, StringTable, and SymbolTable
classes for assembling valid ELF data in memory. It also does some slight
refactoring to make WriteSymbolFile and Module::Write take a std::ostream
instead of a FILE*, since that winds up being a bit easier to unit test (with
stringstream, you can see how module_unittest.cc looks nicer already). It then
adds some basic tests for WriteSymbolFile, really just testing my new code from
my previous patch. Ideally we'd test more of the code paths here, I just ran out
of steam. I had thought about how to make testing separate debug files easier,
since currently the code looks for a separate file on disk. I thought perhaps
WriteSymbolFileInternal could instead take an interface that allowed path
manipulation and returned mmapped file pointers, to make it easy to swap in a
Mock, but I didn't find the motivation to do that.

[Ted Mielczarek]

Oh, there's also some additional refactoring/cleanup that could be done as a
result of this patch, but I didn't attempt that either. good followup work,
certainly.

[jimb]

LGTM. I strongly support the use of <iostream>, for exactly this sort of reason.

http://breakpad.appspot.com/277001/diff/1/4
File src/common/dwarf_cu_to_module.cc (right):

http://breakpad.appspot.com/277001/diff/1/4#newcode43
Line 43: #include <stdio.h>
For the fprintfs in WarningReporter? Thanks.

http://breakpad.appspot.com/277001/diff/1/5
File src/common/dwarf_line_to_module.cc (right):

http://breakpad.appspot.com/277001/diff/1/5#newcode37
Line 37: #include <stdio.h>
Thanks again.

http://breakpad.appspot.com/277001/diff/1/6
File src/common/linux/dump_symbols.cc (right):

http://breakpad.appspot.com/277001/diff/1/6#newcode732
Line 732: ElfW(Ehdr) *elf_header = reinterpret_cast<ElfW(Ehdr) *>(obj_file);
It's a pity that we don't actually check for a truncated ELF file; we'd just
read off the end. But that's a pre-existing problem, and would take a lot of
work to fix.

Also, the code in LoadELF is crap: take a look at the check for an error return
from fstat --- that should be an ||, shouldn't it? Again, not your problem.

http://breakpad.appspot.com/277001/diff/1/10
File src/common/linux/synth_elf.cc (right):

http://breakpad.appspot.com/277001/diff/1/10#newcode13
Line 13: addr_size_(machine == EM_X86_64 ? 8 : 4),
There really should be an explicit 'file class' argument to the constructor,
ELFCLASS32 or ELFCLASS64, instead of inferring the class this way.

[Ted Mielczarek]

http://breakpad.appspot.com/277001/diff/1/10
File src/common/linux/synth_elf.cc (right):

http://breakpad.appspot.com/277001/diff/1/10#newcode13
Line 13: addr_size_(machine == EM_X86_64 ? 8 : 4),
On 2011/07/05 19:21:58, jimb wrote:
> There really should be an explicit 'file class' argument to the constructor,
> ELFCLASS32 or ELFCLASS64, instead of inferring the class this way.

I kind of just wanted it to be easy for consumers to use, since it's just
testing code. Having to specify both the machine and the elfclass seemed
redundant when they have to match anyway. I can change it, it's not a big deal.

[jimb]

On 2011/07/06 15:45:08, Ted Mielczarek wrote:
> Having to specify both the machine and the elfclass seemed
> redundant when they have to match anyway. I can change it, it's not a big
deal.

Well, change it if you think it's an improvement; LGTM either way.