win: Retrieve module version/type information

win: Retrieve module version/type information

Refactor version retrieval from system snapshot to use when
retrieving the module version information.

Follows https://codereview.chromium.org/1133203002/.

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

[scottmg, 2015-05-12 20:33:20]

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

[Robert Sesek, 2015-05-12 22:44:57]

https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/module_sna...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/module_sna...
snapshot/win/module_snapshot_win.cc:150: // TODO(scottmg): Not sure if there's
anything equivalent on Windows.
This would be the UUID for the PDB.

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

https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:103: os_version_major_ =
(ffi.dwFileVersionMS & 0xffff0000) >> 16;
This is the third time this code occurs. Perhaps add a helper to extract the
four uint16_t's from a VS_FIXEDFILEINFO ?

https://codereview.chromium.org/1126273003/diff/40001/util/win/module_version.cc
File util/win/module_version.cc (right):

https://codereview.chromium.org/1126273003/diff/40001/util/win/module_version...
util/win/module_version.cc:30: << base::UTF16ToUTF8(path.value());
Aren't there new wstring operators for the log macros? Is this conversion
necessary?

[scottmg, 2015-05-12 23:28:38]

Thanks

https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/module_sna...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/module_sna...
snapshot/win/module_snapshot_win.cc:150: // TODO(scottmg): Not sure if there's
anything equivalent on Windows.
On 2015/05/12 22:44:57, Robert Sesek wrote:
> This would be the UUID for the PDB.

Ah, so it is, I should have looked at the writer.

That requires some more digging around in the PE header, so I reverted this bit
back to a CHECK and I'll fill out UUID() in a future CL.

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

https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:103: os_version_major_ =
(ffi.dwFileVersionMS & 0xffff0000) >> 16;
On 2015/05/12 22:44:57, Robert Sesek wrote:
> This is the third time this code occurs. Perhaps add a helper to extract the
> four uint16_t's from a VS_FIXEDFILEINFO ?

It's a bit awkward because there's both dwFileVersion[LM]S and
dwProductVersion[LM]S, and by the time you pass in the top/bottom and 4 output
parameters, it feels just as long.

I removed the unnecessary masking, which makes it a little less cluttery.

https://codereview.chromium.org/1126273003/diff/40001/util/win/module_version.cc
File util/win/module_version.cc (right):

https://codereview.chromium.org/1126273003/diff/40001/util/win/module_version...
util/win/module_version.cc:30: << base::UTF16ToUTF8(path.value());
On 2015/05/12 22:44:57, Robert Sesek wrote:
> Aren't there new wstring operators for the log macros? Is this conversion
> necessary?

No, unfortunately there isn't. I had a CL to pull in Chrome's hack (adding an
operator<<(wstring) to std::), but we decided that it was better to avoid that
and just convert them when necessary.

[scottmg, 2015-05-12 23:45:59]

Patchset #5 (id:80001) has been deleted

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

lgtm

https://codereview.chromium.org/1126273003/diff/60001/util/win/module_version.cc
File util/win/module_version.cc (right):

https://codereview.chromium.org/1126273003/diff/60001/util/win/module_version...
util/win/module_version.cc:24: 
fyi you can have modules without fixedfileinfo, I guess they should not be using
crashpad.

https://codereview.chromium.org/1126273003/diff/60001/util/win/module_version...
util/win/module_version.cc:34: PLOG(WARNING) << "GetFileVersionInfo: "
fwiw, this is slooow. Specially for a big binary.

[scottmg, 2015-05-13 02:12:41]

Thanks

https://codereview.chromium.org/1126273003/diff/60001/util/win/module_version.cc
File util/win/module_version.cc (right):

https://codereview.chromium.org/1126273003/diff/60001/util/win/module_version...
util/win/module_version.cc:24: 
On 2015/05/13 01:34:12, cpu wrote:
> fyi you can have modules without fixedfileinfo, I guess they should not be
using
> crashpad.

That fails at GetFileVersionInfo() and then will end up storing a version of
"0.0.0.0" later. It does log spam for those modules, but maybe that's ok.

https://codereview.chromium.org/1126273003/diff/60001/util/win/module_version...
util/win/module_version.cc:34: PLOG(WARNING) << "GetFileVersionInfo: "
On 2015/05/13 01:34:12, cpu wrote:
> fwiw, this is slooow. Specially for a big binary.

I was wondering about that. I was vaguely hoping it wouldn't be too bad since
it'll be in memory, but I didn't check. Do you think we should cache? Or maybe
find the resource ourselves? Or just leave it?

[Robert Sesek, 2015-05-13 19:06:00]

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

https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
snapshot/win/system_snapshot_win.cc:103: os_version_major_ =
(ffi.dwFileVersionMS & 0xffff0000) >> 16;
On 2015/05/12 23:28:38, scottmg wrote:
> On 2015/05/12 22:44:57, Robert Sesek wrote:
> > This is the third time this code occurs. Perhaps add a helper to extract the
> > four uint16_t's from a VS_FIXEDFILEINFO ?
> 
> It's a bit awkward because there's both dwFileVersion[LM]S and
> dwProductVersion[LM]S, and by the time you pass in the top/bottom and 4 output
> parameters, it feels just as long.
> 
> I removed the unnecessary masking, which makes it a little less cluttery.

Why can't you pass in the entire VS_FIXEDFILEINFO? Something like:

void GetFileInfoVersionComponents(const VS_FIXEDFILEINFO&, uint16_t* major,
uint16_t* minor, uint16_t* bugfix, uint16_t* build)

?

[Robert Sesek, 2015-05-13 19:06:15]

On 2015/05/13 19:06:00, Robert Sesek wrote:
>
https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
> File snapshot/win/system_snapshot_win.cc (right):
> 
>
https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
> snapshot/win/system_snapshot_win.cc:103: os_version_major_ =
> (ffi.dwFileVersionMS & 0xffff0000) >> 16;
> On 2015/05/12 23:28:38, scottmg wrote:
> > On 2015/05/12 22:44:57, Robert Sesek wrote:
> > > This is the third time this code occurs. Perhaps add a helper to extract
the
> > > four uint16_t's from a VS_FIXEDFILEINFO ?
> > 
> > It's a bit awkward because there's both dwFileVersion[LM]S and
> > dwProductVersion[LM]S, and by the time you pass in the top/bottom and 4
output
> > parameters, it feels just as long.
> > 
> > I removed the unnecessary masking, which makes it a little less cluttery.
> 
> Why can't you pass in the entire VS_FIXEDFILEINFO? Something like:
> 
> void GetFileInfoVersionComponents(const VS_FIXEDFILEINFO&, uint16_t* major,
> uint16_t* minor, uint16_t* bugfix, uint16_t* build)
> 
> ?

… which is also then testable.

[scottmg, 2015-05-13 19:28:07]

On 2015/05/13 19:06:00, Robert Sesek wrote:
>
https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
> File snapshot/win/system_snapshot_win.cc (right):
> 
>
https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
> snapshot/win/system_snapshot_win.cc:103: os_version_major_ =
> (ffi.dwFileVersionMS & 0xffff0000) >> 16;
> On 2015/05/12 23:28:38, scottmg wrote:
> > On 2015/05/12 22:44:57, Robert Sesek wrote:
> > > This is the third time this code occurs. Perhaps add a helper to extract
the
> > > four uint16_t's from a VS_FIXEDFILEINFO ?
> > 
> > It's a bit awkward because there's both dwFileVersion[LM]S and
> > dwProductVersion[LM]S, and by the time you pass in the top/bottom and 4
output
> > parameters, it feels just as long.
> > 
> > I removed the unnecessary masking, which makes it a little less cluttery.
> 
> Why can't you pass in the entire VS_FIXEDFILEINFO? Something like:
> 
> void GetFileInfoVersionComponents(const VS_FIXEDFILEINFO&, uint16_t* major,
> uint16_t* minor, uint16_t* bugfix, uint16_t* build)
> 
> ?

You mean two separate functions then, one for dwFileVersion and
dwProductVersion?

[Robert Sesek, 2015-05-13 19:29:16]

On 2015/05/13 19:28:07, scottmg wrote:
> On 2015/05/13 19:06:00, Robert Sesek wrote:
> >
>
https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
> > File snapshot/win/system_snapshot_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
> > snapshot/win/system_snapshot_win.cc:103: os_version_major_ =
> > (ffi.dwFileVersionMS & 0xffff0000) >> 16;
> > On 2015/05/12 23:28:38, scottmg wrote:
> > > On 2015/05/12 22:44:57, Robert Sesek wrote:
> > > > This is the third time this code occurs. Perhaps add a helper to extract
> the
> > > > four uint16_t's from a VS_FIXEDFILEINFO ?
> > > 
> > > It's a bit awkward because there's both dwFileVersion[LM]S and
> > > dwProductVersion[LM]S, and by the time you pass in the top/bottom and 4
> output
> > > parameters, it feels just as long.
> > > 
> > > I removed the unnecessary masking, which makes it a little less cluttery.
> > 
> > Why can't you pass in the entire VS_FIXEDFILEINFO? Something like:
> > 
> > void GetFileInfoVersionComponents(const VS_FIXEDFILEINFO&, uint16_t* major,
> > uint16_t* minor, uint16_t* bugfix, uint16_t* build)
> > 
> > ?
> 
> You mean two separate functions then, one for dwFileVersion and
> dwProductVersion?

Yeah, or at least just dwFileVersion. It may not be worth it, though, so
consider that optional. LGTM

[scottmg, 2015-05-13 19:51:58]

On 2015/05/13 19:29:16, Robert Sesek wrote:
> On 2015/05/13 19:28:07, scottmg wrote:
> > On 2015/05/13 19:06:00, Robert Sesek wrote:
> > >
> >
>
https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
> > > File snapshot/win/system_snapshot_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1126273003/diff/40001/snapshot/win/system_sna...
> > > snapshot/win/system_snapshot_win.cc:103: os_version_major_ =
> > > (ffi.dwFileVersionMS & 0xffff0000) >> 16;
> > > On 2015/05/12 23:28:38, scottmg wrote:
> > > > On 2015/05/12 22:44:57, Robert Sesek wrote:
> > > > > This is the third time this code occurs. Perhaps add a helper to
extract
> > the
> > > > > four uint16_t's from a VS_FIXEDFILEINFO ?
> > > > 
> > > > It's a bit awkward because there's both dwFileVersion[LM]S and
> > > > dwProductVersion[LM]S, and by the time you pass in the top/bottom and 4
> > output
> > > > parameters, it feels just as long.
> > > > 
> > > > I removed the unnecessary masking, which makes it a little less
cluttery.
> > > 
> > > Why can't you pass in the entire VS_FIXEDFILEINFO? Something like:
> > > 
> > > void GetFileInfoVersionComponents(const VS_FIXEDFILEINFO&, uint16_t*
major,
> > > uint16_t* minor, uint16_t* bugfix, uint16_t* build)
> > > 
> > > ?
> > 
> > You mean two separate functions then, one for dwFileVersion and
> > dwProductVersion?
> 
> Yeah, or at least just dwFileVersion. It may not be worth it, though, so
> consider that optional. LGTM

Bleh. I did this, but then the interfaces for ModuleSnapshot and SystemSnapshot
differ (one is uint16_t the other is int) so then there's a bunch of converting
and extra fiddling that makes it more cumbersome. So I think I'll just land as
is.

[scottmg, 2015-05-15 00:43:56]

Committed patchset #5 (id:100001) manually as
b0889f61eefa6f89dd823df0b8f4e17aa8ed6ba4 (presubmit successful).