Re: [PATCH v2 6/8] perf mem: Fix wrong verbose output for recording events

[Jin, Yao]

Hi Jiri,

On 6/1/2021 6:42 AM, Jiri Olsa wrote:
> On Thu, May 27, 2021 at 08:16:08AM +0800, Jin Yao wrote:
> > Current code:
> >
> > for (j = 0; j < argc; j++, i++)
> >          rec_argv[i] = argv[j];
> >
> > if (verbose > 0) {
> >          pr_debug("calling: record ");
> >
> >          while (rec_argv[j]) {
> >                  pr_debug("%s ", rec_argv[j]);
> >                  j++;
> >          }
> >          pr_debug("\n");
> > }
> >
> > The entries of argv[] are copied to the end of rec_argv[], not
> > copied to the beginning of rec_argv[]. So the index j at
> > rec_argv[] doesn't point to the first event.
> >
> > Now we record the start index and end index for events in rec_argv[],
> > and print them if verbose is enabled.
> >
> > Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > ---
> > v2:
> >   - New in v2.
> >
> >   tools/perf/builtin-mem.c | 8 +++++---
>
> hi,
> do we need the same in c2c as well?
>
> jirka

perf c2c is a bit different. It sets 'j = 0;' before 'while' loop so it prints all of rec_argv[].

In test,

# perf c2c record -vvv -a -- sleep 1

calling: record -W -d --phys-data --sample-cpu -e 
{cpu_core/mem-loads-aux/,cpu_core/mem-loads,ldlat=30/}:P -e cpu_atom/mem-loads,ldlat=30/P -e 
cpu_core/mem-stores/P -e cpu_atom/mem-stores/P -a sleep 1

The verbose output looks OK.

Thanks
Jin Yao

> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> > index 6b633df458c2..0fd2a74dbaca 100644
> > --- a/tools/perf/builtin-mem.c
> > +++ b/tools/perf/builtin-mem.c
> > @@ -65,6 +65,7 @@ static const char * const *record_mem_usage = __usage;
> >   static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
> >   {
> >   	int rec_argc, i = 0, j, tmp_nr = 0;
> > +	int start, end;
> >   	const char **rec_argv;
> >   	char **rec_tmp;
> >   	int ret;
> > @@ -144,9 +145,11 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
> >   	if (mem->data_page_size)
> >   		rec_argv[i++] = "--data-page-size";
> >   
> > +	start = i;
> >   	ret = perf_mem_events__record_args(rec_argv, &i, rec_tmp, &tmp_nr);
> >   	if (ret)
> >   		goto out;
> > +	end = i;
> >   
> >   	if (all_user)
> >   		rec_argv[i++] = "--all-user";
> > @@ -160,10 +163,9 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
> >   	if (verbose > 0) {
> >   		pr_debug("calling: record ");
> >   
> > -		while (rec_argv[j]) {
> > +		for (j = start; j < end; j++)
> >   			pr_debug("%s ", rec_argv[j]);
> > -			j++;
> > -		}
> > +
> >   		pr_debug("\n");
> >   	}
> >   
> > --
> > 2.17.1