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

From: Ian Rogers
Date: Wed Mar 20 2024 - 11:13:33 EST


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