Put PUBLIC lines in Mac and Linux symbol files

This is an updated version of Mark's patch from
http://breakpad.appspot.com/163001. The changes are just to address Jim's review
comments, and adding unit tests.

[Ted Mielczarek]

I was testing this by running it on everything in /usr/lib and
/System/Library/Frameworks on my Mac, and it apparently crashes in cxa_demangle
on certain symbols, like "_MorphTableGetNextMorphChain" for example. We pass
that symbol without the leading underscore to cxa_demangle and it segfaults. I
think this might be some ancient GCC bug that Apple didn't pick up, but we'll
probably have to work around it.

[Mark Mentovai]

Perhaps we ought not cxa_demangle things that aren’t _Z?

[jimb]

Thanks for revising this!

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

http://breakpad.appspot.com/267001/diff/1/2#newcode235
Line 235: if (ext->address != last_address) {
If we're just going to drop duplicate symbols with the same address, then why
shouldn't we just omit them from the set in the first place, by making the
comparison function ignore the symbol name?

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

http://breakpad.appspot.com/267001/diff/1/8#newcode137
Line 137: if (!name.empty() && name[0] == '_')
I think dropping the underscore here is only correct because externs only occur
in STABS on Mac, where they have an underscore. I have a feeling this is going
to bite us later, but it's fine for now.

http://breakpad.appspot.com/267001/diff/1/10
File src/common/stabs_to_module_unittest.cc (right):

http://breakpad.appspot.com/267001/diff/1/10#newcode83
Line 83: EXPECT_TRUE(h.Extern("__Z21dyldGlobalLockAcquirev", 0xaaaa));
Isn't this the first dependency on mangling syntax in the test suite? I don't
think we can really test that portably.

[jimb]

Also, what did you think of my suggestion that the ExternSet simply *own* the
Externs, instead of holding pointers to them?

[Ted Mielczarek]

On 2011/03/02 22:50:59, jimb wrote:
> Also, what did you think of my suggestion that the ExternSet simply *own* the
> Externs, instead of holding pointers to them?

I looked at that, but that goes against the style of everything else in the
Module class, and it felt wrong. I could refactor the entire class to copy
instead of owning pointers, but that seemed like a lot of churn. I'd be happy to
file a bug to do that as follow-up work, but it didn't seem worthwhile to fold
into this patch.

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

http://breakpad.appspot.com/267001/diff/1/2#newcode235
Line 235: if (ext->address != last_address) {
On 2011/03/02 22:08:00, jimb wrote:
> If we're just going to drop duplicate symbols with the same address, then why
> shouldn't we just omit them from the set in the first place, by making the
> comparison function ignore the symbol name?

Yes, that probably makes more sense. I think I just wrote the test because I was
surprised by the existing behavior and wanted it tested. The only change in
behavior would be that we might pick a different symbol to output if there are
multiple names for an address, but that doesn't seem harmful.

http://breakpad.appspot.com/267001/diff/1/10
File src/common/stabs_to_module_unittest.cc (right):

http://breakpad.appspot.com/267001/diff/1/10#newcode83
Line 83: EXPECT_TRUE(h.Extern("__Z21dyldGlobalLockAcquirev", 0xaaaa));
On 2011/03/02 22:08:00, jimb wrote:
> Isn't this the first dependency on mangling syntax in the test suite? I don't
> think we can really test that portably.

You are correct. I did want to test that, since it's something we do, but I
could make that #ifdef an appropriate GCC version if that would make you feel
better. (There's not a preprocessor symbol that indicates that the compiler is
using this particular C++ ABI, is there?)

[Ted Mielczarek]

http://breakpad.appspot.com/267001/diff/1/10
File src/common/stabs_to_module_unittest.cc (right):

http://breakpad.appspot.com/267001/diff/1/10#newcode83
Line 83: EXPECT_TRUE(h.Extern("__Z21dyldGlobalLockAcquirev", 0xaaaa));
On 2011/03/02 22:08:00, jimb wrote:
> Isn't this the first dependency on mangling syntax in the test suite? I don't
> think we can really test that portably.

Oh, no, actually, your existing tests have function name demangling tests:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/common/stabs...

So this isn't any worse than what we already have.

[jimb]

On 2011/03/03 18:36:49, Ted Mielczarek wrote:
> On 2011/03/02 22:50:59, jimb wrote:
> > Also, what did you think of my suggestion that the ExternSet simply *own*
the
> > Externs, instead of holding pointers to them?
> 
> I looked at that, but that goes against the style of everything else in the
> Module class, and it felt wrong. I could refactor the entire class to copy
> instead of owning pointers, but that seemed like a lot of churn. I'd be happy
to
> file a bug to do that as follow-up work, but it didn't seem worthwhile to fold
> into this patch.

All right. I don't think it's that important; I just like avoiding pointers.

> http://breakpad.appspot.com/267001/diff/1/10
> File src/common/stabs_to_module_unittest.cc (right):
> 
> http://breakpad.appspot.com/267001/diff/1/10#newcode83
> Line 83: EXPECT_TRUE(h.Extern("__Z21dyldGlobalLockAcquirev", 0xaaaa));
> On 2011/03/02 22:08:00, jimb wrote:
> > Isn't this the first dependency on mangling syntax in the test suite? I
don't
> > think we can really test that portably.
> 
> You are correct. I did want to test that, since it's something we do, but I
> could make that #ifdef an appropriate GCC version if that would make you feel
> better. (There's not a preprocessor symbol that indicates that the compiler is
> using this particular C++ ABI, is there?)

Well, versions of GCC that are old enough to use a different ABI on Linux or Mac
are probably too old to compile the code at all; I believe the _Z mangling was
introduced in G++ 3.0. So probably an #ifdef __GNUC__ is enough.

[jimb]

On 2011/03/03 19:37:11, Ted Mielczarek wrote:
> http://breakpad.appspot.com/267001/diff/1/10
> File src/common/stabs_to_module_unittest.cc (right):
> 
> http://breakpad.appspot.com/267001/diff/1/10#newcode83
> Line 83: EXPECT_TRUE(h.Extern("__Z21dyldGlobalLockAcquirev", 0xaaaa));
> On 2011/03/02 22:08:00, jimb wrote:
> > Isn't this the first dependency on mangling syntax in the test suite? I
don't
> > think we can really test that portably.
> 
> Oh, no, actually, your existing tests have function name demangling tests:

Oh. [embarrassed]

Could you put that in an #ifdef __GNUC__?