win: Add implementation of system_snapshot for Windows

win: Add implementation of system_snapshot for Windows

ProcessReaderWin only a stub for now.

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

[scottmg, 2015-02-19 21:13:07]

Patchset #2 (id:20001) has been deleted

[scottmg, 2015-02-19 21:13:11]

Patchset #1 (id:1) has been deleted

[scottmg, 2015-02-19 21:14:35]

Patchset #1 (id:40001) has been deleted

[Mark Mentovai, 2015-02-20 18:58:52]

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
File snapshot/win/system_snapshot_win.cc (right):

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:52: if
(!GetVersionEx(reinterpret_cast<OSVERSIONINFO*>(&version_info))) {
Looking at the docs for GetVersionEx(), I’m learning that its behavior has
changed for Windows 8.1 and later. Is there a way to reliably determine the OS
version number on these newer operating systems?

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:59: base::StringPrintf("Service Pack %d",
version_info.wServicePackMajor);
If GetVersionEx() failed, you shouldn’t do this.

Everything from line 55 to 70 can be in an “else”.

Also, if both version_info.wServicePackMajor and version_info.wServicePackMinor
are 0, do you need a service pack string at all? I don’t think so.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:66: os_version_full_ =
base::StringPrintf("Windows %d.%d.%d %s",
Should this be Windows NT instead of Windows? Avoid version number confusion…

Is there a way to get the OS to give us the name to use here?

If os_version_build_ might be empty, this shouldn’t end up with a trailing
space.

The %d should probably be %u, same on lines 59 and 62.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:85: // TODO(scottmg): How does this differ
from CPUX86Signature below?
CPURevision() is not CPU architecture-specific, but CPUX86Signature() is
x86-family-specific.

CPURevision() for x86-family CPUs returns interpreted family/model/stepping
values as 16/8/8 bits. The extended family ID and extended model ID are taken
into account.

CPUX86Signature() returns cpuid 1 eax in its raw form. The family, model,
stepping, extended family, and extended model numbers are encoded in that, but
more work needs to be done to make use of them. cpuid 1 eax also carries two
“processor type” bits and some reserved bits that are not preserved in
CPURevision(). Things that need the actual value of cpuid 1 eax from an
x86-family CPU use this instead of CPURevision().

In a minidump file, MINIDUMP_SYSTEM_INFO::ProcessorLevel (family) and
MINIDUMP_SYSTEM_INFO::ProcessorRevision (model and stepping) are set using
CPURevision(). These two fields are valid for all architectures, not just x86.
MINIDUMP_SYSTEM_INFO::Cpu.X86CpuInfo.VersionInformation is set to
CPUX86Signature() for 32-bit x86 only (not x86_64). This field isn’t present for
other CPUs.

The comments in our copy of dbghelp.h explain this as far as minidumps are
concerned, and system_snapshot.h explains it for the snapshot interface,
although I agree that the difference is subtle.

This was done with a new method for CPUX86Signature() in case the “processor
type” or other reserved bits become significant in the future. This design
ensures that they’re captured, since there’s a spot for all of cpuid 1 eax to be
carried without modification, instead of attempting to rebuild cpuid 1 eax from
the decoded family/model/stepping in CPURevision().

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:95: return
base::saturated_cast<uint8_t>(system_info.dwNumberOfProcessors);
Log a warning if this saturates.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:107: return vendor;
std::string(vendor, 12)

or make vendor be size 12 and do std::string(vendor, sizeof(vendor))

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:111: // "Note that this structure definition
was accidentally omitted from WinNT.h."
Belongs in compat then?

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:137: // TODO(scottmg): Return all? Average?
Current?
Good question. Max of current and max of max?

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:138: const uint64_t kMhzToHz = 1000000;
1E6

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:171: return
(static_cast<uint64_t>(cpu_info[2]) << 32) |
You already returned this.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:244: return "";
std::string() (I was reminded of this during the readability review). I don’t
know if there’s anything sensible you can get on Windows either, since the
hardware’s a lot more diverse.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:257: TIME_ZONE_INFORMATION
time_zone_information;
Comment that this returns the current time zone status in dst_status, and not
the status at the time of the snapshot. This is unlike the Mac implementation.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
File snapshot/win/system_snapshot_win_test.cc (right):

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win_test.cc:72: EXPECT_TRUE(cpu_vendor ==
"GenuineIntel" || cpu_vendor == "AuthenticAMD");
This deserves a comment at least acknowledging that there are other CPU vendors
out there but you’re intentionally not caring.

[scottmg, 2015-02-20 21:58:57]

Patchset #2 (id:80001) has been deleted

[scottmg, 2015-02-20 22:00:45]

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
File snapshot/win/system_snapshot_win.cc (right):

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:52: if
(!GetVersionEx(reinterpret_cast<OSVERSIONINFO*>(&version_info))) {
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> Looking at the docs for GetVersionEx(), I’m learning that its behavior has
> changed for Windows 8.1 and later. Is there a way to reliably determine the OS
> version number on these newer operating systems?

GetVersionEx works as long as the manifest is set correctly (it may not yet be
in the crashpad test binaries).

We could use these tedious functions
https://msdn.microsoft.com/en-us/library/windows/desktop/dn424972(v=vs.85).aspx
but then wouldn't get a number for Win 10.

I also sort of prefer using this function, as it matches what Chromium does, on
the off-chance that there's some minor discrepancy that causes confusion.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:59: base::StringPrintf("Service Pack %d",
version_info.wServicePackMajor);
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> If GetVersionEx() failed, you shouldn’t do this.
> 
> Everything from line 55 to 70 can be in an “else”.
> 
> Also, if both version_info.wServicePackMajor and
version_info.wServicePackMinor
> are 0, do you need a service pack string at all? I don’t think so.

Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:66: os_version_full_ =
base::StringPrintf("Windows %d.%d.%d %s",
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> Should this be Windows NT instead of Windows? Avoid version number confusion…
> 
> Is there a way to get the OS to give us the name to use here?
> 
> If os_version_build_ might be empty, this shouldn’t end up with a trailing
> space.
> 
> The %d should probably be %u, same on lines 59 and 62.

Changed in Service Pack, but not here. They're printed from ints which are
converted to match OSVersion()s outputs.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:85: // TODO(scottmg): How does this differ
from CPUX86Signature below?
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> CPURevision() is not CPU architecture-specific, but CPUX86Signature() is
> x86-family-specific.
> 
> CPURevision() for x86-family CPUs returns interpreted family/model/stepping
> values as 16/8/8 bits. The extended family ID and extended model ID are taken
> into account.
> 
> CPUX86Signature() returns cpuid 1 eax in its raw form. The family, model,
> stepping, extended family, and extended model numbers are encoded in that, but
> more work needs to be done to make use of them. cpuid 1 eax also carries two
> “processor type” bits and some reserved bits that are not preserved in
> CPURevision(). Things that need the actual value of cpuid 1 eax from an
> x86-family CPU use this instead of CPURevision().
> 
> In a minidump file, MINIDUMP_SYSTEM_INFO::ProcessorLevel (family) and
> MINIDUMP_SYSTEM_INFO::ProcessorRevision (model and stepping) are set using
> CPURevision(). These two fields are valid for all architectures, not just x86.
> MINIDUMP_SYSTEM_INFO::Cpu.X86CpuInfo.VersionInformation is set to
> CPUX86Signature() for 32-bit x86 only (not x86_64). This field isn’t present
for
> other CPUs.
> 
> The comments in our copy of dbghelp.h explain this as far as minidumps are
> concerned, and system_snapshot.h explains it for the snapshot interface,
> although I agree that the difference is subtle.
> 
> This was done with a new method for CPUX86Signature() in case the “processor
> type” or other reserved bits become significant in the future. This design
> ensures that they’re captured, since there’s a spot for all of cpuid 1 eax to
be
> carried without modification, instead of attempting to rebuild cpuid 1 eax
from
> the decoded family/model/stepping in CPURevision().

Thanks for the explanation. Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:95: return
base::saturated_cast<uint8_t>(system_info.dwNumberOfProcessors);
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> Log a warning if this saturates.

Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:107: return vendor;
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> std::string(vendor, 12)
> 
> or make vendor be size 12 and do std::string(vendor, sizeof(vendor))

Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:111: // "Note that this structure definition
was accidentally omitted from WinNT.h."
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> Belongs in compat then?

Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:137: // TODO(scottmg): Return all? Average?
Current?
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> Good question. Max of current and max of max?

Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:138: const uint64_t kMhzToHz = 1000000;
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> 1E6

Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:171: return
(static_cast<uint64_t>(cpu_info[2]) << 32) |
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> You already returned this.

Hm, maybe I should look into enabling the unreachable code warning. Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:244: return "";
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> std::string() (I was reminded of this during the readability review). I don’t
> know if there’s anything sensible you can get on Windows either, since the
> hardware’s a lot more diverse.

Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win.cc:257: TIME_ZONE_INFORMATION
time_zone_information;
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> Comment that this returns the current time zone status in dst_status, and not
> the status at the time of the snapshot. This is unlike the Mac implementation.

Done.

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
File snapshot/win/system_snapshot_win_test.cc (right):

https://codereview.chromium.org/936333004/diff/60001/snapshot/win/system_snap...
snapshot/win/system_snapshot_win_test.cc:72: EXPECT_TRUE(cpu_vendor ==
"GenuineIntel" || cpu_vendor == "AuthenticAMD");
On 2015/02/20 18:58:52, Mark Mentovai wrote:
> This deserves a comment at least acknowledging that there are other CPU
vendors
> out there but you’re intentionally not caring.

It may actually fail on bots if they report "VMwareVMware" (or whatever we use),
but we'll see when it does. Done.

[scottmg, 2015-02-27 21:51:29]

Hi Mark, do you have time to take another look at this patch?

[Mark Mentovai, 2015-02-27 21:59:37]

Major oops on my part, I hadn’t realized that you came back with another round.
I’m looking at this right now.

[Mark Mentovai, 2015-02-27 22:27:43]

Really sorry about the delay, I hope you weren’t blocked waiting on this. A
week’s way longer than reasonable, don’t feel bad about pinging sooner.

https://codereview.chromium.org/936333004/diff/100001/compat/win/processor_po...
File compat/win/processor_power_information.h (right):

https://codereview.chromium.org/936333004/diff/100001/compat/win/processor_po...
compat/win/processor_power_information.h:20: struct PROCESSOR_POWER_INFORMATION
{
I thought that we could just put this into winnt.h and #include ../winnt.h,
since the doc says that the struct will show up in winnt.h in the future.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
File snapshot/win/system_snapshot_win.cc (right):

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:17: #include <immintrin.h>
Just <intrin.h>? I don’t see you using any multimedia intrinsics.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:27: #include
"compat/win/processor_power_information.h"
Things in compat should be treated as system headers and #included <this way>
(which is another reason to use winnt.h).

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:54: if
(!GetVersionEx(reinterpret_cast<OSVERSIONINFO*>(&version_info))) {
The only concern here is that if the manifest doesn’t know about Windows 18 but
some early Windows 18 tester runs this code, it’ll only report itself as what
the manifest says. We’ll be confused by crashes that claim to be from Windows 17
or whatever we’ve declared in the manifest, but that are actually from a newer
OS altogether.

You can leave a TODO here if there’s no good path forward immediately, but I
think it’s ultimately worth figuring out how to read the real version from the
system if GetVersionEx() will start “lying”.

(The version helper wrapper functions you sound seem like non-starters.)

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:70: os_version_full_ =
base::StringPrintf("Windows %d.%d.%d",
I asked if this should be Windows NT or if there was a way to get the system to
give us the string that goes with these numbers, you didn’t directly answer that
question, although the new TODO may be your response to it.

My concern with the label “Windows” is that “Windows 7” = “Windows NT 6.1”, and
this would print 6.1. Although as time goes on I understand Microsoft is
correcting this deficiency and 10 will be 10 once and for all.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:92: uint32_t stepping = raw & 0xf;
If you can stick this into a uint8_t…

Same with 8/16 for the next 4 variables and the adjusted variables.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:100: extended_model = 0;
extended_model is valid when family is 15 or 6.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:101: extended_family = 0;
but this is correct for extended_family.

See Intel 253666 for CPUID under “Input EAX = 1”. In -053, that’s page 3-173 (or
page 239 of the PDF).

This is actually relevant to the computer I’m using right now, which has an
i7-4980HQ whose signature is 0x40661. This should result in 6/70/1.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:151: *current_hz =
std::max_element(info.begin(),
#include <utility>

[scottmg, 2015-02-28 00:33:34]

Patchset #3 (id:120001) has been deleted

[scottmg, 2015-02-28 00:35:56]

Patchset #3 (id:140001) has been deleted

[scottmg, 2015-02-28 00:37:33]

No problem on the delay, I've been busy with other things this week anyway.

https://codereview.chromium.org/936333004/diff/100001/compat/win/processor_po...
File compat/win/processor_power_information.h (right):

https://codereview.chromium.org/936333004/diff/100001/compat/win/processor_po...
compat/win/processor_power_information.h:20: struct PROCESSOR_POWER_INFORMATION
{
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> I thought that we could just put this into winnt.h and #include ../winnt.h,
> since the doc says that the struct will show up in winnt.h in the future.

Yeah, OK. I'm pretty skeptical that it'll ever get added, but I guess it's still
consistent with the docs.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
File snapshot/win/system_snapshot_win.cc (right):

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:17: #include <immintrin.h>
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> Just <intrin.h>? I don’t see you using any multimedia intrinsics.

Done.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:27: #include
"compat/win/processor_power_information.h"
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> Things in compat should be treated as system headers and #included <this way>
> (which is another reason to use winnt.h).

Done.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:54: if
(!GetVersionEx(reinterpret_cast<OSVERSIONINFO*>(&version_info))) {
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> The only concern here is that if the manifest doesn’t know about Windows 18
but
> some early Windows 18 tester runs this code, it’ll only report itself as what
> the manifest says. We’ll be confused by crashes that claim to be from Windows
17
> or whatever we’ve declared in the manifest, but that are actually from a newer
> OS altogether.
> 
> You can leave a TODO here if there’s no good path forward immediately, but I
> think it’s ultimately worth figuring out how to read the real version from the
> system if GetVersionEx() will start “lying”.
> 
> (The version helper wrapper functions you sound seem like non-starters.)

Understood.

I hunted a bit more and it seems the way to do this is to get the version
information from a system dll, which I've switched to doing here. This avoids
the manifest problem.

This also resolves the Windows vs. Windows NT concern below.

Unfortunately, it doesn't include an indication of workstation vs. server, so
that still has to be retrieved separately. I don't believe that will differ
between manifested and unmanifested, though it's possible it could in the
future.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:70: os_version_full_ =
base::StringPrintf("Windows %d.%d.%d",
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> I asked if this should be Windows NT or if there was a way to get the system
to
> give us the string that goes with these numbers, you didn’t directly answer
that
> question, although the new TODO may be your response to it.
> 
> My concern with the label “Windows” is that “Windows 7”&nbsp;= “Windows NT
6.1”, and
> this would print 6.1. Although as time goes on I understand Microsoft is
> correcting this deficiency and 10 will be 10 once and for all.

Switched to using the dwFileOS field.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:92: uint32_t stepping = raw & 0xf;
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> If you can stick this into a uint8_t…
> 
> Same with 8/16 for the next 4 variables and the adjusted variables.

Done.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:100: extended_model = 0;
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> extended_model is valid when family is 15 or 6.

Done.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:101: extended_family = 0;
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> but this is correct for extended_family.
> 
> See Intel 253666 for CPUID under “Input EAX = 1”. In -053, that’s page 3-173
(or
> page 239 of the PDF).
> 
> This is actually relevant to the computer I’m using right now, which has an
> i7-4980HQ whose signature is 0x40661. This should result in 6/70/1.

Oops, that'll teach me to use Wikipedia!
http://en.wikipedia.org/wiki/CPUID#EAX.3D1:_Processor_Info_and_Feature_Bits

Done.

https://codereview.chromium.org/936333004/diff/100001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:151: *current_hz =
std::max_element(info.begin(),
On 2015/02/27 22:27:42, Mark Mentovai wrote:
> #include <utility>

Done.

[Mark Mentovai, 2015-03-02 20:15:17]

LGTM

[scottmg, 2015-03-02 21:06:36]

Committed patchset #3 (id:160001) manually as
bcc580e561fe89fbf078a83b3ea96cb8b3091865 (presubmit successful).