win: Retrieve thread context for x64

win: Retrieve thread context for x64

Retrieve context and save to thread context. NtQueryInformationThread
is no longer required (right now?) because to retrieve the CONTEXT, the
thread needs to be Suspend/ResumeThread'd anyway, and the return value
of SuspendThread is the previous SuspendCount.

I haven't handle the x86 case yet -- that would ideally be via
Wow64GetThreadContext (I think) but unfortunately that's Vista+, so I'll
likely need to to a bit of fiddling to get that sorted out. (It's actually
likely going to be NtQueryInformationThread again, but one thing at a
time for now.)

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

[scottmg, 2015-05-11 23:30:38]

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

[cpu_(ooo_6.6-7.5), 2015-05-12 01:06:50]

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_reader...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_reader...
snapshot/win/process_reader_win.cc:175: CONTEXT context;
= {0} ? or you imagine that GTC in 176 fills everything?

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_reader...
snapshot/win/process_reader_win.cc:303: // its stack fields.
TIB is reliably afaik.

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_snapsh...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_snapsh...
snapshot/win/process_snapshot_win.cc:152: //CHECK(false) << "TODO(scottmg):
Exception()";
you meant to delete this line?  in chrome we don't comment out code. I don't
know what the costume is in this land.

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/thread_snapsho...
File snapshot/win/thread_snapshot_win.cc (right):

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/thread_snapsho...
snapshot/win/thread_snapshot_win.cc:44: out->rflags = context.EFlags;
44 is the only one that breaks uniform naming... intended I assume.

[scottmg, 2015-05-12 01:55:33]

Sorry, wasn't ready for review yet. I'll take a look at those and ping you for a
look later though.

[scottmg, 2015-05-12 17:38:35]

Patchset #3 (id:40001) has been deleted

[scottmg, 2015-05-12 17:48:59]

OK, ready for a look now, thanks.

https://codereview.chromium.org/1133203002/diff/60001/snapshot/win/process_re...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1133203002/diff/60001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:308: thread.stack_region_address =
tib.StackLimit;
This confused me as I naturally think of Base as the smaller number and Limit as
the larger one. But since the stack grows the other way, Limit is the smaller
one and Base is the bigger one. As we're converting this into a range which is a
standard base+size, we use Limit as the base, and Base-Limit as the size.

https://codereview.chromium.org/1133203002/diff/60001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1133203002/diff/60001/tools/generate_dump.cc#...
tools/generate_dump.cc:202: file_writer.Close();
On write failure, the unlink() was failing on Windows because the file was still
open.

[scottmg, 2015-05-12 19:37:05]

(Added a check for suspending ourself in the thread grabber -- one of the tests
walks the current process, oops!)

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_reader...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_reader...
snapshot/win/process_reader_win.cc:175: CONTEXT context;
On 2015/05/12 01:06:49, cpu wrote:
> = {0} ? or you imagine that GTC in 176 fills everything?

This was all wrong in this ps, better now.

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_snapsh...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/process_snapsh...
snapshot/win/process_snapshot_win.cc:152: //CHECK(false) << "TODO(scottmg):
Exception()";
On 2015/05/12 01:06:50, cpu wrote:
> you meant to delete this line?  in chrome we don't comment out code. I don't
> know what the costume is in this land.

Removed.

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/thread_snapsho...
File snapshot/win/thread_snapshot_win.cc (right):

https://codereview.chromium.org/1133203002/diff/1/snapshot/win/thread_snapsho...
snapshot/win/thread_snapshot_win.cc:44: out->rflags = context.EFlags;
On 2015/05/12 01:06:50, cpu wrote:
> 44 is the only one that breaks uniform naming... intended I assume.

Do you mean the capitalization of the name? (This `CONTEXT` is from windows.h.)
Or that it's 'E' vs. 'R'? Even in x64, the EFlags register is only 32 bit. It
looks like on Mac/Mach, the system headers decided to extend it a uint64_t
anyway, so Crashpad has a 64 bit field there, but the Windows headers kept it as
a 32 bit value.

[cpu_(ooo_6.6-7.5), 2015-05-12 21:31:22]

https://codereview.chromium.org/1133203002/diff/80001/snapshot/win/process_re...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1133203002/diff/80001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:169: NtCurrentTeb())->ClientId.UniqueThread;
seems we can do is_current_thread before we open it?

https://codereview.chromium.org/1133203002/diff/80001/util/win/process_structs.h
File util/win/process_structs.h (right):

https://codereview.chromium.org/1133203002/diff/80001/util/win/process_struct...
util/win/process_structs.h:306: // fields of it. See
http://www.nirsoft.net/kernel_struct/vista/TEB.html.
do we want to make sure the packing or the alignment are explicit? here and
above, other defs.

If so maybe in a different CL. Since it is templatized I don't know how it will
look.

[scottmg, 2015-05-12 22:06:37]

https://codereview.chromium.org/1133203002/diff/80001/snapshot/win/process_re...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1133203002/diff/80001/snapshot/win/process_re...
snapshot/win/process_reader_win.cc:169: NtCurrentTeb())->ClientId.UniqueThread;
On 2015/05/12 21:31:21, cpu wrote:
> seems we can do is_current_thread before we open it?

Done.

https://codereview.chromium.org/1133203002/diff/80001/util/win/process_structs.h
File util/win/process_structs.h (right):

https://codereview.chromium.org/1133203002/diff/80001/util/win/process_struct...
util/win/process_structs.h:306: // fields of it. See
http://www.nirsoft.net/kernel_struct/vista/TEB.html.
On 2015/05/12 21:31:21, cpu wrote:
> do we want to make sure the packing or the alignment are explicit? here and
> above, other defs.
> 
> If so maybe in a different CL. Since it is templatized I don't know how it
will
> look.

Yes, it's not obvious per struct unfortunately, but this whole file is pragma
pack 1.

In general, the Traits template parameter is intended to handle padding, see
e.g. SYSTEM_EXTENDED_THREAD_INFORMATION at line 324, where WaitTime is a ULONG
on both x86 and x64, so it's put in a union with a Traits::Pad, which is a 64bit
sized field intended to make alignment correct when the template is used for
reading from an x64 process.

In this particular case for the head of the TEB, I *think* the padding is
correct -- all the TIB fields are pointers, followed by a pointer for
EnvironmentPointer, followed by two pointers in ClientId, all of which should be
naturally aligned. (Having said that, I've only tried it on x64 so far, not x86
because of a few other places that don't handle x86 yet.)

[Robert Sesek, 2015-05-12 22:33:23]

https://codereview.chromium.org/1133203002/diff/100001/snapshot/win/thread_sn...
File snapshot/win/thread_snapshot_win.cc (right):

https://codereview.chromium.org/1133203002/diff/100001/snapshot/win/thread_sn...
snapshot/win/thread_snapshot_win.cc:52: out->dr3 = context.Dr3;
Dr4, 5, and 6 are missing?

[scottmg, 2015-05-12 23:09:24]

https://codereview.chromium.org/1133203002/diff/100001/snapshot/win/thread_sn...
File snapshot/win/thread_snapshot_win.cc (right):

https://codereview.chromium.org/1133203002/diff/100001/snapshot/win/thread_sn...
snapshot/win/thread_snapshot_win.cc:52: out->dr3 = context.Dr3;
On 2015/05/12 22:33:22, Robert Sesek wrote:
> Dr4, 5, and 6 are missing?

They don't appear in the CONTEXT structure on Windows. Updated to set them to
6/7 per comment.

[cpu_(ooo_6.6-7.5), 2015-05-13 00:58:29]

https://codereview.chromium.org/1133203002/diff/140001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1133203002/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:190: return;
"If you call GetThreadContext for the current thread, the function returns
successfully; however, the context returned is not valid."

[scottmg, 2015-05-13 17:16:37]

https://codereview.chromium.org/1133203002/diff/140001/snapshot/win/process_r...
File snapshot/win/process_reader_win.cc (right):

https://codereview.chromium.org/1133203002/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:190: return;
On 2015/05/13 00:58:29, cpu wrote:
> "If you call GetThreadContext for the current thread, the function returns
> successfully; however, the context returned is not valid."
> 

Thanks. I switch to RtlCaptureContext when it's the current thread, and added a
test.

[Robert Sesek, 2015-05-13 21:09:32]

LGTM if cpu is happy

[cpu_(ooo_6.6-7.5), 2015-05-15 00:32:44]

lgtm

[scottmg, 2015-05-15 00:37:08]

Committed patchset #8 (id:160001) manually as
5a21de6a1bf4b75fb0ada218531b9ec7f0cbdb49 (presubmit successful).