win: Plumb module PDB name through snapshot

win: Plumb module PDB name through snapshot

R=mark@chromium.org
BUG=chromium:546288
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/3ee9d891d9e2985b56be1d9b3c389b360a3cb96d

[scottmg, 2015-10-28 19:15:04]

Description was changed from

==========
win: Plumb module PDB name in snapshot

R=mark@chromium.org
BUG=chromium:546288
==========

to

==========
win: Plumb module PDB name through snapshot

R=mark@chromium.org
BUG=chromium:546288
==========

[Mark Mentovai, 2015-10-28 20:01:11]

https://codereview.chromium.org/1415543003/diff/1/snapshot/mac/module_snapsho...
File snapshot/mac/module_snapshot_mac.cc (right):

https://codereview.chromium.org/1415543003/diff/1/snapshot/mac/module_snapsho...
snapshot/mac/module_snapshot_mac.cc:161: std::string name = Name();
Now that we’re in platform-specific code where we know that FilePath will wrap a
string of the right width, this can become a FilePath::BaseName() operation.

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h
File snapshot/module_snapshot.h (right):

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:120: //! `0`.
//! \sa DebugFileName()

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:126: //! module itself. On other platforms, this
should just be the basename of the
“should” → “is normally”

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:127: //! module.
“, because the debug info file’s name is not relevant even in split-debug
scenarios.”

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:127: //! module.
//! \sa UUIDAndAge()

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:128: virtual std::string PDBName() const = 0;
Let’s try to keep things more generic in snapshot, like DebugFileName(). Applies
to the \brief on line 123 too.

https://codereview.chromium.org/1415543003/diff/1/snapshot/win/module_snapsho...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1415543003/diff/1/snapshot/win/module_snapsho...
snapshot/win/module_snapshot_win.cc:64: size_t last_slash =
full_pdb_name.find_last_of("/\\");
I guess this can’t be a BaseName() because this is a rare instance where
Microsoft doesn’t use wchar_t* for a path string.

Consider whether we actually need to do the BaseName() at all. I’d match what
MiniDumpWriteDump() does.

https://codereview.chromium.org/1415543003/diff/1/snapshot/win/module_snapsho...
File snapshot/win/module_snapshot_win.h (right):

https://codereview.chromium.org/1415543003/diff/1/snapshot/win/module_snapsho...
snapshot/win/module_snapshot_win.h:96: UUID uuid_;
Organize for better packing.

[scottmg, 2015-10-28 20:59:25]

Thanks

https://codereview.chromium.org/1415543003/diff/1/snapshot/mac/module_snapsho...
File snapshot/mac/module_snapshot_mac.cc (right):

https://codereview.chromium.org/1415543003/diff/1/snapshot/mac/module_snapsho...
snapshot/mac/module_snapshot_mac.cc:161: std::string name = Name();
On 2015/10/28 20:01:10, Mark Mentovai wrote:
> Now that we’re in platform-specific code where we know that FilePath will wrap
a
> string of the right width, this can become a FilePath::BaseName() operation.

Done.

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h
File snapshot/module_snapshot.h (right):

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:120: //! `0`.
On 2015/10/28 20:01:10, Mark Mentovai wrote:
> //! \sa DebugFileName()

Done.

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:126: //! module itself. On other platforms, this
should just be the basename of the
On 2015/10/28 20:01:10, Mark Mentovai wrote:
> “should” → “is normally”

Done.

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:127: //! module.
On 2015/10/28 20:01:10, Mark Mentovai wrote:
> “, because the debug info file’s name is not relevant even in split-debug
> scenarios.”

Done.

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:127: //! module.
On 2015/10/28 20:01:10, Mark Mentovai wrote:
> “, because the debug info file’s name is not relevant even in split-debug
> scenarios.”

Done.

https://codereview.chromium.org/1415543003/diff/1/snapshot/module_snapshot.h#...
snapshot/module_snapshot.h:128: virtual std::string PDBName() const = 0;
On 2015/10/28 20:01:10, Mark Mentovai wrote:
> Let’s try to keep things more generic in snapshot, like DebugFileName().
Applies
> to the \brief on line 123 too.

Done.

https://codereview.chromium.org/1415543003/diff/1/snapshot/win/module_snapsho...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1415543003/diff/1/snapshot/win/module_snapsho...
snapshot/win/module_snapshot_win.cc:64: size_t last_slash =
full_pdb_name.find_last_of("/\\");
On 2015/10/28 20:01:11, Mark Mentovai wrote:
> I guess this can’t be a BaseName() because this is a rare instance where
> Microsoft doesn’t use wchar_t* for a path string.
> 
> Consider whether we actually need to do the BaseName() at all. I’d match what
> MiniDumpWriteDump() does.

Right on both counts. A quick check against MiniDumpWriteDump() shows it just
uses the name from the RSDS directly.

https://codereview.chromium.org/1415543003/diff/1/snapshot/win/module_snapsho...
File snapshot/win/module_snapshot_win.h (right):

https://codereview.chromium.org/1415543003/diff/1/snapshot/win/module_snapsho...
snapshot/win/module_snapshot_win.h:96: UUID uuid_;
On 2015/10/28 20:01:11, Mark Mentovai wrote:
> Organize for better packing.

Done. (Maybe?)

[Mark Mentovai, 2015-10-29 13:11:11]

LGTM

https://codereview.chromium.org/1415543003/diff/80001/minidump/minidump_modul...
File minidump/minidump_module_writer_test.cc (right):

https://codereview.chromium.org/1415543003/diff/80001/minidump/minidump_modul...
minidump/minidump_module_writer_test.cc:706: module_pdbs[2] = "dyld";
Make one of these more  of a full pathname like we’ll wind up with sometimes on
Windows, to ensure that the minidump writer isn’t doing the basename-ing itself.

https://codereview.chromium.org/1415543003/diff/80001/snapshot/win/module_sna...
File snapshot/win/module_snapshot_win.h (right):

https://codereview.chromium.org/1415543003/diff/80001/snapshot/win/module_sna...
snapshot/win/module_snapshot_win.h:97: scoped_ptr<PEImageReader>
pe_image_reader_;
Best would be name, pdb_name, uuid, pe_image_reader, process_reader, timestamp,
age, initialized.

pe_image_reader, process_reader, and timestamp are the things that might be
either 4- or 8-byte quantities.

Might save a few bytes in a big vector of these.

[scottmg, 2015-10-29 17:47:00]

https://codereview.chromium.org/1415543003/diff/80001/minidump/minidump_modul...
File minidump/minidump_module_writer_test.cc (right):

https://codereview.chromium.org/1415543003/diff/80001/minidump/minidump_modul...
minidump/minidump_module_writer_test.cc:706: module_pdbs[2] = "dyld";
On 2015/10/29 13:11:10, Mark Mentovai wrote:
> Make one of these more  of a full pathname like we’ll wind up with sometimes
on
> Windows, to ensure that the minidump writer isn’t doing the basename-ing
itself.

Done.

https://codereview.chromium.org/1415543003/diff/80001/snapshot/win/module_sna...
File snapshot/win/module_snapshot_win.h (right):

https://codereview.chromium.org/1415543003/diff/80001/snapshot/win/module_sna...
snapshot/win/module_snapshot_win.h:97: scoped_ptr<PEImageReader>
pe_image_reader_;
On 2015/10/29 13:11:10, Mark Mentovai wrote:
> Best would be name, pdb_name, uuid, pe_image_reader, process_reader,
timestamp,
> age, initialized.
> 
> pe_image_reader, process_reader, and timestamp are the things that might be
> either 4- or 8-byte quantities.
> 
> Might save a few bytes in a big vector of these.

Done.

[scottmg, 2015-10-29 17:48:29]

Committed patchset #6 (id:100001) manually as
3ee9d891d9e2985b56be1d9b3c389b360a3cb96d (presubmit successful).