win: Add thread snapshot and memory snapshot for stacks

win: Add thread snapshot and memory snapshot for stacks

The next big piece of functionality in snapshot. There's a bit more
grubbing around in the NT internals than would be nice, and it has
made me start to question the value avoiding MinidumpWriteDump. But
this seems to extract most of the data we need (I haven't pulled
the cpu context yet, but I hope that won't be too hard.)

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/658cd3e1a729e3bd183873b81230da44ed195946

[scottmg, 2015-05-07 23:09:03]

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

[Mark Mentovai, 2015-05-08 16:54:21]

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/memory_sn...
File snapshot/win/memory_snapshot_win.cc (right):

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/memory_sn...
snapshot/win/memory_snapshot_win.cc:39: size_ = static_cast<size_t>(size);
Oh, I see. Can you DLOG_IF size can’t fit into size_? I guess other
MemorySnapshot implementations should follow suit.

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

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:45: #define STATUS_BUFFER_TOO_SMALL         
((NTSTATUS)0xC0000023L)
Drop the extra whitespace. Even though it’s just a copy-paste job, it leads to
ugliness if we later needed to copy-paste something like, say,
STATUS_PRIMARY_TRANSPORT_CONNECT_FAILED.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:49: enum class KTHREAD_STATE : uint32_t {
If this isn’t supposed to match names defined elsewhere (and I don’t think it
is), use something more like KThreadState, kInitialized, etc. per our style
guide.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:49: enum class KTHREAD_STATE : uint32_t {
If there’s a good reference for where this came from, put it in a comment.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:59: MaximumThreadState
I don’t like these “max” things in enums unless there’s a good reason, because
when you switch over an enum, it becomes a valid value that you need to handle
or have a default: for. You don’t do that here, but you also don’t use this
value at all.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:72: reinterpret_cast<uint8_t*>(process) +
offset);
Assure no overflow. Ideally, this function would know where the area returned by
NtQuerySystemInformation() ended, and would refuse to walk outside of that area.
It may be easier for you to do this check in GetProcessInformation() proper.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:81: //! error. On error, a message will be
logged.
Hanging indent.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:101: buffer->reset(new uint8_t[buffer_size]);
Maybe leave a little extra slop in here so that if the size grows a little bit
before the next call, there’s a better chance of accommodating it.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:108: LOG(ERROR) << "NtQuerySystemInformation
failed: " << status;
These are more sensible to print in hex, although the stream operations to set
that up are a pain.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:153: InitializeThreads();
You can do this in Threads() at the expense of another member variable to track
whether it’s been called already.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:223:
GetProcessInformation<process_types::internal::Traits64>(process_,
I don’t think that this will work for ARCH_CPU_32_BITS.

Does GetProcessInformation() return SYSTEM_PROCESS_INFORMATION objects in the
bitness of the caller or the bitness of the processes being described?

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:233: // TODO(scottmg): I'm not sure how to
get SuspendCount. A rough version is
Can you add some blank lines before these comments so that they’re easier to
spot?

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:234: // to set a suspended count if not
currently Running.
The ntexapi.h snippets I’ve found online make it seem like you may need to check
WaitReason (if the state is actually waiting) to know that a thread has been
explicitly suspended.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:244: thread.priority_class =
NORMAL_PRIORITY_CLASS;
And also a blank line after this so it’s obvious that the TODO comment only
applies to priority_class and not to priority or teb.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:248: // StackBase and StackLimit, they don't
appear to be correct in practice (or
I’m not totally surprised, the stack is more of a user-space concern. Maybe one
of these fields got recycled for the suspend count. :)

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:254: thread.stack_region_address = 0;
The constructor already zeroed out these fields.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:39: WinVMAddress teb;
Reorder this for better packing: put the uint32s at the end.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:85: const std::vector<Thread>& Threads();
#include <vector>

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:92: //! Initializes the threads_ vector on
behalf of Initialize().
This should be // or //! \brief. (But note that although sometimes we do have
Doxygen documentation for private things, it’s not configured to generate output
for them.)

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

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:288: typename Traits::Pointer StackLimit;
This is a size, so it’s more of an UnsignedIntegral, right?

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:305: struct SYSTEM_EXTENDED_THREAD_INFORMATION{
I don’t see this in the SDK. If there’s a good reference for it, can you leave
it in a comment? Same (although less important) for CLIENT_ID.

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:305: struct SYSTEM_EXTENDED_THREAD_INFORMATION{
Put a space before the {.

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:324: typename Traits::Pointer StackLimit;
This would be an UnsignedIntegral too.

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:327: typename Traits::Pointer Reserved;
No suspend count hiding out in any of these?

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:337: LARGE_INTEGER Reserved[3];
Some of the ntexapi.h things I found explain the reserved fields, but I’m not
sure if they’re reliable or less canonical.

[Mark Mentovai, 2015-05-08 18:14:15]

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

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:72: reinterpret_cast<uint8_t*>(process) +
offset);
Mark Mentovai wrote:
> Assure no overflow. Ideally, this function would know where the area returned
by
> NtQuerySystemInformation() ended, and would refuse to walk outside of that
area.
> It may be easier for you to do this check in GetProcessInformation() proper.

On the second thought, don’t do any of that.

When I initially read this, I thought for some reason that this information was
coming from user space (like ReadProcessMemory()) and I wanted to guard against
corruption. But it comes from kernel space, and I think we should be able to
trust the offsets without paranoia, absent evidence to the contrary.

[scottmg, 2015-05-08 18:24:02]

Thanks!

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/memory_sn...
File snapshot/win/memory_snapshot_win.cc (right):

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/memory_sn...
snapshot/win/memory_snapshot_win.cc:39: size_ = static_cast<size_t>(size);
On 2015/05/08 16:54:19, Mark Mentovai wrote:
> Oh, I see. Can you DLOG_IF size can’t fit into size_? I guess other
> MemorySnapshot implementations should follow suit.

Done.

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

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:45: #define STATUS_BUFFER_TOO_SMALL         
((NTSTATUS)0xC0000023L)
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> Drop the extra whitespace. Even though it’s just a copy-paste job, it leads to
> ugliness if we later needed to copy-paste something like, say,
> STATUS_PRIMARY_TRANSPORT_CONNECT_FAILED.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:49: enum class KTHREAD_STATE : uint32_t {
On 2015/05/08 16:54:19, Mark Mentovai wrote:
> If there’s a good reference for where this came from, put it in a comment.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:49: enum class KTHREAD_STATE : uint32_t {
On 2015/05/08 16:54:19, Mark Mentovai wrote:
> If this isn’t supposed to match names defined elsewhere (and I don’t think it
> is), use something more like KThreadState, kInitialized, etc. per our style
> guide.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:59: MaximumThreadState
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> I don’t like these “max” things in enums unless there’s a good reason, because
> when you switch over an enum, it becomes a valid value that you need to handle
> or have a default: for. You don’t do that here, but you also don’t use this
> value at all.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:72: reinterpret_cast<uint8_t*>(process) +
offset);
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> Assure no overflow. Ideally, this function would know where the area returned
by
> NtQuerySystemInformation() ended, and would refuse to walk outside of that
area.
> It may be easier for you to do this check in GetProcessInformation() proper.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:81: //! error. On error, a message will be
logged.
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> Hanging indent.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:101: buffer->reset(new uint8_t[buffer_size]);
On 2015/05/08 16:54:19, Mark Mentovai wrote:
> Maybe leave a little extra slop in here so that if the size grows a little bit
> before the next call, there’s a better chance of accommodating it.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:108: LOG(ERROR) << "NtQuerySystemInformation
failed: " << status;
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> These are more sensible to print in hex, although the stream operations to set
> that up are a pain.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:153: InitializeThreads();
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> You can do this in Threads() at the expense of another member variable to
track
> whether it’s been called already.

Better? I guess? Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:223:
GetProcessInformation<process_types::internal::Traits64>(process_,
On 2015/05/08 16:54:19, Mark Mentovai wrote:
> I don’t think that this will work for ARCH_CPU_32_BITS.
> 
> Does GetProcessInformation() return SYSTEM_PROCESS_INFORMATION objects in the
> bitness of the caller or the bitness of the processes being described?

Quite right, wouldn't even compile. I was fiddling around getting the structure
layout for x64 and had hardcoded it temporarily to avoid getting confused. I've
tried retrieving from both x64 and x86 processes now, but haven't tried
compiling as x86 recently.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:233: // TODO(scottmg): I'm not sure how to
get SuspendCount. A rough version is
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> Can you add some blank lines before these comments so that they’re easier to
> spot?

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:234: // to set a suspended count if not
currently Running.
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> The ntexapi.h snippets I’ve found online make it seem like you may need to
check
> WaitReason (if the state is actually waiting) to know that a thread has been
> explicitly suspended.

Looking at
https://msdn.microsoft.com/en-us/library/system.diagnostics.processthread.wai...
it seems like ThreadState is the controlling one, and WaitReason is extra info.
But WaitReason is only valid when in the Wait state (as opposed to != Running),
so switched the sense to be == Wait instead.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:244: thread.priority_class =
NORMAL_PRIORITY_CLASS;
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> And also a blank line after this so it’s obvious that the TODO comment only
> applies to priority_class and not to priority or teb.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:248: // StackBase and StackLimit, they don't
appear to be correct in practice (or
On 2015/05/08 16:54:19, Mark Mentovai wrote:
> I’m not totally surprised, the stack is more of a user-space concern. Maybe
one
> of these fields got recycled for the suspend count. :)

They're funny, they sort of "seem" like the right size and are in plausible
locations for a stack, but they're normally not even mapped into the process'
address space. It might just be a matter of translating them relative to some
base.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:254: thread.stack_region_address = 0;
On 2015/05/08 16:54:19, Mark Mentovai wrote:
> The constructor already zeroed out these fields.

Oops, yes. Forgot to remove them when I added Thread::Thread(). Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
File snapshot/win/process_reader_win.h (right):

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:39: WinVMAddress teb;
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> Reorder this for better packing: put the uint32s at the end.

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:85: const std::vector<Thread>& Threads();
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> #include <vector>

Done.

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.h:92: //! Initializes the threads_ vector on
behalf of Initialize().
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> This should be // or //! \brief. (But note that although sometimes we do have
> Doxygen documentation for private things, it’s not configured to generate
output
> for them.)

Done. Then removed.

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

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:288: typename Traits::Pointer StackLimit;
On 2015/05/08 16:54:21, Mark Mentovai wrote:
> This is a size, so it’s more of an UnsignedIntegral, right?

No, they're start/end pointers. Size is calculated in snapshot by subtracting.

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:305: struct SYSTEM_EXTENDED_THREAD_INFORMATION{
On 2015/05/08 16:54:21, Mark Mentovai wrote:
> Put a space before the {.

Done.

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:305: struct SYSTEM_EXTENDED_THREAD_INFORMATION{
On 2015/05/08 16:54:21, Mark Mentovai wrote:
> I don’t see this in the SDK. If there’s a good reference for it, can you leave
> it in a comment? Same (although less important) for CLIENT_ID.

Done.

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:324: typename Traits::Pointer StackLimit;
On 2015/05/08 16:54:20, Mark Mentovai wrote:
> This would be an UnsignedIntegral too.

See above.

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:327: typename Traits::Pointer Reserved;
On 2015/05/08 16:54:21, Mark Mentovai wrote:
> No suspend count hiding out in any of these?

They seem to be all 0 most of the time unfortunately. It has to be somewhere
nearby though, as it's the return value from Suspend/ResumeThread.

https://codereview.chromium.org/1131473005/diff/100001/util/win/process_struc...
util/win/process_structs.h:337: LARGE_INTEGER Reserved[3];
On 2015/05/08 16:54:21, Mark Mentovai wrote:
> Some of the ntexapi.h things I found explain the reserved fields, but I’m not
> sure if they’re reliable or less canonical.

It seems like they were unused on older OSs, and have been filled in later. I
think I'll just leave them as unknown for now as I don't think we need any of
that data anyway.

[scottmg, 2015-05-08 18:25:26]

On 2015/05/08 18:14:15, Mark Mentovai wrote:
>
https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
> File snapshot/win/process_reader_win.cc (right):
> 
>
https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
> snapshot/win/process_reader_win.cc:72: reinterpret_cast<uint8_t*>(process) +
> offset);
> Mark Mentovai wrote:
> > Assure no overflow. Ideally, this function would know where the area
returned
> by
> > NtQuerySystemInformation() ended, and would refuse to walk outside of that
> area.
> > It may be easier for you to do this check in GetProcessInformation() proper.
> 
> On the second thought, don’t do any of that.
> 
> When I initially read this, I thought for some reason that this information
was
> coming from user space (like ReadProcessMemory()) and I wanted to guard
against
> corruption. But it comes from kernel space, and I think we should be able to
> trust the offsets without paranoia, absent evidence to the contrary.

Hmm, that's true. I suppose there's very little chance of that process (or the
kernel continuing to function usefully) if that data is corrupt.

One sec and I'll nuke it again.

[scottmg, 2015-05-08 18:27:10]

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

https://codereview.chromium.org/1131473005/diff/100001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:72: reinterpret_cast<uint8_t*>(process) +
offset);
On 2015/05/08 18:14:14, Mark Mentovai wrote:
> Mark Mentovai wrote:
> > Assure no overflow. Ideally, this function would know where the area
returned
> by
> > NtQuerySystemInformation() ended, and would refuse to walk outside of that
> area.
> > It may be easier for you to do this check in GetProcessInformation() proper.
> 
> On the second thought, don’t do any of that.
> 
> When I initially read this, I thought for some reason that this information
was
> coming from user space (like ReadProcessMemory()) and I wanted to guard
against
> corruption. But it comes from kernel space, and I think we should be able to
> trust the offsets without paranoia, absent evidence to the contrary.

(Un)Done.

[Mark Mentovai, 2015-05-08 20:30:21]

LGTM

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

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:51: enum class KTHREAD_STATE : uint32_t {
KTHREAD_STATE intentional instead of KThreadState?

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:102: buffer->reset(new uint8_t[buffer_size]);
No slop? (It’s OK if you’re against doing it, but you said “done” to this and I
don’t see it.)

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:141: modules_(),
threads_(),

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:202: 
Set initialized_threads_ to true here, so that if an early return is ever
introduced, it won’t cause subsequent calls to Threads() to try again.

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:210:
process_types::SYSTEM_PROCESS_INFORMATION<SizeTraits>* process_information;
No need to declare this until you initialize it, a couple of lines later.

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:216: for (unsigned i = 0; i <
process_information->NumberOfThreads; ++i) {
“unsigned long” to match decltype(process_information->NumberOfThreads)

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:225: thread_info.ThreadState ==
static_cast<ULONG>(KTHREAD_STATE::kGateWait)
Not that Stack Overflow is known for correctness, but
http://stackoverflow.com/questions/22949725/how-to-get-thread-state-e-g-suspe...
made me think that you’d need to check ThreadState == kWaiting && WaitReason ==
kSuspended.

[scottmg, 2015-05-08 21:05:45]

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

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:51: enum class KTHREAD_STATE : uint32_t {
On 2015/05/08 20:30:21, Mark Mentovai wrote:
> KTHREAD_STATE intentional instead of KThreadState?

That was to match the standard kernel/ntinternals format, but I guess since we
renamed the values, it might as well go to our style.

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:102: buffer->reset(new uint8_t[buffer_size]);
On 2015/05/08 20:30:21, Mark Mentovai wrote:
> No slop? (It’s OK if you’re against doing it, but you said “done” to this and
I
> don’t see it.)

Oops, I did it, then undid it when I added the range check, and then forgot to
readd it. I think it makes sense to add a bit.

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:141: modules_(),
On 2015/05/08 20:30:21, Mark Mentovai wrote:
> threads_(),

Done.

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:202: 
On 2015/05/08 20:30:21, Mark Mentovai wrote:
> Set initialized_threads_ to true here, so that if an early return is ever
> introduced, it won’t cause subsequent calls to Threads() to try again.

Done.

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:210:
process_types::SYSTEM_PROCESS_INFORMATION<SizeTraits>* process_information;
On 2015/05/08 20:30:21, Mark Mentovai wrote:
> No need to declare this until you initialize it, a couple of lines later.

Done.

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:216: for (unsigned i = 0; i <
process_information->NumberOfThreads; ++i) {
On 2015/05/08 20:30:21, Mark Mentovai wrote:
> “unsigned long” to match decltype(process_information->NumberOfThreads)

Done.

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:225: thread_info.ThreadState ==
static_cast<ULONG>(KTHREAD_STATE::kGateWait)
On 2015/05/08 20:30:21, Mark Mentovai wrote:
> Not that Stack Overflow is known for correctness, but
>
http://stackoverflow.com/questions/22949725/how-to-get-thread-state-e-g-suspe...
> made me think that you’d need to check ThreadState == kWaiting && WaitReason
==
> kSuspended.

I think that's "sort of" right, in that that is what's required to check if the
thread is literally "suspended". But I checked e.g. a blocked chrome thread pool
worker, and it's in state ThreadState == kWaiting && WaitReason == WrQueue.

That is, I think really we want to know when the thread is "waiting", not when
it's "suspended", even though the value is called suspend_count. (Unless you
disagree about the purpose of SuspendCount, in which case I agree with what you
wrote.)

Hopefully I can find the real SuspendCount at some point anyway.

[Mark Mentovai, 2015-05-08 21:15:00]

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

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:225: thread_info.ThreadState ==
static_cast<ULONG>(KTHREAD_STATE::kGateWait)
scottmg wrote:
> On 2015/05/08 20:30:21, Mark Mentovai wrote:
> > Not that Stack Overflow is known for correctness, but
> >
>
http://stackoverflow.com/questions/22949725/how-to-get-thread-state-e-g-suspe...
> > made me think that you’d need to check ThreadState == kWaiting && WaitReason
> ==
> > kSuspended.
> 
> I think that's "sort of" right, in that that is what's required to check if
the
> thread is literally "suspended". But I checked e.g. a blocked chrome thread
pool
> worker, and it's in state ThreadState == kWaiting && WaitReason == WrQueue.
> 
> That is, I think really we want to know when the thread is "waiting", not when
> it's "suspended", even though the value is called suspend_count. (Unless you
> disagree about the purpose of SuspendCount, in which case I agree with what
you
> wrote.)
> 
> Hopefully I can find the real SuspendCount at some point anyway.

Oh, OK, that’s the source of the discrepancy, then!

I think that this should be the real suspend count, the one that’s returned by
SuspendThread(). I don’t think that we’re really too interested in “waiting”
here. Normally it would be obvious enough from the backtrace that a thread’s
waiting, but there would otherwise be no way to tell that it had been explicitly
suspended. This was my understanding of the field as used in the minidump, and
it’s how I implemented suspend_count for Mac. I wouldn’t expect an ordinary
blocked thread to show up as suspended.

[scottmg, 2015-05-09 02:40:10]

Patchset #12 (id:220001) has been deleted

[scottmg, 2015-05-09 02:46:15]

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

https://codereview.chromium.org/1131473005/diff/140001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:225: thread_info.ThreadState ==
static_cast<ULONG>(KTHREAD_STATE::kGateWait)
On 2015/05/08 21:15:00, Mark Mentovai wrote:
> scottmg wrote:
> > On 2015/05/08 20:30:21, Mark Mentovai wrote:
> > > Not that Stack Overflow is known for correctness, but
> > >
> >
>
http://stackoverflow.com/questions/22949725/how-to-get-thread-state-e-g-suspe...
> > > made me think that you’d need to check ThreadState == kWaiting &&
WaitReason
> > ==
> > > kSuspended.
> > 
> > I think that's "sort of" right, in that that is what's required to check if
> the
> > thread is literally "suspended". But I checked e.g. a blocked chrome thread
> pool
> > worker, and it's in state ThreadState == kWaiting && WaitReason == WrQueue.
> > 
> > That is, I think really we want to know when the thread is "waiting", not
when
> > it's "suspended", even though the value is called suspend_count. (Unless you
> > disagree about the purpose of SuspendCount, in which case I agree with what
> you
> > wrote.)
> > 
> > Hopefully I can find the real SuspendCount at some point anyway.
> 
> Oh, OK, that’s the source of the discrepancy, then!
> 
> I think that this should be the real suspend count, the one that’s returned by
> SuspendThread(). I don’t think that we’re really too interested in “waiting”
> here. Normally it would be obvious enough from the backtrace that a thread’s
> waiting, but there would otherwise be no way to tell that it had been
explicitly
> suspended. This was my understanding of the field as used in the minidump, and
> it’s how I implemented suspend_count for Mac. I wouldn’t expect an ordinary
> blocked thread to show up as suspended.

I see, my mistake.

I created a thread with CREATE_SUSPENDED and wrote that via MiniDumpWriteDump,
and created another that was blocked on a CRITICAL_SECTION, and indeed the first
has a SuspendCount of 1 and the second is 0, so your interpretation is the right
one.

But then it annoyed me even more than I couldn't get that value... I windbg'd
dbghelp briefly and I believe it actually SuspendThread/ResumeThread's each
thread, but it's a bit complicated, so not entirely certain.

But I also was poking around found NtQueryInformationThread and an alternate
THREADINFOCLASS in ntddk.h. Calling that appears to return the correct value. I
also added a failure case of return 1 similar to what was here before.

So, please take another look since ps#10.

[scottmg, 2015-05-11 20:29:58]

Committed patchset #13 (id:260001) manually as
658cd3e1a729e3bd183873b81230da44ed195946 (presubmit successful).