win: Implement exception snapshot

win: Implement exception snapshot

Refactor some of the NT internals helpers and cpu_context to share
between the thread and exception snapshot code.

Add test that runs crashing child and validates the exception in the
snapshot.

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

[scottmg, 2015-05-13 23:01:11]

scottmg@chromium.org changed reviewers:
+ cpu@chromium.org, mark@chromium.org, rsesek@chromium.org

[scottmg, 2015-05-13 23:03:42]

Patchset #7 (id:120001) has been deleted

[cpu_(ooo_6.6-7.5), 2015-05-15 01:24:13]

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:26: 
I must mention this somewhere but unrelated to this CL is that calling Nt
functions does not set the last error so GetLastError() contains basically
garbage. So some people add a SetLastError(NtStatusToDosError(nt_status)) after
each Nt call, others are careful to not assume... I am fine with either.

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:41: return
basic_information.ClientId.UniqueThread;
is it really the same as ::GetThreadId() ?  I mean I think its fine as a unique
identifier but reading the docs its not clear is the same.

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.h (right):

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.h:72: DWORD exception_;
I personally would call this the exception code since the word exception can
have so many meanings.

https://codereview.chromium.org/1126413008/diff/140001/util/win/nt_internals.h
File util/win/nt_internals.h (right):

https://codereview.chromium.org/1126413008/diff/140001/util/win/nt_internals....
util/win/nt_internals.h:30: NTSTATUS NtQuerySystemInformation(
stdcall or WINAPI

[scottmg, 2015-08-18 05:08:40]

Sorry for the ridiculously long delay on this patch.

(With this we should just about be ready to test end-to-end capture and upload.)

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:26: 
On 2015/05/15 01:24:13, cpu wrote:
> I must mention this somewhere but unrelated to this CL is that calling Nt
> functions does not set the last error so GetLastError() contains basically
> garbage. So some people add a SetLastError(NtStatusToDosError(nt_status))
after
> each Nt call, others are careful to not assume... I am fine with either.

Noted. I tried to use PLOG/LOG correctly, it's possible I missed some though.

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:41: return
basic_information.ClientId.UniqueThread;
On 2015/05/15 01:24:12, cpu wrote:
> is it really the same as ::GetThreadId() ?  I mean I think its fine as a
unique
> identifier but reading the docs its not clear is the same.

It is by testing (and ProcessHacker-ing, and it's how reactos implements
GetThreadId), but I can't find anywhere it's documented, unfortunately.

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.h (right):

https://codereview.chromium.org/1126413008/diff/140001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.h:72: DWORD exception_;
On 2015/05/15 01:24:13, cpu wrote:
> I personally would call this the exception code since the word exception can
> have so many meanings.

Done.

https://codereview.chromium.org/1126413008/diff/140001/util/win/nt_internals.h
File util/win/nt_internals.h (right):

https://codereview.chromium.org/1126413008/diff/140001/util/win/nt_internals....
util/win/nt_internals.h:30: NTSTATUS NtQuerySystemInformation(
On 2015/05/15 01:24:13, cpu wrote:
> stdcall or WINAPI
> 

This is crashpad::NtQuerySystemInformation, so I don't think it needs to match
calling convention. The .cc uses ::NtQuerySystemInformation to get the prototype
for ntdll so is __stdcall.

[Mark Mentovai, 2015-08-18 16:17:20]

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_client...
client/crashpad_client_win.cc:59: SleepEx(false, INFINITE);
I should have caught this last week. Are the arguments reversed?
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686307(v=vs.85).aspx

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.cc
File client/crashpad_info.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.c...
client/crashpad_info.cc:92: simple_annotations_(nullptr),
This comma isn’t going to work on non-Windows, it’s got to move into the #ifdef.

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.h
File client/crashpad_info.h (right):

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.h...
client/crashpad_info.h:102: //! \brief Save the crashing thread id for the crash
handler.
Nit: ID.

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.h...
client/crashpad_info.h:106: //! \brief Save an EXCEPTION_POINTERS record for the
crash handler.
Nit: `EXCEPTION_POINTERS`.

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.h...
client/crashpad_info.h:137: DWORD thread_id_;
Organize for better packing: DWORDs follow pointers.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/exception_sna...
File snapshot/exception_snapshot.h (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/exception_sna...
snapshot/exception_snapshot.h:74: //! received will appear at index 1 of
Codes().
Is there anything we should say about Windows here?

https://codereview.chromium.org/1126413008/diff/370001/snapshot/exception_sna...
snapshot/exception_snapshot.h:97: //! handler.
Or here?

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.h (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.h:15: #ifndef
CRASHPAD_SNAPSHOT_WIN_CPU_CONTEXT_WIN_H_
Can you do your next upload with --no-find-copies?

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.h:25: //! \brief Initializes a CPUContextX86
structure from native context structures
Singular “a native context structure” or “a native `CONTEXT` structure”. Same
for the other function.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.h:27: void InitializeX86Context(const CONTEXT&
context, CPUContextX86* out);
Because CONTEXT is one structure that’s CPU-specific, I don’t know if it makes
sense to have all of the various Initialize variants declared. But I don’t yet
see from the .cc file what the plan is to do cross-arch context initialization.
(Maybe there’s a WoW function to help, or maybe you need to declare the
other-arch CONTEXT yourself?)

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win_test.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win_test.cc:27: #if defined(ARCH_CPU_X86_FAMILY)
Doesn’t look like it’ll work as-is on 32-bit x86, though.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:30: uint64_t GetThreadId(HANDLE handle) {
Can you add a test for this that makes sure that it returns the same value as
::GetThreadId()? It’s probably enough to test with GetCurrentThread(), as long
as your GetThreadId() works with that fake handle. On the other hand, if your
function is limited in that way relative to ::GetThreadId(), call it out in the
comment above.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:38: LOG(ERROR) << "could not retrieve
ThreadBasicInformation";
This and a couple of other things we have would be OK candidates for
NTSTATUS_LOG(). No need to do it now, but it’s worth considering if the trend
continues.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:41: return
basic_information.ClientId.UniqueThread;
This is declared in our structs as a Pointer, but the Windows API uses DWORDs
for thread IDs. Are these truly compatible?

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:44: uint64_t
GetThreadId(ProcessReaderWin* process_reader, HANDLE handle) {
Now I feel duped. This doesn’t seem to be called by anything, and it’s in an
unnamed namespace, so…unused?

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:82: LOG(ERROR) << "thread id not found in
process";
Spew out the problem ID here?

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.h (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.h:41: //! \param[in] process_reader A
ProcessReader for the task that sustained the
Is “task” the right nomenclature on Windows?

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.h:43: //! \param[in] thread_id The thread id
in which the exception occurred.
Nits: ID here, `EXCEPTION_POINTERS` on the next line.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:60: DCHECK(!exception_);
Can you add a logging.h #include to the Mac equivalent file too? Looks like it’s
missing, but it does the same thing.

[scottmg, 2015-08-18 16:56:00]

Thanks!

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_client...
File client/crashpad_client_win.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_client...
client/crashpad_client_win.cc:59: SleepEx(false, INFINITE);
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> I should have caught this last week. Are the arguments reversed?
>
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686307(v=vs.85).aspx

Wow, yes. :( Done.

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.cc
File client/crashpad_info.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.c...
client/crashpad_info.cc:92: simple_annotations_(nullptr),
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> This comma isn’t going to work on non-Windows, it’s got to move into the
#ifdef.

Done.

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.h
File client/crashpad_info.h (right):

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.h...
client/crashpad_info.h:102: //! \brief Save the crashing thread id for the crash
handler.
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Nit: ID.

Done.

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.h...
client/crashpad_info.h:106: //! \brief Save an EXCEPTION_POINTERS record for the
crash handler.
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Nit: `EXCEPTION_POINTERS`.

Done.

https://codereview.chromium.org/1126413008/diff/370001/client/crashpad_info.h...
client/crashpad_info.h:137: DWORD thread_id_;
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Organize for better packing: DWORDs follow pointers.

Done.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/exception_sna...
File snapshot/exception_snapshot.h (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/exception_sna...
snapshot/exception_snapshot.h:74: //! received will appear at index 1 of
Codes().
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Is there anything we should say about Windows here?

Done.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/exception_sna...
snapshot/exception_snapshot.h:97: //! handler.
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Or here?

Done.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.h (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.h:15: #ifndef
CRASHPAD_SNAPSHOT_WIN_CPU_CONTEXT_WIN_H_
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Can you do your next upload with --no-find-copies?

Will do.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.h:25: //! \brief Initializes a CPUContextX86
structure from native context structures
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Singular “a native context structure” or “a native `CONTEXT` structure”. Same
> for the other function.

Done.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.h:27: void InitializeX86Context(const CONTEXT&
context, CPUContextX86* out);
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Because CONTEXT is one structure that’s CPU-specific, I don’t know if it makes
> sense to have all of the various Initialize variants declared. But I don’t yet
> see from the .cc file what the plan is to do cross-arch context
initialization.
> (Maybe there’s a WoW function to help, or maybe you need to declare the
> other-arch CONTEXT yourself?)

Yes, you're right. I'm not sure what's the best solution.

On x64 WOW64_CONTEXT matches x86 CONTEXT, so I #ifdef'd them like that for now.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win_test.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win_test.cc:27: #if defined(ARCH_CPU_X86_FAMILY)
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Doesn’t look like it’ll work as-is on 32-bit x86, though.

Added #error on 32 for now.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:30: uint64_t GetThreadId(HANDLE handle) {
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Can you add a test for this that makes sure that it returns the same value as
> ::GetThreadId()? It’s probably enough to test with GetCurrentThread(), as long
> as your GetThreadId() works with that fake handle. On the other hand, if your
> function is limited in that way relative to ::GetThreadId(), call it out in
the
> comment above.

Removed.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:38: LOG(ERROR) << "could not retrieve
ThreadBasicInformation";
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> This and a couple of other things we have would be OK candidates for
> NTSTATUS_LOG(). No need to do it now, but it’s worth considering if the trend
> continues.

Removed.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:41: return
basic_information.ClientId.UniqueThread;
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> This is declared in our structs as a Pointer, but the Windows API uses DWORDs
> for thread IDs. Are these truly compatible?

Removed.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:44: uint64_t
GetThreadId(ProcessReaderWin* process_reader, HANDLE handle) {
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Now I feel duped. This doesn’t seem to be called by anything, and it’s in an
> unnamed namespace, so…unused?

Oops, sorry. I previously had the thread HANDLE being passed from the client,
but passing the thread ID makes much more sense.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:82: LOG(ERROR) << "thread id not found in
process";
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Spew out the problem ID here?

Done.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.h (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.h:41: //! \param[in] process_reader A
ProcessReader for the task that sustained the
On 2015/08/18 16:17:19, Mark Mentovai - out til August wrote:
> Is “task” the right nomenclature on Windows?

Done.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.h:43: //! \param[in] thread_id The thread id
in which the exception occurred.
On 2015/08/18 16:17:20, Mark Mentovai - out til August wrote:
> Nits: ID here, `EXCEPTION_POINTERS` on the next line.

Done.

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:60: DCHECK(!exception_);
On 2015/08/18 16:17:20, Mark Mentovai - out til August wrote:
> Can you add a logging.h #include to the Mac equivalent file too? Looks like
it’s
> missing, but it does the same thing.

Mac looks correct to me? I added it here though.

https://codereview.chromium.org/1126413008/diff/370001/util/win/process_struc...
File util/win/process_structs.h (right):

https://codereview.chromium.org/1126413008/diff/370001/util/win/process_struc...
util/win/process_structs.h:346: // See
http://undocumented.ntinternals.net/source/usermode/structures/thread_basic_i....
No longer needed (was for GetThreadId()).

[Mark Mentovai, 2015-08-18 17:52:13]

LGTM

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:60: DCHECK(!exception_);
scottmg wrote:
> On 2015/08/18 16:17:20, Mark Mentovai - out til August wrote:
> > Can you add a logging.h #include to the Mac equivalent file too? Looks like
> it’s
> > missing, but it does the same thing.
> 
> Mac looks correct to me? I added it here though.

I see process_snapshot_mac.cc using DCHECK() without #including logging.h.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/exception_sna...
File snapshot/exception_snapshot.h (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/exception_sna...
snapshot/exception_snapshot.h:76: //! On Windows, this will either be 0 if the
exception is continuable, or
`0`

https://codereview.chromium.org/1126413008/diff/390001/snapshot/exception_sna...
snapshot/exception_snapshot.h:104: //! `EXCEPTION_RECORD`.
Cool, thanks. You may want to add a little bit of Windows knowledge to our copy
of dbghelp.h now too, in the same way that you’ve done here.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/snapshot_test...
File snapshot/snapshot_test.gyp (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/snapshot_test...
snapshot/snapshot_test.gyp:57: '../handler/handler.gyp:crashpad_handler',
I see what’s happening and it’s OK for this change, but I didn’t actually
envision that “handler” would be a static library, and I certainly didn’t
envision snapshot depending on handler for anything, because handler depends on
snapshot.

This conceptual circle will need to be resolved, but that’s not appropriate for
this change.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:75: #endif  // ARCH_CPU_64_BITS
Blank after.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.h (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.h:27: #if defined(ARCH_CPU_64_BITS)
|| DOXYGEN

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win_test.cc (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win_test.cc:29: #if defined(ARCH_CPU_64_BITS)
Why not just #if defined(ARCH_CPU_X86_64) instead of the nested #ifs?

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:87: LOG(WARNING) << "dropping chained
ExceptionRecord";
Do we care? All we’d want to do at this point is file a bug for future support,
unless we’re certain that we don’t care.

[scottmg, 2015-08-18 18:18:21]

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/370001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:60: DCHECK(!exception_);
On 2015/08/18 17:52:13, Mark Mentovai - out til August wrote:
> scottmg wrote:
> > On 2015/08/18 16:17:20, Mark Mentovai - out til August wrote:
> > > Can you add a logging.h #include to the Mac equivalent file too? Looks
like
> > it’s
> > > missing, but it does the same thing.
> > 
> > Mac looks correct to me? I added it here though.
> 
> I see process_snapshot_mac.cc using DCHECK() without #including logging.h.

Oh, I must have been looking at exception_snapshot_mac.cc. Done.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/exception_sna...
File snapshot/exception_snapshot.h (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/exception_sna...
snapshot/exception_snapshot.h:76: //! On Windows, this will either be 0 if the
exception is continuable, or
On 2015/08/18 17:52:13, Mark Mentovai - out til August wrote:
> `0`

Done.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/exception_sna...
snapshot/exception_snapshot.h:104: //! `EXCEPTION_RECORD`.
On 2015/08/18 17:52:13, Mark Mentovai - out til August wrote:
> Cool, thanks. You may want to add a little bit of Windows knowledge to our
copy
> of dbghelp.h now too, in the same way that you’ve done here.

Done.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.cc (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.cc:75: #endif  // ARCH_CPU_64_BITS
On 2015/08/18 17:52:13, Mark Mentovai - out til August wrote:
> Blank after.

Done.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win.h (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win.h:27: #if defined(ARCH_CPU_64_BITS)
On 2015/08/18 17:52:13, Mark Mentovai - out til August wrote:
> || DOXYGEN

Done.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
File snapshot/win/cpu_context_win_test.cc (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/cpu_conte...
snapshot/win/cpu_context_win_test.cc:29: #if defined(ARCH_CPU_64_BITS)
On 2015/08/18 17:52:13, Mark Mentovai - out til August wrote:
> Why not just #if defined(ARCH_CPU_X86_64) instead of the nested #ifs?

Done.

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/exception...
File snapshot/win/exception_snapshot_win.cc (right):

https://codereview.chromium.org/1126413008/diff/390001/snapshot/win/exception...
snapshot/win/exception_snapshot_win.cc:87: LOG(WARNING) << "dropping chained
ExceptionRecord";
On 2015/08/18 17:52:13, Mark Mentovai - out til August wrote:
> Do we care? All we’d want to do at this point is file a bug for future
support,
> unless we’re certain that we don’t care.

I'm not familiar enough with nested exceptions to say if it's useful. I filed a
bug and added a link for now.

[Mark Mentovai, 2015-08-18 19:23:49]

LGTM still stands. This is a big step forward for the Windows client.

[scottmg, 2015-08-18 19:25:31]

Committed patchset #21 (id:410001) manually as
a691448ffbb8db8576f01b424b2139a6ff21d240 (presubmit successful).