Handle ARM THUMB functions when removing duplicate PUBLIC entries.

Handle ARM THUMB functions when removing duplicate PUBLIC entries.

In ELF symtab/dynsym sections, THUMB function addresses have bit 0 set,
whereas the DWARF function entries are not.

R=mark@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1423

[Lei Zhang (chromium)]

Follow up to yesterday's CL: https://breakpad.appspot.com/2844002/

[Mark Mentovai]

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

https://breakpad.appspot.com/7774002/diff/1/src/common/module.cc#newcode88
src/common/module.cc:88: // ARM THUMB functions are offset by 1 byte. ARM64 does
not have THUMB.
You don’t actually know it’s THUMB here, you’re just assuming that if you find a
function at offset + 1, it’s far more likely to be THUMB code corresponding to
the same symbol than a symbol for a byte of something that precedes non-THUMB
code. Right?

https://breakpad.appspot.com/7774002/diff/1/src/common/module.cc#newcode98
src/common/module.cc:98: // There should be no other PUBLIC symbols that
overlaps with the function.
overlap

[Lei Zhang (chromium)]

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

https://breakpad.appspot.com/7774002/diff/1/src/common/module.cc#newcode88
src/common/module.cc:88: // ARM THUMB functions are offset by 1 byte. ARM64 does
not have THUMB.
On 2015/02/03 22:34:42, Mark Mentovai wrote:
> You don’t actually know it’s THUMB here, you’re just assuming that if you find
a
> function at offset + 1, it’s far more likely to be THUMB code corresponding to
> the same symbol than a symbol for a byte of something that precedes non-THUMB
> code. Right?

From the libc-2.19.so sample, all the THUMB code were off by 1. The ARM ABI doc
says to look at bit 0, so I changed the CL to do that in patch set 2.

https://breakpad.appspot.com/7774002/diff/1/src/common/module.cc#newcode98
src/common/module.cc:98: // There should be no other PUBLIC symbols that
overlaps with the function.
On 2015/02/03 22:34:42, Mark Mentovai wrote:
> overlap

Done.

[Mark Mentovai]

LGTM

https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc
File src/common/module.cc (right):

https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc#newcode90
src/common/module.cc:90: // ARM THUMB functions have bit 0 set. ARM64 does not
have THUMB.
Oh, this is bit 0 set in the address. Based on what you had written before, I
thought you meant that it was bit 0 set in the instruction. Yeah, this is a very
easy check.

[Lei Zhang (chromium)]

Committed patchset #2 (id:40001) manually as r1423 (presubmit successful).

[hashimoto]

https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc
File src/common/module.cc (right):

https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc#newcode126
src/common/module.cc:126: void Module::AddExtern(Extern *ext) {
How about clearing LSB of Extern's address instead? (here, or at the caller
site)

minidump_stackwalk uses information from PUBLIC lines as the last resort option
to look up the function name.
(https://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/b...)
As the current code may output PUBLIC lines with offset +1, IIUC
minidump_stackwalk may display a wrong function name if the address is at the
start of the function.

[Lei Zhang (chromium)]

On 2015/02/04 10:42:46, hashimoto wrote:
> https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc
> File src/common/module.cc (right):
> 
>
https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc#newcode126
> src/common/module.cc:126: void Module::AddExtern(Extern *ext) {
> How about clearing LSB of Extern's address instead? (here, or at the caller
> site)
> 
> minidump_stackwalk uses information from PUBLIC lines as the last resort
option
> to look up the function name.
>
(https://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/b...)
> As the current code may output PUBLIC lines with offset +1, IIUC
> minidump_stackwalk may display a wrong function name if the address is at the
> start of the function.

Is https://breakpad.appspot.com/2854003 what you are thinking of?

[hashimoto]

On 2015/02/05 23:49:02, Lei Zhang (chromium) wrote:
> On 2015/02/04 10:42:46, hashimoto wrote:
> > https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc
> > File src/common/module.cc (right):
> > 
> >
>
https://breakpad.appspot.com/7774002/diff/40001/src/common/module.cc#newcode126
> > src/common/module.cc:126: void Module::AddExtern(Extern *ext) {
> > How about clearing LSB of Extern's address instead? (here, or at the caller
> > site)
> > 
> > minidump_stackwalk uses information from PUBLIC lines as the last resort
> option
> > to look up the function name.
> >
>
(https://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/b...)
> > As the current code may output PUBLIC lines with offset +1, IIUC
> > minidump_stackwalk may display a wrong function name if the address is at
the
> > start of the function.
> 
> Is https://breakpad.appspot.com/2854003 what you are thinking of?

Thanks, yes, that's what I was thinking.

Or, as AddExtern() is called by only two places (stabs_reader.cc and
elf_symbols_to_module.cc), it can be addressed in the caller site too.
elf_symbols_to_module.cc can clear the lsb of the address for ARM before calling
AddExtern().
I don't know much about stabs_reader.cc, but as what .stab contains is debug
info, I think we don't have to do anything to address values stored in stab
(i.e. I guess an address stored in .stab section is the actual PC value, not the
value decorated with Thumb information in LSB).