win: make CrashpadInfo retrievable

win: make CrashpadInfo retrievable

The main goal was to get the beginnings of module iteration and retrieval
of CrashpadInfo in snapshot. The main change for that is to move
crashpad_info_client_options[_test] down out of mac/.

This also requires adding some of the supporting code of snapshot in
ProcessReaderWin, ProcessSnapshotWin, and ModuleSnapshotWin. These are
partially copied from Mac or stubbed out with lots of TODO annotations.
This is a bit unfortunate, but seemed like the most productive way to
make progress incrementally. That is, it's mostly placeholder at the
moment, but hopefully has the right shape for things to come.

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

[scottmg, 2015-04-24 19:46:10]

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

[scottmg, 2015-04-24 19:53:55]

Patchset #16 (id:300001) has been deleted

[Mark Mentovai, 2015-04-29 00:41:50]

I made it through everything but the new Windows code before I had to leave
early today. Here’s the minor nothing feedback I had so far, I’ll look at the
more interesting new code tomorrow.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/crashpad_info...
File snapshot/crashpad_info_client_options_test.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/crashpad_info...
snapshot/crashpad_info_client_options_test.cc:113: using DlHandle = void*;
These should line up with the left margin. I think clang-format messes this up.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/snapshot_test...
File snapshot/snapshot_test.gyp (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/snapshot_test...
snapshot/snapshot_test.gyp:95:
'../third_party/mini_chromium/mini_chromium.gyp:base',
Weird indentation.

[Mark Mentovai, 2015-04-29 19:37:16]

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:47: pe_image_reader_->Initialize(
Check return value? The callee can’t fail as currently written, but as long as
it’s got a bool return value, it might change to  be able to fail in the future.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:60:
options->system_crash_reporter_forwarding = TriState::kUnset;
Does this make sense on Windows at all? I wasn’t sure, but if it doesn’t, we
should #if everything related to it out.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:80: CHECK(false) << "TODO(scottmg)";
Makes it easier to review, too. :)

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:50: if (section.Misc.VirtualSize <
sizeof(process_types::CrashpadInfo)) {
The documentation says “This field is valid only for executable images and
should be set to 0 for object files.” In this instance, executable images means
modules loaded into a process and thus includes DLLs, right? This is just a
sanity check, and there’s no need to rewrite this or add a comment as long as
the answer is “yes.”

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:51: LOG(WARNING) << "small crashpad info section
size "
"base/logging.h"

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:56: uintptr_t crashpad_info_address = address_ +
section.VirtualAddress;
PE modules load completely contiguously in memory on Windows, right? You should
check that the address and end are within the range of the module. See
CheckedMachAddressRange from util/mac/checked_mach_address_range.h. (We don’t
use it in exactly this way in the Mach-O reader on Mac because Mach-O modules
can load discontiguously, but we use it for similar range checks.)

If you can know the size of the image for GetSectionByName, it’d be good to
check the reads in there to ensure that they’re in-range as well.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:61: LOG(WARNING) << "could not read crashpad
info" << module_name_;
Space before module_name. Same on line 67.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:79: LOG(WARNING) << "could not read dos header";
Might want to send module_name_ to the log stream on the messages in this
method.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:85: uintptr_t nt_headers_address = address_ +
dos_header.e_lfanew;
Validate dos_header.e_magic before proceeding. And after reading this, validate
nt_headers.Signature.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:93: nt_headers_address +
FIELD_OFFSET(IMAGE_NT_HEADERS, OptionalHeader) +
If this is the same as offsetof, that’s more standard (although maybe not in
Windows code?)

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:103: if (strncmp(reinterpret_cast<const
char*>(section->Name),
<string.h>

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:104: name.c_str(),
The problem with using strncmp in this way: it also matches a prefix of a
too-long search string. Ideally this whole method would fail if name.size() >=
sizeof(section->Name) instead of trying for a partial match.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:105: IMAGE_SIZEOF_SHORT_NAME) == 0) {
sizeof(section->Name) is more descriptive.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:23: #include "base/memory/scoped_ptr.h"
Unused

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:25: #include "util/misc/uuid.h"
Unused (for now)

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:26: #include "util/stdlib/pointer_container.h"
Unused

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:72: //! \return `true` if the image was read
successfully, including all load
Load commands are a Mach-O thing. You can say what makes sense for PE or omit it
if it’s mostly obvious.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:76: const std::string& name);
The member variable is module_name_ and it’s totally the same as this, so rename
this to match so that it’s easier to see where it winds up.

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

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:49: LOG(ERROR) << "ReadMemory at " << at << "
of " << num_bytes << " failed";
PLOG

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:137: CHECK(false) << "TODO(scottmg)";
It looks like this will actually work. Remove the CHECK if you agree, or leave
it if you’re unsure.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.h (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.h:107: void InitializeThreads();
Or comment out for now with another TODO(scottmg).

https://codereview.chromium.org/1052813002/diff/360001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/util/win/process_info....
util/win/process_info.cc:190: ProcessInfo::Module::Module() : name(),
timestamp(0) {
dll_base(0)

https://codereview.chromium.org/1052813002/diff/360001/util/win/process_info_...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/util/win/process_info_...
util/win/process_info_test.cc:62: modules[1].name.substr(modules[1].name.size()
- wcslen(kNtdllName)));
Verify the sanity of the two other fields in ProcessInfo::Module too.

[scottmg, 2015-04-30 03:31:32]

Thanks Mark.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/crashpad_info...
File snapshot/crashpad_info_client_options_test.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/crashpad_info...
snapshot/crashpad_info_client_options_test.cc:113: using DlHandle = void*;
On 2015/04/29 00:41:50, Mark Mentovai wrote:
> These should line up with the left margin. I think clang-format messes this
up.

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/snapshot_test...
File snapshot/snapshot_test.gyp (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/snapshot_test...
snapshot/snapshot_test.gyp:95:
'../third_party/mini_chromium/mini_chromium.gyp:base',
On 2015/04/29 00:41:50, Mark Mentovai wrote:
> Weird indentation.

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:47: pe_image_reader_->Initialize(
On 2015/04/29 19:37:14, Mark Mentovai wrote:
> Check return value? The callee can’t fail as currently written, but as long as
> it’s got a bool return value, it might change to  be able to fail in the
future.

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:60:
options->system_crash_reporter_forwarding = TriState::kUnset;
On 2015/04/29 19:37:14, Mark Mentovai wrote:
> Does this make sense on Windows at all? I wasn’t sure, but if it doesn’t, we
> should #if everything related to it out.

I'm not sure if it's a good idea (I haven't actually even looked at if we can
implement the current behaviour exactly) but I was thinking we might want to
pass data to
https://msdn.microsoft.com/en-us/library/windows/desktop/bb513635(v=vs.85).aspx
.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:80: CHECK(false) << "TODO(scottmg)";
On 2015/04/29 19:37:14, Mark Mentovai wrote:
> Makes it easier to review, too. :)

Hopefully future reviews will be a bit "harder". :)

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:50: if (section.Misc.VirtualSize <
sizeof(process_types::CrashpadInfo)) {
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> The documentation says “This field is valid only for executable images and
> should be set to 0 for object files.” In this instance, executable images
means
> modules loaded into a process and thus includes DLLs, right? This is just a
> sanity check, and there’s no need to rewrite this or add a comment as long as
> the answer is “yes.”

That's right. The same structures are used for .obj and for PEs, and "executable
images" means all of EXE, DLL, CPL, OCX, etc.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:51: LOG(WARNING) << "small crashpad info section
size "
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> "base/logging.h"

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:56: uintptr_t crashpad_info_address = address_ +
section.VirtualAddress;
On 2015/04/29 19:37:14, Mark Mentovai wrote:
> PE modules load completely contiguously in memory on Windows, right? You
should
> check that the address and end are within the range of the module. See
> CheckedMachAddressRange from util/mac/checked_mach_address_range.h. (We don’t
> use it in exactly this way in the Mach-O reader on Mac because Mach-O modules
> can load discontiguously, but we use it for similar range checks.)
> 
> If you can know the size of the image for GetSectionByName, it’d be good to
> check the reads in there to ensure that they’re in-range as well.

Done. (I moved most of util/mac/checked_mach_address_range to util/numeric/ in
doing so.)

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:61: LOG(WARNING) << "could not read crashpad
info" << module_name_;
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> Space before module_name. Same on line 67.

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:79: LOG(WARNING) << "could not read dos header";
On 2015/04/29 19:37:14, Mark Mentovai wrote:
> Might want to send module_name_ to the log stream on the messages in this
> method.

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:85: uintptr_t nt_headers_address = address_ +
dos_header.e_lfanew;
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> Validate dos_header.e_magic before proceeding. And after reading this,
validate
> nt_headers.Signature.

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:93: nt_headers_address +
FIELD_OFFSET(IMAGE_NT_HEADERS, OptionalHeader) +
On 2015/04/29 19:37:14, Mark Mentovai wrote:
> If this is the same as offsetof, that’s more standard (although maybe not in
> Windows code?)

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:103: if (strncmp(reinterpret_cast<const
char*>(section->Name),
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> <string.h>

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:104: name.c_str(),
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> The problem with using strncmp in this way: it also matches a prefix of a
> too-long search string. Ideally this whole method would fail if name.size() >=
> sizeof(section->Name) instead of trying for a partial match.

Good point, validated `name` at function entry. > rather than >= as it's nul
padded, but not necessarily nul terminated.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:105: IMAGE_SIZEOF_SHORT_NAME) == 0) {
On 2015/04/29 19:37:14, Mark Mentovai wrote:
> sizeof(section->Name) is more descriptive.

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:23: #include "base/memory/scoped_ptr.h"
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> Unused

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:25: #include "util/misc/uuid.h"
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> Unused (for now)

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:26: #include "util/stdlib/pointer_container.h"
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> Unused

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:72: //! \return `true` if the image was read
successfully, including all load
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> Load commands are a Mach-O thing. You can say what makes sense for PE or omit
it
> if it’s mostly obvious.

Oops, missed that one. Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:76: const std::string& name);
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> The member variable is module_name_ and it’s totally the same as this, so
rename
> this to match so that it’s easier to see where it winds up.

Done.

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

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_r...
snapshot/win/process_reader_win.cc:49: LOG(ERROR) << "ReadMemory at " << at << "
of " << num_bytes << " failed";
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> PLOG

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.cc:137: CHECK(false) << "TODO(scottmg)";
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> It looks like this will actually work. Remove the CHECK if you agree, or leave
> it if you’re unsure.

Done.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_s...
File snapshot/win/process_snapshot_win.h (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/process_s...
snapshot/win/process_snapshot_win.h:107: void InitializeThreads();
On 2015/04/29 19:37:15, Mark Mentovai wrote:
> Or comment out for now with another TODO(scottmg).

Done.

https://codereview.chromium.org/1052813002/diff/360001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/util/win/process_info....
util/win/process_info.cc:190: ProcessInfo::Module::Module() : name(),
timestamp(0) {
On 2015/04/29 19:37:16, Mark Mentovai wrote:
> dll_base(0)

Done.

https://codereview.chromium.org/1052813002/diff/360001/util/win/process_info_...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/util/win/process_info_...
util/win/process_info_test.cc:62: modules[1].name.substr(modules[1].name.size()
- wcslen(kNtdllName)));
On 2015/04/29 19:37:16, Mark Mentovai wrote:
> Verify the sanity of the two other fields in ProcessInfo::Module too.

Done.

[Mark Mentovai, 2015-04-30 20:58:36]

I like this! This is some real serious stuff we’re getting into now.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:60:
options->system_crash_reporter_forwarding = TriState::kUnset;
On 2015/04/30 03:31:31, scottmg wrote:
> On 2015/04/29 19:37:14, Mark Mentovai wrote:
> > Does this make sense on Windows at all? I wasn’t sure, but if it doesn’t, we
> > should #if everything related to it out.
> 
> I'm not sure if it's a good idea (I haven't actually even looked at if we can
> implement the current behaviour exactly) but I was thinking we might want to
> pass data to
>
https://msdn.microsoft.com/en-us/library/windows/desktop/bb513635(v=vs.85).aspx
> .

That’s reasonable. If we find that this doesn’t make sense on Windows at all
once we have more up and running, we can take it out later.

https://codereview.chromium.org/1052813002/diff/360001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:80: CHECK(false) << "TODO(scottmg)";
scottmg wrote:
> On 2015/04/29 19:37:14, Mark Mentovai wrote:
> > Makes it easier to review, too. :)
> 
> Hopefully future reviews will be a bit "harder". :)

:)

https://codereview.chromium.org/1052813002/diff/480001/snapshot/crashpad_info...
File snapshot/crashpad_info_client_options_test_module.cc (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/crashpad_info...
snapshot/crashpad_info_client_options_test_module.cc:44: BOOL WINAPI
DllMain(HINSTANCE hinstance, DWORD reason, LPVOID reserved) {
Or link with /NOENTRY, right? (You can leave DllMain here, this question is just
for my own curiosity.)

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:51: LOG(WARNING) << "pe reader
initialization failed";
PEImageReader is documented as logging something if Initialize() fails, so this
isn’t strictly necessary unless you think there will be lots of entry points to
PEImageReader::Initialize and it’ll be important to pinpoint this one.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:85: CHECK(false) << "TODO(scottmg)";
Like Timestamp(), you can implement Address() and Size() easily with what you
have now. The PEImageReader already stores both of them.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:28: : process_reader_(nullptr), address_(0),
module_name_(), initialized_() {
size, module_range

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:43:
module_range_.SetRange(process_reader_->Is64Bit(), address_, size_);
Check the validity of module_range_ once you set it. It won’t be valid if
address_ is nonzero and size_ is its maximum, for example.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:66:
CheckedAddressRange(process_reader_->Is64Bit(),
I think that you need to check the new CheckedAddressRange() for validity before
you try using it. Otherwise, I don’t think that just calling ContainsRange()
will make sure that it’s valid. Same on line 148.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:68: sizeof(process_types::CrashpadInfo)))) {
This should be section.Misc.VirtualSize, since you already know that this is >=
sizeof(process_types::CrashpadInfo).

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:69: LOG(WARNING) << "invalid address for
crashpad info "
This message is a little misleading, because it checks more than just the
address of the CrashpadInfo, it checks its entire range.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:75: uintptr_t size,
I don’t love uintptr_t for sizes. size_t is better.

Not that we’re sure we’ll need 32-reads-64 (you actually mentioned that you’re
more sure we won’t), but if you want to preserve the ability to do 32-reads-64,
these shouldn’t be uintptr_t or size_t. They should be uint64_t or new typedefs
equivalent to uint64_t. On the other hand, while most of this class does seem to
accept 32-reads-64 (once the TODOs are resolved), if you’re still very against
it, this isn’t necessary. The typedefs still might be nice for documentary
value, though. On Mac, we’re able to use the system-supplied mach_vm_address_t
and mach_vm_size_t.

I think that these two things bubble through this file and even to other files
(checked_address_range) a little bit, but I’m only mentioning them here.

https://codereview.chromium.org/1052813002/diff/480001/util/mac/checked_mach_...
File util/mac/checked_mach_address_range.h (right):

https://codereview.chromium.org/1052813002/diff/480001/util/mac/checked_mach_...
util/mac/checked_mach_address_range.h:33: using CheckedMachAddressRange =
Cool!

https://codereview.chromium.org/1052813002/diff/480001/util/numeric/checked_a...
File util/numeric/checked_address_range.h (right):

https://codereview.chromium.org/1052813002/diff/480001/util/numeric/checked_a...
util/numeric/checked_address_range.h:203: using CheckedAddressRange =
CheckedAddressRangeGeneric<uintptr_t, uintptr_t>;
I think that inlining this results in lots of bloat at each call site. We can
cut down on template inlining by retaining the .h/.cc structure that this class
had when it was just checked_mach_address_range, and doing explicit template
instantiation of the forms used for CheckedAddressRange and
CheckedMachAddressRange in the .cc file where all of the implementations are
available (see minidump/minidump_string_writer.cc).

https://codereview.chromium.org/1052813002/diff/480001/util/win/process_info_...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1052813002/diff/480001/util/win/process_info_...
util/win/process_info_test.cc:35: wchar_t filename[_MAX_PATH];
I wasn’t familiar with _MAX_PATH with an underscore so I went looking for it. I
found out that it’s a CRT thing. Since you use this with a win32 call, it should
be MAX_PATH. (They’re both 260, though.)

https://codereview.chromium.org/1052813002/diff/480001/util/win/process_info_...
util/win/process_info_test.cc:36: if (!GetModuleFileName(module, filename,
sizeof(filename)))
arraysize, not sizeof?

https://codereview.chromium.org/1052813002/diff/480001/util/win/process_info_...
util/win/process_info_test.cc:41: return 0;
EXPECT_EQ(0, rv) also to make sure that gtest sees this problem even if it
thinks it’s looking for a 0 timestamp for some reason.

[scottmg, 2015-04-30 22:09:44]

Thanks

https://codereview.chromium.org/1052813002/diff/480001/snapshot/crashpad_info...
File snapshot/crashpad_info_client_options_test_module.cc (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/crashpad_info...
snapshot/crashpad_info_client_options_test_module.cc:44: BOOL WINAPI
DllMain(HINSTANCE hinstance, DWORD reason, LPVOID reserved) {
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> Or link with /NOENTRY, right? (You can leave DllMain here, this question is
just
> for my own curiosity.)

Yes, that would be fine too. (I generally lean towards putting as much as
possible in the code to avoid too many required magic build flags, but in this
case, maybe less clutter would be better.)

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:51: LOG(WARNING) << "pe reader
initialization failed";
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> PEImageReader is documented as logging something if Initialize() fails, so
this
> isn’t strictly necessary unless you think there will be lots of entry points
to
> PEImageReader::Initialize and it’ll be important to pinpoint this one.

Done.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:85: CHECK(false) << "TODO(scottmg)";
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> Like Timestamp(), you can implement Address() and Size() easily with what you
> have now. The PEImageReader already stores both of them.

Done.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:28: : process_reader_(nullptr), address_(0),
module_name_(), initialized_() {
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> size, module_range

Done.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:43:
module_range_.SetRange(process_reader_->Is64Bit(), address_, size_);
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> Check the validity of module_range_ once you set it. It won’t be valid if
> address_ is nonzero and size_ is its maximum, for example.

Done.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:66:
CheckedAddressRange(process_reader_->Is64Bit(),
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> I think that you need to check the new CheckedAddressRange() for validity
before
> you try using it. Otherwise, I don’t think that just calling ContainsRange()
> will make sure that it’s valid. Same on line 148.

Done.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:68: sizeof(process_types::CrashpadInfo)))) {
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> This should be section.Misc.VirtualSize, since you already know that this is
>=
> sizeof(process_types::CrashpadInfo).

Done.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:69: LOG(WARNING) << "invalid address for
crashpad info "
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> This message is a little misleading, because it checks more than just the
> address of the CrashpadInfo, it checks its entire range.

Done.

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:75: uintptr_t size,
On 2015/04/30 20:58:35, Mark Mentovai wrote:
> I don’t love uintptr_t for sizes. size_t is better.
> 
> Not that we’re sure we’ll need 32-reads-64 (you actually mentioned that you’re
> more sure we won’t), but if you want to preserve the ability to do
32-reads-64,
> these shouldn’t be uintptr_t or size_t. They should be uint64_t or new
typedefs
> equivalent to uint64_t. On the other hand, while most of this class does seem
to
> accept 32-reads-64 (once the TODOs are resolved), if you’re still very against
> it, this isn’t necessary. The typedefs still might be nice for documentary
> value, though. On Mac, we’re able to use the system-supplied mach_vm_address_t
> and mach_vm_size_t.
> 
> I think that these two things bubble through this file and even to other files
> (checked_address_range) a little bit, but I’m only mentioning them here.

I started making an address_types.h, but then recalled that e.g.
ReadProcessMemory
https://msdn.microsoft.com/en-ca/library/windows/desktop/ms680553%28v=vs.85%2...
takes a base address of "void*". So it would mean a separate code path for
escaping WoW64 for only the 32-to-64 case that I suspect won't be worth the
effort in writing or testing.

So, I've switched to uintptr_t/size_t for now. Hopefully it won't be a major
effort to amend that in the future anyway.

https://codereview.chromium.org/1052813002/diff/480001/util/numeric/checked_a...
File util/numeric/checked_address_range.h (right):

https://codereview.chromium.org/1052813002/diff/480001/util/numeric/checked_a...
util/numeric/checked_address_range.h:203: using CheckedAddressRange =
CheckedAddressRangeGeneric<uintptr_t, uintptr_t>;
On 2015/04/30 20:58:36, Mark Mentovai wrote:
> I think that inlining this results in lots of bloat at each call site. We can
> cut down on template inlining by retaining the .h/.cc structure that this
class
> had when it was just checked_mach_address_range, and doing explicit template
> instantiation of the forms used for CheckedAddressRange and
> CheckedMachAddressRange in the .cc file where all of the implementations are
> available (see minidump/minidump_string_writer.cc).

Done.

https://codereview.chromium.org/1052813002/diff/480001/util/win/process_info_...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1052813002/diff/480001/util/win/process_info_...
util/win/process_info_test.cc:35: wchar_t filename[_MAX_PATH];
On 2015/04/30 20:58:36, Mark Mentovai wrote:
> I wasn’t familiar with _MAX_PATH with an underscore so I went looking for it.
I
> found out that it’s a CRT thing. Since you use this with a win32 call, it
should
> be MAX_PATH. (They’re both 260, though.)

Done.

https://codereview.chromium.org/1052813002/diff/480001/util/win/process_info_...
util/win/process_info_test.cc:36: if (!GetModuleFileName(module, filename,
sizeof(filename)))
On 2015/04/30 20:58:36, Mark Mentovai wrote:
> arraysize, not sizeof?

Oops!

https://codereview.chromium.org/1052813002/diff/480001/util/win/process_info_...
util/win/process_info_test.cc:41: return 0;
On 2015/04/30 20:58:36, Mark Mentovai wrote:
> EXPECT_EQ(0, rv) also to make sure that gtest sees this problem even if it
> thinks it’s looking for a 0 timestamp for some reason.

Done.

[scottmg, 2015-04-30 22:12:23]

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:75: uintptr_t size,
On 2015/04/30 22:09:44, scottmg wrote:
> On 2015/04/30 20:58:35, Mark Mentovai wrote:
> > I don’t love uintptr_t for sizes. size_t is better.
> > 
> > Not that we’re sure we’ll need 32-reads-64 (you actually mentioned that
you’re
> > more sure we won’t), but if you want to preserve the ability to do
> 32-reads-64,
> > these shouldn’t be uintptr_t or size_t. They should be uint64_t or new
> typedefs
> > equivalent to uint64_t. On the other hand, while most of this class does
seem
> to
> > accept 32-reads-64 (once the TODOs are resolved), if you’re still very
against
> > it, this isn’t necessary. The typedefs still might be nice for documentary
> > value, though. On Mac, we’re able to use the system-supplied
mach_vm_address_t
> > and mach_vm_size_t.
> > 
> > I think that these two things bubble through this file and even to other
files
> > (checked_address_range) a little bit, but I’m only mentioning them here.
> 
> I started making an address_types.h, but then recalled that e.g.
> ReadProcessMemory
>
https://msdn.microsoft.com/en-ca/library/windows/desktop/ms680553%28v=vs.85%2...
> takes a base address of "void*". So it would mean a separate code path for
> escaping WoW64 for only the 32-to-64 case that I suspect won't be worth the
> effort in writing or testing.
> 
> So, I've switched to uintptr_t/size_t for now. Hopefully it won't be a major
> effort to amend that in the future anyway.

Although now that I say that... e.g. ModuleSnapshot is uint64_t directly. So...
I'll make a couple types I think. It's useful for documentation value as you say
and there's not much cost.

[scottmg, 2015-04-30 22:28:23]

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.h (right):

https://codereview.chromium.org/1052813002/diff/480001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.h:75: uintptr_t size,
On 2015/04/30 22:12:23, scottmg wrote:
> On 2015/04/30 22:09:44, scottmg wrote:
> > On 2015/04/30 20:58:35, Mark Mentovai wrote:
> > > I don’t love uintptr_t for sizes. size_t is better.
> > > 
> > > Not that we’re sure we’ll need 32-reads-64 (you actually mentioned that
> you’re
> > > more sure we won’t), but if you want to preserve the ability to do
> > 32-reads-64,
> > > these shouldn’t be uintptr_t or size_t. They should be uint64_t or new
> > typedefs
> > > equivalent to uint64_t. On the other hand, while most of this class does
> seem
> > to
> > > accept 32-reads-64 (once the TODOs are resolved), if you’re still very
> against
> > > it, this isn’t necessary. The typedefs still might be nice for documentary
> > > value, though. On Mac, we’re able to use the system-supplied
> mach_vm_address_t
> > > and mach_vm_size_t.
> > > 
> > > I think that these two things bubble through this file and even to other
> files
> > > (checked_address_range) a little bit, but I’m only mentioning them here.
> > 
> > I started making an address_types.h, but then recalled that e.g.
> > ReadProcessMemory
> >
>
https://msdn.microsoft.com/en-ca/library/windows/desktop/ms680553%28v=vs.85%2...
> > takes a base address of "void*". So it would mean a separate code path for
> > escaping WoW64 for only the 32-to-64 case that I suspect won't be worth the
> > effort in writing or testing.
> > 
> > So, I've switched to uintptr_t/size_t for now. Hopefully it won't be a major
> > effort to amend that in the future anyway.
> 
> Although now that I say that... e.g. ModuleSnapshot is uint64_t directly.
So...
> I'll make a couple types I think. It's useful for documentation value as you
say
> and there's not much cost.

Lacking a better name, I used win_vm_address_t and win_vm_size_t. WinVmAddress
and WinVmSize would I guess be more standard, but makes them feel like maybe
they're more than they actually are.

[Mark Mentovai, 2015-05-01 19:05:32]

https://codereview.chromium.org/1052813002/diff/550001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1052813002/diff/550001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:56: address_ = address;
Do you need distinct fields for address_ and size_? They’re already tracked in
module_range_.

https://codereview.chromium.org/1052813002/diff/550001/util/numeric/checked_a...
File util/numeric/checked_address_range.h (right):

https://codereview.chromium.org/1052813002/diff/550001/util/numeric/checked_a...
util/numeric/checked_address_range.h:36: class CheckedAddressRangeGeneric {
This can move into namespace internal since there are typedefs for the public
API.

https://codereview.chromium.org/1052813002/diff/550001/util/numeric/checked_a...
File util/numeric/checked_address_range_test.cc (right):

https://codereview.chromium.org/1052813002/diff/550001/util/numeric/checked_a...
util/numeric/checked_address_range_test.cc:47: uintptr_t base;
I think these should both be uint64_t or this test will fall apart when run on
32-bit systems. It might even legitimately fail to compile, because many of the
test cases use constants too big to be expressed with 32 bits. Same on lines 29,
136, and 189.

https://codereview.chromium.org/1052813002/diff/550001/util/util.gyp
File util/util.gyp (right):

https://codereview.chromium.org/1052813002/diff/550001/util/util.gyp#newcode144
util/util.gyp:144: 'win/process_info.cc',
Add address_types.h and checked_win_address_range.h.

https://codereview.chromium.org/1052813002/diff/550001/util/win/address_types.h
File util/win/address_types.h (right):

https://codereview.chromium.org/1052813002/diff/550001/util/win/address_types...
util/win/address_types.h:22: using win_vm_address_t = uint64_t;
Provide a quick //! \brief for each of these saying what it’s intended for, and
perhaps they “why” also.

https://codereview.chromium.org/1052813002/diff/550001/util/win/address_types...
util/win/address_types.h:22: using win_vm_address_t = uint64_t;
I initially wrote a comment here suggesting WinVMAddress and WinVMSize to match
Google style for type names, but then I saw your last comment about considering
and rejecting those, so I came back and fixed this comment to acknowledge. OK,
if you want to keep using these names, it’s fine, and you can read any other
comments I’ve written with the cappy names as though they had the underscorey
ones instead.

https://codereview.chromium.org/1052813002/diff/550001/util/win/checked_win_a...
File util/win/checked_win_address_range.h (right):

https://codereview.chromium.org/1052813002/diff/550001/util/win/checked_win_a...
util/win/checked_win_address_range.h:26: //! This class checks bases of type
`win_vm_address_t` and sizes of type
If you rename these to WinVMAddress and WinVMSize, you won’t need to backtick
them, because Doxygen will automatically link to their definitions. If you don’t
rename them, Doxygen won’t auto-link because they don’t have at least one
uppercase letter, so you’ll need to write ::win_vm_address_t instead of
`win_vm_address_t` to get proper linking. Backticks are for system types that we
don’t provide the docs for and can’t link to.

https://codereview.chromium.org/1052813002/diff/550001/util/win/checked_win_a...
util/win/checked_win_address_range.h:27: //! `mach_vm_size_t` against a process
whose pointer type is either 32 or 64
This one shouldn’t talk about Mach.

https://codereview.chromium.org/1052813002/diff/550001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1052813002/diff/550001/util/win/process_info....
util/win/process_info.cc:83: template <class T> bool ReadStruct(HANDLE process,
uintptr_t at, T* into) {
All of these uintptr_ts can be WinVMAddress.

https://codereview.chromium.org/1052813002/diff/550001/util/win/process_info.h
File util/win/process_info.h (right):

https://codereview.chromium.org/1052813002/diff/550001/util/win/process_info....
util/win/process_info.h:91: uintptr_t peb_address_uintptr,
This can be WinVMAddress too.

[scottmg, 2015-05-01 19:46:32]

https://codereview.chromium.org/1052813002/diff/550001/snapshot/win/pe_image_...
File snapshot/win/pe_image_reader.cc (right):

https://codereview.chromium.org/1052813002/diff/550001/snapshot/win/pe_image_...
snapshot/win/pe_image_reader.cc:56: address_ = address;
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> Do you need distinct fields for address_ and size_? They’re already tracked in
> module_range_.

Done.

https://codereview.chromium.org/1052813002/diff/550001/util/numeric/checked_a...
File util/numeric/checked_address_range.h (right):

https://codereview.chromium.org/1052813002/diff/550001/util/numeric/checked_a...
util/numeric/checked_address_range.h:36: class CheckedAddressRangeGeneric {
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> This can move into namespace internal since there are typedefs for the public
> API.

Done.

https://codereview.chromium.org/1052813002/diff/550001/util/numeric/checked_a...
File util/numeric/checked_address_range_test.cc (right):

https://codereview.chromium.org/1052813002/diff/550001/util/numeric/checked_a...
util/numeric/checked_address_range_test.cc:47: uintptr_t base;
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> I think these should both be uint64_t or this test will fall apart when run on
> 32-bit systems. It might even legitimately fail to compile, because many of
the
> test cases use constants too big to be expressed with 32 bits. Same on lines
29,
> 136, and 189.

Sorry, that was dumb. Done.

https://codereview.chromium.org/1052813002/diff/550001/util/util.gyp
File util/util.gyp (right):

https://codereview.chromium.org/1052813002/diff/550001/util/util.gyp#newcode144
util/util.gyp:144: 'win/process_info.cc',
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> Add address_types.h and checked_win_address_range.h.

Done.

https://codereview.chromium.org/1052813002/diff/550001/util/win/address_types.h
File util/win/address_types.h (right):

https://codereview.chromium.org/1052813002/diff/550001/util/win/address_types...
util/win/address_types.h:22: using win_vm_address_t = uint64_t;
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> Provide a quick //! \brief for each of these saying what it’s intended for,
and
> perhaps they “why” also.

Done.

https://codereview.chromium.org/1052813002/diff/550001/util/win/address_types...
util/win/address_types.h:22: using win_vm_address_t = uint64_t;
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> I initially wrote a comment here suggesting WinVMAddress and WinVMSize to
match
> Google style for type names, but then I saw your last comment about
considering
> and rejecting those, so I came back and fixed this comment to acknowledge. OK,
> if you want to keep using these names, it’s fine, and you can read any other
> comments I’ve written with the cappy names as though they had the underscorey
> ones instead.

Switched to WinVMAddress/Size.

https://codereview.chromium.org/1052813002/diff/550001/util/win/checked_win_a...
File util/win/checked_win_address_range.h (right):

https://codereview.chromium.org/1052813002/diff/550001/util/win/checked_win_a...
util/win/checked_win_address_range.h:26: //! This class checks bases of type
`win_vm_address_t` and sizes of type
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> If you rename these to WinVMAddress and WinVMSize, you won’t need to backtick
> them, because Doxygen will automatically link to their definitions. If you
don’t
> rename them, Doxygen won’t auto-link because they don’t have at least one
> uppercase letter, so you’ll need to write ::win_vm_address_t instead of
> `win_vm_address_t` to get proper linking. Backticks are for system types that
we
> don’t provide the docs for and can’t link to.

Done.

https://codereview.chromium.org/1052813002/diff/550001/util/win/checked_win_a...
util/win/checked_win_address_range.h:27: //! `mach_vm_size_t` against a process
whose pointer type is either 32 or 64
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> This one shouldn’t talk about Mach.

Done.

https://codereview.chromium.org/1052813002/diff/550001/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1052813002/diff/550001/util/win/process_info....
util/win/process_info.cc:83: template <class T> bool ReadStruct(HANDLE process,
uintptr_t at, T* into) {
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> All of these uintptr_ts can be WinVMAddress.

Done.

https://codereview.chromium.org/1052813002/diff/550001/util/win/process_info.h
File util/win/process_info.h (right):

https://codereview.chromium.org/1052813002/diff/550001/util/win/process_info....
util/win/process_info.h:91: uintptr_t peb_address_uintptr,
On 2015/05/01 19:05:31, Mark Mentovai wrote:
> This can be WinVMAddress too.

Done.

[Mark Mentovai, 2015-05-01 20:00:14]

LGTM, this is awesome!

https://codereview.chromium.org/1052813002/diff/610001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/610001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:17: #include "base/strings/stringprintf.h"
Unused.

https://codereview.chromium.org/1052813002/diff/610001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:22: #include "util/stdlib/strnlen.h"
Unused.

https://codereview.chromium.org/1052813002/diff/610001/util/win/address_types.h
File util/win/address_types.h (right):

https://codereview.chromium.org/1052813002/diff/610001/util/win/address_types...
util/win/address_types.h:22: // \brief Type used to represent an address in a
process, potentially across
These need to be //!.

[scottmg, 2015-05-01 20:45:59]

Thanks!

https://codereview.chromium.org/1052813002/diff/610001/snapshot/win/module_sn...
File snapshot/win/module_snapshot_win.cc (right):

https://codereview.chromium.org/1052813002/diff/610001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:17: #include "base/strings/stringprintf.h"
On 2015/05/01 20:00:14, Mark Mentovai wrote:
> Unused.

Done.

https://codereview.chromium.org/1052813002/diff/610001/snapshot/win/module_sn...
snapshot/win/module_snapshot_win.cc:22: #include "util/stdlib/strnlen.h"
On 2015/05/01 20:00:14, Mark Mentovai wrote:
> Unused.

Done.

https://codereview.chromium.org/1052813002/diff/610001/util/win/address_types.h
File util/win/address_types.h (right):

https://codereview.chromium.org/1052813002/diff/610001/util/win/address_types...
util/win/address_types.h:22: // \brief Type used to represent an address in a
process, potentially across
On 2015/05/01 20:00:14, Mark Mentovai wrote:
> These need to be //!.

Done.

[scottmg, 2015-05-01 20:48:34]

Committed patchset #32 (id:630001) manually as
69d135acda60d4de456e682a7d4d52f0eb14bb88 (presubmit successful).