Have dump_syms output the full symbol table.

Have dump_syms output the full symbol table.

Some of the symbols in the stack trace are not found in the .dynsym
section but were located in the full symbol table .symtab section
instead. This was causing some of our stack traces to be incomplete or
point to incorrect function names.

Since we only output function names, there are actually not that many
more symbols located in .symtab that aren't in .dynsym. It is better to
simply output all symbols found so our stack traces are complete.

R=mark@chromium.org, thestig@chromium.org
BUG=561447
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/512cac3a1b69721ab727f3079f4d29e4580467b1

[David Yen, 2016-03-22 18:20:34]

Description was changed from

==========
Have dump_syms output the full symbol table.

Some of the symbols in the stack trace are not found in the .dynsym
section but were located in the full symbol table .symtab section
instead. This was causing some of our stack traces to be incomplete or
point to incorrect function names.

Since we only output function names, there are actually not that many
more symbols located in .symtab that aren't in .dynsym. It is better to
simply output all symbols found so our stack traces are complete.

R=thestig@chromium.org
BUG=561447
==========

to

==========
Have dump_syms output the full symbol table.

Some of the symbols in the stack trace are not found in the .dynsym
section but were located in the full symbol table .symtab section
instead. This was causing some of our stack traces to be incomplete or
point to incorrect function names.

Since we only output function names, there are actually not that many
more symbols located in .symtab that aren't in .dynsym. It is better to
simply output all symbols found so our stack traces are complete.

R=thestig@chromium.org
BUG=561447
==========

[David Yen, 2016-03-22 18:20:34]

dyen@chromium.org changed reviewers:
+ mark@chromium.org

[David Yen, 2016-03-22 18:20:54]

On 2016/03/22 18:20:07, David Yen wrote:

+mark@ since thestig@ is OOO.

[Mark Mentovai, 2016-03-22 21:03:11]

I thought that .dynsym and .symtab didn’t duplicate each other, at least not
fully, and would have expected that if we wanted to look at .symtab, we’d need
to look at it in addition to .dynsym, not instead of .dynsym. Is my
understanding incorrect?

https://codereview.chromium.org/1824063002/diff/1/src/common/linux/dump_symbo...
File src/common/linux/dump_symbols.cc (right):

https://codereview.chromium.org/1824063002/diff/1/src/common/linux/dump_symbo...
src/common/linux/dump_symbols.cc:724: info->LoadedSection(".symtab");
.dynsym?

[David Yen, 2016-03-22 21:14:08]

From my understanding .dynsym is a strict subset of .symtab. The .symtab section
is the full symbol table, while .dynsym is the global symbols for dynamic
linking.

Here is the description from the man page:
http://man7.org/linux/man-pages/man5/elf.5.html

SHT_SYMTAB: This section holds a symbol table. Typically, SHT_SYMTAB provides
symbols for link editing, though it may also be used for dynamic linking.  As a
complete symbol table, it may contain many symbols unnecessary for dynamic
linking.  An object file can also contain a SHT_DYNSYM section.

https://codereview.chromium.org/1824063002/diff/1/src/common/linux/dump_symbo...
File src/common/linux/dump_symbols.cc (right):

https://codereview.chromium.org/1824063002/diff/1/src/common/linux/dump_symbo...
src/common/linux/dump_symbols.cc:724: info->LoadedSection(".symtab");
On 2016/03/22 21:03:11, Mark Mentovai wrote:
> .dynsym?

Oops, good catch. Done.

[Mark Mentovai, 2016-03-22 22:03:14]

In my experience, I see symbols with “nm -D” that I don’t see with just “nm”. Is
your experience different? Or do you have a different explanation for that than
the difference between .symtab and .dynsym?

[David Yen, 2016-03-22 22:11:52]

On 2016/03/22 22:03:14, Mark Mentovai wrote:
> In my experience, I see symbols with “nm -D” that I don’t see with just “nm”.
Is
> your experience different? Or do you have a different explanation for that
than
> the difference between .symtab and .dynsym?

I can't say I use nm enough for that purpose to have seen that, but from the
docs my understanding is that it shouldn't happen.

I just tried testing this with libcontent.so and chrome in python and it looks
like there are no extra symbols. Here is what I ran:

>>> import subprocess
>>> set(subprocess.check_output(['nm', '-D', 'libcontent.so']).splitlines()) -
set(subprocess.check_output(['nm', 'libcontent.so']).splitlines())
set([])
>>> set(subprocess.check_output(['nm', '-D', 'chrome']).splitlines()) -
set(subprocess.check_output(['nm', 'chrome']).splitlines())
set([])

[Mark Mentovai, 2016-03-22 22:38:35]

$ nm /lib/x86_64-linux-gnu/libc.so.6 | wc -l
nm: /lib/x86_64-linux-gnu/libc.so.6: no symbols
0
$ nm -D /lib/x86_64-linux-gnu/libc.so.6 | wc -l
2222

[Mark Mentovai, 2016-03-22 22:40:24]

Oh, but I guess there’s no .symtab at all in that case, so your approach should
still work…

[David Yen, 2016-03-22 22:42:06]

On 2016/03/22 22:40:24, Mark Mentovai wrote:
> Oh, but I guess there’s no .symtab at all in that case, so your approach
should
> still work…

Ah yes, I think the only case is when symbols were stripped. In that case,
.symtab is the section that is stripped and only the symbols necessary for
runtime will be there (.dynsym). Perhaps that is the case you were thinking of.

[Mark Mentovai, 2016-03-22 22:46:42]

https://codereview.chromium.org/1824063002/diff/20001/src/common/linux/dump_s...
File src/common/linux/dump_symbols.cc (right):

https://codereview.chromium.org/1824063002/diff/20001/src/common/linux/dump_s...
src/common/linux/dump_symbols.cc:696: info->LoadedSection(".symtab");
Can the boy of this conditional be factored out into its own function so that
this and the nearly-identical portion in the else branch can be shared?

[David Yen, 2016-03-23 00:01:05]

https://codereview.chromium.org/1824063002/diff/20001/src/common/linux/dump_s...
File src/common/linux/dump_symbols.cc (right):

https://codereview.chromium.org/1824063002/diff/20001/src/common/linux/dump_s...
src/common/linux/dump_symbols.cc:696: info->LoadedSection(".symtab");
On 2016/03/22 22:46:42, Mark Mentovai wrote:
> Can the boy of this conditional be factored out into its own function so that
> this and the nearly-identical portion in the else branch can be shared?

I thought about that originally, but I was thinking I would need to pass too
many variables. I suspect that is why the other sections are mostly duplicated
as well.

I have refactored the code into it's own function in patch #3, perhaps you can
take a look and see which you prefer.

[Mark Mentovai, 2016-03-23 15:32:50]

Yuck, that is too many. LGTM to #2 then.

[David Yen, 2016-03-23 16:51:37]

On 2016/03/23 15:32:50, Mark Mentovai wrote:
> Yuck, that is too many. LGTM to #2 then.

Thanks! I reverted to #2 and added a fix I found into #4. I'm actually not a
committer, could you land this for me?

[Mark Mentovai, 2016-03-23 17:17:47]

Description was changed from

==========
Have dump_syms output the full symbol table.

Some of the symbols in the stack trace are not found in the .dynsym
section but were located in the full symbol table .symtab section
instead. This was causing some of our stack traces to be incomplete or
point to incorrect function names.

Since we only output function names, there are actually not that many
more symbols located in .symtab that aren't in .dynsym. It is better to
simply output all symbols found so our stack traces are complete.

R=thestig@chromium.org
BUG=561447
==========

to

==========
Have dump_syms output the full symbol table.

Some of the symbols in the stack trace are not found in the .dynsym
section but were located in the full symbol table .symtab section
instead. This was causing some of our stack traces to be incomplete or
point to incorrect function names.

Since we only output function names, there are actually not that many
more symbols located in .symtab that aren't in .dynsym. It is better to
simply output all symbols found so our stack traces are complete.

R=mark@chromium.org, thestig@chromium.org
BUG=561447

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/512cac3a1b69721ab727f30...
==========

[Mark Mentovai, 2016-03-23 17:17:48]

Committed patchset #4 (id:60001) manually as
512cac3a1b69721ab727f3079f4d29e4580467b1 (presubmit successful).