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

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

[paranlee]

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[] = {
 };
 
 const char *const arm64_triplets[] = {
+	"aarch64-unknown-linux-",
 	"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
+};
+
 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;		
+	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;
-- 
2.34.1

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@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[] = {
>  };
>
>  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
>

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

[Paran Lee]

23. 3. 12. 15:27에 Ian Rogers 이(가) 쓴 글:
> On Sat, Mar 11, 2023 at 3:22 AM 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[] = {
>>  };
>>
>>  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.

Thank you for review! I agree. So I would split the patch.

>> -
>>  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?

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 = 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

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

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

[Ian Rogers]

On Sun, Mar 12, 2023 at 5:53 AM Paran Lee <p4ranlee@gmail.com> wrote:
>
>
>
> 23. 3. 12. 15:27에 Ian Rogers 이(가) 쓴 글:
> > On Sat, Mar 11, 2023 at 3:22 AM 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[] = {
> >>  };
> >>
> >>  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.
>
> Thank you for review! I agree. So I would split the patch.
>
> >> -
> >>  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?
>
> 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 = 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
>
> 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=fWxF6in4vQyGuh=0kpAYEXAYZN_KobXCY=TX2oxssZ+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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

On Mon, Mar 13, 2023 at 09:27:35AM -0700, Ian Rogers wrote:

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

Apologies if I asked something silly, I've been ill and kinda out of it,
just noticed it as I tried to keep up with mail and was curious...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]