2021-07-03 15:36:40

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 0/3] perf: Fix perf probe for Fedora34 glibc update

Hi Arnaldo,

Here is a series of patches to fix the perf-probe error against the
Fedora34 glibc update, which moves most of symbols from .symtab to
.dynsym. The key is that the "most of" symbols moved, but it still
have some PLT symbols in .symtab. Thus the perf symbol-elf failes to
decode symbols.

Here is the original report from Thomas about this issue.

https://lore.kernel.org/linux-perf-users/[email protected]/

Thank you,

---

Masami Hiramatsu (3):
perf-probe: Fix debuginfo__new() to enable build-id based debuginfo
perf symbol-elf: Decode dynsym even if symtab exists
perf probe: Do not show @plt function by default


tools/perf/builtin-probe.c | 2 -
tools/perf/util/probe-finder.c | 5 ++
tools/perf/util/symbol-elf.c | 82 ++++++++++++++++++++++++++--------------
3 files changed, 60 insertions(+), 29 deletions(-)

--
Masami Hiramatsu (Linaro) <[email protected]>


2021-07-03 15:36:48

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 1/3] perf-probe: Fix debuginfo__new() to enable build-id based debuginfo

From: Masami Hiramatsu <[email protected]>

Fix debuginfo__new() to set the build-id to dso before
dso__read_binary_type_filename() so that it can find
DSO_BINARY_TYPE__BUILDID_DEBUGINFO debuginfo correctly.
However, this may not change the result, because elfutils
(libdwfl) has its own debuginfo finder. With/without this patch,
the perf probe correctly find the debuginfo file.

This is just a failsafe and keep code's sanity (if you use
dso__read_binary_type_filename(), you must set the build-id
to the dso.)

Reported-by: Thomas Richter <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/perf/util/probe-finder.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index b029c29ce227..02ef0d78053b 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -118,12 +118,17 @@ struct debuginfo *debuginfo__new(const char *path)
char buf[PATH_MAX], nil = '\0';
struct dso *dso;
struct debuginfo *dinfo = NULL;
+ struct build_id bid;

/* Try to open distro debuginfo files */
dso = dso__new(path);
if (!dso)
goto out;

+ /* Set the build id for DSO_BINARY_TYPE__BUILDID_DEBUGINFO */
+ if (is_regular_file(path) && filename__read_build_id(path, &bid) > 0)
+ dso__set_build_id(dso, &bid);
+
for (type = distro_dwarf_types;
!dinfo && *type != DSO_BINARY_TYPE__NOT_FOUND;
type++) {

2021-07-03 15:37:37

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 2/3] perf symbol-elf: Decode dynsym even if symtab exists

In Fedora34, libc-2.33.so has both .dynsym and .symtab sections and
most of (not all) symbols moved to .dynsym. In this case, perf only
decode the symbols in .symtab, and perf probe can not list up the
functions in the library.

To fix this issue, decode both .symtab and .dynsym sections.

Without this fix,
-----
$ ./perf probe -x /usr/lib64/libc-2.33.so -F
@plt
@plt
calloc@plt
free@plt
malloc@plt
memalign@plt
realloc@plt
-----

With this fix.

-----
$ ./perf probe -x /usr/lib64/libc-2.33.so -F
@plt
@plt
a64l
abort
abs
accept
accept4
access
acct
addmntent
-----

Reported-by: Thomas Richter <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/perf/util/symbol-elf.c | 82 ++++++++++++++++++++++++++++--------------
1 file changed, 54 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index a73345730ba9..eb10b4e0d888 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1074,8 +1074,9 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
return 0;
}

-int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
- struct symsrc *runtime_ss, int kmodule)
+static int
+dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
+ struct symsrc *runtime_ss, int kmodule, int dynsym)
{
struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL;
struct maps *kmaps = kmap ? map__kmaps(map) : NULL;
@@ -1098,34 +1099,15 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
if (kmap && !kmaps)
return -1;

- dso->symtab_type = syms_ss->type;
- dso->is_64_bit = syms_ss->is_64_bit;
- dso->rel = syms_ss->ehdr.e_type == ET_REL;
-
- /*
- * Modules may already have symbols from kallsyms, but those symbols
- * have the wrong values for the dso maps, so remove them.
- */
- if (kmodule && syms_ss->symtab)
- symbols__delete(&dso->symbols);
-
- if (!syms_ss->symtab) {
- /*
- * If the vmlinux is stripped, fail so we will fall back
- * to using kallsyms. The vmlinux runtime symbols aren't
- * of much use.
- */
- if (dso->kernel)
- goto out_elf_end;
-
- syms_ss->symtab = syms_ss->dynsym;
- syms_ss->symshdr = syms_ss->dynshdr;
- }
-
elf = syms_ss->elf;
ehdr = syms_ss->ehdr;
- sec = syms_ss->symtab;
- shdr = syms_ss->symshdr;
+ if (dynsym) {
+ sec = syms_ss->dynsym;
+ shdr = syms_ss->dynshdr;
+ } else {
+ sec = syms_ss->symtab;
+ shdr = syms_ss->symshdr;
+ }

if (elf_section_by_name(runtime_ss->elf, &runtime_ss->ehdr, &tshdr,
".text", NULL))
@@ -1312,6 +1294,50 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
return err;
}

+int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
+ struct symsrc *runtime_ss, int kmodule)
+{
+ int nr = 0;
+ int err = -1;
+
+ dso->symtab_type = syms_ss->type;
+ dso->is_64_bit = syms_ss->is_64_bit;
+ dso->rel = syms_ss->ehdr.e_type == ET_REL;
+
+ /*
+ * Modules may already have symbols from kallsyms, but those symbols
+ * have the wrong values for the dso maps, so remove them.
+ */
+ if (kmodule && syms_ss->symtab)
+ symbols__delete(&dso->symbols);
+
+ if (!syms_ss->symtab) {
+ /*
+ * If the vmlinux is stripped, fail so we will fall back
+ * to using kallsyms. The vmlinux runtime symbols aren't
+ * of much use.
+ */
+ if (dso->kernel)
+ return err;
+ } else {
+ err = dso__load_sym_internal(dso, map, syms_ss, runtime_ss,
+ kmodule, 0);
+ if (err < 0)
+ return err;
+ nr = err;
+ }
+
+ if (syms_ss->dynsym) {
+ err = dso__load_sym_internal(dso, map, syms_ss, runtime_ss,
+ kmodule, 1);
+ if (err < 0)
+ return err;
+ err += nr;
+ }
+
+ return err;
+}
+
static int elf_read_maps(Elf *elf, bool exe, mapfn_t mapfn, void *data)
{
GElf_Phdr phdr;

2021-07-03 15:39:46

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 3/3] perf probe: Do not show @plt function by default

From: Masami Hiramatsu <[email protected]>

Fix the perf-probe --functions option do not show the PLT
stub symbols (*@plt) by default.

-----
$ ./perf probe -x /usr/lib64/libc-2.33.so -F | head
a64l
abort
abs
accept
accept4
access
acct
addmntent
addseverity
adjtime
-----

Reported-by: Thomas Richter <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/perf/builtin-probe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index 2bfd41df621c..e1dd51f2874b 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -31,7 +31,7 @@
#include <linux/zalloc.h>

#define DEFAULT_VAR_FILTER "!__k???tab_* & !__crc_*"
-#define DEFAULT_FUNC_FILTER "!_*"
+#define DEFAULT_FUNC_FILTER "!_* & !*@plt"
#define DEFAULT_LIST_FILTER "*"

/* Session management structure */

2021-07-05 12:13:04

by Thomas Richter

[permalink] [raw]
Subject: Re: [PATCH 3/3] perf probe: Do not show @plt function by default

On 7/3/21 5:35 PM, Masami Hiramatsu wrote:
> From: Masami Hiramatsu <[email protected]>
>
> Fix the perf-probe --functions option do not show the PLT
> stub symbols (*@plt) by default.
>
> -----
> $ ./perf probe -x /usr/lib64/libc-2.33.so -F | head
> a64l
> abort
> abs
> accept
> accept4
> access
> acct
> addmntent
> addseverity
> adjtime
> -----
>
> Reported-by: Thomas Richter <[email protected]>
> Signed-off-by: Masami Hiramatsu <[email protected]>
> ---
> tools/perf/builtin-probe.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 2bfd41df621c..e1dd51f2874b 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -31,7 +31,7 @@
> #include <linux/zalloc.h>
>
> #define DEFAULT_VAR_FILTER "!__k???tab_* & !__crc_*"
> -#define DEFAULT_FUNC_FILTER "!_*"
> +#define DEFAULT_FUNC_FILTER "!_* & !*@plt"
> #define DEFAULT_LIST_FILTER "*"
>
> /* Session management structure */
>

Thanks, works again ...

--
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

2021-07-05 17:48:57

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 3/3] perf probe: Do not show @plt function by default

Em Mon, Jul 05, 2021 at 02:11:11PM +0200, Thomas Richter escreveu:
> On 7/3/21 5:35 PM, Masami Hiramatsu wrote:
> > +++ b/tools/perf/builtin-probe.c
> > @@ -31,7 +31,7 @@
> > #include <linux/zalloc.h>

> > #define DEFAULT_VAR_FILTER "!__k???tab_* & !__crc_*"
> > -#define DEFAULT_FUNC_FILTER "!_*"
> > +#define DEFAULT_FUNC_FILTER "!_* & !*@plt"
> > #define DEFAULT_LIST_FILTER "*"

> Thanks, works again ...

I took this as an Acked-by.

Thanks, applied.

- Arnaldo

2021-07-05 20:51:07

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH 0/3] perf: Fix perf probe for Fedora34 glibc update

On Sat, Jul 3, 2021 at 8:36 AM Masami Hiramatsu <[email protected]> wrote:
>
> Hi Arnaldo,
>
> Here is a series of patches to fix the perf-probe error against the
> Fedora34 glibc update, which moves most of symbols from .symtab to
> .dynsym. The key is that the "most of" symbols moved, but it still
> have some PLT symbols in .symtab. Thus the perf symbol-elf failes to
> decode symbols.

Do you know what's the rationale of the move?
Is it a change from glibc or Fedora?

Thanks,
Namhyung


>
> Here is the original report from Thomas about this issue.
>
> https://lore.kernel.org/linux-perf-users/[email protected]/
>
> Thank you,
>
> ---
>
> Masami Hiramatsu (3):
> perf-probe: Fix debuginfo__new() to enable build-id based debuginfo
> perf symbol-elf: Decode dynsym even if symtab exists
> perf probe: Do not show @plt function by default
>
>
> tools/perf/builtin-probe.c | 2 -
> tools/perf/util/probe-finder.c | 5 ++
> tools/perf/util/symbol-elf.c | 82 ++++++++++++++++++++++++++--------------
> 3 files changed, 60 insertions(+), 29 deletions(-)
>
> --
> Masami Hiramatsu (Linaro) <[email protected]>

2021-07-06 05:53:21

by Masami Hiramatsu

[permalink] [raw]
Subject: Re: [PATCH 0/3] perf: Fix perf probe for Fedora34 glibc update

On Mon, 5 Jul 2021 13:48:04 -0700
Namhyung Kim <[email protected]> wrote:

> On Sat, Jul 3, 2021 at 8:36 AM Masami Hiramatsu <[email protected]> wrote:
> >
> > Hi Arnaldo,
> >
> > Here is a series of patches to fix the perf-probe error against the
> > Fedora34 glibc update, which moves most of symbols from .symtab to
> > .dynsym. The key is that the "most of" symbols moved, but it still
> > have some PLT symbols in .symtab. Thus the perf symbol-elf failes to
> > decode symbols.
>
> Do you know what's the rationale of the move?
> Is it a change from glibc or Fedora?

I don't know, but it seems that this happens when updating glibc package,
the version is same but revision is different. Also, in the Ubuntu, I saw
all symbols has been moved to .dynsym in older glibc.
Thus I guess that this depends on the build option or configuration.
Might Fedora change the packaging script?

Thank you,

>
> Thanks,
> Namhyung
>
>
> >
> > Here is the original report from Thomas about this issue.
> >
> > https://lore.kernel.org/linux-perf-users/[email protected]/
> >
> > Thank you,
> >
> > ---
> >
> > Masami Hiramatsu (3):
> > perf-probe: Fix debuginfo__new() to enable build-id based debuginfo
> > perf symbol-elf: Decode dynsym even if symtab exists
> > perf probe: Do not show @plt function by default
> >
> >
> > tools/perf/builtin-probe.c | 2 -
> > tools/perf/util/probe-finder.c | 5 ++
> > tools/perf/util/symbol-elf.c | 82 ++++++++++++++++++++++++++--------------
> > 3 files changed, 60 insertions(+), 29 deletions(-)
> >
> > --
> > Masami Hiramatsu (Linaro) <[email protected]>


--
Masami Hiramatsu <[email protected]>