* [RFC PATCH v1] perf record: Correct address filters for duplicated symbols
@ 2023-01-06 20:37 Dmitrii Dolgov
2023-01-09 7:02 ` Adrian Hunter
0 siblings, 1 reply; 5+ messages in thread
From: Dmitrii Dolgov @ 2023-01-06 20:37 UTC (permalink / raw)
To: linux-perf-users
Cc: acme, mingo, jolsa, irogers, adrian.hunter, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH v1] perf record: Correct address filters for duplicated symbols
2023-01-06 20:37 [RFC PATCH v1] perf record: Correct address filters for duplicated symbols Dmitrii Dolgov
@ 2023-01-09 7:02 ` Adrian Hunter
2023-01-10 10:48 ` Dmitry Dolgov
0 siblings, 1 reply; 5+ messages in thread
From: Adrian Hunter @ 2023-01-09 7:02 UTC (permalink / raw)
To: Dmitrii Dolgov, linux-perf-users; +Cc: acme, mingo, jolsa, irogers
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;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH v1] perf record: Correct address filters for duplicated symbols
2023-01-09 7:02 ` Adrian Hunter
@ 2023-01-10 10:48 ` Dmitry Dolgov
2023-01-11 13:17 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Dolgov @ 2023-01-10 10:48 UTC (permalink / raw)
To: Adrian Hunter; +Cc: linux-perf-users, acme, mingo, jolsa, irogers
> 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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH v1] perf record: Correct address filters for duplicated symbols
2023-01-10 10:48 ` Dmitry Dolgov
@ 2023-01-11 13:17 ` Arnaldo Carvalho de Melo
2023-01-11 13:34 ` Dmitry Dolgov
0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-01-11 13:17 UTC (permalink / raw)
To: Dmitry Dolgov; +Cc: Adrian Hunter, linux-perf-users, mingo, jolsa, irogers
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH v1] perf record: Correct address filters for duplicated symbols
2023-01-11 13:17 ` Arnaldo Carvalho de Melo
@ 2023-01-11 13:34 ` Dmitry Dolgov
0 siblings, 0 replies; 5+ messages in thread
From: Dmitry Dolgov @ 2023-01-11 13:34 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Adrian Hunter, linux-perf-users, mingo, jolsa, irogers
> 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!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-11 13:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-06 20:37 [RFC PATCH v1] perf record: Correct address filters for duplicated symbols Dmitrii Dolgov
2023-01-09 7:02 ` Adrian Hunter
2023-01-10 10:48 ` Dmitry Dolgov
2023-01-11 13:17 ` Arnaldo Carvalho de Melo
2023-01-11 13:34 ` Dmitry Dolgov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).