On 04/10/2017 08:11 AM, Masami Hiramatsu wrote:
On Wed, 5 Apr 2017 22:30:03 -0500
Paul Clarke <[email protected]> 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 <[email protected]>
---
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

Reply via email to