Add MachOImageSymbolTableReader and hook it up to MachOImageReader

Add MachOImageSymbolTableReader and hook it up to MachOImageReader.

TEST=util_test MachOImageReader.*
R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/2cae118b606cbc8cacfb09dfb0eee39457d2f380

[Mark Mentovai, 2014-09-04 20:43:42]

mark@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2014-09-05 19:04:17]

Nifty!

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
File util/mac/mach_o_image_reader.h (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
util/mac/mach_o_image_reader.h:167: //!     This parameter can be `NULL`.
Ownership/lifetime?

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
util/mac/mach_o_image_reader.h:201: //! symbols cannot be located through this
mechanism, nor can `N_PEXT`
This sounds contradictory to what you wrote on line 198.

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
util/mac/mach_o_image_reader.h:339: // symbol_table_initialized_ protects
symbol_table_: symbol_table_ can only
Why are there members between the state and the table variable? Couldn't they be
next to each other?

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
File util/mac/mach_o_image_reader_test.cc (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
util/mac/mach_o_image_reader_test.cc:430: const char* commands_base =
reinterpret_cast<const char*>(&expect_image[1]);
Why [1]?

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
File util/mac/mach_o_image_symbol_table_reader.cc (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
util/mac/mach_o_image_symbol_table_reader.cc:240: const MachOImageSegmentReader*
linkedit_segment,
It seems like it'd be handy to have access to the slid linkedit_address and
_size in the segment, maybe accessed through getters/setters. That way you don't
have to sidecar around those two pieces of data.

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
File util/mac/mach_o_image_symbol_table_reader.h (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
util/mac/mach_o_image_symbol_table_reader.h:45: struct SymbolInformation {
For ProcessReader, you didn't use a nested struct but created
ProcessReaderModule. To be consistent, you could also make that
ProcessReader::Module, or make this non-nested.

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
util/mac/mach_o_image_symbol_table_reader.h:135: friend class
internal::MachOImageSymbolTableReaderInitializer;
Is the friend only required to access the SymbolInformationMap type?

[Mark Mentovai, 2014-09-05 20:21:45]

Patchset #3 (id:40001) has been deleted

[Mark Mentovai, 2014-09-05 20:22:32]

Revised. All feedback addressed.

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
File util/mac/mach_o_image_reader.h (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
util/mac/mach_o_image_reader.h:339: // symbol_table_initialized_ protects
symbol_table_: symbol_table_ can only
rsesek wrote:
> Why are there members between the state and the table variable? Couldn't they
be
> next to each other?

I’ve packed all of my structures for minimal wasted space (sparseness).

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
File util/mac/mach_o_image_reader_test.cc (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
util/mac/mach_o_image_reader_test.cc:430: const char* commands_base =
reinterpret_cast<const char*>(&expect_image[1]);
rsesek wrote:
> Why [1]?

To move past the mach_header[_64] to the beginning of the load commands. Is this
clearer as

&expect_image[1]
expect_image + 1

or

reinterpret_cast<char*>(expect_image) + sizeof(*expect_image)?

The last is probably the clearest, but it’s also the ugliest. Do you have a
preference? Or should I just add a comment?

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
File util/mac/mach_o_image_symbol_table_reader.h (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
util/mac/mach_o_image_symbol_table_reader.h:45: struct SymbolInformation {
rsesek wrote:
> For ProcessReader, you didn't use a nested struct but created
> ProcessReaderModule. To be consistent, you could also make that
> ProcessReader::Module, or make this non-nested.

Good point! Spun off https://codereview.chromium.org/543193002.

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
util/mac/mach_o_image_symbol_table_reader.h:135: friend class
internal::MachOImageSymbolTableReaderInitializer;
rsesek wrote:
> Is the friend only required to access the SymbolInformationMap type?

Yeah. I made it public instead of having a friend, but left a comment explaining
why it’s public. Is that better?

[Robert Sesek, 2014-09-05 20:41:31]

LGTM

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
File util/mac/mach_o_image_reader_test.cc (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_re...
util/mac/mach_o_image_reader_test.cc:430: const char* commands_base =
reinterpret_cast<const char*>(&expect_image[1]);
On 2014/09/05 20:22:31, Mark Mentovai wrote:
> rsesek wrote:
> > Why [1]?
> 
> To move past the mach_header[_64] to the beginning of the load commands. Is
this
> clearer as
> 
> &expect_image[1]
> expect_image + 1
> 
> or
> 
> reinterpret_cast<char*>(expect_image) + sizeof(*expect_image)?
> 
> The last is probably the clearest, but it’s also the ugliest. Do you have a
> preference? Or should I just add a comment?

I'd just add a comment saying it's moving past the mach_header.

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
File util/mac/mach_o_image_symbol_table_reader.h (right):

https://codereview.chromium.org/539263003/diff/20001/util/mac/mach_o_image_sy...
util/mac/mach_o_image_symbol_table_reader.h:135: friend class
internal::MachOImageSymbolTableReaderInitializer;
On 2014/09/05 20:22:31, Mark Mentovai wrote:
> rsesek wrote:
> > Is the friend only required to access the SymbolInformationMap type?
> 
> Yeah. I made it public instead of having a friend, but left a comment
explaining
> why it’s public. Is that better?

Yeah, I think so.

[Mark Mentovai, 2014-09-05 20:53:32]

Committed patchset #4 (id:50009) manually as 2cae118 (presubmit successful).