win: Retrieve "simple map" annotations from modules

win: Retrieve "simple map" annotations from modules

Follows https://codereview.chromium.org/1126273003/.

R=rsesek@chromium.org, cpu@chromium.org
TBR=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/58df54fffbb530eb8f5fcb8cf2bd1608ba579cdd

[scottmg, 2015-05-12 21:00:15]

Patchset #3 (id:40001) has been deleted

[Robert Sesek, 2015-05-14 18:55:27]

This should have a test. Does the Multiprocess test harness work on Windows yet?

https://codereview.chromium.org/1138923004/diff/100001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1138923004/diff/100001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:158: // any of these on Windows, but could
in the future.
Is this worth filing a bug for?

[scottmg, 2015-05-14 19:08:18]

Patchset #6 (id:120001) has been deleted

[scottmg, 2015-05-14 19:09:11]

On 2015/05/14 18:55:27, Robert Sesek wrote:
> This should have a test. Does the Multiprocess test harness work on Windows
yet?

Unfortunately not. The current setup is very POSIX/Mach-y so looks like it'll
require a lot of rework and I hadn't had the energy yet. I'll take a look at
that first.

> 
>
https://codereview.chromium.org/1138923004/diff/100001/snapshot/win/module_sn...
> File snapshot/win/module_snapshot_win.cc (right):
> 
>
https://codereview.chromium.org/1138923004/diff/100001/snapshot/win/module_sn...
> snapshot/win/module_snapshot_win.cc:158: // any of these on Windows, but could
> in the future.
> Is this worth filing a bug for?

[scottmg, 2015-05-14 19:10:03]

https://codereview.chromium.org/1138923004/diff/100001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1138923004/diff/100001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:158: // any of these on Windows, but could
in the future.
On 2015/05/14 18:55:27, Robert Sesek wrote:
> Is this worth filing a bug for?

Good idea, https://code.google.com/p/crashpad/issues/detail?id=38

[scottmg, 2015-05-14 19:23:20]

On 2015/05/14 19:09:11, scottmg wrote:
> On 2015/05/14 18:55:27, Robert Sesek wrote:
> > This should have a test. Does the Multiprocess test harness work on Windows
> yet?
> 
> Unfortunately not. The current setup is very POSIX/Mach-y so looks like it'll
> require a lot of rework and I hadn't had the energy yet. I'll take a look at
> that first.

Wait! I did do some of it https://codereview.chromium.org/880763002/, I'm going
crazy.

I think what's mostly missing is something at the top level to handle it not
being fork()-based. That is, on relaunching the test binary it needs some sort
of argument to let the entry point thunk back to the child side of test code.
Hmm...

> > 
> >
>
https://codereview.chromium.org/1138923004/diff/100001/snapshot/win/module_sn...
> > File snapshot/win/module_snapshot_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/1138923004/diff/100001/snapshot/win/module_sn...
> > snapshot/win/module_snapshot_win.cc:158: // any of these on Windows, but
could
> > in the future.
> > Is this worth filing a bug for?

[Robert Sesek, 2015-05-15 17:08:45]

On 2015/05/14 19:23:20, scottmg (OoO until May 19th) wrote:
> On 2015/05/14 19:09:11, scottmg wrote:
> > On 2015/05/14 18:55:27, Robert Sesek wrote:
> > > This should have a test. Does the Multiprocess test harness work on
Windows
> > yet?
> > 
> > Unfortunately not. The current setup is very POSIX/Mach-y so looks like
it'll
> > require a lot of rework and I hadn't had the energy yet. I'll take a look at
> > that first.
> 
> Wait! I did do some of it https://codereview.chromium.org/880763002/, I'm
going
> crazy.
> 
> I think what's mostly missing is something at the top level to handle it not
> being fork()-based. That is, on relaunching the test binary it needs some sort
> of argument to let the entry point thunk back to the child side of test code.
> Hmm...

How does gtest implement death tests? One potential way would be to relaunch
with a --gtest_filter flag and then another magic flag --is-child-process that
Multiprocess knows to interpret.

I think this is important to have the test infrastructure up for this, otherwise
snapshot/ will be very hard to unittest.

[scottmg, 2015-05-21 18:33:47]

Patchset #7 (id:160001) has been deleted

[scottmg, 2015-05-21 19:35:58]

Patchset #7 (id:180001) has been deleted

[scottmg, 2015-05-28 16:26:15]

PTAL, rebased on new test/win_multiprocess, and added non-crashing and crashing
tests, modeled after Mac.

[Robert Sesek, 2015-05-28 20:42:26]

Code LGTM.

My only concern is that the code is really similar between this and the MachO
version. That probably stems from the fact that ProcessReader isn't a
platform-agnostic interface. Do you think this is going to come up in the
future, or is this near-code-duplication limited to just a few instances like
this? (I don't know for sure, but it's also something that can possibly be
cleaned up later).

[scottmg, 2015-05-28 20:53:56]

On 2015/05/28 20:42:26, Robert Sesek wrote:
> Code LGTM.

Thanks.
 
> My only concern is that the code is really similar between this and the MachO
> version. That probably stems from the fact that ProcessReader isn't a
> platform-agnostic interface. Do you think this is going to come up in the
> future, or is this near-code-duplication limited to just a few instances like
> this? (I don't know for sure, but it's also something that can possibly be
> cleaned up later).

Yeah, some of of the snapshot code is like that. I'm not sure what to do about
it. The code is similar at various places, but then differs in awkward ways
because of the necessarily platform-specific details in reading processes, etc.
I tried in some places to extract an interface but it felt forced.

So, I agree we should attempt to de-duplicate some of it, but it hasn't been
clear how yet.

What I'd like to do is continue to pursue getting the base functionality
implemented, and then attempt to combine at least at the interface layer later.
I'd been thinking perhaps this would be best to do while adding a 3rd platform
(Android or Linux I guess) so that the new interface would hopefully be general
enough for all the platforms we would care about, rather than not-quite-generic
enough if we do it too early.

[Robert Sesek, 2015-05-28 21:05:11]

On 2015/05/28 20:53:56, scottmg wrote:
> On 2015/05/28 20:42:26, Robert Sesek wrote:
> > Code LGTM.
> 
> Thanks.
>  
> > My only concern is that the code is really similar between this and the
MachO
> > version. That probably stems from the fact that ProcessReader isn't a
> > platform-agnostic interface. Do you think this is going to come up in the
> > future, or is this near-code-duplication limited to just a few instances
like
> > this? (I don't know for sure, but it's also something that can possibly be
> > cleaned up later).
> 
> Yeah, some of of the snapshot code is like that. I'm not sure what to do about
> it. The code is similar at various places, but then differs in awkward ways
> because of the necessarily platform-specific details in reading processes,
etc.
> I tried in some places to extract an interface but it felt forced.
> 
> So, I agree we should attempt to de-duplicate some of it, but it hasn't been
> clear how yet.
> 
> What I'd like to do is continue to pursue getting the base functionality
> implemented, and then attempt to combine at least at the interface layer
later.
> I'd been thinking perhaps this would be best to do while adding a 3rd platform
> (Android or Linux I guess) so that the new interface would hopefully be
general
> enough for all the platforms we would care about, rather than
not-quite-generic
> enough if we do it too early.

That's what I was thinking, too. Since we will ultimately be adding at least one
(and maybe two, depending on how divergent the Android and Linux impls are),
this is something that can be tackled later, with a better view of the problem
scope.

[scottmg, 2015-05-28 21:41:42]

Committed patchset #13 (id:320001) manually as
58df54fffbb530eb8f5fcb8cf2bd1608ba579cdd (presubmit successful).