* [PATCH v4] tools/perf: Allow user probes on versioned symbols
@ 2017-04-06 3:30 Paul Clarke
2017-04-06 14:36 ` Arnaldo Carvalho de Melo
2017-04-10 13:11 ` Masami Hiramatsu
0 siblings, 2 replies; 6+ messages in thread
From: Paul Clarke @ 2017-04-06 3:30 UTC (permalink / raw)
To: LKML, Arnaldo Carvalho de Melo, Masami Hiramatsu, David Ahern
Cc: linux-perf-users
Symbol versioning, as in glibc, results in symbols being defined as:
<real symbol>@[@]<version>
(Note that "@@" identifies a default symbol, if the symbol name
is repeated.)
perf is currently unable to deal with this, and is unable to create
user probes at such symbols:
--
$ nm /lib/powerpc64le-linux-gnu/libpthread.so.0 | grep pthread_create
0000000000008d30 t __pthread_create_2_1
0000000000008d30 T pthread_create@@GLIBC_2.17
$ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
probe-definition(0): pthread_create
symbol:pthread_create file:(null) line:0 offset:0 return:0 lazy:(null)
0 arguments
Open Debuginfo file: /usr/lib/debug/lib/powerpc64le-linux-gnu/libpthread-2.19.so
Try to find probe point from debuginfo.
Probe point 'pthread_create' not found.
Error: Failed to add events. Reason: No such file or directory (Code: -2)
--
One is not able to specify the fully versioned symbol, either, due to
syntactic conflicts with other uses of "@" by perf:
--
$ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create@@GLIBC_2.17
probe-definition(0): pthread_create@@GLIBC_2.17
Semantic error :SRC@SRC is not allowed.
0 arguments
Error: Command Parse Error. Reason: Invalid argument (Code: -22)
--
This patch ignores the versioning tag for default symbols, thus
allowing probes to be created for these symbols:
--
$ /usr/bin/sudo ./perf probe -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
Added new event:
probe_libpthread:pthread_create (on pthread_create in /lib/powerpc64le-linux-gnu/libpthread-2.19.so)
You can now use it in all perf tools, such as:
perf record -e probe_libpthread:pthread_create -aR sleep 1
$ /usr/bin/sudo ./perf record -e probe_libpthread:pthread_create -aR ./test 2
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.052 MB perf.data (2 samples) ]
$ /usr/bin/sudo ./perf script
test 2915 [000] 19124.260729: probe_libpthread:pthread_create: (3fff99248d38)
test 2916 [000] 19124.260962: probe_libpthread:pthread_create: (3fff99248d38)
$ /usr/bin/sudo ./perf probe --del=probe_libpthread:pthread_create
Removed event: probe_libpthread:pthread_create
--
Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
---
v4:
- rebased to acme/perf/core
- moved changes from "map" namespace to "symbol" namespace,
- rewrote symbol__compare (now *match) to avoid need for strdup
- new symbol__match_symbol_name to support versioned symbols, ignoring default
tags
- new arch__compare_symbol_names_n to map to strncmp
- dso__find_symbol_by_name: now tries exact match (as before), then tries
also adding symbols tagged as default (@@)
- symbols__find_by_name: new parameter to support finding with or without default
tags
- does NOT handle setting probes using the tagged symbol name (symbol@[@]tag)
v3:
- code style fixes per David Ahern
v2:
- move logic from arch__compare_symbol_names to new map__compare_symbol_names
- call through from map__compare_symbol_names to arch__compare_symbol_names
- redirect uses of arch__compare_symbol_names
- send patch to LKML
tools/perf/arch/powerpc/util/sym-handling.c | 12 ++++++
tools/perf/util/map.c | 5 ---
tools/perf/util/map.h | 5 ++-
tools/perf/util/symbol.c | 62 +++++++++++++++++++++++------
tools/perf/util/symbol.h | 9 +++++
5 files changed, 73 insertions(+), 20 deletions(-)
diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
index 39dbe51..0d40e17 100644
--- a/tools/perf/arch/powerpc/util/sym-handling.c
+++ b/tools/perf/arch/powerpc/util/sym-handling.c
@@ -52,6 +52,18 @@ int arch__compare_symbol_names(const char *namea, const char *nameb)
return strcmp(namea, nameb);
}
+
+int arch__compare_symbol_names_n(const char *namea, const char *nameb,
+ unsigned int n)
+{
+ /* Skip over initial dot */
+ if (*namea == '.')
+ namea++;
+ if (*nameb == '.')
+ nameb++;
+
+ return strncmp(namea, nameb, n);
+}
#endif
#if defined(_CALL_ELF) && _CALL_ELF == 2
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index c1870ac..f4d8272 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -325,11 +325,6 @@ int map__load(struct map *map)
return 0;
}
-int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
-{
- return strcmp(namea, nameb);
-}
-
struct symbol *map__find_symbol(struct map *map, u64 addr)
{
if (map__load(map) < 0)
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index c8a5a64..325bbc8 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -130,13 +130,14 @@ struct thread;
*/
#define __map__for_each_symbol_by_name(map, sym_name, pos) \
for (pos = map__find_symbol_by_name(map, sym_name); \
- pos && arch__compare_symbol_names(pos->name, sym_name) == 0; \
+ pos && \
+ symbol__match_symbol_name(pos->name, sym_name, \
+ SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY) == 0; \
pos = symbol__next_by_name(pos))
#define map__for_each_symbol_by_name(map, sym_name, pos) \
__map__for_each_symbol_by_name(map, sym_name, (pos))
-int arch__compare_symbol_names(const char *namea, const char *nameb);
void map__init(struct map *map, enum map_type type,
u64 start, u64 end, u64 pgoff, struct dso *dso);
struct map *map__new(struct machine *machine, u64 start, u64 len,
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 9b4d8ba..c29440d 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -87,6 +87,17 @@ static int prefix_underscores_count(const char *str)
return tail - str;
}
+int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
+{
+ return strcmp(namea, nameb);
+}
+
+int __weak arch__compare_symbol_names_n(const char *namea, const char *nameb,
+ unsigned int n)
+{
+ return strncmp(namea, nameb, n);
+}
+
int __weak arch__choose_best_symbol(struct symbol *syma,
struct symbol *symb __maybe_unused)
{
@@ -396,8 +407,26 @@ static void symbols__sort_by_name(struct rb_root *symbols,
}
}
+int symbol__match_symbol_name(const char *name, const char *str,
+ int including_tags)
+{
+ const char *versioning;
+
+ if (including_tags == SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY &&
+ (versioning = strstr(name,"@@"))) {
+
+ unsigned int len = strlen(str);
+ if (len < versioning - name)
+ len = versioning - name;
+
+ return arch__compare_symbol_names_n(name, str, len);
+ } else
+ return arch__compare_symbol_names(name, str);
+}
+
static struct symbol *symbols__find_by_name(struct rb_root *symbols,
- const char *name)
+ const char *name,
+ unsigned int including_tags)
{
struct rb_node *n;
struct symbol_name_rb_node *s = NULL;
@@ -411,11 +440,12 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
int cmp;
s = rb_entry(n, struct symbol_name_rb_node, rb_node);
- cmp = arch__compare_symbol_names(name, s->sym.name);
+ cmp = symbol__match_symbol_name(s->sym.name, name,
+ including_tags);
- if (cmp < 0)
+ if (cmp > 0)
n = n->rb_left;
- else if (cmp > 0)
+ else if (cmp < 0)
n = n->rb_right;
else
break;
@@ -424,16 +454,17 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
if (n == NULL)
return NULL;
- /* return first symbol that has same name (if any) */
- for (n = rb_prev(n); n; n = rb_prev(n)) {
- struct symbol_name_rb_node *tmp;
+ if (including_tags != SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY)
+ /* return first symbol that has same name (if any) */
+ for (n = rb_prev(n); n; n = rb_prev(n)) {
+ struct symbol_name_rb_node *tmp;
- tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
- if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
- break;
+ tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
+ if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
+ break;
- s = tmp;
- }
+ s = tmp;
+ }
return &s->sym;
}
@@ -500,7 +531,12 @@ struct symbol *symbol__next_by_name(struct symbol *sym)
struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
const char *name)
{
- return symbols__find_by_name(&dso->symbol_names[type], name);
+ struct symbol *s = symbols__find_by_name(&dso->symbol_names[type], name,
+ SYMBOLS__TAG_INCLUDE_NONE);
+ if (!s)
+ s = symbols__find_by_name(&dso->symbol_names[type], name,
+ SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY);
+ return s;
}
void dso__sort_by_name(struct dso *dso, enum map_type type)
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 5245d2f..bde2100 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -348,8 +348,17 @@ void arch__sym_update(struct symbol *s, GElf_Sym *sym);
#define SYMBOL_A 0
#define SYMBOL_B 1
+int arch__compare_symbol_names(const char *namea, const char *nameb);
+int arch__compare_symbol_names_n(const char *namea, const char *nameb,
+ unsigned int n);
int arch__choose_best_symbol(struct symbol *syma, struct symbol *symb);
+#define SYMBOLS__TAG_INCLUDE_NONE 0
+#define SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY 1
+
+int symbol__match_symbol_name(const char *namea, const char *nameb,
+ int including_tags);
+
/* structure containing an SDT note's info */
struct sdt_note {
char *name; /* name of the note*/
--
2.1.4
Regards,
PC
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] tools/perf: Allow user probes on versioned symbols
2017-04-06 3:30 [PATCH v4] tools/perf: Allow user probes on versioned symbols Paul Clarke
@ 2017-04-06 14:36 ` Arnaldo Carvalho de Melo
2017-04-06 15:56 ` Paul Clarke
2017-04-10 13:11 ` Masami Hiramatsu
1 sibling, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-04-06 14:36 UTC (permalink / raw)
To: Paul Clarke
Cc: LKML, Arnaldo Carvalho de Melo, Masami Hiramatsu, David Ahern,
linux-perf-users
Em Wed, Apr 05, 2017 at 10:30:03PM -0500, Paul Clarke escreveu:
> Symbol versioning, as in glibc, results in symbols being defined as:
> <real symbol>@[@]<version>
> (Note that "@@" identifies a default symbol, if the symbol name
> is repeated.)
>
> perf is currently unable to deal with this, and is unable to create
> user probes at such symbols:
> --
> $ nm /lib/powerpc64le-linux-gnu/libpthread.so.0 | grep pthread_create
> 0000000000008d30 t __pthread_create_2_1
> 0000000000008d30 T pthread_create@@GLIBC_2.17
> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
> probe-definition(0): pthread_create
> symbol:pthread_create file:(null) line:0 offset:0 return:0 lazy:(null)
> 0 arguments
> Open Debuginfo file: /usr/lib/debug/lib/powerpc64le-linux-gnu/libpthread-2.19.so
> Try to find probe point from debuginfo.
> Probe point 'pthread_create' not found.
> Error: Failed to add events. Reason: No such file or directory (Code: -2)
> --
>
> One is not able to specify the fully versioned symbol, either, due to
> syntactic conflicts with other uses of "@" by perf:
> --
> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create@@GLIBC_2.17
> probe-definition(0): pthread_create@@GLIBC_2.17
> Semantic error :SRC@SRC is not allowed.
> 0 arguments
> Error: Command Parse Error. Reason: Invalid argument (Code: -22)
> --
>
> This patch ignores the versioning tag for default symbols, thus
> allowing probes to be created for these symbols:
> --
> $ /usr/bin/sudo ./perf probe -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
> Added new event:
> probe_libpthread:pthread_create (on pthread_create in /lib/powerpc64le-linux-gnu/libpthread-2.19.so)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe_libpthread:pthread_create -aR sleep 1
>
> $ /usr/bin/sudo ./perf record -e probe_libpthread:pthread_create -aR ./test 2
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.052 MB perf.data (2 samples) ]
> $ /usr/bin/sudo ./perf script
> test 2915 [000] 19124.260729: probe_libpthread:pthread_create: (3fff99248d38)
> test 2916 [000] 19124.260962: probe_libpthread:pthread_create: (3fff99248d38)
> $ /usr/bin/sudo ./perf probe --del=probe_libpthread:pthread_create
> Removed event: probe_libpthread:pthread_create
> --
>
> Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
> ---
> v4:
> - rebased to acme/perf/core
> - moved changes from "map" namespace to "symbol" namespace,
> - rewrote symbol__compare (now *match) to avoid need for strdup
> - new symbol__match_symbol_name to support versioned symbols, ignoring default
> tags
> - new arch__compare_symbol_names_n to map to strncmp
> - dso__find_symbol_by_name: now tries exact match (as before), then tries
> also adding symbols tagged as default (@@)
> - symbols__find_by_name: new parameter to support finding with or without default
> tags
> - does NOT handle setting probes using the tagged symbol name (symbol@[@]tag)
>
> v3:
> - code style fixes per David Ahern
>
> v2:
> - move logic from arch__compare_symbol_names to new map__compare_symbol_names
> - call through from map__compare_symbol_names to arch__compare_symbol_names
> - redirect uses of arch__compare_symbol_names
> - send patch to LKML
> tools/perf/arch/powerpc/util/sym-handling.c | 12 ++++++
> tools/perf/util/map.c | 5 ---
> tools/perf/util/map.h | 5 ++-
> tools/perf/util/symbol.c | 62 +++++++++++++++++++++++------
> tools/perf/util/symbol.h | 9 +++++
> 5 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
> index 39dbe51..0d40e17 100644
> --- a/tools/perf/arch/powerpc/util/sym-handling.c
> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
> @@ -52,6 +52,18 @@ int arch__compare_symbol_names(const char *namea, const char *nameb)
> return strcmp(namea, nameb);
> }
> +
> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
> + unsigned int n)
> +{
> + /* Skip over initial dot */
> + if (*namea == '.')
> + namea++;
> + if (*nameb == '.')
> + nameb++;
> +
> + return strncmp(namea, nameb, n);
> +}
> #endif
> #if defined(_CALL_ELF) && _CALL_ELF == 2
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index c1870ac..f4d8272 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -325,11 +325,6 @@ int map__load(struct map *map)
> return 0;
> }
> -int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
> -{
> - return strcmp(namea, nameb);
> -}
> -
> struct symbol *map__find_symbol(struct map *map, u64 addr)
> {
> if (map__load(map) < 0)
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index c8a5a64..325bbc8 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -130,13 +130,14 @@ struct thread;
> */
> #define __map__for_each_symbol_by_name(map, sym_name, pos) \
> for (pos = map__find_symbol_by_name(map, sym_name); \
> - pos && arch__compare_symbol_names(pos->name, sym_name) == 0; \
> + pos && \
> + symbol__match_symbol_name(pos->name, sym_name, \
> + SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY) == 0; \
> pos = symbol__next_by_name(pos))
> #define map__for_each_symbol_by_name(map, sym_name, pos) \
> __map__for_each_symbol_by_name(map, sym_name, (pos))
> -int arch__compare_symbol_names(const char *namea, const char *nameb);
> void map__init(struct map *map, enum map_type type,
> u64 start, u64 end, u64 pgoff, struct dso *dso);
> struct map *map__new(struct machine *machine, u64 start, u64 len,
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 9b4d8ba..c29440d 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -87,6 +87,17 @@ static int prefix_underscores_count(const char *str)
> return tail - str;
> }
> +int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
> +{
> + return strcmp(namea, nameb);
> +}
> +
> +int __weak arch__compare_symbol_names_n(const char *namea, const char *nameb,
> + unsigned int n)
> +{
> + return strncmp(namea, nameb, n);
> +}
> +
> int __weak arch__choose_best_symbol(struct symbol *syma,
> struct symbol *symb __maybe_unused)
> {
> @@ -396,8 +407,26 @@ static void symbols__sort_by_name(struct rb_root *symbols,
> }
> }
> +int symbol__match_symbol_name(const char *name, const char *str,
> + int including_tags)
> +{
> + const char *versioning;
> +
> + if (including_tags == SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY &&
> + (versioning = strstr(name,"@@"))) {
> +
> + unsigned int len = strlen(str);
> + if (len < versioning - name)
> + len = versioning - name;
> +
> + return arch__compare_symbol_names_n(name, str, len);
> + } else
> + return arch__compare_symbol_names(name, str);
> +}
> +
> static struct symbol *symbols__find_by_name(struct rb_root *symbols,
> - const char *name)
> + const char *name,
> + unsigned int including_tags)
> {
> struct rb_node *n;
> struct symbol_name_rb_node *s = NULL;
> @@ -411,11 +440,12 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
> int cmp;
> s = rb_entry(n, struct symbol_name_rb_node, rb_node);
> - cmp = arch__compare_symbol_names(name, s->sym.name);
> + cmp = symbol__match_symbol_name(s->sym.name, name,
> + including_tags);
> - if (cmp < 0)
> + if (cmp > 0)
> n = n->rb_left;
> - else if (cmp > 0)
> + else if (cmp < 0)
What is this inversion of left/right for? Why the new cmp function
inverts the results? does it really?
> n = n->rb_right;
> else
> break;
> @@ -424,16 +454,17 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
> if (n == NULL)
> return NULL;
> - /* return first symbol that has same name (if any) */
> - for (n = rb_prev(n); n; n = rb_prev(n)) {
> - struct symbol_name_rb_node *tmp;
> + if (including_tags != SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY)
> + /* return first symbol that has same name (if any) */
> + for (n = rb_prev(n); n; n = rb_prev(n)) {
> + struct symbol_name_rb_node *tmp;
> - tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
> - if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
> - break;
> + tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
> + if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
> + break;
> - s = tmp;
> - }
> + s = tmp;
> + }
> return &s->sym;
> }
> @@ -500,7 +531,12 @@ struct symbol *symbol__next_by_name(struct symbol *sym)
> struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
> const char *name)
> {
> - return symbols__find_by_name(&dso->symbol_names[type], name);
> + struct symbol *s = symbols__find_by_name(&dso->symbol_names[type], name,
> + SYMBOLS__TAG_INCLUDE_NONE);
> + if (!s)
> + s = symbols__find_by_name(&dso->symbol_names[type], name,
> + SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY);
> + return s;
> }
> void dso__sort_by_name(struct dso *dso, enum map_type type)
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 5245d2f..bde2100 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -348,8 +348,17 @@ void arch__sym_update(struct symbol *s, GElf_Sym *sym);
> #define SYMBOL_A 0
> #define SYMBOL_B 1
> +int arch__compare_symbol_names(const char *namea, const char *nameb);
> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
> + unsigned int n);
> int arch__choose_best_symbol(struct symbol *syma, struct symbol *symb);
> +#define SYMBOLS__TAG_INCLUDE_NONE 0
> +#define SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY 1
> +
> +int symbol__match_symbol_name(const char *namea, const char *nameb,
> + int including_tags);
> +
> /* structure containing an SDT note's info */
> struct sdt_note {
> char *name; /* name of the note*/
> --
> 2.1.4
>
> Regards,
> PC
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] tools/perf: Allow user probes on versioned symbols
2017-04-06 14:36 ` Arnaldo Carvalho de Melo
@ 2017-04-06 15:56 ` Paul Clarke
0 siblings, 0 replies; 6+ messages in thread
From: Paul Clarke @ 2017-04-06 15:56 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: LKML, Arnaldo Carvalho de Melo, Masami Hiramatsu, David Ahern,
linux-perf-users
On 04/06/2017 09:36 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 05, 2017 at 10:30:03PM -0500, Paul Clarke escreveu:
>> Symbol versioning, as in glibc, results in symbols being defined as:
>> <real symbol>@[@]<version>
>> (Note that "@@" identifies a default symbol, if the symbol name
>> is repeated.)
>>
>> perf is currently unable to deal with this, and is unable to create
>> user probes at such symbols:
>> --
>> $ nm /lib/powerpc64le-linux-gnu/libpthread.so.0 | grep pthread_create
>> 0000000000008d30 t __pthread_create_2_1
>> 0000000000008d30 T pthread_create@@GLIBC_2.17
>> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
>> probe-definition(0): pthread_create
>> symbol:pthread_create file:(null) line:0 offset:0 return:0 lazy:(null)
>> 0 arguments
>> Open Debuginfo file: /usr/lib/debug/lib/powerpc64le-linux-gnu/libpthread-2.19.so
>> Try to find probe point from debuginfo.
>> Probe point 'pthread_create' not found.
>> Error: Failed to add events. Reason: No such file or directory (Code: -2)
>> --
>>
>> One is not able to specify the fully versioned symbol, either, due to
>> syntactic conflicts with other uses of "@" by perf:
>> --
>> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create@@GLIBC_2.17
>> probe-definition(0): pthread_create@@GLIBC_2.17
>> Semantic error :SRC@SRC is not allowed.
>> 0 arguments
>> Error: Command Parse Error. Reason: Invalid argument (Code: -22)
>> --
>>
>> This patch ignores the versioning tag for default symbols, thus
>> allowing probes to be created for these symbols:
>> --
>> $ /usr/bin/sudo ./perf probe -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
>> Added new event:
>> probe_libpthread:pthread_create (on pthread_create in /lib/powerpc64le-linux-gnu/libpthread-2.19.so)
>>
>> You can now use it in all perf tools, such as:
>>
>> perf record -e probe_libpthread:pthread_create -aR sleep 1
>>
>> $ /usr/bin/sudo ./perf record -e probe_libpthread:pthread_create -aR ./test 2
>> [ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.052 MB perf.data (2 samples) ]
>> $ /usr/bin/sudo ./perf script
>> test 2915 [000] 19124.260729: probe_libpthread:pthread_create: (3fff99248d38)
>> test 2916 [000] 19124.260962: probe_libpthread:pthread_create: (3fff99248d38)
>> $ /usr/bin/sudo ./perf probe --del=probe_libpthread:pthread_create
>> Removed event: probe_libpthread:pthread_create
>> --
>>
>> Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
>> ---
>> v4:
>> - rebased to acme/perf/core
>> - moved changes from "map" namespace to "symbol" namespace,
>> - rewrote symbol__compare (now *match) to avoid need for strdup
>> - new symbol__match_symbol_name to support versioned symbols, ignoring default
>> tags
>> - new arch__compare_symbol_names_n to map to strncmp
>> - dso__find_symbol_by_name: now tries exact match (as before), then tries
>> also adding symbols tagged as default (@@)
>> - symbols__find_by_name: new parameter to support finding with or without default
>> tags
>> - does NOT handle setting probes using the tagged symbol name (symbol@[@]tag)
>>
>> v3:
>> - code style fixes per David Ahern
>>
>> v2:
>> - move logic from arch__compare_symbol_names to new map__compare_symbol_names
>> - call through from map__compare_symbol_names to arch__compare_symbol_names
>> - redirect uses of arch__compare_symbol_names
>> - send patch to LKML
>> tools/perf/arch/powerpc/util/sym-handling.c | 12 ++++++
>> tools/perf/util/map.c | 5 ---
>> tools/perf/util/map.h | 5 ++-
>> tools/perf/util/symbol.c | 62 +++++++++++++++++++++++------
>> tools/perf/util/symbol.h | 9 +++++
>> 5 files changed, 73 insertions(+), 20 deletions(-)
[...]
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 9b4d8ba..c29440d 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -87,6 +87,17 @@ static int prefix_underscores_count(const char *str)
>> return tail - str;
>> }
>> +int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
>> +{
>> + return strcmp(namea, nameb);
>> +}
>> +
>> +int __weak arch__compare_symbol_names_n(const char *namea, const char *nameb,
>> + unsigned int n)
>> +{
>> + return strncmp(namea, nameb, n);
>> +}
>> +
>> int __weak arch__choose_best_symbol(struct symbol *syma,
>> struct symbol *symb __maybe_unused)
>> {
>> @@ -396,8 +407,26 @@ static void symbols__sort_by_name(struct rb_root *symbols,
>> }
>> }
>> +int symbol__match_symbol_name(const char *name, const char *str,
>> + int including_tags)
>> +{
>> + const char *versioning;
>> +
>> + if (including_tags == SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY &&
>> + (versioning = strstr(name,"@@"))) {
>> +
>> + unsigned int len = strlen(str);
>> + if (len < versioning - name)
>> + len = versioning - name;
>> +
>> + return arch__compare_symbol_names_n(name, str, len);
>> + } else
>> + return arch__compare_symbol_names(name, str);
>> +}
>> +
>> static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>> - const char *name)
>> + const char *name,
>> + unsigned int including_tags)
>> {
>> struct rb_node *n;
>> struct symbol_name_rb_node *s = NULL;
>> @@ -411,11 +440,12 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>> int cmp;
>> s = rb_entry(n, struct symbol_name_rb_node, rb_node);
>> - cmp = arch__compare_symbol_names(name, s->sym.name);
>> + cmp = symbol__match_symbol_name(s->sym.name, name,
>> + including_tags);
>> - if (cmp < 0)
>> + if (cmp > 0)
>> n = n->rb_left;
>> - else if (cmp > 0)
>> + else if (cmp < 0)
>
> What is this inversion of left/right for? Why the new cmp function
> inverts the results? does it really?
Good question. I changed the symbol/string matching function from a "compare two strings exactly" function to a "match a (possibly versioned) symbol name with a string" function. A small optimization, to avoid worrying about both strings being versioned, is that the new function's API expects the possibly versioned symbol name as the first parameter, and the string as the 2nd. I figured the first parameter is the most significant and should be the symbol. That forced the parameter reversal, and the subsequent comparison inversions.
>> n = n->rb_right;
>> else
>> break;
PC
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] tools/perf: Allow user probes on versioned symbols
2017-04-06 3:30 [PATCH v4] tools/perf: Allow user probes on versioned symbols Paul Clarke
2017-04-06 14:36 ` Arnaldo Carvalho de Melo
@ 2017-04-10 13:11 ` Masami Hiramatsu
2017-04-10 14:53 ` Paul Clarke
1 sibling, 1 reply; 6+ messages in thread
From: Masami Hiramatsu @ 2017-04-10 13:11 UTC (permalink / raw)
To: pc; +Cc: LKML, Arnaldo Carvalho de Melo, David Ahern, linux-perf-users
On Wed, 5 Apr 2017 22:30:03 -0500
Paul Clarke <pc@us.ibm.com> wrote:
> Symbol versioning, as in glibc, results in symbols being defined as:
> <real symbol>@[@]<version>
> (Note that "@@" identifies a default symbol, if the symbol name
> is repeated.)
>
> perf is currently unable to deal with this, and is unable to create
> user probes at such symbols:
> --
> $ nm /lib/powerpc64le-linux-gnu/libpthread.so.0 | grep pthread_create
> 0000000000008d30 t __pthread_create_2_1
> 0000000000008d30 T pthread_create@@GLIBC_2.17
> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
> probe-definition(0): pthread_create
> symbol:pthread_create file:(null) line:0 offset:0 return:0 lazy:(null)
> 0 arguments
> Open Debuginfo file: /usr/lib/debug/lib/powerpc64le-linux-gnu/libpthread-2.19.so
> Try to find probe point from debuginfo.
> Probe point 'pthread_create' not found.
> Error: Failed to add events. Reason: No such file or directory (Code: -2)
> --
>
> One is not able to specify the fully versioned symbol, either, due to
> syntactic conflicts with other uses of "@" by perf:
> --
> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create@@GLIBC_2.17
> probe-definition(0): pthread_create@@GLIBC_2.17
> Semantic error :SRC@SRC is not allowed.
> 0 arguments
> Error: Command Parse Error. Reason: Invalid argument (Code: -22)
> --
>
> This patch ignores the versioning tag for default symbols, thus
> allowing probes to be created for these symbols:
> --
> $ /usr/bin/sudo ./perf probe -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
> Added new event:
> probe_libpthread:pthread_create (on pthread_create in /lib/powerpc64le-linux-gnu/libpthread-2.19.so)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe_libpthread:pthread_create -aR sleep 1
>
> $ /usr/bin/sudo ./perf record -e probe_libpthread:pthread_create -aR ./test 2
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.052 MB perf.data (2 samples) ]
> $ /usr/bin/sudo ./perf script
> test 2915 [000] 19124.260729: probe_libpthread:pthread_create: (3fff99248d38)
> test 2916 [000] 19124.260962: probe_libpthread:pthread_create: (3fff99248d38)
> $ /usr/bin/sudo ./perf probe --del=probe_libpthread:pthread_create
> Removed event: probe_libpthread:pthread_create
> --
>
> Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
> ---
> v4:
> - rebased to acme/perf/core
> - moved changes from "map" namespace to "symbol" namespace,
> - rewrote symbol__compare (now *match) to avoid need for strdup
> - new symbol__match_symbol_name to support versioned symbols, ignoring default
> tags
> - new arch__compare_symbol_names_n to map to strncmp
> - dso__find_symbol_by_name: now tries exact match (as before), then tries
> also adding symbols tagged as default (@@)
> - symbols__find_by_name: new parameter to support finding with or without default
> tags
> - does NOT handle setting probes using the tagged symbol name (symbol@[@]tag)
>
> v3:
> - code style fixes per David Ahern
>
> v2:
> - move logic from arch__compare_symbol_names to new map__compare_symbol_names
> - call through from map__compare_symbol_names to arch__compare_symbol_names
> - redirect uses of arch__compare_symbol_names
> - send patch to LKML
> tools/perf/arch/powerpc/util/sym-handling.c | 12 ++++++
> tools/perf/util/map.c | 5 ---
> tools/perf/util/map.h | 5 ++-
> tools/perf/util/symbol.c | 62 +++++++++++++++++++++++------
> tools/perf/util/symbol.h | 9 +++++
> 5 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
> index 39dbe51..0d40e17 100644
> --- a/tools/perf/arch/powerpc/util/sym-handling.c
> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
> @@ -52,6 +52,18 @@ int arch__compare_symbol_names(const char *namea, const char *nameb)
>
> return strcmp(namea, nameb);
> }
> +
> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
> + unsigned int n)
> +{
> + /* Skip over initial dot */
> + if (*namea == '.')
> + namea++;
> + if (*nameb == '.')
> + nameb++;
> +
> + return strncmp(namea, nameb, n);
> +}
> #endif
>
> #if defined(_CALL_ELF) && _CALL_ELF == 2
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index c1870ac..f4d8272 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -325,11 +325,6 @@ int map__load(struct map *map)
> return 0;
> }
>
> -int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
> -{
> - return strcmp(namea, nameb);
> -}
> -
> struct symbol *map__find_symbol(struct map *map, u64 addr)
> {
> if (map__load(map) < 0)
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index c8a5a64..325bbc8 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -130,13 +130,14 @@ struct thread;
> */
> #define __map__for_each_symbol_by_name(map, sym_name, pos) \
> for (pos = map__find_symbol_by_name(map, sym_name); \
> - pos && arch__compare_symbol_names(pos->name, sym_name) == 0; \
> + pos && \
> + symbol__match_symbol_name(pos->name, sym_name, \
> + SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY) == 0; \
> pos = symbol__next_by_name(pos))
>
> #define map__for_each_symbol_by_name(map, sym_name, pos) \
> __map__for_each_symbol_by_name(map, sym_name, (pos))
>
> -int arch__compare_symbol_names(const char *namea, const char *nameb);
> void map__init(struct map *map, enum map_type type,
> u64 start, u64 end, u64 pgoff, struct dso *dso);
> struct map *map__new(struct machine *machine, u64 start, u64 len,
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 9b4d8ba..c29440d 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -87,6 +87,17 @@ static int prefix_underscores_count(const char *str)
> return tail - str;
> }
>
> +int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
> +{
> + return strcmp(namea, nameb);
> +}
> +
> +int __weak arch__compare_symbol_names_n(const char *namea, const char *nameb,
> + unsigned int n)
> +{
> + return strncmp(namea, nameb, n);
> +}
> +
> int __weak arch__choose_best_symbol(struct symbol *syma,
> struct symbol *symb __maybe_unused)
> {
> @@ -396,8 +407,26 @@ static void symbols__sort_by_name(struct rb_root *symbols,
> }
> }
>
> +int symbol__match_symbol_name(const char *name, const char *str,
> + int including_tags)
Hmm, why wouldn't you use bool for including_tags?
> +{
> + const char *versioning;
> +
> + if (including_tags == SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY &&
> + (versioning = strstr(name,"@@"))) {
> +
> + unsigned int len = strlen(str);
> + if (len < versioning - name)
> + len = versioning - name;
> +
> + return arch__compare_symbol_names_n(name, str, len);
> + } else
> + return arch__compare_symbol_names(name, str);
> +}
> +
> static struct symbol *symbols__find_by_name(struct rb_root *symbols,
> - const char *name)
> + const char *name,
> + unsigned int including_tags)
And here, you are using "unsigned int".
> {
> struct rb_node *n;
> struct symbol_name_rb_node *s = NULL;
> @@ -411,11 +440,12 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
> int cmp;
>
> s = rb_entry(n, struct symbol_name_rb_node, rb_node);
> - cmp = arch__compare_symbol_names(name, s->sym.name);
> + cmp = symbol__match_symbol_name(s->sym.name, name,
> + including_tags);
>
> - if (cmp < 0)
> + if (cmp > 0)
> n = n->rb_left;
> - else if (cmp > 0)
> + else if (cmp < 0)
> n = n->rb_right;
> else
> break;
> @@ -424,16 +454,17 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
> if (n == NULL)
> return NULL;
>
> - /* return first symbol that has same name (if any) */
> - for (n = rb_prev(n); n; n = rb_prev(n)) {
> - struct symbol_name_rb_node *tmp;
> + if (including_tags != SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY)
> + /* return first symbol that has same name (if any) */
> + for (n = rb_prev(n); n; n = rb_prev(n)) {
> + struct symbol_name_rb_node *tmp;
>
> - tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
> - if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
> - break;
> + tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
> + if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
> + break;
>
> - s = tmp;
> - }
> + s = tmp;
> + }
>
> return &s->sym;
> }
> @@ -500,7 +531,12 @@ struct symbol *symbol__next_by_name(struct symbol *sym)
> struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
> const char *name)
> {
> - return symbols__find_by_name(&dso->symbol_names[type], name);
> + struct symbol *s = symbols__find_by_name(&dso->symbol_names[type], name,
> + SYMBOLS__TAG_INCLUDE_NONE);
> + if (!s)
> + s = symbols__find_by_name(&dso->symbol_names[type], name,
> + SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY);
> + return s;
> }
>
> void dso__sort_by_name(struct dso *dso, enum map_type type)
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 5245d2f..bde2100 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -348,8 +348,17 @@ void arch__sym_update(struct symbol *s, GElf_Sym *sym);
> #define SYMBOL_A 0
> #define SYMBOL_B 1
>
> +int arch__compare_symbol_names(const char *namea, const char *nameb);
> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
> + unsigned int n);
> int arch__choose_best_symbol(struct symbol *syma, struct symbol *symb);
>
> +#define SYMBOLS__TAG_INCLUDE_NONE 0
> +#define SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY 1
Hmm, even if you like to pass an obvious flag, these should be
defined by enum, and please choose simpler and clear flag name.
E.g.
enum {
SYMBOLS__COMP_WITH_TAG,
SYMBOLS__COMP_WITHOUT_TAG,
};
Thank you,
> +
> +int symbol__match_symbol_name(const char *namea, const char *nameb,
> + int including_tags);
> +
> /* structure containing an SDT note's info */
> struct sdt_note {
> char *name; /* name of the note*/
> --
> 2.1.4
>
> Regards,
> PC
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] tools/perf: Allow user probes on versioned symbols
2017-04-10 13:11 ` Masami Hiramatsu
@ 2017-04-10 14:53 ` Paul Clarke
2017-04-11 0:00 ` Masami Hiramatsu
0 siblings, 1 reply; 6+ messages in thread
From: Paul Clarke @ 2017-04-10 14:53 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: LKML, Arnaldo Carvalho de Melo, David Ahern, linux-perf-users
On 04/10/2017 08:11 AM, Masami Hiramatsu wrote:
> On Wed, 5 Apr 2017 22:30:03 -0500
> Paul Clarke <pc@us.ibm.com> wrote:
>> Symbol versioning, as in glibc, results in symbols being defined as:
>> <real symbol>@[@]<version>
>> (Note that "@@" identifies a default symbol, if the symbol name
>> is repeated.)
>>
>> perf is currently unable to deal with this, and is unable to create
>> user probes at such symbols:
>> --
>> $ nm /lib/powerpc64le-linux-gnu/libpthread.so.0 | grep pthread_create
>> 0000000000008d30 t __pthread_create_2_1
>> 0000000000008d30 T pthread_create@@GLIBC_2.17
>> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
>> probe-definition(0): pthread_create
>> symbol:pthread_create file:(null) line:0 offset:0 return:0 lazy:(null)
>> 0 arguments
>> Open Debuginfo file: /usr/lib/debug/lib/powerpc64le-linux-gnu/libpthread-2.19.so
>> Try to find probe point from debuginfo.
>> Probe point 'pthread_create' not found.
>> Error: Failed to add events. Reason: No such file or directory (Code: -2)
>> --
>>
>> One is not able to specify the fully versioned symbol, either, due to
>> syntactic conflicts with other uses of "@" by perf:
>> --
>> $ /usr/bin/sudo perf probe -v -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create@@GLIBC_2.17
>> probe-definition(0): pthread_create@@GLIBC_2.17
>> Semantic error :SRC@SRC is not allowed.
>> 0 arguments
>> Error: Command Parse Error. Reason: Invalid argument (Code: -22)
>> --
>>
>> This patch ignores the versioning tag for default symbols, thus
>> allowing probes to be created for these symbols:
>> --
>> $ /usr/bin/sudo ./perf probe -x /lib/powerpc64le-linux-gnu/libpthread.so.0 pthread_create
>> Added new event:
>> probe_libpthread:pthread_create (on pthread_create in /lib/powerpc64le-linux-gnu/libpthread-2.19.so)
>>
>> You can now use it in all perf tools, such as:
>>
>> perf record -e probe_libpthread:pthread_create -aR sleep 1
>>
>> $ /usr/bin/sudo ./perf record -e probe_libpthread:pthread_create -aR ./test 2
>> [ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 0.052 MB perf.data (2 samples) ]
>> $ /usr/bin/sudo ./perf script
>> test 2915 [000] 19124.260729: probe_libpthread:pthread_create: (3fff99248d38)
>> test 2916 [000] 19124.260962: probe_libpthread:pthread_create: (3fff99248d38)
>> $ /usr/bin/sudo ./perf probe --del=probe_libpthread:pthread_create
>> Removed event: probe_libpthread:pthread_create
>> --
>>
>> Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
>> ---
>> v4:
>> - rebased to acme/perf/core
>> - moved changes from "map" namespace to "symbol" namespace,
>> - rewrote symbol__compare (now *match) to avoid need for strdup
>> - new symbol__match_symbol_name to support versioned symbols, ignoring default
>> tags
>> - new arch__compare_symbol_names_n to map to strncmp
>> - dso__find_symbol_by_name: now tries exact match (as before), then tries
>> also adding symbols tagged as default (@@)
>> - symbols__find_by_name: new parameter to support finding with or without default
>> tags
>> - does NOT handle setting probes using the tagged symbol name (symbol@[@]tag)
>>
>> v3:
>> - code style fixes per David Ahern
>>
>> v2:
>> - move logic from arch__compare_symbol_names to new map__compare_symbol_names
>> - call through from map__compare_symbol_names to arch__compare_symbol_names
>> - redirect uses of arch__compare_symbol_names
>> - send patch to LKML
>> tools/perf/arch/powerpc/util/sym-handling.c | 12 ++++++
>> tools/perf/util/map.c | 5 ---
>> tools/perf/util/map.h | 5 ++-
>> tools/perf/util/symbol.c | 62 +++++++++++++++++++++++------
>> tools/perf/util/symbol.h | 9 +++++
>> 5 files changed, 73 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
>> index 39dbe51..0d40e17 100644
>> --- a/tools/perf/arch/powerpc/util/sym-handling.c
>> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
>> @@ -52,6 +52,18 @@ int arch__compare_symbol_names(const char *namea, const char *nameb)
>>
>> return strcmp(namea, nameb);
>> }
>> +
>> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
>> + unsigned int n)
>> +{
>> + /* Skip over initial dot */
>> + if (*namea == '.')
>> + namea++;
>> + if (*nameb == '.')
>> + nameb++;
>> +
>> + return strncmp(namea, nameb, n);
>> +}
>> #endif
>>
>> #if defined(_CALL_ELF) && _CALL_ELF == 2
>> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
>> index c1870ac..f4d8272 100644
>> --- a/tools/perf/util/map.c
>> +++ b/tools/perf/util/map.c
>> @@ -325,11 +325,6 @@ int map__load(struct map *map)
>> return 0;
>> }
>>
>> -int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
>> -{
>> - return strcmp(namea, nameb);
>> -}
>> -
>> struct symbol *map__find_symbol(struct map *map, u64 addr)
>> {
>> if (map__load(map) < 0)
>> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
>> index c8a5a64..325bbc8 100644
>> --- a/tools/perf/util/map.h
>> +++ b/tools/perf/util/map.h
>> @@ -130,13 +130,14 @@ struct thread;
>> */
>> #define __map__for_each_symbol_by_name(map, sym_name, pos) \
>> for (pos = map__find_symbol_by_name(map, sym_name); \
>> - pos && arch__compare_symbol_names(pos->name, sym_name) == 0; \
>> + pos && \
>> + symbol__match_symbol_name(pos->name, sym_name, \
>> + SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY) == 0; \
>> pos = symbol__next_by_name(pos))
>>
>> #define map__for_each_symbol_by_name(map, sym_name, pos) \
>> __map__for_each_symbol_by_name(map, sym_name, (pos))
>>
>> -int arch__compare_symbol_names(const char *namea, const char *nameb);
>> void map__init(struct map *map, enum map_type type,
>> u64 start, u64 end, u64 pgoff, struct dso *dso);
>> struct map *map__new(struct machine *machine, u64 start, u64 len,
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 9b4d8ba..c29440d 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -87,6 +87,17 @@ static int prefix_underscores_count(const char *str)
>> return tail - str;
>> }
>>
>> +int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
>> +{
>> + return strcmp(namea, nameb);
>> +}
>> +
>> +int __weak arch__compare_symbol_names_n(const char *namea, const char *nameb,
>> + unsigned int n)
>> +{
>> + return strncmp(namea, nameb, n);
>> +}
>> +
>> int __weak arch__choose_best_symbol(struct symbol *syma,
>> struct symbol *symb __maybe_unused)
>> {
>> @@ -396,8 +407,26 @@ static void symbols__sort_by_name(struct rb_root *symbols,
>> }
>> }
>>
>> +int symbol__match_symbol_name(const char *name, const char *str,
>> + int including_tags)
>
> Hmm, why wouldn't you use bool for including_tags?
This was to allow for the possibility of returning multiple matches in the future.
>> +{
>> + const char *versioning;
>> +
>> + if (including_tags == SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY &&
>> + (versioning = strstr(name,"@@"))) {
>> +
>> + unsigned int len = strlen(str);
>> + if (len < versioning - name)
>> + len = versioning - name;
>> +
>> + return arch__compare_symbol_names_n(name, str, len);
>> + } else
>> + return arch__compare_symbol_names(name, str);
>> +}
>> +
>> static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>> - const char *name)
>> + const char *name,
>> + unsigned int including_tags)
>
> And here, you are using "unsigned int".
That's my mistake.
>> {
>> struct rb_node *n;
>> struct symbol_name_rb_node *s = NULL;
>> @@ -411,11 +440,12 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>> int cmp;
>>
>> s = rb_entry(n, struct symbol_name_rb_node, rb_node);
>> - cmp = arch__compare_symbol_names(name, s->sym.name);
>> + cmp = symbol__match_symbol_name(s->sym.name, name,
>> + including_tags);
>>
>> - if (cmp < 0)
>> + if (cmp > 0)
>> n = n->rb_left;
>> - else if (cmp > 0)
>> + else if (cmp < 0)
>> n = n->rb_right;
>> else
>> break;
>> @@ -424,16 +454,17 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>> if (n == NULL)
>> return NULL;
>>
>> - /* return first symbol that has same name (if any) */
>> - for (n = rb_prev(n); n; n = rb_prev(n)) {
>> - struct symbol_name_rb_node *tmp;
>> + if (including_tags != SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY)
>> + /* return first symbol that has same name (if any) */
>> + for (n = rb_prev(n); n; n = rb_prev(n)) {
>> + struct symbol_name_rb_node *tmp;
>>
>> - tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
>> - if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
>> - break;
>> + tmp = rb_entry(n, struct symbol_name_rb_node, rb_node);
>> + if (arch__compare_symbol_names(tmp->sym.name, s->sym.name))
>> + break;
>>
>> - s = tmp;
>> - }
>> + s = tmp;
>> + }
>>
>> return &s->sym;
>> }
>> @@ -500,7 +531,12 @@ struct symbol *symbol__next_by_name(struct symbol *sym)
>> struct symbol *dso__find_symbol_by_name(struct dso *dso, enum map_type type,
>> const char *name)
>> {
>> - return symbols__find_by_name(&dso->symbol_names[type], name);
>> + struct symbol *s = symbols__find_by_name(&dso->symbol_names[type], name,
>> + SYMBOLS__TAG_INCLUDE_NONE);
>> + if (!s)
>> + s = symbols__find_by_name(&dso->symbol_names[type], name,
>> + SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY);
>> + return s;
>> }
>>
>> void dso__sort_by_name(struct dso *dso, enum map_type type)
>> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
>> index 5245d2f..bde2100 100644
>> --- a/tools/perf/util/symbol.h
>> +++ b/tools/perf/util/symbol.h
>> @@ -348,8 +348,17 @@ void arch__sym_update(struct symbol *s, GElf_Sym *sym);
>> #define SYMBOL_A 0
>> #define SYMBOL_B 1
>>
>> +int arch__compare_symbol_names(const char *namea, const char *nameb);
>> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
>> + unsigned int n);
>> int arch__choose_best_symbol(struct symbol *syma, struct symbol *symb);
>>
>> +#define SYMBOLS__TAG_INCLUDE_NONE 0
>> +#define SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY 1
>
> Hmm, even if you like to pass an obvious flag, these should be
> defined by enum, and please choose simpler and clear flag name.
>
> E.g.
> enum {
> SYMBOLS__COMP_WITH_TAG,
> SYMBOLS__COMP_WITHOUT_TAG,
> };
Earlier in this thread, we discussed supporting non-default tagged symbols (those tagged with "@version"). To allow for that, I was thinking of these three cases:
- SYMBOLS__TAG_INCLUDE_NONE: ignore all tagged symbols (the current behavior)
- SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY: match tagged symbols when tagged as default ("@@"; added with this patch)
- SYMBOLS__TAG_INCLUDE_ALL: match all tagged symbols ("@" and "@@"; future)
For the names of the enums, I chose "SYMBOLS__TAG..." to put it first in the SYMBOLS namespace, and second in a "TAG" sub-namespace.
I'm happy to fix the discrepancy with signed/unsigned by changing to enum.
Let me know what enum names are preferred, given the above discussion, and I'll post a new patch.
Thanks for the review!
PC
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] tools/perf: Allow user probes on versioned symbols
2017-04-10 14:53 ` Paul Clarke
@ 2017-04-11 0:00 ` Masami Hiramatsu
0 siblings, 0 replies; 6+ messages in thread
From: Masami Hiramatsu @ 2017-04-11 0:00 UTC (permalink / raw)
To: pc; +Cc: LKML, Arnaldo Carvalho de Melo, David Ahern, linux-perf-users
On Mon, 10 Apr 2017 09:53:48 -0500
Paul Clarke <pc@us.ibm.com> wrote:
> >> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> >> index 5245d2f..bde2100 100644
> >> --- a/tools/perf/util/symbol.h
> >> +++ b/tools/perf/util/symbol.h
> >> @@ -348,8 +348,17 @@ void arch__sym_update(struct symbol *s, GElf_Sym *sym);
> >> #define SYMBOL_A 0
> >> #define SYMBOL_B 1
> >>
> >> +int arch__compare_symbol_names(const char *namea, const char *nameb);
> >> +int arch__compare_symbol_names_n(const char *namea, const char *nameb,
> >> + unsigned int n);
> >> int arch__choose_best_symbol(struct symbol *syma, struct symbol *symb);
> >>
> >> +#define SYMBOLS__TAG_INCLUDE_NONE 0
> >> +#define SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY 1
> >
> > Hmm, even if you like to pass an obvious flag, these should be
> > defined by enum, and please choose simpler and clear flag name.
> >
> > E.g.
> > enum {
> > SYMBOLS__COMP_WITH_TAG,
> > SYMBOLS__COMP_WITHOUT_TAG,
> > };
>
> Earlier in this thread, we discussed supporting non-default tagged symbols (those tagged with "@version"). To allow for that, I was thinking of these three cases:
> - SYMBOLS__TAG_INCLUDE_NONE: ignore all tagged symbols (the current behavior)
> - SYMBOLS__TAG_INCLUDE_DEFAULT_ONLY: match tagged symbols when tagged as default ("@@"; added with this patch)
> - SYMBOLS__TAG_INCLUDE_ALL: match all tagged symbols ("@" and "@@"; future)
Ah, I see now.
> For the names of the enums, I chose "SYMBOLS__TAG..." to put it first in the SYMBOLS namespace, and second in a "TAG" sub-namespace.
For the namespace, we should use same namespase as enum's name.
> I'm happy to fix the discrepancy with signed/unsigned by changing to enum.
>
> Let me know what enum names are preferred, given the above discussion, and I'll post a new patch.
OK, then what about enum symbol_tag and use "SYMBOL_TAG__" for namespace?
Thank you,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-11 0:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-06 3:30 [PATCH v4] tools/perf: Allow user probes on versioned symbols Paul Clarke
2017-04-06 14:36 ` Arnaldo Carvalho de Melo
2017-04-06 15:56 ` Paul Clarke
2017-04-10 13:11 ` Masami Hiramatsu
2017-04-10 14:53 ` Paul Clarke
2017-04-11 0:00 ` Masami Hiramatsu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.