perf tools: Filter out hidden symbols from labels

RFC In review Jiri Olsa 6 participants 19 messages Jan 15, 2019 – Feb 9, 2019 perf events

lore

Share & tools

lore

Jiri Olsa

author reviewer

2019-01-15 13:54 UTC #0 lore raw

RFC 1/1 perf tools: Filter out hidden symbols from labels

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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/symbol-elf.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

tools/perf/util/symbol-elf.c +7 -1

index 66a84d5846c8..39ef2bde6d6d 100644
@@ -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)

Signature · 1 line

2.17.2

Nick Clifton

2019-01-15 16:13 UTC #1 lore raw

Hi Jiri,

Quoted 2 lines

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".

tools/perf/util/symbol-elf.c static inline int elf_sym__is_label(const GElf_Sym *sym) · line 120

Quoted 1 line

tools/perf/util/symbol-elf.c +1 -0
+		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 Carvalho de Melo

maintainer

2019-01-15 16:35 UTC #2 lore raw

Em Tue, Jan 15, 2019 at 04:13:16PM +0000, Nick Clifton escreveu:

Quoted 5 lines

Hi Jiri,
Quoted 2 lines
When perf is built with annobin plugin (RHEL8 build) extra symbols are added to its binary:

Quoted 9 lines

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

Quoted 19 lines

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".
tools/perf/util/symbol-elf.c static inline int elf_sym__is_label(const GElf_Sym *sym) · line 120
Quoted 1 line
tools/perf/util/symbol-elf.c +1 -0
+		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

Signature · 1 line

- Arnaldo

Namhyung Kim

maintainer

2019-01-16 04:37 UTC #3 lore raw

Hi,

Quoted 16 lines · Arnaldo Carvalho de Melo

Em Tue, Jan 15, 2019 at 04:13:16PM +0000, Nick Clifton escreveu:
Quoted 5 lines
Hi Jiri,
Quoted 2 lines
When perf is built with annobin plugin (RHEL8 build) extra symbols are added to its binary:
Quoted 9 lines
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.

Quoted 5 lines

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

Nick Clifton

2019-01-16 11:38 UTC #4 lore raw

Hi Guys,

Quoted 2 lines

Quoted 2 lines
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).

Quoted 3 lines

Quoted 3 lines
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.

Quoted 2 lines

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

Arnaldo Carvalho de Melo

maintainer

2019-01-16 13:31 UTC #5 lore raw

Em Wed, Jan 16, 2019 at 11:38:30AM +0000, Nick Clifton escreveu:

Quoted 15 lines

Hi Guys,
Quoted 2 lines
Quoted 2 lines
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,

Review

Quoted 2 lines

(quoted diff) +0 -2

-fcf-protection, -fpic (and variants), -fshort-enum, -fstack-clash-protection,
-fstack-protector, -mstackrealign, -fexceptions).

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

Quoted 5 lines

Quoted 3 lines
Quoted 3 lines
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...

Quoted 1 line

Quoted 1 line
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.

Quoted 3 lines

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

:-)

- Arnaldo

Nick Clifton

2019-01-16 15:47 UTC #6 lore raw

Hi Arnaldo,

Quoted 7 lines

Review
Quoted 5 lines
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,
(quoted diff) +0 -2
-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

Arnaldo Carvalho de Melo

maintainer

2019-01-16 16:04 UTC #7 lore raw

Em Wed, Jan 16, 2019 at 03:47:50PM +0000, Nick Clifton escreveu:

Quoted 10 lines

Hi Arnaldo,
Quoted 7 lines
Review
Quoted 5 lines
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,
(quoted diff) +0 -2
-fcf-protection, -fpic (and variants), -fshort-enum, -fstack-clash-protection,
-fstack-protector, -mstackrealign, -fexceptions).
Humm, is -fno-omit-frame-pointer there by any chance? :-)

Quoted 4 lines

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

Nick Clifton

2019-01-17 10:25 UTC #8 lore raw

Hi Arnaldo,

Quoted 4 lines

Quoted 1 line
Quoted 1 line
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

Arnaldo Carvalho de Melo

maintainer

2019-01-17 13:26 UTC #9 lore raw

Em Thu, Jan 17, 2019 at 10:25:24AM +0000, Nick Clifton escreveu:

Quoted 4 lines

Hi Arnaldo,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Humm, is -fno-omit-frame-pointer there by any chance? :-)

Quoted 8 lines

Quoted 2 lines
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.

Quoted 2 lines

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.

Quoted 3 lines

(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

Jiri Olsa

2019-01-15 17:38 UTC #10 lore raw

RFC v2 1/1 perf tools: Filter out hidden symbols from labels

In reply to Nick Clifton

Quoted 22 lines · Nick Clifton

Hi Jiri,
Quoted 2 lines
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".

Quoted 6 lines

Review
Quoted 1 line
(quoted diff) +1 -0
+		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.

Quoted 6 lines

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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/symbol-elf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

tools/perf/util/symbol-elf.c +8 -1

index 66a84d5846c8..03cb8c6d620a 100644
@@ -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)

Signature · 1 line

2.17.2

Jiri Olsa

2019-01-28 13:35 UTC #11 lore raw

PATCH 1/1 perf tools: Filter out hidden symbols from labels

Quoted 23 lines · Jiri Olsa

Quoted 22 lines · Nick Clifton
Hi Jiri,
Quoted 2 lines
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".

Quoted 8 lines

Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+		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.

Quoted 4 lines

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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/symbol-elf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

tools/perf/util/symbol-elf.c +8 -1

index 66a84d5846c8..03cb8c6d620a 100644
@@ -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)

Signature · 1 line

2.17.2

Arnaldo Carvalho de Melo

maintainer

2019-01-29 09:07 UTC #12 lore raw

Em Mon, Jan 28, 2019 at 02:35:26PM +0100, Jiri Olsa escreveu:

Quoted 10 lines · Jiri Olsa

Quoted 7 lines · Nick Clifton
Quoted 4 lines
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

Quoted 83 lines

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.
Quoted 4 lines
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/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/symbol-elf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
tools/perf/util/symbol-elf.c +8 -1
index 66a84d5846c8..03cb8c6d620a 100644
@@ -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)
Signature · 1 line
2.17.2

Signature · 1 line

- Arnaldo

Nick Clifton

2019-01-29 11:25 UTC #13 lore raw

Hi Arnaldo,

Quoted 5 lines

Quoted 1 line
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

Arnaldo Carvalho de Melo

maintainer

2019-01-29 11:39 UTC #14 lore raw

Em Tue, Jan 29, 2019 at 11:25:35AM +0000, Nick Clifton escreveu:

Quoted 4 lines

Hi Arnaldo,
Quoted 2 lines
Quoted 1 line
there were no objections for rfc, sending patch

Quoted 8 lines

Quoted 3 lines
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

Nick Clifton

2019-01-29 12:57 UTC #15 lore raw

Hi Arnaldo,

Quoted 18 lines

Quoted 4 lines
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?

Nick Clifton

2019-01-29 12:58 UTC #16 lore raw

In reply to Arnaldo Carvalho de Melo

Hi Arnaldo,

[oops- hit send by mistake...]

Quoted 13 lines

Quoted 1 line
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).
----------------------------

Quoted 2 lines

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

Arnaldo Carvalho de Melo

maintainer Applied

2019-02-04 14:45 UTC #17 lore raw

In reply to Jiri Olsa

Em Mon, Jan 28, 2019 at 02:35:26PM +0100, Jiri Olsa escreveu:

Quoted 4 lines · Jiri Olsa

Quoted 3 lines · Nick Clifton
Quoted 2 lines
Quoted 2 lines
When perf is built with annobin plugin (RHEL8 build) extra symbols are added to its binary:

<SNIP>

Quoted 10 lines

Quoted 8 lines
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+		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-bot for Jiri Olsa

merge bot

2019-02-09 12:23 UTC #18 lore raw

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

In reply to Jiri Olsa

perf symbols: Filter out hidden symbols from labels

Commit-ID:  59a17706915fe5ea6f711e1f92d4fb706bce07fe
Gitweb:     https://git.kernel.org/tip/59a17706915fe5ea6f711e1f92d4fb706bce07fe
Author:     Jiri Olsa <[email protected]>
AuthorDate: Mon, 28 Jan 2019 14:35:26 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
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.

Quoted 4 lines

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 <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Michael Petlan <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Nick Clifton <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/20190128133526.GD15461@krava open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/symbol-elf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

tools/perf/util/symbol-elf.c +8 -1

index 695a73940329..dca7dfae69ad 100644
@@ -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)