Issue 2844002: Follow debug link correctly (Closed)
|
Created:
9 years, 2 months ago by hashimoto Modified:
9 years, 2 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionFollow debug link correctly As thestig@chromium.org pointed out in https://breakpad.appspot.com/9684002, LoadSymbols() should return false if |read_gnu_debug_link| is false. BUG=chromium:453498 R=thestig@chromium.org Committed: https://code.google.com/p/google-breakpad/source/detail?r=1422 Patch Set 1 : #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 4
MessagesTotal messages: 16
Ted, could you review this change?
Sign in to reply to this message.
In the case with a debug link, there seem to be function name duplication. e.g. with dump_syms /lib/x86_64-linux-gnu/libc-2.19.so /usr/lib/debug/lib/x86_64-linux-gnu on Ubuntu 14.04, __libc_init_first appears twice in the output: FUNC 21c10 2 0 __libc_init_first PUBLIC 21c10 0 __libc_init_first
Sign in to reply to this message.
I'm guessing with your CL, the PUBLIC entries are getting added first, so the deduplication code in Module::AddExtern() no longer works.
Sign in to reply to this message.
On 2015/02/03 01:28:12, Lei Zhang (chromium) wrote: > I'm guessing with your CL, the PUBLIC entries are getting added first, so the > deduplication code in Module::AddExtern() no longer works. I also found some duplicates from before this patch. Some FUNC and PUBLIC entries have the same address, but different names. In this case, the processor prefers the FUNC entry, so the PUBLIC entry is probably not useful.
Sign in to reply to this message.
It’s best for PUBLIC to not appear when FUNC appears. It’s true that there’s minimal difference for things that occur inside functions (except FUNC provides line number information). However, FUNCs come with sizes, and PUBLICs do not. It’s possible for the processor to map a far-off offset in a module to map to the highest PUBLIC below it, which might be quite a distance away. When a FUNC is available, it specifies the true limit of the function, and anything beyond that limit will not be mapped to the function. Providing a PUBLIC in addition to a FUNC negates this behavior.
Sign in to reply to this message.
On 2015/02/03 02:01:39, Mark Mentovai wrote: > It’s best for PUBLIC to not appear when FUNC appears. It’s true that there’s > minimal difference for things that occur inside functions (except FUNC provides > line number information). However, FUNCs come with sizes, and PUBLICs do not. > It’s possible for the processor to map a far-off offset in a module to map to > the highest PUBLIC below it, which might be quite a distance away. When a FUNC > is available, it specifies the true limit of the function, and anything beyond > that limit will not be mapped to the function. Providing a PUBLIC in addition to > a FUNC negates this behavior. Ok, we should revert the module.cc change from r1400, and then modify Module::AddFunction() to delete PUBLIC entries that have the same address as FUNC entries.
Sign in to reply to this message.
PTAL Reverted r1400's module.cc change and modified Module::AddFunction() to delete PUBLIC entries. Verified that the same name is not present in PUBLIC and FUNC lines with libc-2.19.so of daisy (arm) and peppy (amd64).
Sign in to reply to this message.
https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc File src/common/module.cc (right): https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc#newcode86 src/common/module.cc:86: ExternSet::iterator it_ext = externs_.lower_bound(&ext); Can we just do this instead? if (externs_.find(&ext) != externs_.end()) { // delete... } It doesn't make sense for there to be publicly visible linker symbol that points into the middle of a function.
Sign in to reply to this message.
https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc File src/common/module.cc (right): https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc#newcode86 src/common/module.cc:86: ExternSet::iterator it_ext = externs_.lower_bound(&ext); On 2015/02/03 06:13:51, Lei Zhang (chromium) wrote: > Can we just do this instead? > > if (externs_.find(&ext) != externs_.end()) { > // delete... > } > > It doesn't make sense for there to be publicly visible linker symbol that points > into the middle of a function. With libc-2.19.so of daisy (an ARM device), I got these lines: FUNC 161c0 2 0 __libc_init_first PUBLIC 161c1 0 __libc_init_first Can it be harmful to have this logic?
Sign in to reply to this message.
lgtm I'm worried the Breakpad symbols files generated with this patch might be slightly wrong, but it's still way better than the existing code that's missing all the FILE/FUNC lines. https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc File src/common/module.cc (right): https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc#newcode86 src/common/module.cc:86: ExternSet::iterator it_ext = externs_.lower_bound(&ext); On 2015/02/03 06:37:06, hashimoto wrote: > On 2015/02/03 06:13:51, Lei Zhang (chromium) wrote: > > Can we just do this instead? > > > > if (externs_.find(&ext) != externs_.end()) { > > // delete... > > } > > > > It doesn't make sense for there to be publicly visible linker symbol that > points > > into the middle of a function. > With libc-2.19.so of daisy (an ARM device), I got these lines: > FUNC 161c0 2 0 __libc_init_first > PUBLIC 161c1 0 __libc_init_first > > Can it be harmful to have this logic? Here, with an ARM libc-2.19.so and its .debug file from November 2014, I get 161c1 for both libc-2.19.so and libc-2.19.so.debug with readelf. I also see FUNC 161c0 when I run dump_syms on it. Not sure what's wrong there, but I will investigate. Can you just put a TODO(thestig) here? I will take a look tomorrow.
Sign in to reply to this message.
Message was sent while issue was closed.
Committed patchset #4 (id:130001) manually as r1422 (presubmit successful).
Sign in to reply to this message.
Message was sent while issue was closed.
https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc File src/common/module.cc (right): https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc#newcode86 src/common/module.cc:86: ExternSet::iterator it_ext = externs_.lower_bound(&ext); On 2015/02/03 07:14:53, Lei Zhang (chromium) wrote: > On 2015/02/03 06:37:06, hashimoto wrote: > > On 2015/02/03 06:13:51, Lei Zhang (chromium) wrote: > > > Can we just do this instead? > > > > > > if (externs_.find(&ext) != externs_.end()) { > > > // delete... > > > } > > > > > > It doesn't make sense for there to be publicly visible linker symbol that > > points > > > into the middle of a function. > > With libc-2.19.so of daisy (an ARM device), I got these lines: > > FUNC 161c0 2 0 __libc_init_first > > PUBLIC 161c1 0 __libc_init_first > > > > Can it be harmful to have this logic? > > Here, with an ARM libc-2.19.so and its .debug file from November 2014, I get > 161c1 for both libc-2.19.so and libc-2.19.so.debug with readelf. I also see FUNC > 161c0 when I run dump_syms on it. Not sure what's wrong there, but I will > investigate. Both "arm-linux-gnueabihf-objdump -d" and "readelf -w" say __libc_init_first starts from 161c0, but "readelf -s" says it's on 161c1. Thank you for handling this. > > Can you just put a TODO(thestig) here? I will take a look tomorrow. Oops, sorry I forgot to put TODO.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2015/02/03 07:50:18, hashimoto wrote: > https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc > File src/common/module.cc (right): > > https://breakpad.appspot.com/2844002/diff/130001/src/common/module.cc#newcode86 > src/common/module.cc:86: ExternSet::iterator it_ext = > externs_.lower_bound(&ext); > On 2015/02/03 07:14:53, Lei Zhang (chromium) wrote: > > On 2015/02/03 06:37:06, hashimoto wrote: > > > On 2015/02/03 06:13:51, Lei Zhang (chromium) wrote: > > > > Can we just do this instead? > > > > > > > > if (externs_.find(&ext) != externs_.end()) { > > > > // delete... > > > > } > > > > > > > > It doesn't make sense for there to be publicly visible linker symbol that > > > points > > > > into the middle of a function. > > > With libc-2.19.so of daisy (an ARM device), I got these lines: > > > FUNC 161c0 2 0 __libc_init_first > > > PUBLIC 161c1 0 __libc_init_first > > > > > > Can it be harmful to have this logic? > > > > Here, with an ARM libc-2.19.so and its .debug file from November 2014, I get > > 161c1 for both libc-2.19.so and libc-2.19.so.debug with readelf. I also see > FUNC > > 161c0 when I run dump_syms on it. Not sure what's wrong there, but I will > > investigate. > Both "arm-linux-gnueabihf-objdump -d" and "readelf -w" say __libc_init_first > starts from 161c0, but "readelf -s" says it's on 161c1. > Thank you for handling this. hamaji@chromium.org told me that ARM uses the lsb of the function address to enter Thumb mode. So when the CPU is given 161c1 as the address value while branching, PC is set to 161c0 and it enters Thumb state. Probably the easiest solution might be unconditionally clearing LSB of all addresses. > > > > Can you just put a TODO(thestig) here? I will take a look tomorrow. > > Oops, sorry I forgot to put TODO.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2015/02/03 08:47:22, hashimoto wrote: > hamaji@chromium.org told me that ARM uses the lsb of the function address to > enter Thumb mode. > So when the CPU is given 161c1 as the address value while branching, PC is set > to 161c0 and it enters Thumb state. > Probably the easiest solution might be unconditionally clearing LSB of all > addresses. Yep, the DWARF data that dump_syms reads does say 161c0, whereas the ELF .symtab and .dynsym say 161c1. The ARM ELF ABI says: ---- In addition to the normal rules for symbol values the following rules shall also apply to symbols of type STT_FUNC: - If the symbol addresses an ARM instruction, its value is the address of the instruction (in a relocatable object, the offset of the instruction from the start of the section containing it). - If the symbol addresses a Thumb instruction, its value is the address of the instruction with bit zero set (in a relocatable object, the section offset with bit zero set). ---- So if we are on ARM and we encounter an odd address, we should fix the offset. > Oops, sorry I forgot to put TODO. No worries.
Sign in to reply to this message.
|
