MIME-Version: 1.0
References: <20230311112122.28894-1-p4ranlee@gmail.com>
In-Reply-To: <20230311112122.28894-1-p4ranlee@gmail.com>
From:   Ian Rogers <irogers@google.com>
Date:   Sat, 11 Mar 2023 22:27:41 -0800
Message-ID: <CAP-5=fWkAqX+Uy_XhXHjEx6sj-wpOJ+WQf=vMtRzRBS-0Kj32Q@mail.gmail.com>
Subject: Re: [PATCH] perf tools riscv: Add support for riscv lookup_binutils_path
To:     paranlee <p4ranlee@gmail.com>
Cc:     Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Ananth N Mavinakayanahalli <ananth@in.ibm.com>,
        Anton Blanchard <anton@ozlabs.org>,
        Daniel Axtens <dja@axtens.net>,
        Michael Ellerman <mpe@ellerman.id.au>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-riscv@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Mar 11, 2023 at 3:22=E2=80=AFAM paranlee <p4ranlee@gmail.com> wrote=
:
>
> Add to know RISC-V binutils path.
> Secondarily, edit the code block with alphabetical order.
>
> Signed-off-by: Paran Lee <p4ranlee@gmail.com>
> ---
>  tools/perf/arch/common.c | 51 +++++++++++++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/tools/perf/arch/common.c b/tools/perf/arch/common.c
> index 59dd875fd5e4..058527ededdd 100644
> --- a/tools/perf/arch/common.c
> +++ b/tools/perf/arch/common.c
> @@ -29,11 +29,23 @@ const char *const arm_triplets[] =3D {
>  };
>
>  const char *const arm64_triplets[] =3D {
> +       "aarch64-unknown-linux-",

Modifying ARM64 behavior should be a separate change.

>         "aarch64-linux-android-",
>         "aarch64-linux-gnu-",
>         NULL
>  };
>
> +const char *const mips_triplets[] =3D {
> +       "mips-unknown-linux-gnu-",
> +       "mipsel-linux-android-",
> +       "mips-linux-gnu-",
> +       "mips64-linux-gnu-",
> +       "mips64el-linux-gnuabi64-",
> +       "mips64-linux-gnuabi64-",
> +       "mipsel-linux-gnu-",
> +       NULL
> +};
> +

This will affect the blame history. It should probably be its own change to=
o.

>  const char *const powerpc_triplets[] =3D {
>         "powerpc-unknown-linux-gnu-",
>         "powerpc-linux-gnu-",
> @@ -43,6 +55,20 @@ const char *const powerpc_triplets[] =3D {
>         NULL
>  };
>
> +const char *const riscv32_triplets[] =3D {
> +       "riscv32-unknown-linux-gnu-",
> +       "riscv32-linux-android-",
> +       "riscv32-linux-gnu-",
> +       NULL
> +};
> +
> +const char *const riscv64_triplets[] =3D {
> +       "riscv64-unknown-linux-gnu-",
> +       "riscv64-linux-android-",
> +       "riscv64-linux-gnu-",
> +       NULL
> +};
> +
>  const char *const s390_triplets[] =3D {
>         "s390-ibm-linux-",
>         "s390x-linux-gnu-",
> @@ -78,17 +104,6 @@ const char *const x86_triplets[] =3D {
>         NULL
>  };
>
> -const char *const mips_triplets[] =3D {
> -       "mips-unknown-linux-gnu-",
> -       "mipsel-linux-android-",
> -       "mips-linux-gnu-",
> -       "mips64-linux-gnu-",
> -       "mips64el-linux-gnuabi64-",
> -       "mips64-linux-gnuabi64-",
> -       "mipsel-linux-gnu-",
> -       NULL
> -};
> -
>  static bool lookup_path(char *name)
>  {
>         bool found =3D false;
> @@ -164,18 +179,22 @@ static int perf_env__lookup_binutils_path(struct pe=
rf_env *env,
>                 path_list =3D arm_triplets;
>         else if (!strcmp(arch, "arm64"))
>                 path_list =3D arm64_triplets;
> +       else if (!strcmp(arch, "mips"))
> +               path_list =3D mips_triplets;
>         else if (!strcmp(arch, "powerpc"))
>                 path_list =3D powerpc_triplets;
> -       else if (!strcmp(arch, "sh"))
> -               path_list =3D sh_triplets;
> +       else if (!strcmp(arch, "riscv32"))
> +               path_list =3D riscv32_triplets;
> +       else if (!strcmp(arch, "riscv64"))
> +               path_list =3D riscv64_triplets;
>         else if (!strcmp(arch, "s390"))
> -               path_list =3D s390_triplets;
> +               path_list =3D s390_triplets;

whitespace issue?

> +       else if (!strcmp(arch, "sh"))
> +               path_list =3D sh_triplets;
>         else if (!strcmp(arch, "sparc"))
>                 path_list =3D sparc_triplets;
>         else if (!strcmp(arch, "x86"))
>                 path_list =3D x86_triplets;
> -       else if (!strcmp(arch, "mips"))
> -               path_list =3D mips_triplets;
>         else {
>                 ui__error("binutils for %s not supported.\n", arch);
>                 goto out_error;

I think in general we need to revamp this code. Two things that I see
that are missing are (1) support for perf config and (2) addr2line
should be configurable, you may want llvm-addr2line. Adding RISC-V is
of course important too :-)

Thanks,
Ian

> --
> 2.34.1
>