Get generate_dump compiling on Windows

Get generate_dump compiling on Windows

Sort of works in that the process is opened, modules retrieved, etc.
but eventually CHECKs due to missing functionality in snapshot.

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

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

[scottmg, 2015-05-04 18:36:52]

Patchset #2 (id:20001) has been deleted

[scottmg, 2015-05-04 18:38:03]

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

https://codereview.chromium.org/1120383003/diff/1/util/win/process_info.cc#ne...
util/win/process_info.cc:262: process_id_ =
static_cast<DWORD>(process_basic_information.UniqueProcessId);
Ref:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
on 32 bit being the correct size for HANDLEs for processes, even on Windows x64
(and also for the change in sys/types.h).

API functions (e.g. OpenProcess) take only a DWORD, so there's otherwise
truncation warnings later.

[Mark Mentovai, 2015-05-04 21:21:59]

https://codereview.chromium.org/1120383003/diff/40001/tools/crashpad_database...
File tools/crashpad_database_util.cc (left):

https://codereview.chromium.org/1120383003/diff/40001/tools/crashpad_database...
tools/crashpad_database_util.cc:53: std::string Basename(const std::string&
path) {
I’m skipping this for now because I suspect this is going to turn into a merge
conflict with its dependency change in any case.

https://codereview.chromium.org/1120383003/diff/40001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/40001/tools/generate_dump.cc#...
tools/generate_dump.cc:130: #if defined(OS_MACOSX)
I hope that over time, we can refactor things well enough to the point that the
code doesn’t have to be structured as “if-Mac-elseif-Win, if-Mac-elseif-Win”
over and over.

https://codereview.chromium.org/1120383003/diff/40001/util/file/basename.h
File util/file/basename.h (right):

https://codereview.chromium.org/1120383003/diff/40001/util/file/basename.h#ne...
util/file/basename.h:22: //! \brief Splits a file path into the directory and
filename parts, and returns
I was going to say that this should be done in compat for Windows, but that’s
got unexpected C semantics that return a pointer to something in the input
string. But this also basically duplicates something in file_path. Is it more
sensible to have this, or to use FilePath’s BaseName()?

https://codereview.chromium.org/1120383003/diff/40001/util/file/basename_win.cc
File util/file/basename_win.cc (right):

https://codereview.chromium.org/1120383003/diff/40001/util/file/basename_win....
util/file/basename_win.cc:22: _splitpath(path.c_str(), nullptr, nullptr, fname,
ext);
Compatibility test between basename() and _splitpath(): what happens if the path
argument ends in a directory separator?

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

https://codereview.chromium.org/1120383003/diff/40001/util/win/process_info.c...
util/win/process_info.cc:262: process_id_ =
static_cast<DWORD>(process_basic_information.UniqueProcessId);
The comment you left in Rietveld was very helpful to justify that this is
correct. Can you leave it or something similar here as a source code comment?

[scottmg, 2015-05-04 23:30:07]

(Should have probably merged these two CLs, oh well.)

https://codereview.chromium.org/1120383003/diff/40001/util/file/basename.h
File util/file/basename.h (right):

https://codereview.chromium.org/1120383003/diff/40001/util/file/basename.h#ne...
util/file/basename.h:22: //! \brief Splits a file path into the directory and
filename parts, and returns
On 2015/05/04 21:21:58, Mark Mentovai wrote:
> I was going to say that this should be done in compat for Windows, but that’s
> got unexpected C semantics that return a pointer to something in the input
> string. But this also basically duplicates something in file_path. Is it more
> sensible to have this, or to use FilePath’s BaseName()?

Oh, for some reason I thought we didn't have all of FilePath's implementation in
mini_chromium yet. I'll drop these, use FilePath::BaseName, and make ToolSupport
take a FilePath for these things.

[scottmg, 2015-05-05 19:26:23]

Rebased on the database_util tool, and generalizes some of the string handling
into ToolSupport now that it's needed in two tools.

https://codereview.chromium.org/1120383003/diff/40001/tools/crashpad_database...
File tools/crashpad_database_util.cc (left):

https://codereview.chromium.org/1120383003/diff/40001/tools/crashpad_database...
tools/crashpad_database_util.cc:53: std::string Basename(const std::string&
path) {
On 2015/05/04 21:21:58, Mark Mentovai wrote:
> I’m skipping this for now because I suspect this is going to turn into a merge
> conflict with its dependency change in any case.

All gone now.

https://codereview.chromium.org/1120383003/diff/40001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/40001/tools/generate_dump.cc#...
tools/generate_dump.cc:130: #if defined(OS_MACOSX)
On 2015/05/04 21:21:58, Mark Mentovai wrote:
> I hope that over time, we can refactor things well enough to the point that
the
> code doesn’t have to be structured as “if-Mac-elseif-Win, if-Mac-elseif-Win”
> over and over.

OK. It seems that having a typedef for task_t/process HANDLE would get us some
of the way. That would allow for a ScopedProcessSuspend (with the same name) and
a factory function to create a snapshot. There's still the process opening that
talks to OS-level things, but that could be moved to a helper function at least.

https://codereview.chromium.org/1120383003/diff/40001/util/file/basename_win.cc
File util/file/basename_win.cc (right):

https://codereview.chromium.org/1120383003/diff/40001/util/file/basename_win....
util/file/basename_win.cc:22: _splitpath(path.c_str(), nullptr, nullptr, fname,
ext);
On 2015/05/04 21:21:59, Mark Mentovai wrote:
> Compatibility test between basename() and _splitpath(): what happens if the
path
> argument ends in a directory separator?

Deleted.

[Mark Mentovai, 2015-05-05 22:41:36]

https://codereview.chromium.org/1120383003/diff/40001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/40001/tools/generate_dump.cc#...
tools/generate_dump.cc:130: #if defined(OS_MACOSX)
scottmg wrote:
> OK. It seems that having a typedef for task_t/process HANDLE would get us some
> of the way. That would allow for a ScopedProcessSuspend (with the same name)
and
> a factory function to create a snapshot. There's still the process opening
that
> talks to OS-level things, but that could be moved to a helper function at
least.

Yup, that would be a good future change.

https://codereview.chromium.org/1120383003/diff/180001/compat/win/sys/types.h
File compat/win/sys/types.h (right):

https://codereview.chromium.org/1120383003/diff/180001/compat/win/sys/types.h...
compat/win/sys/types.h:25: typedef unsigned int pid_t;
This doesn’t depend on _WIN64 anymore.

https://codereview.chromium.org/1120383003/diff/180001/tools/crashpad_databas...
File tools/crashpad_database_util.cc (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:591: return crashpad::ToolSupport::Wmain(argc,
argv, &crashpad::DatabaseUtilMain);
Ah, yup, you did it. :) I think you can skip the & on DatabaseUtilMain. Same
with GenerateDumpMain in the other tool.

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:34: #include <libgen.h>
This was only present for basename(). You can remove it.

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:70: const base::FilePath
me(base::FilePath(ToolSupport::UTF8ToFilePathStringType(
Not loving what clang-format does with this, but I guess it’s OK. These are hard
ones to pick a good format for.

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:129: argv[0]);
Where this happens, it’s writing UTF-8 to stdout, and if stdout is the console,
Windows will be expecting a native narrow string. I don’t think that it’s
necessary to worry about that now, but maybe we should have a bug on file to
deal with this in tool output.

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:172: LOG(ERROR) << "TODO(scottmg): --suspend not
implemented yet.";
suspend is the default, so technically, it’s “--no-suspend is required for now”.

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.cc
File tools/tool_support.cc (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.cc#...
tools/tool_support.cc:93: return path;
The #ifs can go inside the function body for this one, since the signature’s the
same.

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.h
File tools/tool_support.h (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.h#n...
tools/tool_support.h:57: //! \brief Converts \a argv wchar_t UTF16 to UTF8, and
passes onwards to a
`wchar_t`, UTF-16 (dash), UTF-8. Also in the next comment.

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.h#n...
tools/tool_support.h:67: static base::FilePath::StringType
UTF8ToFilePathStringType(const char* path);
path might not be UTF-8. It is on Windows because Wmain will have converted
known UTF-16 to known UTF-8, but on some systems, the native path encoding is
not UTF-8. It’s correct in those cases to just pass whatever string we’ve got on
through to an output string as-is, so the code is correct, but the function name
shouldn’t be misleading in this way.

Unfortunately, any naming like Narrow or MultiByte is then confusing on Windows,
where the strings are actually UTF-8 thanks to Wmain(), and are definitely not
in the encoding that Narrow or MultiByte would imply.

Maybe CommandLineArgumentToFilePathStringType()? That’s pretty wordy.
ArgumentToFilePathStringType()? That’s a little bit ambiguous. Something with
Argv in it?

[scottmg, 2015-05-05 23:23:53]

https://codereview.chromium.org/1120383003/diff/180001/compat/win/sys/types.h
File compat/win/sys/types.h (right):

https://codereview.chromium.org/1120383003/diff/180001/compat/win/sys/types.h...
compat/win/sys/types.h:25: typedef unsigned int pid_t;
On 2015/05/05 22:41:35, Mark Mentovai wrote:
> This doesn’t depend on _WIN64 anymore.

Done.

https://codereview.chromium.org/1120383003/diff/180001/tools/crashpad_databas...
File tools/crashpad_database_util.cc (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:591: return crashpad::ToolSupport::Wmain(argc,
argv, &crashpad::DatabaseUtilMain);
On 2015/05/05 22:41:35, Mark Mentovai wrote:
> Ah, yup, you did it. :) I think you can skip the & on DatabaseUtilMain. Same
> with GenerateDumpMain in the other tool.

Done.

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:34: #include <libgen.h>
On 2015/05/05 22:41:35, Mark Mentovai wrote:
> This was only present for basename(). You can remove it.

Done.

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:70: const base::FilePath
me(base::FilePath(ToolSupport::UTF8ToFilePathStringType(
On 2015/05/05 22:41:35, Mark Mentovai wrote:
> Not loving what clang-format does with this, but I guess it’s OK. These are
hard
> ones to pick a good format for.

I broke them into two statements, as it got even worse after "UTF8" changed to
"CommandLineArgument".

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:129: argv[0]);
On 2015/05/05 22:41:36, Mark Mentovai wrote:
> Where this happens, it’s writing UTF-8 to stdout, and if stdout is the
console,
> Windows will be expecting a native narrow string. I don’t think that it’s
> necessary to worry about that now, but maybe we should have a bug on file to
> deal with this in tool output.

Filed: https://code.google.com/p/crashpad/issues/detail?id=36

I think it's writing UTF16 here, or is that what %ls in fprintf does?

But either way, Windows tends to be terrible about anything other than
ASCII/native codepage going to the console, so we should figure out what we want
to do/support.

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:172: LOG(ERROR) << "TODO(scottmg): --suspend not
implemented yet.";
On 2015/05/05 22:41:36, Mark Mentovai wrote:
> suspend is the default, so technically, it’s “--no-suspend is required for
now”.

Done.

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.cc
File tools/tool_support.cc (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.cc#...
tools/tool_support.cc:93: return path;
On 2015/05/05 22:41:36, Mark Mentovai wrote:
> The #ifs can go inside the function body for this one, since the signature’s
the
> same.

Done.

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.h
File tools/tool_support.h (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.h#n...
tools/tool_support.h:57: //! \brief Converts \a argv wchar_t UTF16 to UTF8, and
passes onwards to a
On 2015/05/05 22:41:36, Mark Mentovai wrote:
> `wchar_t`, UTF-16 (dash), UTF-8. Also in the next comment.

Done.

https://codereview.chromium.org/1120383003/diff/180001/tools/tool_support.h#n...
tools/tool_support.h:67: static base::FilePath::StringType
UTF8ToFilePathStringType(const char* path);
On 2015/05/05 22:41:36, Mark Mentovai wrote:
> path might not be UTF-8. It is on Windows because Wmain will have converted
> known UTF-16 to known UTF-8, but on some systems, the native path encoding is
> not UTF-8. It’s correct in those cases to just pass whatever string we’ve got
on
> through to an output string as-is, so the code is correct, but the function
name
> shouldn’t be misleading in this way.
> 
> Unfortunately, any naming like Narrow or MultiByte is then confusing on
Windows,
> where the strings are actually UTF-8 thanks to Wmain(), and are definitely not
> in the encoding that Narrow or MultiByte would imply.
> 
> Maybe CommandLineArgumentToFilePathStringType()? That’s pretty wordy.
> ArgumentToFilePathStringType()? That’s a little bit ambiguous. Something with
> Argv in it?

I went with CommandLineArgumentToFilePathStringType. It's (hopefully) not used
that frequently anyway.

[Mark Mentovai, 2015-05-06 04:44:16]

LGTM

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:129: argv[0]);
On 2015/05/05 23:23:52, scottmg wrote:
> On 2015/05/05 22:41:36, Mark Mentovai wrote:
> > Where this happens, it’s writing UTF-8 to stdout, and if stdout is the
> console,
> > Windows will be expecting a native narrow string. I don’t think that it’s
> > necessary to worry about that now, but maybe we should have a bug on file to
> > deal with this in tool output.
> 
> Filed: https://code.google.com/p/crashpad/issues/detail?id=36
> 
> I think it's writing UTF16 here, or is that what %ls in fprintf does?

%ls is for me.value().c_str(). %ls says UTF-16, so I’m assuming that it does the
right thing for this FilePath. But argv[0] is UTF-8 and formatted with %s.
That’s where we might run into trouble.

https://codereview.chromium.org/1120383003/diff/200001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/200001/tools/generate_dump.cc...
tools/generate_dump.cc:193: base::FilePath
dump_path(base::UTF8ToUTF16(options.dump_path));
Use CommandLineArgumentToFilePathStringType for this to get rid of the #ifdef.

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h
File tools/tool_support.h (right):

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h#n...
tools/tool_support.h:60: //! \return The return values of \a entry.
value, singular.

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h#n...
tools/tool_support.h:67: //! On POSIX, this is a no-op. On Windows, we assume
that Wmain was used, and
Avoid “we.”

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h#n...
tools/tool_support.h:67: //! On POSIX, this is a no-op. On Windows, we assume
that Wmain was used, and
Wmain(), also in the \sa.

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h#n...
tools/tool_support.h:73: const char* arg);
Take a const std::string& here, because it’s going to be turned into one anyway,
and making the argument be a const string& instead of a const char* means that
it gets to keep its size if it’s a string to begin with.

Ideally this would actually be a const StringPiece&, but I didn’t have
StringPiece in mini_chromium when I added its utf_string_conversions.h. We do
have it now, though. We can make UTF8ToUTF16 accept a const StringPiece& instead
of a const string&, and then this can do the same.

[scottmg, 2015-05-06 18:02:19]

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/180001/tools/generate_dump.cc...
tools/generate_dump.cc:129: argv[0]);
On 2015/05/06 04:44:15, Mark Mentovai wrote:
> On 2015/05/05 23:23:52, scottmg wrote:
> > On 2015/05/05 22:41:36, Mark Mentovai wrote:
> > > Where this happens, it’s writing UTF-8 to stdout, and if stdout is the
> > console,
> > > Windows will be expecting a native narrow string. I don’t think that it’s
> > > necessary to worry about that now, but maybe we should have a bug on file
to
> > > deal with this in tool output.
> > 
> > Filed: https://code.google.com/p/crashpad/issues/detail?id=36
> > 
> > I think it's writing UTF16 here, or is that what %ls in fprintf does?
> 
> %ls is for me.value().c_str(). %ls says UTF-16, so I’m assuming that it does
the
> right thing for this FilePath. But argv[0] is UTF-8 and formatted with %s.
> That’s where we might run into trouble.

Oh, I misunderstood and was looking at `me`, rather than argv[0].

https://codereview.chromium.org/1120383003/diff/200001/tools/generate_dump.cc
File tools/generate_dump.cc (right):

https://codereview.chromium.org/1120383003/diff/200001/tools/generate_dump.cc...
tools/generate_dump.cc:193: base::FilePath
dump_path(base::UTF8ToUTF16(options.dump_path));
On 2015/05/06 04:44:15, Mark Mentovai wrote:
> Use CommandLineArgumentToFilePathStringType for this to get rid of the #ifdef.

Done.

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h
File tools/tool_support.h (right):

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h#n...
tools/tool_support.h:60: //! \return The return values of \a entry.
On 2015/05/06 04:44:15, Mark Mentovai wrote:
> value, singular.

Done.

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h#n...
tools/tool_support.h:67: //! On POSIX, this is a no-op. On Windows, we assume
that Wmain was used, and
On 2015/05/06 04:44:15, Mark Mentovai wrote:
> Wmain(), also in the \sa.

Done.

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h#n...
tools/tool_support.h:67: //! On POSIX, this is a no-op. On Windows, we assume
that Wmain was used, and
On 2015/05/06 04:44:15, Mark Mentovai wrote:
> Avoid “we.”

Done.

https://codereview.chromium.org/1120383003/diff/200001/tools/tool_support.h#n...
tools/tool_support.h:73: const char* arg);
On 2015/05/06 04:44:15, Mark Mentovai wrote:
> Take a const std::string& here, because it’s going to be turned into one
anyway,
> and making the argument be a const string& instead of a const char* means that
> it gets to keep its size if it’s a string to begin with.
> 
> Ideally this would actually be a const StringPiece&, but I didn’t have
> StringPiece in mini_chromium when I added its utf_string_conversions.h. We do
> have it now, though. We can make UTF8ToUTF16 accept a const StringPiece&
instead
> of a const string&, and then this can do the same.

Done.

[Mark Mentovai, 2015-05-06 18:05:15]

LGTM!

[scottmg, 2015-05-06 18:09:35]

Committed patchset #15 (id:300001) manually as
ff52791faf302cf95c860ee13dd9ea637a7775a9 (presubmit successful).