MIME-Version: 1.0
References: <20230311112122.28894-1-p4ranlee@gmail.com> <CAP-5=fWkAqX+Uy_XhXHjEx6sj-wpOJ+WQf=vMtRzRBS-0Kj32Q@mail.gmail.com>
 <09d44d74-f3c9-cf99-8d14-74499485e956@gmail.com>
In-Reply-To: <09d44d74-f3c9-cf99-8d14-74499485e956@gmail.com>
From:   Ian Rogers <irogers@google.com>
Date:   Mon, 13 Mar 2023 09:27:35 -0700
Message-ID: <CAP-5=fUqLdCD_Ks2WQxjyRoNO53PxzDv_P7GJuY320+gKF=vEw@mail.gmail.com>
Subject: Re: [PATCH] perf tools riscv: Add support for riscv lookup_binutils_path
To:     Paran Lee <p4ranlee@gmail.com>
Cc:     Namhyung Kim <namhyung@kernel.org>,
        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 Sun, Mar 12, 2023 at 5:53=E2=80=AFAM Paran Lee <p4ranlee@gmail.com> wrot=
e:
>
>
>
> 23. 3. 12. 15:27=EC=97=90 Ian Rogers =EC=9D=B4(=EA=B0=80) =EC=93=B4 =EA=
=B8=80:
> > On Sat, Mar 11, 2023 at 3:22=E2=80=AFAM paranlee <p4ranlee@gmail.com> w=
rote:
> >>
> >> 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 chang=
e too.
>
> Thank you for review! I agree. So I would split the patch.
>
> >> -
> >>  static bool lookup_path(char *name)
> >>  {
> >>         bool found =3D false;
> >> @@ -164,18 +179,22 @@ static int perf_env__lookup_binutils_path(struct=
 perf_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?
>
> I tried to correct the alphabetical order because it was vaguely sorted.
> And I'll try to work on blame history on each arch code block as well.
>
> >> +       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
>
> May I ask documentation or hint that I can help work with?
>
> P.S.
>
> I'm interested in the Google Summer Of code perf category this year,
> especially the part about risc-v architecture, I recently purchased a
> development board and would like to be able to test perf on a Sifive U74
> CPU based environment.
> But I've only used perf with command tool and don't know much about the
> internals, so if there is a roadmap for perf development or
> contribution, I have interest in perf internals both kernel and user side=
.
> May I ask information to apply?
> I am developing Linux Security Driver drivers for a security company.
>
> BR
> Paran Lee

Hi Paran,

Thanks for being interested in GSoC with Linux perf. Here is what I
posted on the mailing list:
https://lore.kernel.org/linux-perf-users/CAP-5=3DfWxF6in4vQyGuh=3D0kpAYEXAY=
ZN_KobXCY=3DTX2oxssZ+HQ@mail.gmail.com/
Applications are ultimately sent to:
https://summerofcode.withgoogle.com/
and the entry requirements are there. I believe they are less strict
than previously.

Wrt the patch, could you reply to Conor's response.

Thanks,
Ian