Write adjusted range back to module

Write adjusted range back to module

In Android, the mmap could be overlapped by /dev/ashmem, we adjusted
the range in https://breakpad.appspot.com/9744002/, but adjusted
range isn't written back to module, this caused the corresponding
module be dropped in BasicCodeModules copy constructor.

This also fix a lot of 'unable to store module' warnings
when dumping Android's minidump.

BUG=606972
R=mark@chromium.org, wfh@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/4f417c8c0ffceb6c2516c6ef00cd91ca5746d852

[michaelbai, 2016-05-02 22:19:01]

Description was changed from

==========
Write adjusted range back to module

In Android, the mmap could be overlapped by /dev/ashmem, we adjusted
the range in https://breakpad.appspot.com/9744002/, but adjusted
range isn't written back to module, this caused the corresponding
module be dropped in BasicCodeModules copy constructor.

BUG=606972
==========

to

==========
Write adjusted range back to module

In Android, the mmap could be overlapped by /dev/ashmem, we adjusted
the range in https://breakpad.appspot.com/9744002/, but adjusted
range isn't written back to module, this caused the corresponding
module be dropped in BasicCodeModules copy constructor.

This also fix a lot of 'unable to store module' warnings when dumping Android's
minidump. 

BUG=606972
==========

[michaelbai, 2016-05-02 22:19:18]

Description was changed from

==========
Write adjusted range back to module

In Android, the mmap could be overlapped by /dev/ashmem, we adjusted
the range in https://breakpad.appspot.com/9744002/, but adjusted
range isn't written back to module, this caused the corresponding
module be dropped in BasicCodeModules copy constructor.

This also fix a lot of 'unable to store module' warnings when dumping Android's
minidump. 

BUG=606972
==========

to

==========
Write adjusted range back to module

In Android, the mmap could be overlapped by /dev/ashmem, we adjusted
the range in https://breakpad.appspot.com/9744002/, but adjusted
range isn't written back to module, this caused the corresponding
module be dropped in BasicCodeModules copy constructor.

This also fix a lot of 'unable to store module' warnings
when dumping Android's minidump. 

BUG=606972
==========

[michaelbai, 2016-05-02 22:22:42]

michaelbai@chromium.org changed reviewers:
+ mark@chromium.org, wfh@chromium.org

[michaelbai, 2016-05-02 22:22:57]

PTAL

[Will Harris, 2016-05-02 22:25:43]

https://codereview.chromium.org/1939333002/diff/1/processor/minidump.cc
File processor/minidump.cc (right):

https://codereview.chromium.org/1939333002/diff/1/processor/minidump.cc#newco...
processor/minidump.cc:2645: module->adjust_base_address_and_size(base_address,
module_size);
does this only apply to ashmem, or is this generic? if so can we remove the
kDevAshmem carve out entirely?

[michaelbai, 2016-05-02 22:33:13]

https://codereview.chromium.org/1939333002/diff/1/processor/minidump.cc
File processor/minidump.cc (right):

https://codereview.chromium.org/1939333002/diff/1/processor/minidump.cc#newco...
processor/minidump.cc:2645: module->adjust_base_address_and_size(base_address,
module_size);
On 2016/05/02 22:25:43, Will Harris wrote:
> does this only apply to ashmem, or is this generic? if so can we remove the
> kDevAshmem carve out entirely?

Yes, it only applies to ashmem.

[Will Harris, 2016-05-02 22:35:24]

lgtm but wait for mark to take a look

https://codereview.chromium.org/1939333002/diff/1/processor/minidump.cc
File processor/minidump.cc (right):

https://codereview.chromium.org/1939333002/diff/1/processor/minidump.cc#newco...
processor/minidump.cc:2645: module->adjust_base_address_and_size(base_address,
module_size);
On 2016/05/02 22:33:13, michaelbai wrote:
> On 2016/05/02 22:25:43, Will Harris wrote:
> > does this only apply to ashmem, or is this generic? if so can we remove the
> > kDevAshmem carve out entirely?
> 
> Yes, it only applies to ashmem.

Acknowledged.

[Mark Mentovai, 2016-05-03 04:07:07]

https://codereview.chromium.org/1939333002/diff/1/google_breakpad/processor/m...
File google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1939333002/diff/1/google_breakpad/processor/m...
google_breakpad/processor/minidump.h:385: void
adjust_base_address_and_size(uint64_t base_address, uint64_t size);
“Set” instead of “adjust”. “Adjust” makes it sound like the arguments might be
offsets. “Set” makes it clear that they’re not.

Since this is weird and we wouldn’t want willy-nilly setting, please also add a
brief comment explaining why it’s necessary.

[michaelbai, 2016-05-03 19:40:57]

https://codereview.chromium.org/1939333002/diff/1/google_breakpad/processor/m...
File google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1939333002/diff/1/google_breakpad/processor/m...
google_breakpad/processor/minidump.h:385: void
adjust_base_address_and_size(uint64_t base_address, uint64_t size);
On 2016/05/03 04:07:07, Mark Mentovai wrote:
> “Set” instead of “adjust”. “Adjust” makes it sound like the arguments might be
> offsets. “Set” makes it clear that they’re not.
> 
> Since this is weird and we wouldn’t want willy-nilly setting, please also add
a
> brief comment explaining why it’s necessary.

Done.

[Mark Mentovai, 2016-05-03 20:39:32]

LGTM

https://codereview.chromium.org/1939333002/diff/20001/src/google_breakpad/pro...
File src/google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1939333002/diff/20001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:385: // This method is dedicated for
the Android where the module could overlap
This method is intended to handle the case on Android

https://codereview.chromium.org/1939333002/diff/20001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:386: // with ashmem, and not supposed
to be used in anywhere else.
and is not supposed to be used anywhere else.

[michaelbai, 2016-05-03 21:59:19]

Mark, could you help to land this? I don't have permission.

https://codereview.chromium.org/1939333002/diff/20001/src/google_breakpad/pro...
File src/google_breakpad/processor/minidump.h (right):

https://codereview.chromium.org/1939333002/diff/20001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:385: // This method is dedicated for
the Android where the module could overlap
On 2016/05/03 20:39:32, Mark Mentovai wrote:
> This method is intended to handle the case on Android

Done.

https://codereview.chromium.org/1939333002/diff/20001/src/google_breakpad/pro...
src/google_breakpad/processor/minidump.h:386: // with ashmem, and not supposed
to be used in anywhere else.
On 2016/05/03 20:39:32, Mark Mentovai wrote:
> and is not supposed to be used anywhere else.

Done.

[Mark Mentovai, 2016-05-03 22:11:26]

LGTM

[Mark Mentovai, 2016-05-03 22:14:34]

Description was changed from

==========
Write adjusted range back to module

In Android, the mmap could be overlapped by /dev/ashmem, we adjusted
the range in https://breakpad.appspot.com/9744002/, but adjusted
range isn't written back to module, this caused the corresponding
module be dropped in BasicCodeModules copy constructor.

This also fix a lot of 'unable to store module' warnings
when dumping Android's minidump. 

BUG=606972
==========

to

==========
Write adjusted range back to module

In Android, the mmap could be overlapped by /dev/ashmem, we adjusted
the range in https://breakpad.appspot.com/9744002/, but adjusted
range isn't written back to module, this caused the corresponding
module be dropped in BasicCodeModules copy constructor.

This also fix a lot of 'unable to store module' warnings
when dumping Android's minidump.

BUG=606972
R=mark@chromium.org, wfh@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/4f417c8c0ffceb6c2516c6e...
==========

[Mark Mentovai, 2016-05-03 22:14:35]

Committed patchset #3 (id:40001) manually as
4f417c8c0ffceb6c2516c6ef00cd91ca5746d852 (presubmit successful).

[Primiano Tucci (use gerrit), 2016-05-10 17:29:33]

> In Android, the mmap could be overlapped by /dev/ashmem,
It is not clear to me under which condition this should happen.
The linker is supposed to create guard regions to compensate the breakpad
adjustement for the load bias, to guarantee that that adjusted module does not
overlap with any other module (other than the guard region itself, which is a
null mapping, which breakpad skips).

https://breakpad.appspot.com/9744002/ was about duplicate mappings that overlap
with each other.
Why should they overlap with libchrome.so?

This smells like a linker (better: loader) bug to me.
Can I see a dump of mmaps that cause problems somewhere?