Make Windows dump_syms output a BINARY line with code_file and code_identifier

I have two things I'd like to fix, and both of them require knowing the code
file and code identifier of binary files. These are easy to get when you have
the exe file, and outputing them from dump_syms seems like a natural fit. The
only downside is that I can't easily shoehorn them into the MODULE line, since
the "debug filename" field there can contain spaces, so I've invented a new line
that I named BINARY. It's not perfect, since anything that parses symbols needs
to be uploaded to know about it, but I don't think it's that bad.

I needed to use wcstombs, so I added a safe_wcstombs to WindowsStringUtils.
While I was there I refactored safe_mbstowcs to use a vector<wchar_t> instead of
heap allocation.

This patch also fixes basic_source_line_resolver to skip BINARY lines and adds a
BINARY line to one of the test files to ensure that it works.

Specifically, the two things I want to fix using this are:
http://code.google.com/p/google-breakpad/issues/detail?id=389 - Teach symbol
supplier to locate symbols in absence of CodeView record
https://bugzilla.mozilla.org/show_bug.cgi?id=528092 - Supply binaries
(firefox.exe and DLLs) from symbol server

[jimb]

Be sure to update the symbol file format description on the wiki.

[Mark Mentovai]

http://breakpad.appspot.com/180001/diff/1/2
File src/common/windows/pdb_source_line_writer.cc (right):

http://breakpad.appspot.com/180001/diff/1/2#newcode511
Line 511: fprintf(output_, "BINARY %ws %ws\n",
BINARY isn’t a terribly descriptive name. Since the fields are for
code_identifier and code_file, maybe call it CODE?

http://breakpad.appspot.com/180001/diff/1/2#newcode566
Line 566: const wchar_t *extensions[2] = { L"exe", L"dll" };
[2] unnecessary (and potentially harmful), you only need [].

http://breakpad.appspot.com/180001/diff/1/2#newcode567
Line 567: for (int i = 0; i < 2; i++) {
Use an arraysize sort of thing instead of hard-coding 2.

http://breakpad.appspot.com/180001/diff/1/2#newcode786
Line 786: PrintPEInfo() &&
This is now a critical step, so if there’s no .exe or .dll, the whole mess
fails?

Hmm.

http://breakpad.appspot.com/180001/diff/1/2#newcode898
Line 898: PLOADED_IMAGE img = ImageLoad((PSTR)code_file.c_str(), NULL);
Can a scoper own this to guard against future early returns leaking the image?

http://breakpad.appspot.com/180001/diff/1/5
File src/common/windows/string_utils.cc (right):

http://breakpad.appspot.com/180001/diff/1/5#newcode56
Line 56: if ((err = mbstowcs_s(&wcs_length, NULL, 0, mbs.c_str(), _TRUNCATE)) !=
0) {
Does anything guarantee that wcs_length > 0 in this branch?

http://breakpad.appspot.com/180001/diff/1/5#newcode68
Line 68: std::vector<wchar_t> wcs_c(wcs_length);
_c was for “C string” but this is a vector now so the name is wrong.

http://breakpad.appspot.com/180001/diff/1/5#newcode90
Line 90: bool WindowsStringUtils::safe_wcstombs(const wstring &wcs, string *mbs)
{
Same comments as in the other function apply here.

http://breakpad.appspot.com/180001/diff/1/6
File src/processor/basic_source_line_resolver.cc (right):

http://breakpad.appspot.com/180001/diff/1/6#newcode397
Line 397: } else if (strncmp(buffer, "BINARY ", 7) == 0) {
This means that new symbol files will cause older processors to choke.

That’s OK, but I wonder if it might be a good idea to not activate the code that
writes these new lines in the symbol files quite yet to allow the processor
users a chance to upgrade.

[jimb]

http://breakpad.appspot.com/180001/diff/1/2
File src/common/windows/pdb_source_line_writer.cc (right):

http://breakpad.appspot.com/180001/diff/1/2#newcode511
Line 511: fprintf(output_, "BINARY %ws %ws\n",
Or "CODE-IDENTIFIER"?  It's not like this is a space or typing issue. :)

[Ted Mielczarek]

http://breakpad.appspot.com/180001/diff/1/2
File src/common/windows/pdb_source_line_writer.cc (right):

http://breakpad.appspot.com/180001/diff/1/2#newcode511
Line 511: fprintf(output_, "BINARY %ws %ws\n",
On 2010/09/01 17:03:47, Mark Mentovai wrote:
> BINARY isn’t a terribly descriptive name. Since the fields are for
> code_identifier and code_file, maybe call it CODE?

I didn't have any other great ideas, yours and Jim's both sound good.

http://breakpad.appspot.com/180001/diff/1/2#newcode786
Line 786: PrintPEInfo() &&
On 2010/09/01 17:03:47, Mark Mentovai wrote:
> This is now a critical step, so if there’s no .exe or .dll, the whole mess
> fails?

Yeah, I thought about that, but since it involved slight refactoring here I
think I forgot to do it. I'll make it non-fatal since it's purely informational.

http://breakpad.appspot.com/180001/diff/1/2#newcode898
Line 898: PLOADED_IMAGE img = ImageLoad((PSTR)code_file.c_str(), NULL);
On 2010/09/01 17:03:47, Mark Mentovai wrote:
> Can a scoper own this to guard against future early returns leaking the image?

I thought about that but didn't get around to it since there's currently no
early returns in between, but future-proofing sounds smart.

http://breakpad.appspot.com/180001/diff/1/5
File src/common/windows/string_utils.cc (right):

http://breakpad.appspot.com/180001/diff/1/5#newcode56
Line 56: if ((err = mbstowcs_s(&wcs_length, NULL, 0, mbs.c_str(), _TRUNCATE)) !=
0) {
On 2010/09/01 17:03:47, Mark Mentovai wrote:
> Does anything guarantee that wcs_length > 0 in this branch?

The documentation says:
"If mbstowcs_s successfully converts the source string, it puts the size in wide
characters of the converted string, including the null terminator, into
*pReturnValue (provided pReturnValue is not NULL). This occurs even if the wcstr
argument is NULL and provides a way to determine the required buffer size."

so presumably even with an empty input string you'd at least get an output > 0
to contain the null byte.

http://breakpad.appspot.com/180001/diff/1/6
File src/processor/basic_source_line_resolver.cc (right):

http://breakpad.appspot.com/180001/diff/1/6#newcode397
Line 397: } else if (strncmp(buffer, "BINARY ", 7) == 0) {
On 2010/09/01 17:03:47, Mark Mentovai wrote:
> This means that new symbol files will cause older processors to choke.
> 
> That’s OK, but I wonder if it might be a good idea to not activate the code
that
> writes these new lines in the symbol files quite yet to allow the processor
> users a chance to upgrade.

My thought was to land the code change but not update the checked-in binaries
yet.

Also, what do you think about future-proofing the symbol parsing here? We could
avoid the "assume it's line data" case unless we previously hit a FUNC, and
otherwise just ignore unknown lines. Not that I expect us to add lots of new
types, but it would at least avoid this situation in the future.

[Ted Mielczarek]

Ping? I think I addressed all your review comments.

[Mark Mentovai]

Yes, you did.

I was hoping we could land the processor-side stuff first and then wait a bit
before landing the dump_syms-side stuff.

[Ted Mielczarek]

http://breakpad.appspot.com/180001/diff/10001/11001
File src/common/windows/pdb_source_line_writer.cc (right):

http://breakpad.appspot.com/180001/diff/10001/11001#newcode805
Line 805: // PrintPEInfo();
This patch has the PrintPEInfo line commented out, so it won't change the output
format, but does change the processor. We can give some time to adapt and then I
can uncomment this line (a patch I have in my local patch queue) later.