Put PUBLIC lines in Mac and Linux symbol files

Put PUBLIC lines in Mac and Linux symbol files.

BUG=397

Landed elsewhere

[jimb]

I'm sorry about missing this --- I had it on a "to-do" list that got buried. 
Thanks very much for taking care of it.

The new code should be covered by unit tests.

As noted below, I believe the new code will break on Linux.

http://breakpad.appspot.com/163001/diff/1/3
File src/common/module.h (right):

http://breakpad.appspot.com/163001/diff/1/3#newcode124
Line 124: static bool CompareByAddress(const Module::Line &x, const Module::Line
&y) {
I don't see that this is ever used.

http://breakpad.appspot.com/163001/diff/1/3#newcode216
Line 216: // TODO(mark): comment
Need the actual comment here. Be sure to note ownership.

If I had my druthers, this would take a const Extern &, the set would contain
Externs directly, AddExtern would be a one-liner, and the destructor would be
left alone. The fewer 'new's and 'delete's we have, the better.

Note that all widely used std::string implementations use reference-counted
buffers, so the set<Extern> will "steal" the string storage allocated when you
initialize the Extern instance you pass by value.

http://breakpad.appspot.com/163001/diff/1/3#newcode298
Line 298: typedef set<Extern *, ExternCompare> ExternSet;
Any interest in doing a drive-by cleanup and adding comments for both this and
FunctionSet?

http://breakpad.appspot.com/163001/diff/1/3#newcode310
Line 310: ExternSet externs_;
This deserves a comment, too. Be sure to note ownership, if you don't make the
set hold the Externs directly.

http://breakpad.appspot.com/163001/diff/1/4
File src/common/stabs_reader.cc (right):

http://breakpad.appspot.com/163001/diff/1/4#newcode110
Line 110: } else if ((iterator_->type & N_STAB) == 0 &&
I believe this will break Linux, which has no definitions for N_SECT that I can
see.

There should be a brief comment here explaining that this is what Mac library
symbols look like, and why we need to care about them. Or perhaps the comment
should go over ProcessExtern?

http://breakpad.appspot.com/163001/diff/1/4#newcode297
Line 297: std::string extern_string(extern_string_c);
Can't you rely on the automatic conversion from 'char *' to 'std::string' here,
and just pass it directly to handler_->Extern?

http://breakpad.appspot.com/163001/diff/1/5
File src/common/stabs_reader.h (right):

http://breakpad.appspot.com/163001/diff/1/5#newcode192
Line 192: // TODO(mark): comment
More comments To Do.

http://breakpad.appspot.com/163001/diff/1/5#newcode307
Line 307: // TODO(mark): comment
More comments to do.

http://breakpad.appspot.com/163001/diff/1/6
File src/common/stabs_to_module.cc (right):

http://breakpad.appspot.com/163001/diff/1/6#newcode137
Line 137: if (!name.empty() && name[0] == '_')
If the name is empty, do we want to record it at all?

Stripping the underscore here seems wrong, but I remember being righteously
confused about when Mac symbols had underscores added and when they didn't, so I
can't really suggest anything better.

http://breakpad.appspot.com/163001/diff/1/7
File src/common/stabs_to_module.h (right):

http://breakpad.appspot.com/163001/diff/1/7#newcode55
Line 55: // symbol table information from a StabsReader, and uses that to
populate
Well, it doesn't receive symbol table information under ELF. I would say
something like, "When processing Darwin Mach-O files, this also receives public
linker symbols, like those found in system libraries."

[Ted Mielczarek]

http://breakpad.appspot.com/163001/diff/1/2
File src/common/module.cc (right):

http://breakpad.appspot.com/163001/diff/1/2#newcode257
Line 257: last_address = ext->address;
Not terribly important, but could you move this up above the STACK CFI writing?
I find it a lot easier to have function names ahead of the STACK data when
manually inspecting symbol files.

[jimb]

http://breakpad.appspot.com/163001/diff/1/2
File src/common/module.cc (right):

http://breakpad.appspot.com/163001/diff/1/2#newcode257
Line 257: last_address = ext->address;
Yeah, I agree; let's write PUBLIC symbols before CFI.

[jimb]

Hi, Mark. Any progress on getting this revised and in upstream?

[Mark Mentovai]

I just noticed this again yesterday. I hope to have time next week.

[Ted Mielczarek]

On 2010/10/01 19:42:49, Mark Mentovai wrote:
> I just noticed this again yesterday. I hope to have time next week.

Do you think you'll have time to get back to this? If not, would you like me to
update your patch for you? I'd like to try to get some OS X system symbols into
our symbol store, but without this they won't be very useful.

[Mark Mentovai]

Yes, it’d be helpful if you could integrate the review comments. I can review
(or Jim can continue to if he’s available.) I’ve been too busy to pick this back
up.

[Ted Mielczarek]

On 2011/02/22 19:35:48, Mark Mentovai wrote:
> Yes, it’d be helpful if you could integrate the review comments. I can review
> (or Jim can continue to if he’s available.) I’ve been too busy to pick this
back
> up.

Updated patch with review comments addressed + unit tests is up at
http://breakpad.appspot.com/267001.

[Ted Mielczarek]

FYI, this patch landed, and I have a patch hanging out to implement this for
Linux here:
http://breakpad.appspot.com/275001/show