[Linux] Force dump_syms to always read public symbols

If we extract symbols from an object with debug info, but some there is no
.debug_info for some public symbols, we miss them.

We now force dump_syms to read public symbol information after reading debug
info.

To avoid name collisions in the symbol file, we only add public symbols if they
don't have debug info.

[Ted Mielczarek]

https://breakpad.appspot.com/9684002/diff/1/common/linux/dump_symbols.cc
File common/linux/dump_symbols.cc (right):

https://breakpad.appspot.com/9684002/diff/1/common/linux/dump_symbols.cc#newc...
common/linux/dump_symbols.cc:715: if (!found_debug_info_section ||
options.read_public_symbols) {
The way you've modified AddExtern it should be safe to always call this code.

https://breakpad.appspot.com/9684002/diff/1/common/module.cc
File common/module.cc (right):

https://breakpad.appspot.com/9684002/diff/1/common/module.cc#newcode106
common/module.cc:106: // mutual exclusive, check if we already have read the
symbol
nit: "mutually"

https://breakpad.appspot.com/9684002/diff/1/tools/linux/dump_syms/dump_syms.cc
File tools/linux/dump_syms/dump_syms.cc (right):

https://breakpad.appspot.com/9684002/diff/1/tools/linux/dump_syms/dump_syms.c...
tools/linux/dump_syms/dump_syms.cc:47: fprintf(stderr, "  -u    Read public
symbols even if debug section is found\n");
I don't think we need an option here, we should just do this by default.

[walac]

On 2014/08/29 00:31:31, Ted Mielczarek wrote:
> https://breakpad.appspot.com/9684002/diff/1/common/linux/dump_symbols.cc
> File common/linux/dump_symbols.cc (right):
> 
>
https://breakpad.appspot.com/9684002/diff/1/common/linux/dump_symbols.cc#newc...
> common/linux/dump_symbols.cc:715: if (!found_debug_info_section ||
> options.read_public_symbols) {
> The way you've modified AddExtern it should be safe to always call this code.
> 
> https://breakpad.appspot.com/9684002/diff/1/common/module.cc
> File common/module.cc (right):
> 
> https://breakpad.appspot.com/9684002/diff/1/common/module.cc#newcode106
> common/module.cc:106: // mutual exclusive, check if we already have read the
> symbol
> nit: "mutually"
> 
> https://breakpad.appspot.com/9684002/diff/1/tools/linux/dump_syms/dump_syms.cc
> File tools/linux/dump_syms/dump_syms.cc (right):
> 
>
https://breakpad.appspot.com/9684002/diff/1/tools/linux/dump_syms/dump_syms.c...
> tools/linux/dump_syms/dump_syms.cc:47: fprintf(stderr, "  -u    Read public
> symbols even if debug section is found\n");
> I don't think we need an option here, we should just do this by default.

Patch set 2 removes the "-u" option and always parse public info. The spelling
error was fixed too.

[Ted Mielczarek]

This needs a few comment changes and one small logic fix and it should be good
to land.

https://breakpad.appspot.com/9684002/diff/110001/common/linux/dump_symbols.cc
File common/linux/dump_symbols.cc (right):

https://breakpad.appspot.com/9684002/diff/110001/common/linux/dump_symbols.cc...
common/linux/dump_symbols.cc:717: // See if there are export symbols available.
This comment is no longer correct since you moved it out of the else block
above. I'd just remove the first line.

https://breakpad.appspot.com/9684002/diff/110001/common/linux/dump_symbols.cc...
common/linux/dump_symbols.cc:748: // the caller doesn't want to use
.gnu_debuglink.
This comment is also no longer correct. You can probably just remove the last
bit of the sentence after the comma.

https://breakpad.appspot.com/9684002/diff/110001/common/linux/dump_symbols.cc...
common/linux/dump_symbols.cc:749: return found_usable_info;
I'm not sure the return value here is correct here given the existing semantics.
I think you need to do something like:
if (read_gnu_debug_link) {
  return found_debug_info_section;
}

If you look at the caller of this function (ReadSymbolDataElfClass):
https://code.google.com/p/google-breakpad/source/browse/trunk/src/common/linu...

you can see how this needs to work.
return found_usable_info;

https://breakpad.appspot.com/9684002/diff/110001/common/module.cc
File common/module.cc (right):

https://breakpad.appspot.com/9684002/diff/110001/common/module.cc#newcode106
common/module.cc:106: // mutually exclusive, check if we already have read the
symbol
I think I pointed this out in bugzilla, but this should be in third-person.

[walac]

On 2014/09/10 18:44:57, Ted Mielczarek wrote:
> This needs a few comment changes and one small logic fix and it should be good
> to land.
> 
> https://breakpad.appspot.com/9684002/diff/110001/common/linux/dump_symbols.cc
> File common/linux/dump_symbols.cc (right):
> 
>
https://breakpad.appspot.com/9684002/diff/110001/common/linux/dump_symbols.cc...
> common/linux/dump_symbols.cc:717: // See if there are export symbols
available.
> This comment is no longer correct since you moved it out of the else block
> above. I'd just remove the first line.
> 
>
https://breakpad.appspot.com/9684002/diff/110001/common/linux/dump_symbols.cc...
> common/linux/dump_symbols.cc:748: // the caller doesn't want to use
> .gnu_debuglink.
> This comment is also no longer correct. You can probably just remove the last
> bit of the sentence after the comma.
> 
>
https://breakpad.appspot.com/9684002/diff/110001/common/linux/dump_symbols.cc...
> common/linux/dump_symbols.cc:749: return found_usable_info;
> I'm not sure the return value here is correct here given the existing
semantics.
> I think you need to do something like:
> if (read_gnu_debug_link) {
>   return found_debug_info_section;
> }
> 
> If you look at the caller of this function (ReadSymbolDataElfClass):
>
https://code.google.com/p/google-breakpad/source/browse/trunk/src/common/linu...
> 
> you can see how this needs to work.
> return found_usable_info;
> 

In this case, I think we need to do in Module::AddFunction the same collision
check we do in AddExtern, don't we?

> https://breakpad.appspot.com/9684002/diff/110001/common/module.cc
> File common/module.cc (right):
> 
> https://breakpad.appspot.com/9684002/diff/110001/common/module.cc#newcode106
> common/module.cc:106: // mutually exclusive, check if we already have read the
> symbol
> I think I pointed this out in bugzilla, but this should be in third-person.

My mistake, this was supposed to be in patch set 3, sorry.

[Ted Mielczarek]

LGTM, I'll land this.

[Lei Zhang (chromium)]

On 2014/10/29 19:26:02, Ted Mielczarek wrote:
> LGTM, I'll land this.

walac: A kind reminder to sign the CLA if you haven't already.
https://cla.developers.google.com/about/google-individual?csw=1

[Ted Mielczarek]

On 2014/10/29 22:36:22, Lei Zhang (chromium) wrote:
> walac: A kind reminder to sign the CLA if you haven't already.
> https://cla.developers.google.com/about/google-individual?csw=1

He's a Mozilla employee, and already covered by our corporate CLA.

[Lei Zhang (chromium)]

On 2014/10/29 22:47:08, Ted Mielczarek wrote:
> On 2014/10/29 22:36:22, Lei Zhang (chromium) wrote:
> > walac: A kind reminder to sign the CLA if you haven't already.
> > https://cla.developers.google.com/about/google-individual?csw=1
> 
> He's a Mozilla employee, and already covered by our corporate CLA.

Great. Thanks. Wasn't obvious from the @gmail.com account.

[Lei Zhang (chromium)]

https://breakpad.appspot.com/9684002/diff/160001/common/linux/dump_symbols.cc
File common/linux/dump_symbols.cc (right):

https://breakpad.appspot.com/9684002/diff/160001/common/linux/dump_symbols.cc...
common/linux/dump_symbols.cc:746: return found_debug_info_section;
This seems wrong. I build dump_syms and now it no longer follows the
.gnu_debug_link. See delete comment in the previous version at line 748.

[Lei Zhang (chromium)]

On 2014/11/05 19:22:44, Lei Zhang (chromium) wrote:
> https://breakpad.appspot.com/9684002/diff/160001/common/linux/dump_symbols.cc
> File common/linux/dump_symbols.cc (right):
> 
>
https://breakpad.appspot.com/9684002/diff/160001/common/linux/dump_symbols.cc...
> common/linux/dump_symbols.cc:746: return found_debug_info_section;
> This seems wrong. I build dump_syms and now it no longer follows the
> .gnu_debug_link. See delete comment in the previous version at line 748.

Err, "deleted"