win: Capture some memory pointed at by context

win: Capture some memory pointed at by context

R=mark@chromium.org
BUG=crashpad:86, chromium:571144
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/5af9c426386dddef758704ca902934c69cebb235

[scottmg, 2016-01-08 18:40:14]

Oops, apparently it helps get things reviewed if I actually mail them out.

[Mark Mentovai, 2016-01-08 19:20:52]

It seems like it might be beneficial for this to happen at a different layer
rather than OS-specific context snapshots, so that we can get this easily on
other platforms.

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
File snapshot/win/capture_context_memory.cc (right):

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
snapshot/win/capture_context_memory.cc:32: if
(process_reader->GetProcessInfo().LoggingRangeIsFullyReadable(
If the address is readable at all, we should do as much of the grab around it as
possible, even if the full 128-byte window isn’t grabbable.

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
snapshot/win/capture_context_memory.cc:53:
MaybeCaptureMemoryAround(process_reader, context.x86_64->rsi, into);
If bp doesn’t point to the thread’s stack, you should grab it too.

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
File snapshot/win/capture_context_memory.h (right):

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
snapshot/win/capture_context_memory.h:1: // Copyright 2015 The Crashpad Authors.
All rights reserved.
2016, and in the .cc too.

[scottmg, 2016-01-08 19:52:02]

Not sure about making it non-OS-specific. The function with the list of
registers could be shared out of capture_context_win, but it'd have to be sliced
up a bunch because of the XyzWin objects that are used.

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
File snapshot/win/capture_context_memory.cc (right):

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
snapshot/win/capture_context_memory.cc:32: if
(process_reader->GetProcessInfo().LoggingRangeIsFullyReadable(
On 2016/01/08 19:20:52, Mark Mentovai wrote:
> If the address is readable at all, we should do as much of the grab around it
as
> possible, even if the full 128-byte window isn’t grabbable.

Done.

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
snapshot/win/capture_context_memory.cc:53:
MaybeCaptureMemoryAround(process_reader, context.x86_64->rsi, into);
On 2016/01/08 19:20:52, Mark Mentovai wrote:
> If bp doesn’t point to the thread’s stack, you should grab it too.

Done.

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
File snapshot/win/capture_context_memory.h (right):

https://codereview.chromium.org/1533183002/diff/40001/snapshot/win/capture_co...
snapshot/win/capture_context_memory.h:1: // Copyright 2015 The Crashpad Authors.
All rights reserved.
On 2016/01/08 19:20:52, Mark Mentovai wrote:
> 2016, and in the .cc too.

Done.

[scottmg, 2016-01-08 19:54:02]

Patchset #5 (id:80001) has been deleted

[Mark Mentovai, 2016-01-08 22:05:42]

LGTM

For the OS-neutral thing to work, we’d need snapshot to just expose a “memory
for address” interface based on the ProcessReader, but then we could have an
OS-neutral memory snapshot provide that data to minidump. I envision that the
context snapshot would have a CPU-aware “get pointers” thing that would be used
to find interesting pointers, and then we’d ask the snapshot for memory regions
around those pointers.

No need to do it right now if you’re in a hurry to get this data on Windows, but
there’s nothing really Windows-specific here if we just add a small amount of
extra abstraction.

https://codereview.chromium.org/1533183002/diff/100001/snapshot/win/capture_c...
File snapshot/win/capture_context_memory.cc (right):

https://codereview.chromium.org/1533183002/diff/100001/snapshot/win/capture_c...
snapshot/win/capture_context_memory.cc:28: if (address > 0x10000) {
-1 and other high numbers are valid register contents that probably aren’t
pointers too. Maybe we should make sure address is > 0x10000 and <=
max_for_bitness - 0x10000.

https://codereview.chromium.org/1533183002/diff/100001/snapshot/win/capture_c...
snapshot/win/capture_context_memory.cc:37: into->push_back(snapshot);
Interesting, so this might miss address but pick up something near address if
address is just outside of a mapped range. I suppose that’s desirable?

[scottmg, 2016-01-08 22:23:28]

https://codereview.chromium.org/1533183002/diff/100001/snapshot/win/capture_c...
File snapshot/win/capture_context_memory.cc (right):

https://codereview.chromium.org/1533183002/diff/100001/snapshot/win/capture_c...
snapshot/win/capture_context_memory.cc:28: if (address > 0x10000) {
On 2016/01/08 22:05:42, Mark Mentovai wrote:
> -1 and other high numbers are valid register contents that probably aren’t
> pointers too. Maybe we should make sure address is > 0x10000 and <=
> max_for_bitness - 0x10000.

Done.

https://codereview.chromium.org/1533183002/diff/100001/snapshot/win/capture_c...
snapshot/win/capture_context_memory.cc:37: into->push_back(snapshot);
On 2016/01/08 22:05:42, Mark Mentovai wrote:
> Interesting, so this might miss address but pick up something near address if
> address is just outside of a mapped range. I suppose that’s desirable?

Yeah, I'm not sure how useful/likely it really is. If the register is a bad
pointer for an object, maybe it ends up grabbing the last write to a field
somehow.

[scottmg, 2016-01-09 01:22:47]

Patchset #8 (id:160001) has been deleted

[scottmg, 2016-01-09 01:24:09]

Description was changed from

==========
win: Capture some memory pointed at by context

R=mark@chromium.org
BUG=crashpad:86, chromium:571144
==========

to

==========
win: Capture some memory pointed at by context

R=mark@chromium.org
BUG=crashpad:86, chromium:571144

Committed:
https://chromium.googlesource.com/crashpad/crashpad/+/5af9c426386dddef758704c...
==========

[scottmg, 2016-01-09 01:24:11]

Committed patchset #8 (id:180001) manually as
5af9c426386dddef758704ca902934c69cebb235 (presubmit successful).