Re: [PATCH] perf tools riscv: Add support for riscv lookup_binutils_path

[Ian Rogers]

On Sat, Mar 11, 2023 at 3:22 AM paranlee <p4ranlee@xxxxxxxxx> wrote:
>
> Add to know RISC-V binutils path.
> Secondarily, edit the code block with alphabetical order.
>
> Signed-off-by: Paran Lee <p4ranlee@xxxxxxxxx>
> ---
>  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[] = {
>  };
>
>  const char *const arm64_triplets[] = {
> +       "aarch64-unknown-linux-",

Modifying ARM64 behavior should be a separate change.

>         "aarch64-linux-android-",
>         "aarch64-linux-gnu-",
>         NULL
>  };
>
> +const char *const mips_triplets[] = {
> +       "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 too.

>  const char *const powerpc_triplets[] = {
>         "powerpc-unknown-linux-gnu-",
>         "powerpc-linux-gnu-",
> @@ -43,6 +55,20 @@ const char *const powerpc_triplets[] = {
>         NULL
>  };
>
> +const char *const riscv32_triplets[] = {
> +       "riscv32-unknown-linux-gnu-",
> +       "riscv32-linux-android-",
> +       "riscv32-linux-gnu-",
> +       NULL
> +};
> +
> +const char *const riscv64_triplets[] = {
> +       "riscv64-unknown-linux-gnu-",
> +       "riscv64-linux-android-",
> +       "riscv64-linux-gnu-",
> +       NULL
> +};
> +
>  const char *const s390_triplets[] = {
>         "s390-ibm-linux-",
>         "s390x-linux-gnu-",
> @@ -78,17 +104,6 @@ const char *const x86_triplets[] = {
>         NULL
>  };
>
> -const char *const mips_triplets[] = {
> -       "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 = false;
> @@ -164,18 +179,22 @@ static int perf_env__lookup_binutils_path(struct perf_env *env,
>                 path_list = arm_triplets;
>         else if (!strcmp(arch, "arm64"))
>                 path_list = arm64_triplets;
> +       else if (!strcmp(arch, "mips"))
> +               path_list = mips_triplets;
>         else if (!strcmp(arch, "powerpc"))
>                 path_list = powerpc_triplets;
> -       else if (!strcmp(arch, "sh"))
> -               path_list = sh_triplets;
> +       else if (!strcmp(arch, "riscv32"))
> +               path_list = riscv32_triplets;
> +       else if (!strcmp(arch, "riscv64"))
> +               path_list = riscv64_triplets;
>         else if (!strcmp(arch, "s390"))
> -               path_list = s390_triplets;
> +               path_list = s390_triplets;

whitespace issue?

> +       else if (!strcmp(arch, "sh"))
> +               path_list = sh_triplets;
>         else if (!strcmp(arch, "sparc"))
>                 path_list = sparc_triplets;
>         else if (!strcmp(arch, "x86"))
>                 path_list = x86_triplets;
> -       else if (!strcmp(arch, "mips"))
> -               path_list = 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
>