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]
Signed-off-by: Jiri Olsa <[email protected]>
---
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
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
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
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/[email protected]
Signed-off-by: Jiri Olsa <[email protected]>
---
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
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
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
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
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
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
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
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
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/[email protected]
Signed-off-by: Jiri Olsa <[email protected]>
---
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
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/[email protected]
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> 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
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
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
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
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?
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
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.
> 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
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
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)