Re: [PATCH v2 1/2] perf: Suggest inbuilt commands for unknown command

[Ian Rogers]

On Wed, Mar 20, 2024 at 8:00 AM Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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:~#
> >
> > ⬢[acme@toolbox perf-tools-next]$ git log --oneline -1
> > a65ef8052854ba75 (HEAD) perf: Suggest inbuilt commands for unknown command
> > ⬢[acme@toolbox perf-tools-next]$
> >
> > I use O= with install-bin, trying:
> >
> > ⬢[acme@toolbox perf-tools-next]$ make -C  tools/perf install-bin
> > ⬢[acme@toolbox perf-tools-next]$ perf raccord
> > Failed to run command 'raccord': No such file or directory
> > ⬢[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_unknown_cmd(), continuing after a conf call...
>
> (gdb) p *argv
> $7 = 0x7fffffffe4c5 "raccord"
> (gdb) s
> run_argv (argcp=0x7fffffffdfbc, argv=0x7fffffffdfb0) at perf.c:445
> 445     {
> (gdb) n
> 447             handle_internal_command(*argcp, *argv);
> (gdb) n
> 450             execv_dashed_external(*argv);
> (gdb) p *argv
> $8 = (const char **) 0x7fffffffe1d0
> (gdb) p **argv
> $9 = 0x7fffffffe4c5 "raccord"
> (gdb) n
> [Detaching after fork from child process 3245070]
> 451             return 0;
> (gdb) n
> 452     }
> (gdb) n
> main (argc=1, argv=0x7fffffffe1d0) at perf.c:565
> 565                     if (errno != ENOENT)
> (gdb) p; errno
> Invalid character ';' in expression.
> (gdb) p errno
> $10 = 2
> (gdb) n
> 568                     if (!done_help) {
> (gdb) p done_help
> $11 = 32767
> (gdb) list
> 563                     run_argv(&argc, &argv);
> 564
> 565                     if (errno != ENOENT)
> 566                             break;
> 567
> 568                     if (!done_help) {
> 569                             struct cmdnames main_cmds;
> 570
> 571                             for (unsigned int i = 0; i < ARRAY_SIZE(commands); i++) {
> 572                                     add_cmdname(&main_cmds,
> (gdb)
> 573                                                 commands[i].cmd,
> 574                                                 strlen(commands[i].cmd));
> 575                             }
> 576                             cmd = argv[0] = help_unknown_cmd(cmd, &main_cmds);
> 577                             clean_cmdnames(&main_cmds);
> 578                             done_help = 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