Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(27)

Issue 277001: Add some unit tests for Linux WriteSymbolFile (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 12 months ago by Ted Mielczarek
Modified:
12 years, 8 months ago
Reviewers:
jimb
CC:
google-breakpad-dev_googlegroups.com
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M Makefile.am View 1 chunk +3 lines, -0 lines 0 comments Download
M Makefile.in View 8 chunks +66 lines, -0 lines 0 comments Download
M src/common/dwarf_cu_to_module.cc View 1 chunk +1 line, -0 lines 1 comment Download
M src/common/dwarf_line_to_module.cc View 1 chunk +2 lines, -0 lines 1 comment Download
M src/common/linux/dump_symbols.cc View 3 chunks +34 lines, -14 lines 1 comment Download
M src/common/linux/dump_symbols.h View 1 chunk +4 lines, -4 lines 0 comments Download
A src/common/linux/dump_symbols_unittest.cc View 1 chunk +162 lines, -0 lines 0 comments Download
M src/common/linux/elf_symbols_to_module_unittest.cc View 1 chunk +2 lines, -66 lines 0 comments Download
A src/common/linux/synth_elf.cc View 1 chunk +172 lines, -0 lines 2 comments Download
A src/common/linux/synth_elf.h View 1 chunk +154 lines, -0 lines 0 comments Download
A src/common/linux/synth_elf_unittest.cc View 1 chunk +265 lines, -0 lines 0 comments Download
M src/common/module.cc View 2 chunks +52 lines, -36 lines 0 comments Download
M src/common/module.h View 2 chunks +3 lines, -4 lines 0 comments Download
M src/common/module_unittest.cc View 7 chunks +32 lines, -104 lines 0 comments Download
M src/common/stabs_to_module.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/tools/linux/dump_syms/dump_syms.cc View 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 5
Ted Mielczarek
12 years, 12 months ago #1
Ted Mielczarek
Oh, there's also some additional refactoring/cleanup that could be done as a result of this ...
12 years, 12 months ago #2
jimb
LGTM. I strongly support the use of <iostream>, for exactly this sort of reason. http://breakpad.appspot.com/277001/diff/1/4 ...
12 years, 9 months ago #3
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 ...
12 years, 8 months ago #4
jimb
12 years, 8 months ago #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted