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

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 <redacted>
---
 tools/perf/util/probe-finder.c | 59 +++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 24 deletions(-)

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;

-- 
1.9.3

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 =3D=3D SHT_PROGBITS) {
+		pf->cfi =3D 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

RnJvbTogTWFyayBXaWVsYWFyZCBbbWFpbHRvOm1qd0ByZWRoYXQuY29tXQ0KPg0KPkhpIEhlbWFu
dCwNCj4NCj5PbiBUaHUsIDIwMTUtMDktMjQgYXQgMDc6NDYgKzA1MzAsIEhlbWFudCBLdW1hciB3
cm90ZToNCj4+IHBlcmYgcHJvYmUgdGhyb3VnaCBkZWJ1Z2luZm9fX2ZpbmRfcHJvYmVzKCkgaW4g
dXRpbC9wcm9iZS1maW5kZXIuYw0KPj4gY2hlY2tzIGZvciB0aGUgZnVuY3Rpb25zJyBmcmFtZSBk
ZXNjcmlwdGlvbnMgaW4gZWl0aGVyIC5laF9mcmFtZSBzZWN0aW9uDQo+PiBvZiBhbiBFTEYgb3Ig
dGhlIC5kZWJ1Z19mcmFtZS4gVGhlIGNoZWNrIGlzIGJhc2VkIG9uIHdoZXRoZXIgZWl0aGVyIG9u
ZQ0KPj4gb2YgdGhlc2Ugc2VjdGlvbnMgaXMgcHJlc2VudC4gQnV0IHNvbWV0aW1lcywgaXQgbWF5
IGhhcHBlbiB0aGF0LA0KPj4gLmVoX2ZyYW1lLCBldmVuIGlmIHByZXNlbnQsIG1heSBub3QgYmUg
Y29tcGxldGUgYW5kIG1heSBtaXNzIHNvbWUNCj4+IGRlc2NyaXB0aW9ucy4NCj4NCj5SaWdodC4g
RGVwZW5kaW5nIG9uIGRpc3RybywgdG9vbGNoYWluIGRlZmF1bHRzLCBhcmNoLCBidWlsZCBmbGFn
cywgZXRjLg0KPkNGSSBtaWdodCBiZSBmb3VuZCBpbiBlaXRoZXIgLmVoX2ZyYW1lIGFuZC9vciAu
ZGVidWdfZnJhbWUuIFRvIGJlIHN1cmUNCj55b3UgZmluZCB0aGUgQ0ZJIGNvdmVyaW5nIGFuIGFk
ZHJlc3MgeW91IHdpbGwgYWx3YXlzIGhhdmUgdG8gaW52ZXN0aWdhdGUNCj5ib3RoIGlmIGF2YWls
YWJsZS4NCg0KT0ssIEkgZGlkbid0IGNhcmUgYWJvdXQgdGhhdC4NCg0KPg0KPkkgYW0gbm90IHRv
byBmYW1pbGlhciB3aXRoIHRoZSBjb2RlIHNvIHRoZXJlIG1pZ2h0IGJlIGEgcmVhc29uIGZvcg0K
PnNldHRpbmcgYW5kIHJldXNpbmcgdGhlIHBmLT5jZmkgdG8gZG8gdGhlIHNlYXJjaCB0d2ljZS4g
QnV0IG1pZ2h0IGl0IG5vdA0KPmJlIG1vcmUgY2xlYXIgdG8ganVzdCBzdG9yZSBib3RoIHBmLT5j
ZmlfZWggYW5kIHBmLT5jZmlfZGVidWcgYW5kIHRoZW4NCj5jaGVjayBib3RoIGluIGNhbGxfcHJv
YmVfZmluZGVyICgpIHdpdGggdGhlIGR3YXJmX2NmaV9hZGRyZnJhbWUgKCkgY2FsbD8NCj5XaGlj
aCBpcyB0aGUgb25seSBwbGFjZSBJIHNlZSBhY3R1YWxseSB1c2luZyB0aGUgY2ZpLg0KDQpSaWdo
dCwgYnV0IHNpbmNlIGNhbGxfcHJvYmVfZmluZGVyIGNhbiBiZSBjYWxsZWQgcmVwZWF0ZWRseSBv
biBzYW1lIGJpbmFyeSwNCndlIHNob3VsZCBrZWVwIHBmLT5jZmkgZm9yIGNhY2hpbmcgQ0ZJIHRv
by4NCg0KPkJUVy4gTm90IHJlYWxseSByZWxhdGVkIHRvIHRoaXMgcGF0Y2ggc2luY2UgdGhlIGZv
bGxvd2luZyB3YXMgYWxyZWFkeSBpbg0KPnRoZSBjb2RlLCBhbmQgaXMgbW9zdCBsaWtlbHkgYWx3
YXlzIGNvcnJlY3QgYW55d2F5Og0KPg0KPj4gKwlpZiAoZWxmX3NlY3Rpb25fYnlfbmFtZShlbGYs
ICZlaGRyLCAmc2hkciwgIi5laF9mcmFtZSIsIE5VTEwpICYmDQo+PiArCSAgICBzaGRyLnNoX3R5
cGUgPT0gU0hUX1BST0dCSVRTKSB7DQo+PiArCQlwZi0+Y2ZpID0gZHdhcmZfZ2V0Y2ZpX2VsZihl
bGYpOw0KPg0KPkJ1dCB0aGF0IFNIVF9QUk9HQklUUyBjaGVjayBpcyBvbmx5IG5lY2Vzc2FyeSBi
ZWNhdXNlIG9mIGEgYnVnIGluDQo+ZWxmdXRpbHMgPCAwLjE1Ni4gRm9yIDAuMTU2KyBkd2FyZl9n
ZXRjZmlfZWxmICgpIHdpbGwgcHJvcGVybHkgcmV0dXJuDQo+TlVMTCBpbiBjYXNlIHlvdSBoYXBw
ZW4gdG8gYmUgbG9va2luZyBhdCBhIHNlcGFyYXRlIGRlYnVnIGZpbGUgdGhhdA0KPmhhcyAuZWhf
ZnJhbWUgYXMgTk9CSVRTLiBJbiB0aGVvcnkgdGhpcyBwcmV2ZW50cyBnZXR0aW5nIHRoZSBDRkkg
aWYgdGhlDQo+ZmlsZSBoYXMgc3RyaXBwZWQgYXdheSB0aGUgc2hkcnMuIFdoaWNoIGlzIHJlYXNv
bmFibGUsIHRoZXJlIGFyZQ0KPnByb2JhYmx5IGFsc28gb3RoZXIgdGhpbmdzIHRoYXQgcmVseSBv
biB0aGUgc2hkcnMuDQoNCkFoLCBJIGhhZCBqdXN0IHdhbnRlZCB0byBhdm9pZCBpbnRyb2R1Y2lu
ZyBuZXcgaWZkZWZzLg0KDQo+IEJ1dCBkd2FyZl9nZXRjZmlfZWxmDQo+aXMgYWJsZSB0byBhbHNv
IGdldCB5b3UgdGhlIENGSSB3aXRoIGp1c3QgdGhlIHBoZHJzLg0KDQpIbW0sIGhvdyBjYW4gSSBt
YWtlIHN1Y2ggYmluYXJ5PyBJIGNhbiBmaXggaXQsIGJ1dCB3ZSBuZWVkIGENCnRlc3RjYXNlIGZv
ciB0aGF0Lg0KDQpUaGFua3MhDQoNCj4NCj5DaGVlcnMsDQo+DQo+TWFyaw0K

On Thu, 2015-09-24 at 10:56 +0000, =E5=B9=B3=E6=9D=BE=E9=9B=85=E5=B7=B3 / H=
IRAMATU=EF=BC=8CMASAMI wrote:

quoted

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.

=20
Right, but since call_probe_finder can be called repeatedly on same binar=

y,

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.

quoted

BTW. Not really related to this patch since the following was already in
the code, and is most likely always correct anyway:

quoted

+	if (elf_section_by_name(elf, &ehdr, &shdr, ".eh_frame", NULL) &&
+	    shdr.sh_type =3D=3D SHT_PROGBITS) {
+		pf->cfi =3D 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.

=20
Ah, I had just wanted to avoid introducing new ifdefs.

Yes, understandable. It is a weird corner case anyway.

quoted

But dwarf_getcfi_elf
is able to also get you the CFI with just the phdrs.

=20
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