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 <[email protected]>
---
v2. Drops a merged patch and rebases. No functional change. Arnaldo
reported the patch not working for him, but I've not found a
reproduction and it works for me:
https://lore.kernel.org/lkml/[email protected]/
---
tools/perf/builtin.h | 4 ++-
tools/perf/perf.c | 21 +++++++++++---
tools/perf/util/help-unknown-cmd.c | 45 ++++++++++++++----------------
3 files changed, 41 insertions(+), 29 deletions(-)
diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
index f2ab5bae2150..f4375deabfa3 100644
--- a/tools/perf/builtin.h
+++ b/tools/perf/builtin.h
@@ -2,8 +2,10 @@
#ifndef BUILTIN_H
#define BUILTIN_H
+struct cmdnames;
+
void list_common_cmds_help(void);
-const char *help_unknown_cmd(const char *cmd);
+const char *help_unknown_cmd(const char *cmd, struct cmdnames *main_cmds);
int cmd_annotate(int argc, const char **argv);
int cmd_bench(int argc, const char **argv);
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 921bee0a6437..c719e6ccd9e2 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -18,6 +18,7 @@
#include <subcmd/run-command.h>
#include "util/parse-events.h"
#include <subcmd/parse-options.h>
+#include <subcmd/help.h>
#include "util/debug.h"
#include "util/event.h"
#include "util/util.h" // usage()
@@ -557,7 +558,7 @@ int main(int argc, const char **argv)
pthread__block_sigwinch();
while (1) {
- static int done_help;
+ int done_help;
run_argv(&argc, &argv);
@@ -565,14 +566,26 @@ int main(int argc, const char **argv)
break;
if (!done_help) {
- cmd = argv[0] = help_unknown_cmd(cmd);
+ struct cmdnames main_cmds;
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(commands); i++) {
+ add_cmdname(&main_cmds,
+ commands[i].cmd,
+ strlen(commands[i].cmd));
+ }
+ cmd = argv[0] = help_unknown_cmd(cmd, &main_cmds);
+ clean_cmdnames(&main_cmds);
done_help = 1;
+ if (!cmd)
+ break;
} else
break;
}
- fprintf(stderr, "Failed to run command '%s': %s\n",
- cmd, str_error_r(errno, sbuf, sizeof(sbuf)));
+ if (cmd) {
+ fprintf(stderr, "Failed to run command '%s': %s\n",
+ cmd, str_error_r(errno, sbuf, sizeof(sbuf)));
+ }
out:
if (debug_fp)
fclose(debug_fp);
diff --git a/tools/perf/util/help-unknown-cmd.c b/tools/perf/util/help-unknown-cmd.c
index eab99ea6ac01..2ba3369f1620 100644
--- a/tools/perf/util/help-unknown-cmd.c
+++ b/tools/perf/util/help-unknown-cmd.c
@@ -52,46 +52,44 @@ static int add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
return 0;
}
-const char *help_unknown_cmd(const char *cmd)
+const char *help_unknown_cmd(const char *cmd, struct cmdnames *main_cmds)
{
unsigned int i, n = 0, best_similarity = 0;
- struct cmdnames main_cmds, other_cmds;
+ struct cmdnames other_cmds;
- memset(&main_cmds, 0, sizeof(main_cmds));
- memset(&other_cmds, 0, sizeof(main_cmds));
+ memset(&other_cmds, 0, sizeof(other_cmds));
perf_config(perf_unknown_cmd_config, NULL);
- load_command_list("perf-", &main_cmds, &other_cmds);
+ load_command_list("perf-", main_cmds, &other_cmds);
- if (add_cmd_list(&main_cmds, &other_cmds) < 0) {
+ if (add_cmd_list(main_cmds, &other_cmds) < 0) {
fprintf(stderr, "ERROR: Failed to allocate command list for unknown command.\n");
goto end;
}
- qsort(main_cmds.names, main_cmds.cnt,
- sizeof(main_cmds.names), cmdname_compare);
- uniq(&main_cmds);
+ qsort(main_cmds->names, main_cmds->cnt,
+ sizeof(main_cmds->names), cmdname_compare);
+ uniq(main_cmds);
- if (main_cmds.cnt) {
+ if (main_cmds->cnt) {
/* This reuses cmdname->len for similarity index */
- for (i = 0; i < main_cmds.cnt; ++i)
- main_cmds.names[i]->len =
- levenshtein(cmd, main_cmds.names[i]->name, 0, 2, 1, 4);
+ for (i = 0; i < main_cmds->cnt; ++i)
+ main_cmds->names[i]->len =
+ levenshtein(cmd, main_cmds->names[i]->name, 0, 2, 1, 4);
- qsort(main_cmds.names, main_cmds.cnt,
- sizeof(*main_cmds.names), levenshtein_compare);
+ qsort(main_cmds->names, main_cmds->cnt,
+ sizeof(*main_cmds->names), levenshtein_compare);
- best_similarity = main_cmds.names[0]->len;
+ best_similarity = main_cmds->names[0]->len;
n = 1;
- while (n < main_cmds.cnt && best_similarity == main_cmds.names[n]->len)
+ while (n < main_cmds->cnt && best_similarity == main_cmds->names[n]->len)
++n;
}
if (autocorrect && n == 1) {
- const char *assumed = main_cmds.names[0]->name;
+ const char *assumed = main_cmds->names[0]->name;
- main_cmds.names[0] = NULL;
- clean_cmdnames(&main_cmds);
+ main_cmds->names[0] = NULL;
clean_cmdnames(&other_cmds);
fprintf(stderr, "WARNING: You called a perf program named '%s', "
"which does not exist.\n"
@@ -107,15 +105,14 @@ const char *help_unknown_cmd(const char *cmd)
fprintf(stderr, "perf: '%s' is not a perf-command. See 'perf --help'.\n", cmd);
- if (main_cmds.cnt && best_similarity < 6) {
+ if (main_cmds->cnt && best_similarity < 6) {
fprintf(stderr, "\nDid you mean %s?\n",
n < 2 ? "this": "one of these");
for (i = 0; i < n; i++)
- fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
+ fprintf(stderr, "\t%s\n", main_cmds->names[i]->name);
}
end:
- clean_cmdnames(&main_cmds);
clean_cmdnames(&other_cmds);
- exit(1);
+ return NULL;
}
--
2.44.0.278.ge034bb2e1d-goog
The levenshtein penalty for deleting a character was far higher than
subsituting or inserting a character. Lower the penalty to match that
of inserting a character.
Before:
```
$ perf recccord
perf: 'recccord' is not a perf-command. See 'perf --help'.
```
After:
```
$ perf recccord
perf: 'recccord' is not a perf-command. See 'perf --help'.
Did you mean this?
record
```
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/help-unknown-cmd.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/help-unknown-cmd.c b/tools/perf/util/help-unknown-cmd.c
index 2ba3369f1620..a0a46e34f8d1 100644
--- a/tools/perf/util/help-unknown-cmd.c
+++ b/tools/perf/util/help-unknown-cmd.c
@@ -73,10 +73,14 @@ const char *help_unknown_cmd(const char *cmd, struct cmdnames *main_cmds)
if (main_cmds->cnt) {
/* This reuses cmdname->len for similarity index */
- for (i = 0; i < main_cmds->cnt; ++i)
+ for (i = 0; i < main_cmds->cnt; ++i) {
main_cmds->names[i]->len =
- levenshtein(cmd, main_cmds->names[i]->name, 0, 2, 1, 4);
-
+ levenshtein(cmd, main_cmds->names[i]->name,
+ /*swap_penalty=*/0,
+ /*substition_penality=*/2,
+ /*insertion_penality=*/1,
+ /*deletion_penalty=*/1);
+ }
qsort(main_cmds->names, main_cmds->cnt,
sizeof(*main_cmds->names), levenshtein_compare);
--
2.44.0.278.ge034bb2e1d-goog
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 <[email protected]>
> ---
> 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...
- Arnaldo
> reproduction and it works for me:
> https://lore.kernel.org/lkml/[email protected]/
> ---
> tools/perf/builtin.h | 4 ++-
> tools/perf/perf.c | 21 +++++++++++---
> tools/perf/util/help-unknown-cmd.c | 45 ++++++++++++++----------------
> 3 files changed, 41 insertions(+), 29 deletions(-)
>
> diff --git a/tools/perf/builtin.h b/tools/perf/builtin.h
> index f2ab5bae2150..f4375deabfa3 100644
> --- a/tools/perf/builtin.h
> +++ b/tools/perf/builtin.h
> @@ -2,8 +2,10 @@
> #ifndef BUILTIN_H
> #define BUILTIN_H
>
> +struct cmdnames;
> +
> void list_common_cmds_help(void);
> -const char *help_unknown_cmd(const char *cmd);
> +const char *help_unknown_cmd(const char *cmd, struct cmdnames *main_cmds);
>
> int cmd_annotate(int argc, const char **argv);
> int cmd_bench(int argc, const char **argv);
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index 921bee0a6437..c719e6ccd9e2 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -18,6 +18,7 @@
> #include <subcmd/run-command.h>
> #include "util/parse-events.h"
> #include <subcmd/parse-options.h>
> +#include <subcmd/help.h>
> #include "util/debug.h"
> #include "util/event.h"
> #include "util/util.h" // usage()
> @@ -557,7 +558,7 @@ int main(int argc, const char **argv)
> pthread__block_sigwinch();
>
> while (1) {
> - static int done_help;
> + int done_help;
>
> run_argv(&argc, &argv);
>
> @@ -565,14 +566,26 @@ int main(int argc, const char **argv)
> break;
>
> if (!done_help) {
> - cmd = argv[0] = help_unknown_cmd(cmd);
> + struct cmdnames main_cmds;
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(commands); i++) {
> + add_cmdname(&main_cmds,
> + commands[i].cmd,
> + strlen(commands[i].cmd));
> + }
> + cmd = argv[0] = help_unknown_cmd(cmd, &main_cmds);
> + clean_cmdnames(&main_cmds);
> done_help = 1;
> + if (!cmd)
> + break;
> } else
> break;
> }
>
> - fprintf(stderr, "Failed to run command '%s': %s\n",
> - cmd, str_error_r(errno, sbuf, sizeof(sbuf)));
> + if (cmd) {
> + fprintf(stderr, "Failed to run command '%s': %s\n",
> + cmd, str_error_r(errno, sbuf, sizeof(sbuf)));
> + }
> out:
> if (debug_fp)
> fclose(debug_fp);
> diff --git a/tools/perf/util/help-unknown-cmd.c b/tools/perf/util/help-unknown-cmd.c
> index eab99ea6ac01..2ba3369f1620 100644
> --- a/tools/perf/util/help-unknown-cmd.c
> +++ b/tools/perf/util/help-unknown-cmd.c
> @@ -52,46 +52,44 @@ static int add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
> return 0;
> }
>
> -const char *help_unknown_cmd(const char *cmd)
> +const char *help_unknown_cmd(const char *cmd, struct cmdnames *main_cmds)
> {
> unsigned int i, n = 0, best_similarity = 0;
> - struct cmdnames main_cmds, other_cmds;
> + struct cmdnames other_cmds;
>
> - memset(&main_cmds, 0, sizeof(main_cmds));
> - memset(&other_cmds, 0, sizeof(main_cmds));
> + memset(&other_cmds, 0, sizeof(other_cmds));
>
> perf_config(perf_unknown_cmd_config, NULL);
>
> - load_command_list("perf-", &main_cmds, &other_cmds);
> + load_command_list("perf-", main_cmds, &other_cmds);
>
> - if (add_cmd_list(&main_cmds, &other_cmds) < 0) {
> + if (add_cmd_list(main_cmds, &other_cmds) < 0) {
> fprintf(stderr, "ERROR: Failed to allocate command list for unknown command.\n");
> goto end;
> }
> - qsort(main_cmds.names, main_cmds.cnt,
> - sizeof(main_cmds.names), cmdname_compare);
> - uniq(&main_cmds);
> + qsort(main_cmds->names, main_cmds->cnt,
> + sizeof(main_cmds->names), cmdname_compare);
> + uniq(main_cmds);
>
> - if (main_cmds.cnt) {
> + if (main_cmds->cnt) {
> /* This reuses cmdname->len for similarity index */
> - for (i = 0; i < main_cmds.cnt; ++i)
> - main_cmds.names[i]->len =
> - levenshtein(cmd, main_cmds.names[i]->name, 0, 2, 1, 4);
> + for (i = 0; i < main_cmds->cnt; ++i)
> + main_cmds->names[i]->len =
> + levenshtein(cmd, main_cmds->names[i]->name, 0, 2, 1, 4);
>
> - qsort(main_cmds.names, main_cmds.cnt,
> - sizeof(*main_cmds.names), levenshtein_compare);
> + qsort(main_cmds->names, main_cmds->cnt,
> + sizeof(*main_cmds->names), levenshtein_compare);
>
> - best_similarity = main_cmds.names[0]->len;
> + best_similarity = main_cmds->names[0]->len;
> n = 1;
> - while (n < main_cmds.cnt && best_similarity == main_cmds.names[n]->len)
> + while (n < main_cmds->cnt && best_similarity == main_cmds->names[n]->len)
> ++n;
> }
>
> if (autocorrect && n == 1) {
> - const char *assumed = main_cmds.names[0]->name;
> + const char *assumed = main_cmds->names[0]->name;
>
> - main_cmds.names[0] = NULL;
> - clean_cmdnames(&main_cmds);
> + main_cmds->names[0] = NULL;
> clean_cmdnames(&other_cmds);
> fprintf(stderr, "WARNING: You called a perf program named '%s', "
> "which does not exist.\n"
> @@ -107,15 +105,14 @@ const char *help_unknown_cmd(const char *cmd)
>
> fprintf(stderr, "perf: '%s' is not a perf-command. See 'perf --help'.\n", cmd);
>
> - if (main_cmds.cnt && best_similarity < 6) {
> + if (main_cmds->cnt && best_similarity < 6) {
> fprintf(stderr, "\nDid you mean %s?\n",
> n < 2 ? "this": "one of these");
>
> for (i = 0; i < n; i++)
> - fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
> + fprintf(stderr, "\t%s\n", main_cmds->names[i]->name);
> }
> end:
> - clean_cmdnames(&main_cmds);
> clean_cmdnames(&other_cmds);
> - exit(1);
> + return NULL;
> }
> --
> 2.44.0.278.ge034bb2e1d-goog
>
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 <[email protected]>
> > ---
> > 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)
On Wed, Mar 20, 2024 at 8:00 AM Arnaldo Carvalho de Melo
<[email protected]> 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 <[email protected]>
> > > ---
> > > 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
On Wed, Mar 20, 2024 at 08:12:56AM -0700, Ian Rogers wrote:
> 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).
⬢[acme@toolbox perf-tools-next]$ git diff
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index c719e6ccd9e27778..f9532b20e87cbf05 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -459,7 +459,7 @@ static int libperf_print(enum libperf_print_level level,
int main(int argc, const char **argv)
{
- int err;
+ int err, done_help = 0;
const char *cmd;
char sbuf[STRERR_BUFSIZE];
@@ -558,8 +558,6 @@ int main(int argc, const char **argv)
pthread__block_sigwinch();
while (1) {
- int done_help;
-
run_argv(&argc, &argv);
if (errno != ENOENT)
⬢[acme@toolbox perf-tools-next]$ perf raccord
Fatal: Out of memory, realloc failed
⬢[acme@toolbox perf-tools-next]$
Then:
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index c719e6ccd9e27778..54f62aa6612bc290 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -558,7 +558,7 @@ int main(int argc, const char **argv)
pthread__block_sigwinch();
while (1) {
- int done_help;
+ static int done_help;
run_argv(&argc, &argv);
⬢[acme@toolbox perf-tools-next]$ perf raccord
Fatal: Out of memory, realloc failed
⬢[acme@toolbox perf-tools-next]$
- Arnaldo
On Wed, Mar 20, 2024 at 8:20 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> On Wed, Mar 20, 2024 at 08:12:56AM -0700, Ian Rogers wrote:
> > 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).
>
> ⬢[acme@toolbox perf-tools-next]$ git diff
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index c719e6ccd9e27778..f9532b20e87cbf05 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -459,7 +459,7 @@ static int libperf_print(enum libperf_print_level level,
>
> int main(int argc, const char **argv)
> {
> - int err;
> + int err, done_help = 0;
> const char *cmd;
> char sbuf[STRERR_BUFSIZE];
>
> @@ -558,8 +558,6 @@ int main(int argc, const char **argv)
> pthread__block_sigwinch();
>
> while (1) {
> - int done_help;
> -
> run_argv(&argc, &argv);
>
> if (errno != ENOENT)
> ⬢[acme@toolbox perf-tools-next]$ perf raccord
> Fatal: Out of memory, realloc failed
> ⬢[acme@toolbox perf-tools-next]$
>
> Then:
>
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index c719e6ccd9e27778..54f62aa6612bc290 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -558,7 +558,7 @@ int main(int argc, const char **argv)
> pthread__block_sigwinch();
>
> while (1) {
> - int done_help;
> + static int done_help;
>
> run_argv(&argc, &argv);
>
> ⬢[acme@toolbox perf-tools-next]$ perf raccord
> Fatal: Out of memory, realloc failed
> ⬢[acme@toolbox perf-tools-next]$
Similar issue on main_cmds, caught by memory sanitizer. I think the
following fixes it:
```
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index c719e6ccd9e2..bd3f80b5bb46 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -459,7 +459,7 @@ static int libperf_print(enum libperf_print_level level,
int main(int argc, const char **argv)
{
- int err;
+ int err, done_help = 0;
const char *cmd;
char sbuf[STRERR_BUFSIZE];
@@ -558,15 +558,13 @@ int main(int argc, const char **argv)
pthread__block_sigwinch();
while (1) {
- int done_help;
-
run_argv(&argc, &argv);
if (errno != ENOENT)
break;
if (!done_help) {
- struct cmdnames main_cmds;
+ struct cmdnames main_cmds = {};
for (unsigned int i = 0; i < ARRAY_SIZE(commands); i++) {
add_cmdname(&main_cmds,
```
Thanks,
Ian
> - Arnaldo
On Wed, Mar 20, 2024 at 12:20:20PM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, Mar 20, 2024 at 08:12:56AM -0700, Ian Rogers wrote:
> > 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).
>
> ⬢[acme@toolbox perf-tools-next]$ git diff
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index c719e6ccd9e27778..f9532b20e87cbf05 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -459,7 +459,7 @@ static int libperf_print(enum libperf_print_level level,
>
> int main(int argc, const char **argv)
> {
> - int err;
> + int err, done_help = 0;
> const char *cmd;
> char sbuf[STRERR_BUFSIZE];
>
> @@ -558,8 +558,6 @@ int main(int argc, const char **argv)
> pthread__block_sigwinch();
>
> while (1) {
> - int done_help;
> -
> run_argv(&argc, &argv);
>
> if (errno != ENOENT)
> ⬢[acme@toolbox perf-tools-next]$ perf raccord
> Fatal: Out of memory, realloc failed
> ⬢[acme@toolbox perf-tools-next]$
>
> Then:
>
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index c719e6ccd9e27778..54f62aa6612bc290 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -558,7 +558,7 @@ int main(int argc, const char **argv)
> pthread__block_sigwinch();
>
> while (1) {
> - int done_help;
> + static int done_help;
>
> run_argv(&argc, &argv);
>
> ⬢[acme@toolbox perf-tools-next]$ perf raccord
> Fatal: Out of memory, realloc failed
> ⬢[acme@toolbox perf-tools-next]$
That main_cmds variable is uninitialized, which ends up making
add_cmdname() to explode, are you sure this is working on your side?
Further clarifying, this is without considering the second patch, I
haven't got to it yet, from what I recall from the description it
shouldn't matter tho.
- Arnaldo
⬢[acme@toolbox perf-tools-next]$ git diff
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index c719e6ccd9e27778..164b3c78baff4204 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -558,7 +558,7 @@ int main(int argc, const char **argv)
pthread__block_sigwinch();
while (1) {
- int done_help;
+ static int done_help;
run_argv(&argc, &argv);
@@ -566,7 +566,7 @@ int main(int argc, const char **argv)
break;
if (!done_help) {
- struct cmdnames main_cmds;
+ struct cmdnames main_cmds = { 0, };
for (unsigned int i = 0; i < ARRAY_SIZE(commands); i++) {
add_cmdname(&main_cmds,
⬢[acme@toolbox perf-tools-next]$ perf raccord
perf: 'raccord' is not a perf-command. See 'perf --help'.
⬢[acme@toolbox perf-tools-next]$
On Wed, Mar 20, 2024 at 8:32 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> On Wed, Mar 20, 2024 at 12:20:20PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Wed, Mar 20, 2024 at 08:12:56AM -0700, Ian Rogers wrote:
> > > 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).
> >
> > ⬢[acme@toolbox perf-tools-next]$ git diff
> > diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> > index c719e6ccd9e27778..f9532b20e87cbf05 100644
> > --- a/tools/perf/perf.c
> > +++ b/tools/perf/perf.c
> > @@ -459,7 +459,7 @@ static int libperf_print(enum libperf_print_level level,
> >
> > int main(int argc, const char **argv)
> > {
> > - int err;
> > + int err, done_help = 0;
> > const char *cmd;
> > char sbuf[STRERR_BUFSIZE];
> >
> > @@ -558,8 +558,6 @@ int main(int argc, const char **argv)
> > pthread__block_sigwinch();
> >
> > while (1) {
> > - int done_help;
> > -
> > run_argv(&argc, &argv);
> >
> > if (errno != ENOENT)
> > ⬢[acme@toolbox perf-tools-next]$ perf raccord
> > Fatal: Out of memory, realloc failed
> > ⬢[acme@toolbox perf-tools-next]$
> >
> > Then:
> >
> > diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> > index c719e6ccd9e27778..54f62aa6612bc290 100644
> > --- a/tools/perf/perf.c
> > +++ b/tools/perf/perf.c
> > @@ -558,7 +558,7 @@ int main(int argc, const char **argv)
> > pthread__block_sigwinch();
> >
> > while (1) {
> > - int done_help;
> > + static int done_help;
> >
> > run_argv(&argc, &argv);
> >
> > ⬢[acme@toolbox perf-tools-next]$ perf raccord
> > Fatal: Out of memory, realloc failed
> > ⬢[acme@toolbox perf-tools-next]$
>
> That main_cmds variable is uninitialized, which ends up making
> add_cmdname() to explode, are you sure this is working on your side?
>
> Further clarifying, this is without considering the second patch, I
> haven't got to it yet, from what I recall from the description it
> shouldn't matter tho.
>
> - Arnaldo
>
> ⬢[acme@toolbox perf-tools-next]$ git diff
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index c719e6ccd9e27778..164b3c78baff4204 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -558,7 +558,7 @@ int main(int argc, const char **argv)
> pthread__block_sigwinch();
>
> while (1) {
> - int done_help;
> + static int done_help;
>
> run_argv(&argc, &argv);
>
> @@ -566,7 +566,7 @@ int main(int argc, const char **argv)
> break;
>
> if (!done_help) {
> - struct cmdnames main_cmds;
> + struct cmdnames main_cmds = { 0, };
>
> for (unsigned int i = 0; i < ARRAY_SIZE(commands); i++) {
> add_cmdname(&main_cmds,
> ⬢[acme@toolbox perf-tools-next]$ perf raccord
> perf: 'raccord' is not a perf-command. See 'perf --help'.
> ⬢[acme@toolbox perf-tools-next]$
Perhaps change the 'a' to an 'e', the 2nd patch just improves the
weights on removing characters. For me:
```
$ /tmp/perf/perf raccord
perf: 'raccord' is not a perf-command. See 'perf --help'.
Did you mean this?
record
```
For a memory sanitizer build with the 2 patches and the 2 line changes
I sent before. Fwiw, I'm trying to get memory sanitizer to test "perf
test" but running into an issue with scandirat not having an msan
interceptor.
Thanks,
Ian
On Wed, Mar 20, 2024 at 12:32:50PM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, Mar 20, 2024 at 12:20:20PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Wed, Mar 20, 2024 at 08:12:56AM -0700, Ian Rogers wrote:
> > > 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).
> >
> > ⬢[acme@toolbox perf-tools-next]$ git diff
> > diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> > index c719e6ccd9e27778..f9532b20e87cbf05 100644
> > --- a/tools/perf/perf.c
> > +++ b/tools/perf/perf.c
> > @@ -459,7 +459,7 @@ static int libperf_print(enum libperf_print_level level,
> >
> > int main(int argc, const char **argv)
> > {
> > - int err;
> > + int err, done_help = 0;
> > const char *cmd;
> > char sbuf[STRERR_BUFSIZE];
> >
> > @@ -558,8 +558,6 @@ int main(int argc, const char **argv)
> > pthread__block_sigwinch();
> >
> > while (1) {
> > - int done_help;
> > -
> > run_argv(&argc, &argv);
> >
> > if (errno != ENOENT)
> > ⬢[acme@toolbox perf-tools-next]$ perf raccord
> > Fatal: Out of memory, realloc failed
> > ⬢[acme@toolbox perf-tools-next]$
> >
> > Then:
> >
> > diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> > index c719e6ccd9e27778..54f62aa6612bc290 100644
> > --- a/tools/perf/perf.c
> > +++ b/tools/perf/perf.c
> > @@ -558,7 +558,7 @@ int main(int argc, const char **argv)
> > pthread__block_sigwinch();
> >
> > while (1) {
> > - int done_help;
> > + static int done_help;
> >
> > run_argv(&argc, &argv);
> >
> > ⬢[acme@toolbox perf-tools-next]$ perf raccord
> > Fatal: Out of memory, realloc failed
> > ⬢[acme@toolbox perf-tools-next]$
>
> That main_cmds variable is uninitialized, which ends up making
> add_cmdname() to explode, are you sure this is working on your side?
>
> Further clarifying, this is without considering the second patch, I
> haven't got to it yet, from what I recall from the description it
> shouldn't matter tho.
>
> - Arnaldo
>
> ⬢[acme@toolbox perf-tools-next]$ git diff
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index c719e6ccd9e27778..164b3c78baff4204 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -558,7 +558,7 @@ int main(int argc, const char **argv)
> pthread__block_sigwinch();
>
> while (1) {
> - int done_help;
> + static int done_help;
>
> run_argv(&argc, &argv);
>
> @@ -566,7 +566,7 @@ int main(int argc, const char **argv)
> break;
>
> if (!done_help) {
> - struct cmdnames main_cmds;
> + struct cmdnames main_cmds = { 0, };
>
> for (unsigned int i = 0; i < ARRAY_SIZE(commands); i++) {
> add_cmdname(&main_cmds,
> ⬢[acme@toolbox perf-tools-next]$ perf raccord
> perf: 'raccord' is not a perf-command. See 'perf --help'.
> ⬢[acme@toolbox perf-tools-next]$
But:
⬢[acme@toolbox perf-tools-next]$ perf raccord
perf: 'raccord' is not a perf-command. See 'perf --help'.
⬢[acme@toolbox perf-tools-next]$ perf ricord
perf: 'ricord' is not a perf-command. See 'perf --help'.
Did you mean this?
record
⬢[acme@toolbox perf-tools-next]$ perf ricord
perf: 'ricord' is not a perf-command. See 'perf --help'.
Did you mean this?
record
⬢[acme@toolbox perf-tools-next]$
So I'll add that and go on from there.
- Arnaldo
On Wed, Mar 20, 2024 at 12:41:45PM -0300, Arnaldo Carvalho de Melo wrote:
> ⬢[acme@toolbox perf-tools-next]$ perf raccord
> perf: 'raccord' is not a perf-command. See 'perf --help'.
This works with the second patch.
- Arnaldo
> ⬢[acme@toolbox perf-tools-next]$ perf ricord
> perf: 'ricord' is not a perf-command. See 'perf --help'.
>
> Did you mean this?
> record
> ⬢[acme@toolbox perf-tools-next]$ perf ricord
> perf: 'ricord' is not a perf-command. See 'perf --help'.
>
> Did you mean this?
> record
> ⬢[acme@toolbox perf-tools-next]$
>
> So I'll add that and go on from there.
>
> - Arnaldo
On Wed, Mar 20, 2024 at 8:44 AM Arnaldo Carvalho de Melo
<[email protected]> wrote:
>
> On Wed, Mar 20, 2024 at 12:41:45PM -0300, Arnaldo Carvalho de Melo wrote:
> > ⬢[acme@toolbox perf-tools-next]$ perf raccord
> > perf: 'raccord' is not a perf-command. See 'perf --help'.
>
> This works with the second patch.
>
> - Arnaldo
>
> > ⬢[acme@toolbox perf-tools-next]$ perf ricord
> > perf: 'ricord' is not a perf-command. See 'perf --help'.
> >
> > Did you mean this?
> > record
> > ⬢[acme@toolbox perf-tools-next]$ perf ricord
> > perf: 'ricord' is not a perf-command. See 'perf --help'.
> >
> > Did you mean this?
> > record
> > ⬢[acme@toolbox perf-tools-next]$
> >
> > So I'll add that and go on from there.
Thanks, let me know if I need to send a v3 with the 2 uninitialized
variables fixed.
Thanks,
Ian
On Wed, Mar 20, 2024 at 10:07:55AM -0700, Ian Rogers wrote:
> On Wed, Mar 20, 2024 at 8:44 AM Arnaldo Carvalho de Melo
> <[email protected]> wrote:
> >
> > On Wed, Mar 20, 2024 at 12:41:45PM -0300, Arnaldo Carvalho de Melo wrote:
> > > ⬢[acme@toolbox perf-tools-next]$ perf raccord
> > > perf: 'raccord' is not a perf-command. See 'perf --help'.
> >
> > This works with the second patch.
> >
> > - Arnaldo
> >
> > > ⬢[acme@toolbox perf-tools-next]$ perf ricord
> > > perf: 'ricord' is not a perf-command. See 'perf --help'.
> > >
> > > Did you mean this?
> > > record
> > > ⬢[acme@toolbox perf-tools-next]$ perf ricord
> > > perf: 'ricord' is not a perf-command. See 'perf --help'.
> > >
> > > Did you mean this?
> > > record
> > > ⬢[acme@toolbox perf-tools-next]$
> > >
> > > So I'll add that and go on from there.
>
> Thanks, let me know if I need to send a v3 with the 2 uninitialized
> variables fixed.
No need, its all in tmp.perf-tools-next now, will go to perf-tools-next
soon,
- Arnaldo