win: get tools/crashpad_database_util mostly working

win: get tools/crashpad_database_util mostly working

- Add public domain getopt implementation to third_party.
- Add timegm to compat/win.
- Add stub of strptime to compat/win.

Requires https://codereview.chromium.org/1119173003/ and
https://codereview.chromium.org/1117013006/.

Rather than working in wchar_t everywhere on Windows, convert
UTF16 command line arguments in wmain to UTF8, work primarily
in UTF8, and convert back when necessary to UTF16 for base::FilePath.
This avoids the need to genericize over all the standard C string
functions, getopt, etc. while still handling non-ASCII properly.

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

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

Congratulations on getting our first tool working on Windows!

https://codereview.chromium.org/1119783005/diff/40001/compat/win/getopt.h
File compat/win/getopt.h (right):

https://codereview.chromium.org/1119783005/diff/40001/compat/win/getopt.h#new...
compat/win/getopt.h:1: #ifndef GETOPT_H
I believe this should actually be in third_party/getopt, with a README.crashpad
talking about where it came from. I guess compat/win/getopt.h should #include
the getopt.h in third_party, and on Windows, the compat library should depend on
the new target in third_party.

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

https://codereview.chromium.org/1119783005/diff/40001/tools/crashpad_database...
tools/crashpad_database_util.cc:63: return _mkgmtime(tm);
Write a timegm wrapper in compat/win/time.{cc,h} instead? And do a compat
wrapper for basename and strptime too?

https://codereview.chromium.org/1119783005/diff/40001/tools/crashpad_database...
tools/crashpad_database_util.cc:252: printf("%sPath: %s\n",
Does it work to use PRFilePath here so that the argument can be
report.file_path.value().c_str() regardless of OS? Or is it desirable to convert
to UTF-8 ourselves? I suspect that neither may be 100% correct on Windows,
because console output is probably expecting narrow strings to be in the
configured codepage, not UTF-8.

https://codereview.chromium.org/1119783005/diff/40001/tools/crashpad_database...
tools/crashpad_database_util.cc:408: base::FilePath
new_report_path(base::UTF8ToUTF16(optarg));
Similarly, when the entry point is main() and not wmain(), argv is in the
configured code page and not UTF-8, right?

https://codereview.chromium.org/1119783005/diff/40001/tools/tools.gyp
File tools/tools.gyp (right):

https://codereview.chromium.org/1119783005/diff/40001/tools/tools.gyp#newcode155
tools/tools.gyp:155: 'targets': [
Now this is in an else block that’s live on non-Mac, but we want it to be live
all the time. Let’s put a top-level 'targets' above the 'conditions' for this.

[scottmg, 2015-05-04 23:23:15]

Patchset #4 (id:60001) has been deleted

[scottmg, 2015-05-04 23:26:36]

https://codereview.chromium.org/1119783005/diff/40001/compat/win/getopt.h
File compat/win/getopt.h (right):

https://codereview.chromium.org/1119783005/diff/40001/compat/win/getopt.h#new...
compat/win/getopt.h:1: #ifndef GETOPT_H
On 2015/05/04 21:04:29, Mark Mentovai wrote:
> I believe this should actually be in third_party/getopt, with a
README.crashpad
> talking about where it came from. I guess compat/win/getopt.h should #include
> the getopt.h in third_party, and on Windows, the compat library should depend
on
> the new target in third_party.

Done.

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

https://codereview.chromium.org/1119783005/diff/40001/tools/crashpad_database...
tools/crashpad_database_util.cc:63: return _mkgmtime(tm);
On 2015/05/04 21:04:29, Mark Mentovai wrote:
> Write a timegm wrapper in compat/win/time.{cc,h} instead? And do a compat
> wrapper for basename and strptime too?

I did timegm and strptime, but basename seems icky, so I pulled that out of the
next CL where it's in util/ with a std::string in/out.

https://codereview.chromium.org/1119783005/diff/40001/tools/crashpad_database...
tools/crashpad_database_util.cc:252: printf("%sPath: %s\n",
On 2015/05/04 21:04:30, Mark Mentovai wrote:
> Does it work to use PRFilePath here so that the argument can be
> report.file_path.value().c_str() regardless of OS? Or is it desirable to
convert
> to UTF-8 ourselves? I suspect that neither may be 100% correct on Windows,
> because console output is probably expecting narrow strings to be in the
> configured codepage, not UTF-8.

Done.

https://codereview.chromium.org/1119783005/diff/40001/tools/crashpad_database...
tools/crashpad_database_util.cc:408: base::FilePath
new_report_path(base::UTF8ToUTF16(optarg));
On 2015/05/04 21:04:30, Mark Mentovai wrote:
> Similarly, when the entry point is main() and not wmain(), argv is in the
> configured code page and not UTF-8, right?

Yes, that's right. I looked at switching to wmain, and then at other option
parsers like e.g. http://optionparser.sourceforge.net/ which support wide chars
but it didn't really seem worth the effort for things in tools/.

So, made this ASCIIToUTF16 to at least be somewhat more accurate (DCHECKs on
>127). This required: https://codereview.chromium.org/1125773004/

https://codereview.chromium.org/1119783005/diff/40001/tools/tools.gyp
File tools/tools.gyp (right):

https://codereview.chromium.org/1119783005/diff/40001/tools/tools.gyp#newcode155
tools/tools.gyp:155: 'targets': [
On 2015/05/04 21:04:30, Mark Mentovai wrote:
> Now this is in an else block that’s live on non-Mac, but we want it to be live
> all the time. Let’s put a top-level 'targets' above the 'conditions' for this.

Oh, geez, oops. Done.

[scottmg, 2015-05-05 00:30:21]

I may have changed my mind on wchar_t/char for main which ripples everywhere. So
please hold off on reviewing the rest of the changes for now.

It seems better at the top level of tools to handle wchar properly, but I'm not
sure how infectious it will be into the lower levels.

Sorry for the dithering.

[Mark Mentovai, 2015-05-05 01:24:50]

No need to apologize. I’d rather get everything right. I’ll put all of your
tools changes on ice until you say otherwise.

[scottmg, 2015-05-05 16:37:56]

I think this should work OK. Please take another look now.

[Mark Mentovai, 2015-05-05 22:19:49]

This wound up being less work than I expected. Nice! Someday we might want to
solve the round-tripping through UTF-8, but I don’t think that’s very important
now. It’s more important to get the tools working, and I’m pleased that there’s
a way for them to be able to operate on non-ASCII paths without having to tear
them apart.

https://codereview.chromium.org/1119783005/diff/180001/compat/compat.gyp
File compat/compat.gyp (right):

https://codereview.chromium.org/1119783005/diff/180001/compat/compat.gyp#newc...
compat/compat.gyp:39: 'win/getopt.h',
getopt < sys

https://codereview.chromium.org/1119783005/diff/180001/compat/compat.gyp#newc...
compat/compat.gyp:65: '../third_party/getopt',
Better to put this in the crashpad_getopt target and then
export_dependent_settings it from here.

https://codereview.chromium.org/1119783005/diff/180001/compat/win/getopt.h
File compat/win/getopt.h (right):

https://codereview.chromium.org/1119783005/diff/180001/compat/win/getopt.h#ne...
compat/win/getopt.h:18: #include "../../third_party/getopt/getopt.h"
I don’t think the ../.. should be necessary.

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/REA...
File third_party/getopt/README.crashpad (right):

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/REA...
third_party/getopt/README.crashpad:1: Copyright (C) 1997 Gregory Pietsch
This should be the LICENSE file.

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/REA...
third_party/getopt/README.crashpad:7: Posted to newlib mailing list November
2005:
The things from this point on can go into README.crashpad. You can use
third_party/apple_cf/README.chromium as a template.

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/get...
File third_party/getopt/getopt.gyp (right):

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/get...
third_party/getopt/getopt.gyp:23: 'target_name': 'crashpad_getopt',
getopt is OK (matching the directory name) since it’s not really ours. Or did
you do this because there’s another target named getopt in the Chromium
universe?

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

https://codereview.chromium.org/1119783005/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:204: gmtime_s(&time_tm, &time_tt);
gmtime_r() and localtime_r() can wrap gmtime_s() and localtime_s() with the
arguments swapped in compat/win/time.*. Then we can write this without #ifdefs.

https://codereview.chromium.org/1119783005/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:327: while ((opt = getopt_long(
This fit on one line before and you haven’t changed anything that would make it
not fit.

https://codereview.chromium.org/1119783005/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:601: scoped_ptr<char*[]> argv_as_utf8(new
char*[argc]);
I imagine that we’ll want this to be in tool_support or something, but you can
move it when another tool is ready to use it. (Maybe you’ve already done that in
the generate_dump change.)

[Mark Mentovai, 2015-05-05 22:25:27]

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

https://codereview.chromium.org/1119783005/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:42: #include <libgen.h>
Remove this. It was only here for basename().

[scottmg, 2015-05-05 23:03:38]

https://codereview.chromium.org/1119783005/diff/180001/compat/compat.gyp
File compat/compat.gyp (right):

https://codereview.chromium.org/1119783005/diff/180001/compat/compat.gyp#newc...
compat/compat.gyp:39: 'win/getopt.h',
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> getopt < sys

Done.

https://codereview.chromium.org/1119783005/diff/180001/compat/compat.gyp#newc...
compat/compat.gyp:65: '../third_party/getopt',
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> Better to put this in the crashpad_getopt target and then
> export_dependent_settings it from here.

Done. (It's not needed at all)

https://codereview.chromium.org/1119783005/diff/180001/compat/win/getopt.h
File compat/win/getopt.h (right):

https://codereview.chromium.org/1119783005/diff/180001/compat/win/getopt.h#ne...
compat/win/getopt.h:18: #include "../../third_party/getopt/getopt.h"
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> I don’t think the ../.. should be necessary.

Done.

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/REA...
File third_party/getopt/README.crashpad (right):

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/REA...
third_party/getopt/README.crashpad:1: Copyright (C) 1997 Gregory Pietsch
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> This should be the LICENSE file.

Done.

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/REA...
third_party/getopt/README.crashpad:7: Posted to newlib mailing list November
2005:
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> The things from this point on can go into README.crashpad. You can use
> third_party/apple_cf/README.chromium as a template.

Done.

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/get...
File third_party/getopt/getopt.gyp (right):

https://codereview.chromium.org/1119783005/diff/180001/third_party/getopt/get...
third_party/getopt/getopt.gyp:23: 'target_name': 'crashpad_getopt',
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> getopt is OK (matching the directory name) since it’s not really ours. Or did
> you do this because there’s another target named getopt in the Chromium
> universe?

Done.

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

https://codereview.chromium.org/1119783005/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:42: #include <libgen.h>
On 2015/05/05 22:25:27, Mark Mentovai wrote:
> Remove this. It was only here for basename().

Done.

https://codereview.chromium.org/1119783005/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:204: gmtime_s(&time_tm, &time_tt);
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> gmtime_r() and localtime_r() can wrap gmtime_s() and localtime_s() with the
> arguments swapped in compat/win/time.*. Then we can write this without
#ifdefs.

Done.

https://codereview.chromium.org/1119783005/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:327: while ((opt = getopt_long(
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> This fit on one line before and you haven’t changed anything that would make
it
> not fit.

Oops, missed that in my conversions back and forth on string encodings.

https://codereview.chromium.org/1119783005/diff/180001/tools/crashpad_databas...
tools/crashpad_database_util.cc:601: scoped_ptr<char*[]> argv_as_utf8(new
char*[argc]);
On 2015/05/05 22:19:49, Mark Mentovai wrote:
> I imagine that we’ll want this to be in tool_support or something, but you can
> move it when another tool is ready to use it. (Maybe you’ve already done that
in
> the generate_dump change.)

Yeah, I did that in the generate_dump CL as it's then required in > 1 place.

[Mark Mentovai, 2015-05-06 04:31:46]

LGTM

https://codereview.chromium.org/1119783005/diff/240001/compat/win/time.h
File compat/win/time.h (right):

https://codereview.chromium.org/1119783005/diff/240001/compat/win/time.h#newc...
compat/win/time.h:20: struct tm* gmtime_r(const time_t* timep, struct tm*
result);
Wrap the declarations in extern "C", and the definitions in time.cc too.

https://codereview.chromium.org/1119783005/diff/240001/compat/win/time.h#newc...
compat/win/time.h:24: const char *strptime(const char* buf, const char* format,
struct tm* tm);
Move the * onto char.

https://codereview.chromium.org/1119783005/diff/240001/third_party/getopt/REA...
File third_party/getopt/README.crashpad (right):

https://codereview.chromium.org/1119783005/diff/240001/third_party/getopt/REA...
third_party/getopt/README.crashpad:13: 
Remove the blank line at EOF.

https://codereview.chromium.org/1119783005/diff/240001/tools/tool_support.h
File tools/tool_support.h (right):

https://codereview.chromium.org/1119783005/diff/240001/tools/tool_support.h#n...
tools/tool_support.h:46: //! \copydoc Version
Not sure if this is going to do what’s intended, because this method is also
named Version().

[scottmg, 2015-05-06 17:28:12]

Committed patchset #13 (id:260001) manually as
17b770ece4d489a07e97ed1f526436955ee35bcc (presubmit successful).

[scottmg, 2015-05-06 17:28:14]

https://codereview.chromium.org/1119783005/diff/240001/compat/win/time.h
File compat/win/time.h (right):

https://codereview.chromium.org/1119783005/diff/240001/compat/win/time.h#newc...
compat/win/time.h:20: struct tm* gmtime_r(const time_t* timep, struct tm*
result);
On 2015/05/06 04:31:46, Mark Mentovai wrote:
> Wrap the declarations in extern "C", and the definitions in time.cc too.

Done.

https://codereview.chromium.org/1119783005/diff/240001/compat/win/time.h#newc...
compat/win/time.h:24: const char *strptime(const char* buf, const char* format,
struct tm* tm);
On 2015/05/06 04:31:46, Mark Mentovai wrote:
> Move the * onto char.

Done.

https://codereview.chromium.org/1119783005/diff/240001/third_party/getopt/REA...
File third_party/getopt/README.crashpad (right):

https://codereview.chromium.org/1119783005/diff/240001/third_party/getopt/REA...
third_party/getopt/README.crashpad:13: 
On 2015/05/06 04:31:46, Mark Mentovai wrote:
> Remove the blank line at EOF.

Done.

https://codereview.chromium.org/1119783005/diff/240001/tools/tool_support.h
File tools/tool_support.h (right):

https://codereview.chromium.org/1119783005/diff/240001/tools/tool_support.h#n...
tools/tool_support.h:46: //! \copydoc Version
On 2015/05/06 04:31:46, Mark Mentovai wrote:
> Not sure if this is going to do what’s intended, because this method is also
> named Version().

I built doxygen at trunk just now, and it seems to be smart enough to figure
that out.