[PATCH v3] Allow user probes on versioned symbols

[PATCH v3] Allow user probes on versioned symbols

[Paul Clarke]

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)
--

The attached patch ignores versioning 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>
---
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/util/auxtrace.c |  2 +-
  tools/perf/util/map.c      | 23 +++++++++++++++++++++++
  tools/perf/util/map.h      |  3 ++-
  tools/perf/util/symbol.c   |  4 ++--
  4 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index c5a6e0b1..f3511a6 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -1875,7 +1875,7 @@ static bool dso_sym_match(struct symbol *sym, const char *name, int *cnt,
  			  int idx)
  {
  	/* Same name, and global or the n'th found or any */
-	return !arch__compare_symbol_names(name, sym->name) &&
+	return !map__compare_symbol_names(name, sym->name) &&
  	       ((!idx && sym->binding == STB_GLOBAL) ||
  		(idx > 0 && ++*cnt == idx) ||
  		idx < 0);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 4f9a71c..26a3e87 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -330,6 +330,29 @@ int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
  	return strcmp(namea, nameb);
  }
  
+int map__compare_symbol_names(const char *namea, const char *nameb)
+{
+	int rc;
+	const char *namea_versioning, *nameb_versioning;
+
+	namea_versioning = strstr(namea,"@@");
+	if (namea_versioning)
+		namea = strndup(namea,namea_versioning-namea);
+
+	nameb_versioning = strstr(nameb,"@@");
+	if (nameb_versioning)
+		nameb = strndup(nameb,nameb_versioning-nameb);
+
+	rc = arch__compare_symbol_names(namea, nameb);
+
+	if (namea_versioning)
+		free((void *)namea);
+	if (nameb_versioning)
+		free((void *)nameb);
+
+	return rc;
+}
+
  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 abdacf8..aaad64d 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 && map__compare_symbol_names(pos->name, sym_name) == 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);
+int map__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 dc93940..4fb9d275 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -411,7 +411,7 @@ 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 = map__compare_symbol_names(name, s->sym.name);
  
  		if (cmp < 0)
  			n = n->rb_left;
@@ -429,7 +429,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
  		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))
+		if (map__compare_symbol_names(tmp->sym.name, s->sym.name))
  			break;
  
  		s = tmp;
-- 
2.1.4

Regards,
PC

Re: [PATCH v3] Allow user probes on versioned symbols

[Arnaldo Carvalho de Melo]

Em Fri, Mar 31, 2017 at 11:06:16AM -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:

On top of what tree/branch should I try to apply this?

Trying on acme/perf/core:

[acme@jouet linux]$ patch -p1 < /wb/1.patch 
patching file tools/perf/util/auxtrace.c
Hunk #1 FAILED at 1875.
1 out of 1 hunk FAILED -- saving rejects to file tools/perf/util/auxtrace.c.rej
patching file tools/perf/util/map.c
Hunk #1 FAILED at 330.
1 out of 1 hunk FAILED -- saving rejects to file tools/perf/util/map.c.rej
patching file tools/perf/util/map.h
Hunk #1 FAILED at 130.
1 out of 1 hunk FAILED -- saving rejects to file tools/perf/util/map.h.rej
patching file tools/perf/util/symbol.c
Hunk #1 FAILED at 411.
Hunk #2 FAILED at 429.
2 out of 2 hunks FAILED -- saving rejects to file tools/perf/util/symbol.c.rej
[acme@jouet linux.copy]$ 

Apart from that, you are not checking the return of strndup, that
however unlikely, can fail, so must be checked.

On the style front you sometimes add a space after commas, sometimes
not, please make sure you add one.

But apart from those problems, I think that one should be able to ask
for a versioned symbol, to probe just apps using that specific version,
for instance, we should consider the whole name as two functions, which
in fact, they are, no?

Additionaly, I can't reproduce your problem here, on x86_64:

[root@jouet linux]# nm /lib64/libpthread-2.24.so  | grep ' pthread_cond_timedwait'
000000000000dd90 T pthread_cond_timedwait@GLIBC_2.2.5
000000000000d6e0 T pthread_cond_timedwait@@GLIBC_2.3.2
[root@jouet linux]#

[root@jouet linux]# perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait@GLIBC_2.2.5
Probe point 'pthread_cond_timedwait@GLIBC_2.2.5' not found.
  Error: Failed to add events.
[root@jouet linux]# 

- Arnaldo

> --
> $ 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)
> --
> 
> The attached patch ignores versioning 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>
> ---
> 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/util/auxtrace.c |  2 +-
>  tools/perf/util/map.c      | 23 +++++++++++++++++++++++
>  tools/perf/util/map.h      |  3 ++-
>  tools/perf/util/symbol.c   |  4 ++--
>  4 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index c5a6e0b1..f3511a6 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -1875,7 +1875,7 @@ static bool dso_sym_match(struct symbol *sym, const char *name, int *cnt,
>  			  int idx)
>  {
>  	/* Same name, and global or the n'th found or any */
> -	return !arch__compare_symbol_names(name, sym->name) &&
> +	return !map__compare_symbol_names(name, sym->name) &&
>  	       ((!idx && sym->binding == STB_GLOBAL) ||
>  		(idx > 0 && ++*cnt == idx) ||
>  		idx < 0);
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 4f9a71c..26a3e87 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -330,6 +330,29 @@ int __weak arch__compare_symbol_names(const char *namea, const char *nameb)
>  	return strcmp(namea, nameb);
>  }
> +int map__compare_symbol_names(const char *namea, const char *nameb)
> +{
> +	int rc;
> +	const char *namea_versioning, *nameb_versioning;
> +
> +	namea_versioning = strstr(namea,"@@");
> +	if (namea_versioning)
> +		namea = strndup(namea,namea_versioning-namea);
> +
> +	nameb_versioning = strstr(nameb,"@@");
> +	if (nameb_versioning)
> +		nameb = strndup(nameb,nameb_versioning-nameb);
> +
> +	rc = arch__compare_symbol_names(namea, nameb);
> +
> +	if (namea_versioning)
> +		free((void *)namea);
> +	if (nameb_versioning)
> +		free((void *)nameb);
> +
> +	return rc;
> +}
> +
>  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 abdacf8..aaad64d 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 && map__compare_symbol_names(pos->name, sym_name) == 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);
> +int map__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 dc93940..4fb9d275 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -411,7 +411,7 @@ 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 = map__compare_symbol_names(name, s->sym.name);
>  		if (cmp < 0)
>  			n = n->rb_left;
> @@ -429,7 +429,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
>  		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))
> +		if (map__compare_symbol_names(tmp->sym.name, s->sym.name))
>  			break;
>  		s = tmp;
> -- 
> 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

Re: [PATCH v3] Allow user probes on versioned symbols

[Paul Clarke]

On 03/31/2017 12:31 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Mar 31, 2017 at 11:06:16AM -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:
>
> On top of what tree/branch should I try to apply this?

I worked from torvalds/linux.

> Trying on acme/perf/core:

Pardon my ignorance, but where can I find that tree?

> [acme@jouet linux]$ patch -p1 < /wb/1.patch
> patching file tools/perf/util/auxtrace.c
> Hunk #1 FAILED at 1875.
[...]

> Apart from that, you are not checking the return of strndup, that
> however unlikely, can fail, so must be checked.

It's in the middle of strcmp-type function, so all return values are valid.  Shall I emit a message and call exit()?

> On the style front you sometimes add a space after commas, sometimes
> not, please make sure you add one.

Ack.

> But apart from those problems, I think that one should be able to ask
> for a versioned symbol, to probe just apps using that specific version,

I agree, but wasn't trying to tackle that at the moment.  I can look into it, though.

> for instance, we should consider the whole name as two functions, which
> in fact, they are, no?

I'm not sure I understand what you mean here.  Do you mean we should set a probe at every version of a given symbol name?  For example, if there are symbols:
a@@V2
a@V1.1
a@V1

...for a request to set a probe at "a", we'd actually set a probe at all 3?

> Additionaly, I can't reproduce your problem here, on x86_64:

I just cloned from acme/linux, and will rebase to there, if that's the best tree.

PC

Re: [PATCH v3] Allow user probes on versioned symbols

[Arnaldo Carvalho de Melo]

Em Fri, Mar 31, 2017 at 02:38:11PM -0500, Paul Clarke escreveu:
> On 03/31/2017 12:31 PM, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Mar 31, 2017 at 11:06:16AM -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:
> > 
> > On top of what tree/branch should I try to apply this?
> 
> I worked from torvalds/linux.
> 
> > Trying on acme/perf/core:
> 
> Pardon my ignorance, but where can I find that tree?

see below, but I think you got it already :-)
 
> > [acme@jouet linux]$ patch -p1 < /wb/1.patch
> > patching file tools/perf/util/auxtrace.c
> > Hunk #1 FAILED at 1875.
> [...]
> 
> > Apart from that, you are not checking the return of strndup, that
> > however unlikely, can fail, so must be checked.
 
> It's in the middle of strcmp-type function, so all return values are
> valid.  Shall I emit a message and call exit()?

See below for an alternative on how to implement this
 
> > On the style front you sometimes add a space after commas, sometimes
> > not, please make sure you add one.
 
> Ack.
 
> > But apart from those problems, I think that one should be able to ask
> > for a versioned symbol, to probe just apps using that specific version,
 
> I agree, but wasn't trying to tackle that at the moment.  I can look into it, though.
 
> > for instance, we should consider the whole name as two functions, which
> > in fact, they are, no?
 
> I'm not sure I understand what you mean here.  Do you mean we should set a probe at every version of a given symbol name?  For example, if there are symbols:
> a@@V2
> a@V1.1
> a@V1
 
> ...for a request to set a probe at "a", we'd actually set a probe at all 3?

I think that we should just probe the default for that symbol and have a
way to probe all of them, perhaps using the wildcard, i.e.:

[root@jouet linux]# nm /lib64/libpthread-2.24.so  | grep ' pthread_cond_timedwait'
000000000000dd90 T pthread_cond_timedwait@GLIBC_2.2.5
000000000000d6e0 T pthread_cond_timedwait@@GLIBC_2.3.2
[root@jouet linux]#

  # perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait

should be equivalent to:

  # perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait@@GLIBC_2.3.2

Which matches how these versioned symbols are resolved by the linker,
no?

I.e. when 'pthread_cond_timedwait' is specified and the symbol table
lookup fails, I think we should re-lookup for
'pthread_cond_timedwait@@*', i.e. we should have a
symbol__find_default_by_name(), which will take the
"pthread_cond_timedwait" and use a symbol comparison using
strncmp(strlen(key)), matching, should then look at right after the
common part looking for the double @@.

Perhaps we should somehow mark a struct dso as being versioned, i.e.
having symbols using @@, so that we speed up the search and don't try to
look for such symbols where there are none.

And if we mark the DSO as versioned, perhaps we can just one search
using both strcmp and, not matching, doing the strncmp +
strcmp(strlen(key) + 1, "@@") thing, no?
 
> > Additionaly, I can't reproduce your problem here, on x86_64:
> 
> I just cloned from acme/linux, and will rebase to there, if that's the best tree.

It is, to be precise:

  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git

Right now it is the same as the next best thing to do perf development,
the tip tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git

You should try, on both, to use the perf/urgent branch if you think this
is a bug fix that should go upstream ASAP, or perf/core, if it is a new
feature or a fix for something introduced during the current development
cycle, i.e. poised to be merged in the next Linus Torvalds merge window.

- Arnaldo

Re: [PATCH v3] Allow user probes on versioned symbols

[Masami Hiramatsu]

On Mon, 3 Apr 2017 11:46:58 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> > > But apart from those problems, I think that one should be able to ask
> > > for a versioned symbol, to probe just apps using that specific version,
>  
> > I agree, but wasn't trying to tackle that at the moment.  I can look into it, though.
>  
> > > for instance, we should consider the whole name as two functions, which
> > > in fact, they are, no?
>  
> > I'm not sure I understand what you mean here.  Do you mean we should set a probe at every version of a given symbol name?  For example, if there are symbols:
> > a@@V2
> > a@V1.1
> > a@V1
>  
> > ...for a request to set a probe at "a", we'd actually set a probe at all 3?
> 
> I think that we should just probe the default for that symbol and have a
> way to probe all of them, perhaps using the wildcard, i.e.:
> 
> [root@jouet linux]# nm /lib64/libpthread-2.24.so  | grep ' pthread_cond_timedwait'
> 000000000000dd90 T pthread_cond_timedwait@GLIBC_2.2.5
> 000000000000d6e0 T pthread_cond_timedwait@@GLIBC_2.3.2
> [root@jouet linux]#
> 
>   # perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait
> 
> should be equivalent to:
> 
>   # perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait@@GLIBC_2.3.2
> 
> Which matches how these versioned symbols are resolved by the linker,
> no?
> 
> I.e. when 'pthread_cond_timedwait' is specified and the symbol table
> lookup fails, I think we should re-lookup for
> 'pthread_cond_timedwait@@*', i.e. we should have a
> symbol__find_default_by_name(), which will take the
> "pthread_cond_timedwait" and use a symbol comparison using
> strncmp(strlen(key)), matching, should then look at right after the
> common part looking for the double @@.

Hm, this 'fallback'process sounds good idea to me.
BTW, how would we support other SYMBOL@VERSION, since we already
use '@' for specifying source code?
One possible way is to support it directly in perf-probe. If it
failed to find probe point from dwarf, try to find from symbol
map by using '@VERSION' suffix.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3] Allow user probes on versioned symbols

[Arnaldo Carvalho de Melo]

Em Tue, Apr 04, 2017 at 11:18:02PM +0900, Masami Hiramatsu escreveu:
> On Mon, 3 Apr 2017 11:46:58 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> > > > But apart from those problems, I think that one should be able to ask
> > > > for a versioned symbol, to probe just apps using that specific version,
> >  
> > > I agree, but wasn't trying to tackle that at the moment.  I can look into it, though.
> >  
> > > > for instance, we should consider the whole name as two functions, which
> > > > in fact, they are, no?
> >  
> > > I'm not sure I understand what you mean here.  Do you mean we should set a probe at every version of a given symbol name?  For example, if there are symbols:
> > > a@@V2
> > > a@V1.1
> > > a@V1
> >  
> > > ...for a request to set a probe at "a", we'd actually set a probe at all 3?
> > 
> > I think that we should just probe the default for that symbol and have a
> > way to probe all of them, perhaps using the wildcard, i.e.:
> > 
> > [root@jouet linux]# nm /lib64/libpthread-2.24.so  | grep ' pthread_cond_timedwait'
> > 000000000000dd90 T pthread_cond_timedwait@GLIBC_2.2.5
> > 000000000000d6e0 T pthread_cond_timedwait@@GLIBC_2.3.2
> > [root@jouet linux]#
> > 
> >   # perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait
> > 
> > should be equivalent to:
> > 
> >   # perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait@@GLIBC_2.3.2
> > 
> > Which matches how these versioned symbols are resolved by the linker,
> > no?
> > 
> > I.e. when 'pthread_cond_timedwait' is specified and the symbol table
> > lookup fails, I think we should re-lookup for
> > 'pthread_cond_timedwait@@*', i.e. we should have a
> > symbol__find_default_by_name(), which will take the
> > "pthread_cond_timedwait" and use a symbol comparison using
> > strncmp(strlen(key)), matching, should then look at right after the
> > common part looking for the double @@.
 
> Hm, this 'fallback'process sounds good idea to me.

This is just trying to keep the semantics used by the original user of
this syntax, i.e. the linker.

> BTW, how would we support other SYMBOL@VERSION, since we already
> use '@' for specifying source code?
> One possible way is to support it directly in perf-probe. If it
> failed to find probe point from dwarf, try to find from symbol
> map by using '@VERSION' suffix.

Right, we would be overloading that @ symbol, since version numbers
usually are very different of file source names :-)

- Arnaldo

Re: [PATCH v3] Allow user probes on versioned symbols

[Paul Clarke]

On 04/04/2017 09:26 AM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 04, 2017 at 11:18:02PM +0900, Masami Hiramatsu escreveu:
>> On Mon, 3 Apr 2017 11:46:58 -0300
>> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>> > > > But apart from those problems, I think that one should be able to ask
>> > > > for a versioned symbol, to probe just apps using that specific version,
>> >
>> > > I agree, but wasn't trying to tackle that at the moment.  I can look into it, though.
>> >
>> > > > for instance, we should consider the whole name as two functions, which
>> > > > in fact, they are, no?
>> >
>> > > I'm not sure I understand what you mean here.  Do you mean we should set a probe at every version of a given symbol name?  For example, if there are symbols:
>> > > a@@V2
>> > > a@V1.1
>> > > a@V1
>> >
>> > > ...for a request to set a probe at "a", we'd actually set a probe at all 3?
>> >
>> > I think that we should just probe the default for that symbol and have a
>> > way to probe all of them, perhaps using the wildcard, i.e.:
>> >
>> > [root@jouet linux]# nm /lib64/libpthread-2.24.so  | grep ' pthread_cond_timedwait'
>> > 000000000000dd90 T pthread_cond_timedwait@GLIBC_2.2.5
>> > 000000000000d6e0 T pthread_cond_timedwait@@GLIBC_2.3.2
>> > [root@jouet linux]#
>> >
>> >   # perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait
>> >
>> > should be equivalent to:
>> >
>> >   # perf probe -x /lib64/libpthread-2.24.so pthread_cond_timedwait@@GLIBC_2.3.2
>> >
>> > Which matches how these versioned symbols are resolved by the linker,
>> > no?
>> >
>> > I.e. when 'pthread_cond_timedwait' is specified and the symbol table
>> > lookup fails, I think we should re-lookup for
>> > 'pthread_cond_timedwait@@*', i.e. we should have a
>> > symbol__find_default_by_name(), which will take the
>> > "pthread_cond_timedwait" and use a symbol comparison using
>> > strncmp(strlen(key)), matching, should then look at right after the
>> > common part looking for the double @@.
>
>> Hm, this 'fallback'process sounds good idea to me.

I just sent a patch that does what you suggest, above.  To avoid duplicating the code in symbols_find_by_name, I added a parameter to tell it whether to ignore default symbol tags or not.

> This is just trying to keep the semantics used by the original user of
> this syntax, i.e. the linker.
>
>> BTW, how would we support other SYMBOL@VERSION, since we already
>> use '@' for specifying source code?
>> One possible way is to support it directly in perf-probe. If it
>> failed to find probe point from dwarf, try to find from symbol
>> map by using '@VERSION' suffix.
>
> Right, we would be overloading that @ symbol, since version numbers
> usually are very different of file source names :-)

There's not a lot of syntactic difference between "file" and "tag".  I don't think there is any standard for what either can be.  One might expect a "file" to be name.extension, where extension is a finite set (possibly fairly large).  A tag can be almost anything, I think.  One might expect it to end with a number.  I don't think there's a guarantee of either case, though.

It would seem one way to determine whether it's a file or a tag is to try to find it in the symbol tables.  If it's not there, assume it's a file.  (Or vice-versa.)

PC