| Message ID | 1443060990-10313-1-git-send-email-hemant@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Michael Ellerman |
| Headers | show |
Hi Hemant, On Thu, 2015-09-24 at 07:46 +0530, Hemant Kumar wrote: > perf probe through debuginfo__find_probes() in util/probe-finder.c > checks for the functions' frame descriptions in either .eh_frame section > of an ELF or the .debug_frame. The check is based on whether either one > of these sections is present. But sometimes, it may happen that, > .eh_frame, even if present, may not be complete and may miss some > descriptions. Right. Depending on distro, toolchain defaults, arch, build flags, etc. CFI might be found in either .eh_frame and/or .debug_frame. To be sure you find the CFI covering an address you will always have to investigate both if available. I am not too familiar with the code so there might be a reason for setting and reusing the pf->cfi to do the search twice. But might it not be more clear to just store both pf->cfi_eh and pf->cfi_debug and then check both in call_probe_finder () with the dwarf_cfi_addrframe () call? Which is the only place I see actually using the cfi. BTW. Not really related to this patch since the following was already in the code, and is most likely always correct anyway: > + if (elf_section_by_name(elf, &ehdr, &shdr, ".eh_frame", NULL) && > + shdr.sh_type == SHT_PROGBITS) { > + pf->cfi = dwarf_getcfi_elf(elf); But that SHT_PROGBITS check is only necessary because of a bug in elfutils < 0.156. For 0.156+ dwarf_getcfi_elf () will properly return NULL in case you happen to be looking at a separate debug file that has .eh_frame as NOBITS. In theory this prevents getting the CFI if the file has stripped away the shdrs. Which is reasonable, there are probably also other things that rely on the shdrs. But dwarf_getcfi_elf is able to also get you the CFI with just the phdrs. Cheers, Mark
From: Mark Wielaard [mailto:mjw@redhat.com] > >Hi Hemant, > >On Thu, 2015-09-24 at 07:46 +0530, Hemant Kumar wrote: >> perf probe through debuginfo__find_probes() in util/probe-finder.c >> checks for the functions' frame descriptions in either .eh_frame section >> of an ELF or the .debug_frame. The check is based on whether either one >> of these sections is present. But sometimes, it may happen that, >> .eh_frame, even if present, may not be complete and may miss some >> descriptions. > >Right. Depending on distro, toolchain defaults, arch, build flags, etc. >CFI might be found in either .eh_frame and/or .debug_frame. To be sure >you find the CFI covering an address you will always have to investigate >both if available. OK, I didn't care about that. > >I am not too familiar with the code so there might be a reason for >setting and reusing the pf->cfi to do the search twice. But might it not >be more clear to just store both pf->cfi_eh and pf->cfi_debug and then >check both in call_probe_finder () with the dwarf_cfi_addrframe () call? >Which is the only place I see actually using the cfi. Right, but since call_probe_finder can be called repeatedly on same binary, we should keep pf->cfi for caching CFI too. >BTW. Not really related to this patch since the following was already in >the code, and is most likely always correct anyway: > >> + if (elf_section_by_name(elf, &ehdr, &shdr, ".eh_frame", NULL) && >> + shdr.sh_type == SHT_PROGBITS) { >> + pf->cfi = dwarf_getcfi_elf(elf); > >But that SHT_PROGBITS check is only necessary because of a bug in >elfutils < 0.156. For 0.156+ dwarf_getcfi_elf () will properly return >NULL in case you happen to be looking at a separate debug file that >has .eh_frame as NOBITS. In theory this prevents getting the CFI if the >file has stripped away the shdrs. Which is reasonable, there are >probably also other things that rely on the shdrs. Ah, I had just wanted to avoid introducing new ifdefs. > But dwarf_getcfi_elf >is able to also get you the CFI with just the phdrs. Hmm, how can I make such binary? I can fix it, but we need a testcase for that. Thanks! > >Cheers, > >Mark
On Thu, 2015-09-24 at 10:56 +0000, 平松雅巳 / HIRAMATU，MASAMI wrote: > >I am not too familiar with the code so there might be a reason for > >setting and reusing the pf->cfi to do the search twice. But might it not > >be more clear to just store both pf->cfi_eh and pf->cfi_debug and then > >check both in call_probe_finder () with the dwarf_cfi_addrframe () call? > >Which is the only place I see actually using the cfi. > > Right, but since call_probe_finder can be called repeatedly on same binary, > we should keep pf->cfi for caching CFI too. Yes. I was suggesting to rename pf->cfi to pf->cfi_eh and add pf->cfi_debug, to make clear why there are two. > >BTW. Not really related to this patch since the following was already in > >the code, and is most likely always correct anyway: > > > >> + if (elf_section_by_name(elf, &ehdr, &shdr, ".eh_frame", NULL) && > >> + shdr.sh_type == SHT_PROGBITS) { > >> + pf->cfi = dwarf_getcfi_elf(elf); > > > >But that SHT_PROGBITS check is only necessary because of a bug in > >elfutils < 0.156. For 0.156+ dwarf_getcfi_elf () will properly return > >NULL in case you happen to be looking at a separate debug file that > >has .eh_frame as NOBITS. In theory this prevents getting the CFI if the > >file has stripped away the shdrs. Which is reasonable, there are > >probably also other things that rely on the shdrs. > > Ah, I had just wanted to avoid introducing new ifdefs. Yes, understandable. It is a weird corner case anyway. > > But dwarf_getcfi_elf > >is able to also get you the CFI with just the phdrs. > > Hmm, how can I make such binary? I can fix it, but we need a > testcase for that. eu-strip --strip-sections can create such binaries. But honestly I wouldn't bother. They are basically useless and nobody (should) do that. The only reason you might want to support them is for getting a backtrace anyway through the CFI. But you won't be able to get any other symbol or debug information from them. I only really mentioned it because I am embarrassed about the bug in elfutils. Just glad that it got fixed and the workaround isn't necessary anymore. But it probably is not worth it now to try to remove the workaround. There is no real benefit in this case. Cheers, Mark
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c index 2da65a7..7ce02b9 100644 --- a/tools/perf/util/probe-finder.c +++ b/tools/perf/util/probe-finder.c @@ -1022,9 +1022,8 @@ static int pubname_search_cb(Dwarf *dbg, Dwarf_Global *gl, void *data) return DWARF_CB_OK; } -/* Find probe points from debuginfo */ -static int debuginfo__find_probes(struct debuginfo *dbg, - struct probe_finder *pf) +static int debuginfo__find_probe_location(struct debuginfo *dbg, + struct probe_finder *pf) { struct perf_probe_point *pp = &pf->pev->point; Dwarf_Off off, noff; @@ -1032,27 +1031,6 @@ static int debuginfo__find_probes(struct debuginfo *dbg, Dwarf_Die *diep; int ret = 0; -#if _ELFUTILS_PREREQ(0, 142) - Elf *elf; - GElf_Ehdr ehdr; - GElf_Shdr shdr; - - /* Get the call frame information from this dwarf */ - elf = dwarf_getelf(dbg->dbg); - if (elf == NULL) - return -EINVAL; - - if (gelf_getehdr(elf, &ehdr) == NULL) - return -EINVAL; - - if (elf_section_by_name(elf, &ehdr, &shdr, ".eh_frame", NULL) && - shdr.sh_type == SHT_PROGBITS) { - pf->cfi = dwarf_getcfi_elf(elf); - } else { - pf->cfi = dwarf_getcfi(dbg->dbg); - } -#endif - off = 0; pf->lcache = intlist__new(NULL); if (!pf->lcache) @@ -1115,6 +1093,39 @@ found: return ret; } +/* Find probe points from debuginfo */ +static int debuginfo__find_probes(struct debuginfo *dbg, + struct probe_finder *pf) +{ + int ret = 0; + +#if _ELFUTILS_PREREQ(0, 142) + Elf *elf; + GElf_Ehdr ehdr; + GElf_Shdr shdr; + + /* Get the call frame information from this dwarf */ + elf = dwarf_getelf(dbg->dbg); + if (elf == NULL) + return -EINVAL; + + if (gelf_getehdr(elf, &ehdr) == NULL) + return -EINVAL; + + if (elf_section_by_name(elf, &ehdr, &shdr, ".eh_frame", NULL) && + shdr.sh_type == SHT_PROGBITS) { + pf->cfi = dwarf_getcfi_elf(elf); + ret = debuginfo__find_probe_location(dbg, pf); + if (ret >= 0) + return ret; + } + pf->cfi = dwarf_getcfi(dbg->dbg); +#endif + + ret = debuginfo__find_probe_location(dbg, pf); + return ret; +} + struct local_vars_finder { struct probe_finder *pf; struct perf_probe_arg *args;
perf probe through debuginfo__find_probes() in util/probe-finder.c checks for the functions' frame descriptions in either .eh_frame section of an ELF or the .debug_frame. The check is based on whether either one of these sections is present. But sometimes, it may happen that, .eh_frame, even if present, may not be complete and may miss some descriptions. For e.g., in powerpc, this may happen : $ gcc -g bin.c -o bin $ objdump --dwarf ./bin <1><145>: Abbrev Number: 7 (DW_TAG_subprogram) <146> DW_AT_external : 1 <146> DW_AT_name : (indirect string, offset: 0x9e): main <14a> DW_AT_decl_file : 1 <14b> DW_AT_decl_line : 39 <14c> DW_AT_prototyped : 1 <14c> DW_AT_type : <0x57> <150> DW_AT_low_pc : 0x100007b8 If the .eh_frame and .debug_frame are checked for the same binary, we will find that, .eh_frame (although present) doesn't contain a description for "main" function. But, .debug_frame has a description : 000000d8 00000024 00000000 FDE cie=00000000 pc=100007b8..10000838 DW_CFA_advance_loc: 16 to 100007c8 DW_CFA_def_cfa_offset: 144 DW_CFA_offset_extended_sf: r65 at cfa+16 ... Due to this (since, perf checks whether .eh_frame is present and goes on searching for that address inside that frame), perf is unable to process the probes : # perf probe -x ./bin main Failed to get call frame on 0x100007b8 Error: Failed to add events. To avoid this issue, we need to check both the sections (.eh_frame and .debug_frame), which is done in this patch. Note that, we can always force everything into both .eh_frame and .debug_frame by : $ gcc bin.c -fasynchronous-unwind-tables -fno-dwarf2-cfi-asm -g -o bin Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com> --- tools/perf/util/probe-finder.c | 59 +++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 24 deletions(-)