[RFC PATCH v1] perf record: Correct address filters for duplicated symbols

[RFC PATCH v1] perf record: Correct address filters for duplicated symbols

[Dmitrii Dolgov]

It seems the logic to handle duplicated symbols is off. In case if the
symbol index is specified it will:

* find a match
* continue searching
* check that start is not null and idx is set on the next loop
* exit with the error code 1

My guess is the expectation was that at this point the second match is
found (because *start is not null), although the new symbol hasn't pass
dso_sym_match yet. Here is how it looks like:

    $ readelf -a postgres | grep simple_heap_update
        4716: 00000000004e23e5   352 FUNC    GLOBAL DEFAULT   13 simple_heap_update
        27829: 00000000004e23e5   352 FUNC    GLOBAL DEFAULT   13 simple_heap_update

    $ perf record -p <pid> -e intel_pt// \
        --filter 'filter simple_heap_update #1 @ postgres' -- sleep 60

        Failed to parse address filter: 'filter simple_heap_update #1 @ postgres'
        Filter format is: filter|start|stop|tracestop <start symbol or address> [/ <end symbol or size>] [@<file name>]
        Where multiple filters are separated by space or comma.

Correct by returning error only after checking whether the symbol is
matching or not. After modifications the case above seems to be working
as expected:

    $ perf record -p <pid> -e intel_pt// \
        --filter 'filter simple_heap_update #1 @ postgres' -- sleep 60

    # update heap a couple of times

    $ perf script --itrace=c

        branches: 4e23fe simple_heap_update+0x19 => 54829c GetCurrentCommandId+0x0
        # [...]
        branches: 4e23fe simple_heap_update+0x19 => 54829c GetCurrentCommandId+0x0
        # [...]

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
 tools/perf/util/auxtrace.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index b59c278fe9ed..112cef56d6c2 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -2593,15 +2593,23 @@ static int find_dso_sym(struct dso *dso, const char *sym_name, u64 *start,
 
 	sym = dso__first_symbol(dso);
 	while (sym) {
+		/* Verify duplicates if one match was already found */
 		if (*start) {
 			if (!*size)
 				*size = sym->start - *start;
-			if (idx > 0) {
-				if (*size)
-					return 1;
-			} else if (dso_sym_match(sym, sym_name, &cnt, idx)) {
-				print_duplicate_syms(dso, sym_name);
-				return -EINVAL;
+
+			if (dso_sym_match(sym, sym_name, &cnt, idx)) {
+				if (idx > 0) {
+					/*
+					 * There is already one match with the specified index, now
+					 * we found the second one, something is wrong.
+					 */
+					if (*size)
+						return 1;
+				} else {
+					print_duplicate_syms(dso, sym_name);
+					return -EINVAL;
+				}
 			}
 		} else if (dso_sym_match(sym, sym_name, &cnt, idx)) {
 			*start = sym->start;
-- 
2.31.1

Re: [RFC PATCH v1] perf record: Correct address filters for duplicated symbols

[Adrian Hunter]

On 6/01/23 22:37, Dmitrii Dolgov wrote:
> It seems the logic to handle duplicated symbols is off. In case if the
> symbol index is specified it will:
> 
> * find a match
> * continue searching
> * check that start is not null and idx is set on the next loop
> * exit with the error code 1
> 
> My guess is the expectation was that at this point the second match is
> found (because *start is not null), although the new symbol hasn't pass
> dso_sym_match yet. Here is how it looks like:

If *start && idx > 0 there is no need to match anymore.  The
code is just getting *size.  Looks to me like it should be:

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index 265d20cc126b..c2e323cd7d49 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -2611,7 +2611,7 @@ static int find_dso_sym(struct dso *dso, const char *sym_name, u64 *start,
 				*size = sym->start - *start;
 			if (idx > 0) {
 				if (*size)
-					return 1;
+					return 0;
 			} else if (dso_sym_match(sym, sym_name, &cnt, idx)) {
 				print_duplicate_syms(dso, sym_name);
 				return -EINVAL;


> 
>     $ readelf -a postgres | grep simple_heap_update
>         4716: 00000000004e23e5   352 FUNC    GLOBAL DEFAULT   13 simple_heap_update
>         27829: 00000000004e23e5   352 FUNC    GLOBAL DEFAULT   13 simple_heap_update
> 
>     $ perf record -p <pid> -e intel_pt// \
>         --filter 'filter simple_heap_update #1 @ postgres' -- sleep 60
> 
>         Failed to parse address filter: 'filter simple_heap_update #1 @ postgres'
>         Filter format is: filter|start|stop|tracestop <start symbol or address> [/ <end symbol or size>] [@<file name>]
>         Where multiple filters are separated by space or comma.
> 
> Correct by returning error only after checking whether the symbol is
> matching or not. After modifications the case above seems to be working
> as expected:
> 
>     $ perf record -p <pid> -e intel_pt// \
>         --filter 'filter simple_heap_update #1 @ postgres' -- sleep 60
> 
>     # update heap a couple of times
> 
>     $ perf script --itrace=c
> 
>         branches: 4e23fe simple_heap_update+0x19 => 54829c GetCurrentCommandId+0x0
>         # [...]
>         branches: 4e23fe simple_heap_update+0x19 => 54829c GetCurrentCommandId+0x0
>         # [...]
> 
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
> ---
>  tools/perf/util/auxtrace.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index b59c278fe9ed..112cef56d6c2 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -2593,15 +2593,23 @@ static int find_dso_sym(struct dso *dso, const char *sym_name, u64 *start,
>  
>  	sym = dso__first_symbol(dso);
>  	while (sym) {
> +		/* Verify duplicates if one match was already found */
>  		if (*start) {
>  			if (!*size)
>  				*size = sym->start - *start;
> -			if (idx > 0) {
> -				if (*size)
> -					return 1;
> -			} else if (dso_sym_match(sym, sym_name, &cnt, idx)) {
> -				print_duplicate_syms(dso, sym_name);
> -				return -EINVAL;
> +
> +			if (dso_sym_match(sym, sym_name, &cnt, idx)) {
> +				if (idx > 0) {
> +					/*
> +					 * There is already one match with the specified index, now
> +					 * we found the second one, something is wrong.
> +					 */
> +					if (*size)
> +						return 1;
> +				} else {
> +					print_duplicate_syms(dso, sym_name);
> +					return -EINVAL;
> +				}
>  			}
>  		} else if (dso_sym_match(sym, sym_name, &cnt, idx)) {
>  			*start = sym->start;

Re: [RFC PATCH v1] perf record: Correct address filters for duplicated symbols

[Dmitry Dolgov]

> On Mon, Jan 09, 2023 at 09:02:02AM +0200, Adrian Hunter wrote:
> On 6/01/23 22:37, Dmitrii Dolgov wrote:
> > It seems the logic to handle duplicated symbols is off. In case if the
> > symbol index is specified it will:
> >
> > * find a match
> > * continue searching
> > * check that start is not null and idx is set on the next loop
> > * exit with the error code 1
> >
> > My guess is the expectation was that at this point the second match is
> > found (because *start is not null), although the new symbol hasn't pass
> > dso_sym_match yet. Here is how it looks like:
>
> If *start && idx > 0 there is no need to match anymore.  The
> code is just getting *size.  Looks to me like it should be:
>
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index 265d20cc126b..c2e323cd7d49 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -2611,7 +2611,7 @@ static int find_dso_sym(struct dso *dso, const char *sym_name, u64 *start,
>  				*size = sym->start - *start;
>  			if (idx > 0) {
>  				if (*size)
> -					return 1;
> +					return 0;
>  			} else if (dso_sym_match(sym, sym_name, &cnt, idx)) {
>  				print_duplicate_syms(dso, sym_name);
>  				return -EINVAL;

Yes, if there was no intention to continue searching symbols, this would
work as well. Just in case I have verified this on my test, everything
is fine. Would you send a patch for that?

Re: [RFC PATCH v1] perf record: Correct address filters for duplicated symbols

[Arnaldo Carvalho de Melo]

Em Tue, Jan 10, 2023 at 11:48:48AM +0100, Dmitry Dolgov escreveu:
> > On Mon, Jan 09, 2023 at 09:02:02AM +0200, Adrian Hunter wrote:
> > On 6/01/23 22:37, Dmitrii Dolgov wrote:
> > > It seems the logic to handle duplicated symbols is off. In case if the
> > > symbol index is specified it will:
> > >
> > > * find a match
> > > * continue searching
> > > * check that start is not null and idx is set on the next loop
> > > * exit with the error code 1
> > >
> > > My guess is the expectation was that at this point the second match is
> > > found (because *start is not null), although the new symbol hasn't pass
> > > dso_sym_match yet. Here is how it looks like:
> >
> > If *start && idx > 0 there is no need to match anymore.  The
> > code is just getting *size.  Looks to me like it should be:
> >
> > diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> > index 265d20cc126b..c2e323cd7d49 100644
> > --- a/tools/perf/util/auxtrace.c
> > +++ b/tools/perf/util/auxtrace.c
> > @@ -2611,7 +2611,7 @@ static int find_dso_sym(struct dso *dso, const char *sym_name, u64 *start,
> >  				*size = sym->start - *start;
> >  			if (idx > 0) {
> >  				if (*size)
> > -					return 1;
> > +					return 0;
> >  			} else if (dso_sym_match(sym, sym_name, &cnt, idx)) {
> >  				print_duplicate_syms(dso, sym_name);
> >  				return -EINVAL;
> 
> Yes, if there was no intention to continue searching symbols, this would
> work as well. Just in case I have verified this on my test, everything
> is fine. Would you send a patch for that?

So I'll take this as a:

Tested-by: Dmitry Dolgov <9erthalion6@gmail.com>

Ok?

- Arnaldo

Re: [RFC PATCH v1] perf record: Correct address filters for duplicated symbols

[Dmitry Dolgov]

> On Wed, Jan 11, 2023 at 10:17:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 10, 2023 at 11:48:48AM +0100, Dmitry Dolgov escreveu:
> > > On Mon, Jan 09, 2023 at 09:02:02AM +0200, Adrian Hunter wrote:
> > > On 6/01/23 22:37, Dmitrii Dolgov wrote:
> > > > It seems the logic to handle duplicated symbols is off. In case if the
> > > > symbol index is specified it will:
> > > >
> > > > * find a match
> > > > * continue searching
> > > > * check that start is not null and idx is set on the next loop
> > > > * exit with the error code 1
> > > >
> > > > My guess is the expectation was that at this point the second match is
> > > > found (because *start is not null), although the new symbol hasn't pass
> > > > dso_sym_match yet. Here is how it looks like:
> > >
> > > If *start && idx > 0 there is no need to match anymore.  The
> > > code is just getting *size.  Looks to me like it should be:
> > >
> > > diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> > > index 265d20cc126b..c2e323cd7d49 100644
> > > --- a/tools/perf/util/auxtrace.c
> > > +++ b/tools/perf/util/auxtrace.c
> > > @@ -2611,7 +2611,7 @@ static int find_dso_sym(struct dso *dso, const char *sym_name, u64 *start,
> > >  				*size = sym->start - *start;
> > >  			if (idx > 0) {
> > >  				if (*size)
> > > -					return 1;
> > > +					return 0;
> > >  			} else if (dso_sym_match(sym, sym_name, &cnt, idx)) {
> > >  				print_duplicate_syms(dso, sym_name);
> > >  				return -EINVAL;
> >
> > Yes, if there was no intention to continue searching symbols, this would
> > work as well. Just in case I have verified this on my test, everything
> > is fine. Would you send a patch for that?
>
> So I'll take this as a:
>
> Tested-by: Dmitry Dolgov <9erthalion6@gmail.com>
>
> Ok?

Sure, forgot about this. Thanks!