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

[Mark Wielaard]

On Thu, 2015-09-24 at 10:56 +, 平松雅巳 / 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, , , ".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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[平松雅巳 / HIRAMATU，MASAMI]

From: Mark Wielaard [mailto:m...@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, , , ".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
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

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

[Mark Wielaard]

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, , , ".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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Mark Wielaard]

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, , , ".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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[平松雅巳 / HIRAMATU，MASAMI]

From: Mark Wielaard [mailto:m...@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, , , ".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
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

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

[Mark Wielaard]

On Thu, 2015-09-24 at 10:56 +, 平松雅巳 / 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, , , ".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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Hemant Kumar]

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  : 0x17b8

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 :

00d8 0024  FDE cie= pc=17b8..1838
  DW_CFA_advance_loc: 16 to 17c8
  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 0x17b8
  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 
---
 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 = >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, ) == NULL)
-   return -EINVAL;
-
-   if (elf_section_by_name(elf, , , ".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, ) == NULL)
+   return -EINVAL;
+
+   if (elf_section_by_name(elf, , , ".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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Hemant Kumar]

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  : 0x17b8

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 :

00d8 0024  FDE cie= pc=17b8..1838
  DW_CFA_advance_loc: 16 to 17c8
  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 0x17b8
  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 
---
 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 = >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, ) == NULL)
-   return -EINVAL;
-
-   if (elf_section_by_name(elf, , , ".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, ) == NULL)
+   return -EINVAL;
+
+   if (elf_section_by_name(elf, , , ".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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/