win: Add and use a custom CaptureContext() implementation

win: Add and use a custom CaptureContext() implementation

RtlCaptureContext() is buggy and limited.

BUG=crashpad:53
R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/c8592b847b99f7f8643c342999c3f8f4f1e9b2e8

[Mark Mentovai, 2015-09-29 20:25:51]

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

[scottmg, 2015-09-29 21:58:41]

Great stuff! I stared at the asm for quite a while and LGTM.

https://codereview.chromium.org/1377963002/diff/1/util/win/capture_context.asm
File util/win/capture_context.asm (right):

https://codereview.chromium.org/1377963002/diff/1/util/win/capture_context.as...
util/win/capture_context.asm:58: mov dword ptr [ebx+156], edi  ; context->Edi
Do you think it would be more readable to define

CONTEXT struct
  ContextFlags DWORD ?
  Dr0 DWORD ?
..
CONTEXT ends

Of course the offsets can't change, so maybe it's just as good this way.

Another option would be a set of `equ`s but that doesn't seem to help much as
they're mostly only used in one location anyway.

Or, maybe just the output of (`dt _CONTEXT` with hex offsets converted to
decimal) in a comment would be helpful.

https://codereview.chromium.org/1377963002/diff/1/util/win/capture_context.as...
util/win/capture_context.asm:171: mov dword ptr [ebx+136], 0  ;
context->FloatSave.Spare0
Add " / .Cr0NpxState" to comment (for WOW64)

[Mark Mentovai, 2015-09-30 15:28:47]

Patchset #3 (id:40001) has been deleted

[Mark Mentovai, 2015-09-30 15:33:22]

Can you double-check at least the diffs from patch set 2 to make sure that
everything’s being put in the right place?

https://codereview.chromium.org/1377963002/diff/1/util/win/capture_context.asm
File util/win/capture_context.asm (right):

https://codereview.chromium.org/1377963002/diff/1/util/win/capture_context.as...
util/win/capture_context.asm:58: mov dword ptr [ebx+156], edi  ; context->Edi
scottmg wrote:
> Do you think it would be more readable to define
> 
> CONTEXT struct
>   ContextFlags DWORD ?
>   Dr0 DWORD ?
> ..
> CONTEXT ends
> 
> Of course the offsets can't change, so maybe it's just as good this way.

I’m so used to reading disassembly that I forget about the things the assembler
can do to help. I do think that the struct definitions are more readable, and
it’s easier to compare the asm redefinition to <winnt.h> than it is to try to
keep track of the offsets.

Done.

https://codereview.chromium.org/1377963002/diff/1/util/win/capture_context.as...
util/win/capture_context.asm:171: mov dword ptr [ebx+136], 0  ;
context->FloatSave.Spare0
scottmg wrote:
> Add " / .Cr0NpxState" to comment (for WOW64)

Obsolete now, but I addressed this in the struct.

[scottmg, 2015-09-30 17:33:38]

Much more readable (at the minor expense of the extra length of the
definitions). I didn't know the assembler could do some of those things, cool
stuff.

lgtm

https://codereview.chromium.org/1377963002/diff/100001/util/win/capture_conte...
File util/win/capture_context.asm (right):

https://codereview.chromium.org/1377963002/diff/100001/util/win/capture_conte...
util/win/capture_context.asm:360: fnsave [ebx.Context].c_FloatSave
Context -> CONTEXT for consistency (and the next 2)

https://codereview.chromium.org/1377963002/diff/100001/util/win/capture_conte...
util/win/capture_context.asm:479: ; and the debug control register fields.
Maybe something about "the rest of the structure" to correspond to the code.

[Mark Mentovai, 2015-09-30 18:10:15]

Committed patchset #6 (id:120001) manually as
c8592b847b99f7f8643c342999c3f8f4f1e9b2e8 (presubmit successful).