[RFC] perf tools: Filter out hidden symbols from labels

[RFC] perf tools: Filter out hidden symbols from labels

[Jiri Olsa]

hi,
sending this as RFC, because I'm not sure this won't break
something else ;-)

thanks,
jirka


---
When perf is built with annobin plugin (RHEL8 build) extra symbols
are added to its binary:

  # nm perf | grep annobin | head -10
  0000000000241100 t .annobin_annotate.c
  0000000000326490 t .annobin_annotate.c
  0000000000249255 t .annobin_annotate.c_end
  00000000003283a8 t .annobin_annotate.c_end
  00000000001bce18 t .annobin_annotate.c_end.hot
  00000000001bce18 t .annobin_annotate.c_end.hot
  00000000001bc3e2 t .annobin_annotate.c_end.unlikely
  00000000001bc400 t .annobin_annotate.c_end.unlikely
  00000000001bce18 t .annobin_annotate.c.hot
  00000000001bce18 t .annobin_annotate.c.hot
  ...

those symbols have no use for report or annotation and should be skipped.
Moreover they interfere with dwarf unwind test on ppc arch, where they
are mixed with checked symbols and test fails:

  # perf test dwarf -v
  59: Test dwarf unwind                                     :
  --- start ---
  test child forked, pid 8515
  unwind: .annobin_dwarf_unwind.c:ip = 0x10dba40dc (0x2740dc)
  ...
  got: .annobin_dwarf_unwind.c 0x10dba40dc, expecting test__arch_unwind_sample
  unwind: failed with 'no error'

The annobin symbols are defined as NOTYPE/LOCAL/HIDDEN:

  # readelf -s ./perf | grep annobin | head -1
    40: 00000000001bce4f     0 NOTYPE  LOCAL  HIDDEN    13 .annobin_init.c

They can still pass the check for the label symbol. Adding
check for HIDDEN visibility and filter out such symbols.

Link: http://lkml.kernel.org/n/tip-4yuna6qhhg0df3q147cjdyuu@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/symbol-elf.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 66a84d5846c8..39ef2bde6d6d 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -87,6 +87,11 @@ static inline uint8_t elf_sym__type(const GElf_Sym *sym)
 	return GELF_ST_TYPE(sym->st_info);
 }
 
+static inline uint8_t elf_sym__visibility(const GElf_Sym *sym)
+{
+	return GELF_ST_VISIBILITY(sym->st_other);
+}
+
 #ifndef STT_GNU_IFUNC
 #define STT_GNU_IFUNC 10
 #endif
@@ -111,7 +116,8 @@ static inline int elf_sym__is_label(const GElf_Sym *sym)
 	return elf_sym__type(sym) == STT_NOTYPE &&
 		sym->st_name != 0 &&
 		sym->st_shndx != SHN_UNDEF &&
-		sym->st_shndx != SHN_ABS;
+		sym->st_shndx != SHN_ABS &&
+		elf_sym__visibility(sym) != STV_HIDDEN;
 }
 
 static bool elf_sym__filter(GElf_Sym *sym)
-- 
2.17.2

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Nick Clifton]

Hi Jiri,

> When perf is built with annobin plugin (RHEL8 build) extra symbols
> are added to its binary:

  A bit of background for those wondering why annobin is creating
  these symbols:  Annobin is a plugin for gcc that records data 
  about how object file were built.  It is specifically designed 
  to be able to cope with files that are built using multiple
  different sets of optimization options.  (Eg because of #pragma
  directives or function specific optimization attributes).  It
  generates notes to cover each compiled region of code, and it
  needs the symbols in order to be able to determine exactly which
  areas in a linked binary were compiled with which options.

  In the course of developing this plugin I encountered various
  problems with tools not expecting to find extra symbols in a
  binary.  Hence I made the symbols local, hidden and with no type.
  It was the best I could do to say "ignore these symbols - they
  are not meant to be seen by anyone but annobin".


> +		elf_sym__visibility(sym) != STV_HIDDEN;

  Just to be awkward, if you are going to ignore STV_HIDDEN 
  symbols then you should probably also ignore STV_INTERNAL ones
  as well...  Annobin does not generate them, but you never know,
  one day some other tool might create some.

Cheers
  Nick

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Arnaldo Carvalho de Melo]

Em Tue, Jan 15, 2019 at 04:13:16PM +0000, Nick Clifton escreveu:
> Hi Jiri,
> 
> > When perf is built with annobin plugin (RHEL8 build) extra symbols
> > are added to its binary:
> 
>   A bit of background for those wondering why annobin is creating
>   these symbols:  Annobin is a plugin for gcc that records data 
>   about how object file were built.  It is specifically designed 
>   to be able to cope with files that are built using multiple
>   different sets of optimization options.  (Eg because of #pragma
>   directives or function specific optimization attributes).  It
>   generates notes to cover each compiled region of code, and it
>   needs the symbols in order to be able to determine exactly which
>   areas in a linked binary were compiled with which options.

Humm, it would be nice for perf annotate to show those options when one
navigates the annotation, something like press some hotkey and see the
optimization flags used. Is there any library that gets those
annotations and put them in some linked list that we could use in
tools/perf/?

- Arnaldo
 
>   In the course of developing this plugin I encountered various
>   problems with tools not expecting to find extra symbols in a
>   binary.  Hence I made the symbols local, hidden and with no type.
>   It was the best I could do to say "ignore these symbols - they
>   are not meant to be seen by anyone but annobin".
> 
> 
> > +		elf_sym__visibility(sym) != STV_HIDDEN;
> 
>   Just to be awkward, if you are going to ignore STV_HIDDEN 
>   symbols then you should probably also ignore STV_INTERNAL ones
>   as well...  Annobin does not generate them, but you never know,
>   one day some other tool might create some.
> 
> Cheers
>   Nick
> 
> 
> 

-- 

- Arnaldo

[RFCv2] perf tools: Filter out hidden symbols from labels

[Jiri Olsa]

On Tue, Jan 15, 2019 at 04:13:16PM +0000, Nick Clifton wrote:
> Hi Jiri,
> 
> > When perf is built with annobin plugin (RHEL8 build) extra symbols
> > are added to its binary:
> 
>   A bit of background for those wondering why annobin is creating
>   these symbols:  Annobin is a plugin for gcc that records data 
>   about how object file were built.  It is specifically designed 
>   to be able to cope with files that are built using multiple
>   different sets of optimization options.  (Eg because of #pragma
>   directives or function specific optimization attributes).  It
>   generates notes to cover each compiled region of code, and it
>   needs the symbols in order to be able to determine exactly which
>   areas in a linked binary were compiled with which options.
> 
>   In the course of developing this plugin I encountered various
>   problems with tools not expecting to find extra symbols in a
>   binary.  Hence I made the symbols local, hidden and with no type.
>   It was the best I could do to say "ignore these symbols - they
>   are not meant to be seen by anyone but annobin".
> 
> 
> > +		elf_sym__visibility(sym) != STV_HIDDEN;
> 
>   Just to be awkward, if you are going to ignore STV_HIDDEN 
>   symbols then you should probably also ignore STV_INTERNAL ones
>   as well...  Annobin does not generate them, but you never know,
>   one day some other tool might create some.

sounds good, thanks

jirka


---
When perf is built with annobin plugin (RHEL8 build) extra symbols
are added to its binary:

  # nm perf | grep annobin | head -10
  0000000000241100 t .annobin_annotate.c
  0000000000326490 t .annobin_annotate.c
  0000000000249255 t .annobin_annotate.c_end
  00000000003283a8 t .annobin_annotate.c_end
  00000000001bce18 t .annobin_annotate.c_end.hot
  00000000001bce18 t .annobin_annotate.c_end.hot
  00000000001bc3e2 t .annobin_annotate.c_end.unlikely
  00000000001bc400 t .annobin_annotate.c_end.unlikely
  00000000001bce18 t .annobin_annotate.c.hot
  00000000001bce18 t .annobin_annotate.c.hot
  ...

those symbols have no use for report or annotation and should be skipped.
Moreover they interfere with dwarf unwind test on ppc arch, where they
are mixed with checked symbols and test fails:

  # perf test dwarf -v
  59: Test dwarf unwind                                     :
  --- start ---
  test child forked, pid 8515
  unwind: .annobin_dwarf_unwind.c:ip = 0x10dba40dc (0x2740dc)
  ...
  got: .annobin_dwarf_unwind.c 0x10dba40dc, expecting test__arch_unwind_sample
  unwind: failed with 'no error'

The annobin symbols are defined as NOTYPE/LOCAL/HIDDEN:

  # readelf -s ./perf | grep annobin | head -1
    40: 00000000001bce4f     0 NOTYPE  LOCAL  HIDDEN    13 .annobin_init.c

They can still pass the check for the label symbol. Adding
check for HIDDEN and INTERNAL (as suggested by Nick below)
visibility and filter out such symbols.

> Nick Clifton wrote:
>
>   Just to be awkward, if you are going to ignore STV_HIDDEN
>   symbols then you should probably also ignore STV_INTERNAL ones
>   as well...  Annobin does not generate them, but you never know,
>   one day some other tool might create some.

Link: http://lkml.kernel.org/n/tip-4yuna6qhhg0df3q147cjdyuu@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/symbol-elf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 66a84d5846c8..03cb8c6d620a 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -87,6 +87,11 @@ static inline uint8_t elf_sym__type(const GElf_Sym *sym)
 	return GELF_ST_TYPE(sym->st_info);
 }
 
+static inline uint8_t elf_sym__visibility(const GElf_Sym *sym)
+{
+	return GELF_ST_VISIBILITY(sym->st_other);
+}
+
 #ifndef STT_GNU_IFUNC
 #define STT_GNU_IFUNC 10
 #endif
@@ -111,7 +116,9 @@ static inline int elf_sym__is_label(const GElf_Sym *sym)
 	return elf_sym__type(sym) == STT_NOTYPE &&
 		sym->st_name != 0 &&
 		sym->st_shndx != SHN_UNDEF &&
-		sym->st_shndx != SHN_ABS;
+		sym->st_shndx != SHN_ABS &&
+		elf_sym__visibility(sym) != STV_HIDDEN &&
+		elf_sym__visibility(sym) != STV_INTERNAL;
 }
 
 static bool elf_sym__filter(GElf_Sym *sym)
-- 
2.17.2

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Namhyung Kim]

Hi,

On Tue, Jan 15, 2019 at 01:35:40PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 15, 2019 at 04:13:16PM +0000, Nick Clifton escreveu:
> > Hi Jiri,
> > 
> > > When perf is built with annobin plugin (RHEL8 build) extra symbols
> > > are added to its binary:
> > 
> >   A bit of background for those wondering why annobin is creating
> >   these symbols:  Annobin is a plugin for gcc that records data 
> >   about how object file were built.  It is specifically designed 
> >   to be able to cope with files that are built using multiple
> >   different sets of optimization options.  (Eg because of #pragma
> >   directives or function specific optimization attributes).  It
> >   generates notes to cover each compiled region of code, and it
> >   needs the symbols in order to be able to determine exactly which
> >   areas in a linked binary were compiled with which options.
> 
> Humm, it would be nice for perf annotate to show those options when one
> navigates the annotation, something like press some hotkey and see the
> optimization flags used. Is there any library that gets those
> annotations and put them in some linked list that we could use in
> tools/perf/?

If it's just an ELF note, we could parse it directly.

https://developers.redhat.com/blog/2018/02/20/annobin-storing-information-binaries/

Thanks,
Namhyung

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Nick Clifton]

Hi Guys,


>> Humm, it would be nice for perf annotate to show those options when one
>> navigates the annotation,

Usually the command line options can also be found in the debug info for
the executable.  Assuming it has not been stripped, of course.

One of the advantages of the annobin strategy of using ELF notes is that
these are not stripped from executables...

Unfortunately the annobin notes will probably not be very helpful as they
only record a minor subset of the typical gcc command line options.
(Specifically: -O, -g, -D_FORTIFY_SOURCE, -D_GLIBCXX_ASSERTIONS, 
-fcf-protection, -fpic (and variants), -fshort-enum, -fstack-clash-protection,
-fstack-protector, -mstackrealign, -fexceptions).

>> Is there any library that gets those
>> annotations and put them in some linked list that we could use in
>> tools/perf/?

Sorry - no such library exists.

> If it's just an ELF note, we could parse it directly.
> https://developers.redhat.com/blog/2018/02/20/annobin-storing-information-binaries/

Exactly - and what a great blog author that person is ... :-)

Cheers
  Nick

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Arnaldo Carvalho de Melo]

Em Wed, Jan 16, 2019 at 11:38:30AM +0000, Nick Clifton escreveu:
> Hi Guys,
> 
> 
> >> Humm, it would be nice for perf annotate to show those options when one
> >> navigates the annotation,
> 
> Usually the command line options can also be found in the debug info for
> the executable.  Assuming it has not been stripped, of course.
> 
> One of the advantages of the annobin strategy of using ELF notes is that
> these are not stripped from executables...
> 
> Unfortunately the annobin notes will probably not be very helpful as they
> only record a minor subset of the typical gcc command line options.
> (Specifically: -O, -g, -D_FORTIFY_SOURCE, -D_GLIBCXX_ASSERTIONS, 
> -fcf-protection, -fpic (and variants), -fshort-enum, -fstack-clash-protection,
> -fstack-protector, -mstackrealign, -fexceptions).

Humm, is -fno-omit-frame-pointer there by any chance? :-)
 
> >> Is there any library that gets those
> >> annotations and put them in some linked list that we could use in
> >> tools/perf/?
> 
> Sorry - no such library exists.

No problem...
 
> > If it's just an ELF note, we could parse it directly.

As we already parse some of the ELF notes, like the buildid, so just one
more to read and make available in the TUI somehow, should be handy.

> > https://developers.redhat.com/blog/2018/02/20/annobin-storing-information-binaries/
> 
> Exactly - and what a great blog author that person is ... :-)

:-)

- Arnaldo

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Nick Clifton]

Hi Arnaldo,

>> Unfortunately the annobin notes will probably not be very helpful as they
>> only record a minor subset of the typical gcc command line options.
>> (Specifically: -O, -g, -D_FORTIFY_SOURCE, -D_GLIBCXX_ASSERTIONS, 
>> -fcf-protection, -fpic (and variants), -fshort-enum, -fstack-clash-protection,
>> -fstack-protector, -mstackrealign, -fexceptions).
> 
> Humm, is -fno-omit-frame-pointer there by any chance? :-)

Not at the moment, although it could be added.  All of the options 
mentioned in the above list are recorded because they have an impact
on the security hardening of the binary.  Other options are ignored
because, at least for now, they have no security implications.

Cheers
  Nick

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Arnaldo Carvalho de Melo]

Em Wed, Jan 16, 2019 at 03:47:50PM +0000, Nick Clifton escreveu:
> Hi Arnaldo,
> 
> >> Unfortunately the annobin notes will probably not be very helpful as they
> >> only record a minor subset of the typical gcc command line options.
> >> (Specifically: -O, -g, -D_FORTIFY_SOURCE, -D_GLIBCXX_ASSERTIONS, 
> >> -fcf-protection, -fpic (and variants), -fshort-enum, -fstack-clash-protection,
> >> -fstack-protector, -mstackrealign, -fexceptions).
> > 
> > Humm, is -fno-omit-frame-pointer there by any chance? :-)
> 
> Not at the moment, although it could be added.  All of the options 
> mentioned in the above list are recorded because they have an impact
> on the security hardening of the binary.  Other options are ignored
> because, at least for now, they have no security implications.

Would be interestint to have that info, as we could hint the user that
backtraces should be done with something else than '--call-graph fp' :-)

- Arnaldo

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Nick Clifton]

Hi Arnaldo,

>>> Humm, is -fno-omit-frame-pointer there by any chance? :-)

> Would be interestint to have that info, as we could hint the user that
> backtraces should be done with something else than '--call-graph fp' :-)

OK, well I have added the feature to annobin 8.67, now in Fedora
rawhide.  Unfortunately it will take a while before the information
actually gets into binaries, since the new data is only recorded
when a package is rebuilt.  So basically you are looking at Fedora 30
at the earliest.

If you want to see the plugin in action, build something on a rawhide
system (or a mock chroot) using the gcc command line option -fplugin=annobin.
(Make sure that you have annobin-8.67 installed).  Then once the binary 
is built run "readelf --wide --notes <file> | grep omit" to see the 
omit-frame-pointer notes.

Cheers
  Nick

Re: [RFC] perf tools: Filter out hidden symbols from labels

[Arnaldo Carvalho de Melo]

Em Thu, Jan 17, 2019 at 10:25:24AM +0000, Nick Clifton escreveu:
> Hi Arnaldo,
> 
> >>> Humm, is -fno-omit-frame-pointer there by any chance? :-)
> 
> > Would be interestint to have that info, as we could hint the user that
> > backtraces should be done with something else than '--call-graph fp' :-)
> 
> OK, well I have added the feature to annobin 8.67, now in Fedora
> rawhide.  Unfortunately it will take a while before the information
> actually gets into binaries, since the new data is only recorded
> when a package is rebuilt.  So basically you are looking at Fedora 30
> at the earliest.

That is not a problem, I have containers for rawhide, so I can just go
ahead and do a podman pull + build, build some binary, and then add the
feature, the warning will get into effect when that note is found and
userspace callchains are used without DWARF info for such binaries.
 
> If you want to see the plugin in action, build something on a rawhide
> system (or a mock chroot) using the gcc command line option -fplugin=annobin.

Right, that is what I'll do as soon as I get the time.

> (Make sure that you have annobin-8.67 installed).  Then once the binary 
> is built run "readelf --wide --notes <file> | grep omit" to see the 
> omit-frame-pointer notes.

Thanks for the hints, I'll get you CCed when I get to work on this.

- Arnaldo

[PATCH] perf tools: Filter out hidden symbols from labels

[Jiri Olsa]

On Tue, Jan 15, 2019 at 06:38:38PM +0100, Jiri Olsa wrote:
> On Tue, Jan 15, 2019 at 04:13:16PM +0000, Nick Clifton wrote:
> > Hi Jiri,
> > 
> > > When perf is built with annobin plugin (RHEL8 build) extra symbols
> > > are added to its binary:
> > 
> >   A bit of background for those wondering why annobin is creating
> >   these symbols:  Annobin is a plugin for gcc that records data 
> >   about how object file were built.  It is specifically designed 
> >   to be able to cope with files that are built using multiple
> >   different sets of optimization options.  (Eg because of #pragma
> >   directives or function specific optimization attributes).  It
> >   generates notes to cover each compiled region of code, and it
> >   needs the symbols in order to be able to determine exactly which
> >   areas in a linked binary were compiled with which options.
> > 
> >   In the course of developing this plugin I encountered various
> >   problems with tools not expecting to find extra symbols in a
> >   binary.  Hence I made the symbols local, hidden and with no type.
> >   It was the best I could do to say "ignore these symbols - they
> >   are not meant to be seen by anyone but annobin".
> > 
> > 
> > > +		elf_sym__visibility(sym) != STV_HIDDEN;
> > 
> >   Just to be awkward, if you are going to ignore STV_HIDDEN 
> >   symbols then you should probably also ignore STV_INTERNAL ones
> >   as well...  Annobin does not generate them, but you never know,
> >   one day some other tool might create some.
> 
> sounds good, thanks

there were no objections for rfc, sending patch

thanks,
jirka


---
When perf is built with annobin plugin (RHEL8 build) extra symbols
are added to its binary:

  # nm perf | grep annobin | head -10
  0000000000241100 t .annobin_annotate.c
  0000000000326490 t .annobin_annotate.c
  0000000000249255 t .annobin_annotate.c_end
  00000000003283a8 t .annobin_annotate.c_end
  00000000001bce18 t .annobin_annotate.c_end.hot
  00000000001bce18 t .annobin_annotate.c_end.hot
  00000000001bc3e2 t .annobin_annotate.c_end.unlikely
  00000000001bc400 t .annobin_annotate.c_end.unlikely
  00000000001bce18 t .annobin_annotate.c.hot
  00000000001bce18 t .annobin_annotate.c.hot
  ...

those symbols have no use for report or annotation and should be skipped.
Moreover they interfere with dwarf unwind test on ppc arch, where they
are mixed with checked symbols and test fails:

  # perf test dwarf -v
  59: Test dwarf unwind                                     :
  --- start ---
  test child forked, pid 8515
  unwind: .annobin_dwarf_unwind.c:ip = 0x10dba40dc (0x2740dc)
  ...
  got: .annobin_dwarf_unwind.c 0x10dba40dc, expecting test__arch_unwind_sample
  unwind: failed with 'no error'

The annobin symbols are defined as NOTYPE/LOCAL/HIDDEN:

  # readelf -s ./perf | grep annobin | head -1
    40: 00000000001bce4f     0 NOTYPE  LOCAL  HIDDEN    13 .annobin_init.c

They can still pass the check for the label symbol. Adding
check for HIDDEN and INTERNAL (as suggested by Nick below)
visibility and filter out such symbols.

>   Just to be awkward, if you are going to ignore STV_HIDDEN
>   symbols then you should probably also ignore STV_INTERNAL ones
>   as well...  Annobin does not generate them, but you never know,
>   one day some other tool might create some.

Link: http://lkml.kernel.org/n/tip-4yuna6qhhg0df3q147cjdyuu@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/symbol-elf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 66a84d5846c8..03cb8c6d620a 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -87,6 +87,11 @@ static inline uint8_t elf_sym__type(const GElf_Sym *sym)
 	return GELF_ST_TYPE(sym->st_info);
 }
 
+static inline uint8_t elf_sym__visibility(const GElf_Sym *sym)
+{
+	return GELF_ST_VISIBILITY(sym->st_other);
+}
+
 #ifndef STT_GNU_IFUNC
 #define STT_GNU_IFUNC 10
 #endif
@@ -111,7 +116,9 @@ static inline int elf_sym__is_label(const GElf_Sym *sym)
 	return elf_sym__type(sym) == STT_NOTYPE &&
 		sym->st_name != 0 &&
 		sym->st_shndx != SHN_UNDEF &&
-		sym->st_shndx != SHN_ABS;
+		sym->st_shndx != SHN_ABS &&
+		elf_sym__visibility(sym) != STV_HIDDEN &&
+		elf_sym__visibility(sym) != STV_INTERNAL;
 }
 
 static bool elf_sym__filter(GElf_Sym *sym)
-- 
2.17.2

Re: [PATCH] perf tools: Filter out hidden symbols from labels

[Arnaldo Carvalho de Melo]

Em Mon, Jan 28, 2019 at 02:35:26PM +0100, Jiri Olsa escreveu:
> On Tue, Jan 15, 2019 at 06:38:38PM +0100, Jiri Olsa wrote:
> > On Tue, Jan 15, 2019 at 04:13:16PM +0000, Nick Clifton wrote:
> > >   Just to be awkward, if you are going to ignore STV_HIDDEN 
> > >   symbols then you should probably also ignore STV_INTERNAL ones
> > >   as well...  Annobin does not generate them, but you never know,
> > >   one day some other tool might create some.

> > sounds good, thanks
 
> there were no objections for rfc, sending patch

I don't see a problem, Nick, can you provide an Acked-by, or better yet,
a Reviewed-by so that Jiri can collect in this patch and I can push it
to perf/urgent?

Thanks,

- Arnaldo
 
> thanks,
> jirka
> 
> 
> ---
> When perf is built with annobin plugin (RHEL8 build) extra symbols
> are added to its binary:
> 
>   # nm perf | grep annobin | head -10
>   0000000000241100 t .annobin_annotate.c
>   0000000000326490 t .annobin_annotate.c
>   0000000000249255 t .annobin_annotate.c_end
>   00000000003283a8 t .annobin_annotate.c_end
>   00000000001bce18 t .annobin_annotate.c_end.hot
>   00000000001bce18 t .annobin_annotate.c_end.hot
>   00000000001bc3e2 t .annobin_annotate.c_end.unlikely
>   00000000001bc400 t .annobin_annotate.c_end.unlikely
>   00000000001bce18 t .annobin_annotate.c.hot
>   00000000001bce18 t .annobin_annotate.c.hot
>   ...
> 
> those symbols have no use for report or annotation and should be skipped.
> Moreover they interfere with dwarf unwind test on ppc arch, where they
> are mixed with checked symbols and test fails:
> 
>   # perf test dwarf -v
>   59: Test dwarf unwind                                     :
>   --- start ---
>   test child forked, pid 8515
>   unwind: .annobin_dwarf_unwind.c:ip = 0x10dba40dc (0x2740dc)
>   ...
>   got: .annobin_dwarf_unwind.c 0x10dba40dc, expecting test__arch_unwind_sample
>   unwind: failed with 'no error'
> 
> The annobin symbols are defined as NOTYPE/LOCAL/HIDDEN:
> 
>   # readelf -s ./perf | grep annobin | head -1
>     40: 00000000001bce4f     0 NOTYPE  LOCAL  HIDDEN    13 .annobin_init.c
> 
> They can still pass the check for the label symbol. Adding
> check for HIDDEN and INTERNAL (as suggested by Nick below)
> visibility and filter out such symbols.
> 
> >   Just to be awkward, if you are going to ignore STV_HIDDEN
> >   symbols then you should probably also ignore STV_INTERNAL ones
> >   as well...  Annobin does not generate them, but you never know,
> >   one day some other tool might create some.
> 
> Link: http://lkml.kernel.org/n/tip-4yuna6qhhg0df3q147cjdyuu@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/symbol-elf.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 66a84d5846c8..03cb8c6d620a 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -87,6 +87,11 @@ static inline uint8_t elf_sym__type(const GElf_Sym *sym)
>  	return GELF_ST_TYPE(sym->st_info);
>  }
>  
> +static inline uint8_t elf_sym__visibility(const GElf_Sym *sym)
> +{
> +	return GELF_ST_VISIBILITY(sym->st_other);
> +}
> +
>  #ifndef STT_GNU_IFUNC
>  #define STT_GNU_IFUNC 10
>  #endif
> @@ -111,7 +116,9 @@ static inline int elf_sym__is_label(const GElf_Sym *sym)
>  	return elf_sym__type(sym) == STT_NOTYPE &&
>  		sym->st_name != 0 &&
>  		sym->st_shndx != SHN_UNDEF &&
> -		sym->st_shndx != SHN_ABS;
> +		sym->st_shndx != SHN_ABS &&
> +		elf_sym__visibility(sym) != STV_HIDDEN &&
> +		elf_sym__visibility(sym) != STV_INTERNAL;
>  }
>  
>  static bool elf_sym__filter(GElf_Sym *sym)
> -- 
> 2.17.2

-- 

- Arnaldo

Re: [PATCH] perf tools: Filter out hidden symbols from labels

[Nick Clifton]

Hi Arnaldo,

>> there were no objections for rfc, sending patch
> 
> I don't see a problem, Nick, can you provide an Acked-by, or better yet,
> a Reviewed-by so that Jiri can collect in this patch and I can push it
> to perf/urgent?

Sorry - I do not think that I have the authority to do that.  I am not a
perftools maintainer (or a kernel maintainer).  For what it is worth the 
patch does look good to me, but I think that a real maintainer needs to 
approve it.

Cheers
  Nick

Re: [PATCH] perf tools: Filter out hidden symbols from labels

[Arnaldo Carvalho de Melo]

Em Tue, Jan 29, 2019 at 11:25:35AM +0000, Nick Clifton escreveu:
> Hi Arnaldo,
> 
> >> there were no objections for rfc, sending patch
> > 
> > I don't see a problem, Nick, can you provide an Acked-by, or better yet,
> > a Reviewed-by so that Jiri can collect in this patch and I can push it
> > to perf/urgent?
> 
> Sorry - I do not think that I have the authority to do that.  I am not a
> perftools maintainer (or a kernel maintainer).  For what it is worth the 
> patch does look good to me, but I think that a real maintainer needs to 
> approve it.

Ok, I was more thinking about this part of
Documentation/process/submittingpatches.txt:

----------------------------
Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
has at least reviewed the patch and has indicated acceptance.  Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by: (but note that it is usually better to ask for an
explicit ack).
----------------------------

But its ok, if you still don't feel this should be added, I'll just
leave a Cc: you, ok?

- Arnaldo

Re: [PATCH] perf tools: Filter out hidden symbols from labels

[Nick Clifton]

Hi Arnaldo,

>> Sorry - I do not think that I have the authority to do that.  I am not a
>> perftools maintainer (or a kernel maintainer).  For what it is worth the 
>> patch does look good to me, but I think that a real maintainer needs to 
>> approve it.
> 
> Ok, I was more thinking about this part of
> Documentation/process/submittingpatches.txt:
> 
> ----------------------------
> Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
> has at least reviewed the patch and has indicated acceptance.  Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by: (but note that it is usually better to ask for an
> explicit ack).
> ----------------------------
> 
> But its ok, if you still don't feel this should be added, I'll just
> leave a Cc: you, ok?

Re: [PATCH] perf tools: Filter out hidden symbols from labels

[Nick Clifton]

Hi Arnaldo,

  [oops- hit send by mistake...]

>> Sorry - I do not think that I have the authority to do that.  I am not a

> Ok, I was more thinking about this part of
> Documentation/process/submittingpatches.txt:
> 
> ----------------------------
> Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
> has at least reviewed the patch and has indicated acceptance.  Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by: (but note that it is usually better to ask for an
> explicit ack).
> ----------------------------
> 
> But its ok, if you still don't feel this should be added, I'll just
> leave a Cc: you, ok?

Yes - this would be fine with me.

Cheers
  Nick

Re: [PATCH] perf tools: Filter out hidden symbols from labels

[Arnaldo Carvalho de Melo]

Em Mon, Jan 28, 2019 at 02:35:26PM +0100, Jiri Olsa escreveu:
> On Tue, Jan 15, 2019 at 06:38:38PM +0100, Jiri Olsa wrote:
> > On Tue, Jan 15, 2019 at 04:13:16PM +0000, Nick Clifton wrote:
> > > > When perf is built with annobin plugin (RHEL8 build) extra symbols
> > > > are added to its binary:
<SNIP>
> > > > +		elf_sym__visibility(sym) != STV_HIDDEN;
> > > 
> > >   Just to be awkward, if you are going to ignore STV_HIDDEN 
> > >   symbols then you should probably also ignore STV_INTERNAL ones
> > >   as well...  Annobin does not generate them, but you never know,
> > >   one day some other tool might create some.
> > 
> > sounds good, thanks
> 
> there were no objections for rfc, sending patch

Thanks, applied to perf/urgent.

- Arnaldo

[tip:perf/urgent] perf symbols: Filter out hidden symbols from labels

[tip-bot for Jiri Olsa]

Commit-ID:  59a17706915fe5ea6f711e1f92d4fb706bce07fe
Gitweb:     https://git.kernel.org/tip/59a17706915fe5ea6f711e1f92d4fb706bce07fe
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Mon, 28 Jan 2019 14:35:26 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 4 Feb 2019 15:50:38 -0300

perf symbols: Filter out hidden symbols from labels

When perf is built with the annobin plugin (RHEL8 build) extra symbols
are added to its binary:

  # nm perf | grep annobin | head -10
  0000000000241100 t .annobin_annotate.c
  0000000000326490 t .annobin_annotate.c
  0000000000249255 t .annobin_annotate.c_end
  00000000003283a8 t .annobin_annotate.c_end
  00000000001bce18 t .annobin_annotate.c_end.hot
  00000000001bce18 t .annobin_annotate.c_end.hot
  00000000001bc3e2 t .annobin_annotate.c_end.unlikely
  00000000001bc400 t .annobin_annotate.c_end.unlikely
  00000000001bce18 t .annobin_annotate.c.hot
  00000000001bce18 t .annobin_annotate.c.hot
  ...

Those symbols have no use for report or annotation and should be
skipped.  Moreover they interfere with the DWARF unwind test on the PPC
arch, where they are mixed with checked symbols and then the test fails:

  # perf test dwarf -v
  59: Test dwarf unwind                                     :
  --- start ---
  test child forked, pid 8515
  unwind: .annobin_dwarf_unwind.c:ip = 0x10dba40dc (0x2740dc)
  ...
  got: .annobin_dwarf_unwind.c 0x10dba40dc, expecting test__arch_unwind_sample
  unwind: failed with 'no error'

The annobin symbols are defined as NOTYPE/LOCAL/HIDDEN:

  # readelf -s ./perf | grep annobin | head -1
    40: 00000000001bce4f     0 NOTYPE  LOCAL  HIDDEN    13 .annobin_init.c

They can still pass the check for the label symbol. Adding check for
HIDDEN and INTERNAL (as suggested by Nick below) visibility and filter
out such symbols.

>   Just to be awkward, if you are going to ignore STV_HIDDEN
>   symbols then you should probably also ignore STV_INTERNAL ones
>   as well...  Annobin does not generate them, but you never know,
>   one day some other tool might create some.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Nick Clifton <nickc@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20190128133526.GD15461@krava
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/symbol-elf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 695a73940329..dca7dfae69ad 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -101,6 +101,11 @@ static inline uint8_t elf_sym__type(const GElf_Sym *sym)
 	return GELF_ST_TYPE(sym->st_info);
 }
 
+static inline uint8_t elf_sym__visibility(const GElf_Sym *sym)
+{
+	return GELF_ST_VISIBILITY(sym->st_other);
+}
+
 #ifndef STT_GNU_IFUNC
 #define STT_GNU_IFUNC 10
 #endif
@@ -125,7 +130,9 @@ static inline int elf_sym__is_label(const GElf_Sym *sym)
 	return elf_sym__type(sym) == STT_NOTYPE &&
 		sym->st_name != 0 &&
 		sym->st_shndx != SHN_UNDEF &&
-		sym->st_shndx != SHN_ABS;
+		sym->st_shndx != SHN_ABS &&
+		elf_sym__visibility(sym) != STV_HIDDEN &&
+		elf_sym__visibility(sym) != STV_INTERNAL;
 }
 
 static bool elf_sym__filter(GElf_Sym *sym)