Received-SPF: pass (google.com: domain of linux-kernel+bounces-109112-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240301201306.2680986-1-irogers@google.com> <Zfr4jX_b8FCOG_x_@x1>
 <Zfr5__Ej3-0C8sJj@x1> <CAP-5=fXXrJRH=Dto2ajD_TUDE1YmkkJZO5Ey2pq5YB0wbVAzeg@mail.gmail.com>
 <Zfr-sQJPbZzbtk8K@x1> <ZfsBopALY3whsst5@x1>
In-Reply-To: <ZfsBopALY3whsst5@x1>
From: Ian Rogers <irogers@google.com>
Date: Wed, 20 Mar 2024 08:40:52 -0700
Message-ID: <CAP-5=fXwfckSBwMVRj_MuZ=aZkkS-bxrcWkzg1y=AZwFQGUjCQ@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:32=E2=80=AFAM Arnaldo Carvalho de Melo
<acme@kernel.org> 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).
> >
> > =E2=AC=A2[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 l=
evel,
> >
> >  int main(int argc, const char **argv)
> >  {
> > -       int err;
> > +       int err, done_help =3D 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 !=3D ENOENT)
> > =E2=AC=A2[acme@toolbox perf-tools-next]$ perf raccord
> >   Fatal: Out of memory, realloc failed
> > =E2=AC=A2[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);
> >
> > =E2=AC=A2[acme@toolbox perf-tools-next]$ perf raccord
> >   Fatal: Out of memory, realloc failed
> > =E2=AC=A2[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
>
> =E2=AC=A2[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 =3D { 0, };
>
>                         for (unsigned int i =3D 0; i < ARRAY_SIZE(command=
s); i++) {
>                                 add_cmdname(&main_cmds,
> =E2=AC=A2[acme@toolbox perf-tools-next]$ perf raccord
> perf: 'raccord' is not a perf-command. See 'perf --help'.
> =E2=AC=A2[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