Re: [PATCH] perf/probe: Search both .eh_frame and .debug_frame sections for probe location

[Mark Wielaard <mjw@redhat.com>]

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