Add a MappedMemory interface to TaskMemory and use it in MachOImageSymbolTableReader

Add a MappedMemory interface to TaskMemory and use it in
MachOImageSymbolTableReader.

This results in a speed boost for MachOImageSymbolTableReader because
it’s able to read the entire string table in one operation, rather than
reading each string from the remote process individually. Copying is
also reduced. In a debug-mode build on my laptop, util_test
MachOImageReader.* has improved from ~1400ms to ~1000ms.

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

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/0869b3e86d704fc55d621a8d3659e40dba370c27

[Mark Mentovai, 2014-09-10 19:28:05]

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

[Robert Sesek, 2014-09-10 21:37:41]

https://codereview.chromium.org/558313002/diff/1/util/mac/mach_o_image_symbol...
File util/mac/mach_o_image_symbol_table_reader.cc (right):

https://codereview.chromium.org/558313002/diff/1/util/mac/mach_o_image_symbol...
util/mac/mach_o_image_symbol_table_reader.cc:135: if
(!string_table->ReadCString(symbol.n_strx, &name)) {
This doesn't take into the size limiting anymore. Is that OK?

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.cc
File util/mach/task_memory.cc (right):

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.cc#new...
util/mach/task_memory.cc:39: DCHECK_GE(user_address, vm_address);
DCHECK_EQ(0, vm_address % PAGE_SIZE) ?

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.cc#new...
util/mach/task_memory.cc:101: return scoped_ptr<MappedMemory>(
You don't need .Pass() when returning scoped_ptr<>?

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h
File util/mach/task_memory.h (right):

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h#newc...
util/mach/task_memory.h:43: //! \param[in] user_offset The offset into the
mapped region where the data
Can you just accept the user variants and do the page rounding internally?

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h#newc...
util/mach/task_memory.h:50: MappedMemory(vm_address_t vm_address,
Can callers meaningfully call this ctor outside of ::ReadMapped()?

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h#newc...
util/mach/task_memory.h:64: //! This method will read contiguous memory until a
`NUL` terminator is
Your test demonstrates that this will also read until the end of the mapped
region, even if a NUL is not found.

[Mark Mentovai, 2014-09-10 22:16:56]

https://codereview.chromium.org/558313002/diff/1/util/mac/mach_o_image_symbol...
File util/mac/mach_o_image_symbol_table_reader.cc (right):

https://codereview.chromium.org/558313002/diff/1/util/mac/mach_o_image_symbol...
util/mac/mach_o_image_symbol_table_reader.cc:135: if
(!string_table->ReadCString(symbol.n_strx, &name)) {
rsesek wrote:
> This doesn't take into the size limiting anymore. Is that OK?

It still does. |strtab_size| was the size given when the string table was read
on line 127, and you can only access up to that size through this interface.
ReadCString will return false if you try to read past strtab_size.

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.cc
File util/mach/task_memory.cc (right):

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.cc#new...
util/mach/task_memory.cc:39: DCHECK_GE(user_address, vm_address);
rsesek wrote:
> DCHECK_EQ(0, vm_address % PAGE_SIZE) ?

That’s redundant. ScopedMachVM already does it, and its DCHECK will happen in
the initializer before it would be able to run here.

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.cc#new...
util/mach/task_memory.cc:101: return scoped_ptr<MappedMemory>(
rsesek wrote:
> You don't need .Pass() when returning scoped_ptr<>?

I don’t think it’s necessary. Look at the comment at scoped_ptr.h:48.

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h
File util/mach/task_memory.h (right):

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h#newc...
util/mach/task_memory.h:43: //! \param[in] user_offset The offset into the
mapped region where the data
rsesek wrote:
> Can you just accept the user variants and do the page rounding internally?

The caller, TaskMemory::ReadMapped(), has necessarily already computed the
rounded pages. But I’m making it a private constructor and a friend per your
next comment. Is that better?

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h#newc...
util/mach/task_memory.h:64: //! This method will read contiguous memory until a
`NUL` terminator is
rsesek wrote:
> Your test demonstrates that this will also read until the end of the mapped
> region, even if a NUL is not found.

Well, the documented behavior here is the intended behavior.

Where do you see the test demonstrating anything contrary?
TaskMemory.MappedMemoryReadCString tests this explicitly. Lines 525-527, with
offsets 7-9 inclusive, don’t find a NUL because only 9 bytes were read, not 10
that would be needed to get the trailing NUL, and those are EXPECT_FALSE. Later
on, ReadCString() is able to see those, but only because the MappedMemory was
read with size 10 so that there’s a trailing NUL in there.

[Robert Sesek, 2014-09-11 18:42:35]

LGTM

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h
File util/mach/task_memory.h (right):

https://codereview.chromium.org/558313002/diff/1/util/mach/task_memory.h#newc...
util/mach/task_memory.h:64: //! This method will read contiguous memory until a
`NUL` terminator is
On 2014/09/10 22:16:56, Mark Mentovai wrote:
> rsesek wrote:
> > Your test demonstrates that this will also read until the end of the mapped
> > region, even if a NUL is not found.
> 
> Well, the documented behavior here is the intended behavior.
> 
> Where do you see the test demonstrating anything contrary?
> TaskMemory.MappedMemoryReadCString tests this explicitly. Lines 525-527, with
> offsets 7-9 inclusive, don’t find a NUL because only 9 bytes were read, not 10
> that would be needed to get the trailing NUL, and those are EXPECT_FALSE.
Later
> on, ReadCString() is able to see those, but only because the MappedMemory was
> read with size 10 so that there’s a trailing NUL in there.

Sorry, forgot that the C string literal will end in a NUL.

[Mark Mentovai, 2014-09-11 19:10:18]

Committed patchset #3 (id:30001) manually as 0869b3e (presubmit successful).