Received-SPF: pass (google.com: domain of linux-kernel+bounces-109070-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240301201306.2680986-1-irogers@google.com> <Zfr4jX_b8FCOG_x_@x1>
 <Zfr5__Ej3-0C8sJj@x1>
In-Reply-To: <Zfr5__Ej3-0C8sJj@x1>
From: Ian Rogers <irogers@google.com>
Date: Wed, 20 Mar 2024 08:12:56 -0700
Message-ID: <CAP-5=fXXrJRH=Dto2ajD_TUDE1YmkkJZO5Ey2pq5YB0wbVAzeg@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] perf: Suggest inbuilt commands for unknown command
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, linux-perf-users@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 20, 2024 at 8:00=E2=80=AFAM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Wed, Mar 20, 2024 at 11:54:09AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Fri, Mar 01, 2024 at 12:13:05PM -0800, Ian Rogers wrote:
> > > The existing unknown command code looks for perf scripts like
> > > perf-archive.sh and perf-iostat.sh, however, inbuilt commands aren't
> > > suggested. Add the inbuilt commands so they may be suggested too.
> > >
> > > Before:
> > > ```
> > > $ perf reccord
> > > perf: 'reccord' is not a perf-command. See 'perf --help'.
> > > ```
> > >
> > > After:
> > > ```
> > > $ perf reccord
> > > perf: 'reccord' is not a perf-command. See 'perf --help'.
> > >
> > > Did you mean this?
> > >         record
> > > ```
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > > v2. Drops a merged patch and rebases. No functional change. Arnaldo
> > >     reported the patch not working for him, but I've not found a
> >
> > Not working:
> >
> > root@number:~# perf reccord
> > Failed to run command 'reccord': No such file or directory
> > root@number:~#
> >
> > =E2=AC=A2[acme@toolbox perf-tools-next]$ git log --oneline -1
> > a65ef8052854ba75 (HEAD) perf: Suggest inbuilt commands for unknown comm=
and
> > =E2=AC=A2[acme@toolbox perf-tools-next]$
> >
> > I use O=3D with install-bin, trying:
> >
> > =E2=AC=A2[acme@toolbox perf-tools-next]$ make -C  tools/perf install-bi=
n
> > =E2=AC=A2[acme@toolbox perf-tools-next]$ perf raccord
> > Failed to run command 'raccord': No such file or directory
> > =E2=AC=A2[acme@toolbox perf-tools-next]$
> >
> > Also didn't work
> >
> > Trying to figure this out...
>
> It somehow gets done_help set to 32767, and this will not run help_unknow=
n_cmd(), continuing after a conf call...
>
> (gdb) p *argv
> $7 =3D 0x7fffffffe4c5 "raccord"
> (gdb) s
> run_argv (argcp=3D0x7fffffffdfbc, argv=3D0x7fffffffdfb0) at perf.c:445
> 445     {
> (gdb) n
> 447             handle_internal_command(*argcp, *argv);
> (gdb) n
> 450             execv_dashed_external(*argv);
> (gdb) p *argv
> $8 =3D (const char **) 0x7fffffffe1d0
> (gdb) p **argv
> $9 =3D 0x7fffffffe4c5 "raccord"
> (gdb) n
> [Detaching after fork from child process 3245070]
> 451             return 0;
> (gdb) n
> 452     }
> (gdb) n
> main (argc=3D1, argv=3D0x7fffffffe1d0) at perf.c:565
> 565                     if (errno !=3D ENOENT)
> (gdb) p; errno
> Invalid character ';' in expression.
> (gdb) p errno
> $10 =3D 2
> (gdb) n
> 568                     if (!done_help) {
> (gdb) p done_help
> $11 =3D 32767
> (gdb) list
> 563                     run_argv(&argc, &argv);
> 564
> 565                     if (errno !=3D ENOENT)
> 566                             break;
> 567
> 568                     if (!done_help) {
> 569                             struct cmdnames main_cmds;
> 570
> 571                             for (unsigned int i =3D 0; i < ARRAY_SIZE=
(commands); i++) {
> 572                                     add_cmdname(&main_cmds,
> (gdb)
> 573                                                 commands[i].cmd,
> 574                                                 strlen(commands[i].cm=
d));
> 575                             }
> 576                             cmd =3D argv[0] =3D help_unknown_cmd(cmd,=
 &main_cmds);
> 577                             clean_cmdnames(&main_cmds);
> 578                             done_help =3D 1;
> 579                             if (!cmd)
> 580                                     break;
> 581                     } else
> 582                             break;
> (gdb)

Ah, the change:

-               static int done_help;
+               int done_help;

created an uninitialized use. Compiler warning/sanitizers? Anyway,
done_help needs hoisting out of the loop and initializing to zero, or
being made static again (ugh).

Thanks,
Ian