mbox series

[v2,0/6] perf parse-regs: Refactor architecture functions

Message ID 20230606014559.21783-1-leo.yan@linaro.org (mailing list archive)
Headers show
Series perf parse-regs: Refactor architecture functions | expand

Message

Leo Yan June 6, 2023, 1:45 a.m. UTC
This patch series is to refactor arch related functions for register
parsing, which follows up the discussion for v1:
https://lore.kernel.org/lkml/20230520025537.1811986-1-leo.yan@linaro.org/

Compared to patch series v1, this patch series introduces new functions
perf_arch_reg_{ip|sp}(), so this can allow the tool to support cross
analysis.

To verify the cross analysis, I used below steps:

- Firstly, I captured perf data on Arm64 machine:

  $ perf record --call-graph fp -- ./test_program

  Or ...

  $ perf record --call-graph dwarf -- ./test_program

  Then, I also archived associated debug data:

  $ perf archive

- Secondly, I copied the perf data file and debug tar file on my x86
  machine:

  $ scp perf.data perf.data.tar.bz2 leoy@IP_ADDRESS:/target/path/

- On x86 machine, I need to build perf for support multi-arch unwinding:

  $ git clone http://git.savannah.gnu.org/r/libunwind.git
  $ cd libunwind
  $ autoreconf -i

  # Build and install libunwind aarch64:
  $ ./configure prefix=/home/leoy/Work/tools/libunwind/install/ \
	--target=aarch64-linux-gnu CC=x86_64-linux-gnu-gcc
  $ make && make install

  # Build and install libunwind x86:
  $ ./configure prefix=/home/leoy/Work/tools/libunwind/install/ \
	--target=x86_64-linux-gnu CC=x86_64-linux-gnu-gcc
  $ make && make install

- Build perf tool for support multi-archs:

  $ cd $LINUX/tools/perf
  $ make VF=1 DEBUG=1 LIBUNWIND_DIR=/home/leoy/Work/tools/libunwind/install

At the end, I verified the x86 perf tool can do cross analysis for aarch64's
perf data file.

Note, I still see x86 perf tool cannot display the complete callgraph
for aarch64, but it should not the issue caused by this series, which
will be addressed by separate patches.

I also built this patch series on my Arm64 and x86 machines, both can
compile perf tool successfully; but I have no chance to build other
archs natively.

Changes from v1:
- For support cross analysis for IP/SP registers, introduced patch 0002
  (James Clark, Ian Rogers).


Leo Yan (6):
  perf parse-regs: Refactor arch register parsing functions
  perf parse-regs: Introduce functions perf_arch_reg_{ip|sp}()
  perf unwind: Use perf_arch_reg_{ip|sp}() to substitute macros
  perf parse-regs: Remove unused macros PERF_REG_{IP|SP}
  perf parse-regs: Remove PERF_REGS_{MAX|MASK} from common code
  perf parse-regs: Move out arch specific header from util/perf_regs.h

 tools/perf/arch/arm/include/perf_regs.h       |   3 -
 tools/perf/arch/arm/util/perf_regs.c          |  11 +
 tools/perf/arch/arm/util/unwind-libdw.c       |   1 +
 tools/perf/arch/arm64/include/perf_regs.h     |   3 -
 tools/perf/arch/arm64/util/machine.c          |   1 +
 tools/perf/arch/arm64/util/perf_regs.c        |   6 +
 tools/perf/arch/arm64/util/unwind-libdw.c     |   1 +
 tools/perf/arch/csky/include/perf_regs.h      |   3 -
 tools/perf/arch/csky/util/perf_regs.c         |  11 +
 tools/perf/arch/csky/util/unwind-libdw.c      |   1 +
 tools/perf/arch/loongarch/include/perf_regs.h |   2 -
 tools/perf/arch/loongarch/util/perf_regs.c    |  11 +
 tools/perf/arch/loongarch/util/unwind-libdw.c |   1 +
 tools/perf/arch/mips/include/perf_regs.h      |   2 -
 tools/perf/arch/mips/util/perf_regs.c         |  11 +
 tools/perf/arch/powerpc/include/perf_regs.h   |   3 -
 tools/perf/arch/powerpc/util/perf_regs.c      |   6 +
 tools/perf/arch/powerpc/util/unwind-libdw.c   |   1 +
 tools/perf/arch/riscv/include/perf_regs.h     |   3 -
 tools/perf/arch/riscv/util/perf_regs.c        |  11 +
 tools/perf/arch/riscv/util/unwind-libdw.c     |   1 +
 tools/perf/arch/s390/include/perf_regs.h      |   3 -
 tools/perf/arch/s390/util/perf_regs.c         |  11 +
 tools/perf/arch/s390/util/unwind-libdw.c      |   1 +
 tools/perf/arch/x86/include/perf_regs.h       |   2 -
 tools/perf/arch/x86/util/perf_regs.c          |   6 +
 tools/perf/arch/x86/util/unwind-libdw.c       |   1 +
 tools/perf/util/Build                         |   1 +
 tools/perf/util/evsel.c                       |   6 +-
 tools/perf/util/libunwind/arm64.c             |   2 -
 tools/perf/util/libunwind/x86_32.c            |   2 -
 tools/perf/util/perf-regs-arch/Build          |   9 +
 .../util/perf-regs-arch/perf_regs_aarch64.c   |  96 +++
 .../perf/util/perf-regs-arch/perf_regs_arm.c  |  60 ++
 .../perf/util/perf-regs-arch/perf_regs_csky.c | 100 +++
 .../util/perf-regs-arch/perf_regs_loongarch.c |  91 +++
 .../perf/util/perf-regs-arch/perf_regs_mips.c |  87 ++
 .../util/perf-regs-arch/perf_regs_powerpc.c   | 145 ++++
 .../util/perf-regs-arch/perf_regs_riscv.c     |  92 +++
 .../perf/util/perf-regs-arch/perf_regs_s390.c |  96 +++
 .../perf/util/perf-regs-arch/perf_regs_x86.c  |  98 +++
 tools/perf/util/perf_regs.c                   | 772 ++----------------
 tools/perf/util/perf_regs.h                   |  49 +-
 tools/perf/util/unwind-libdw.c                |   7 +-
 tools/perf/util/unwind-libunwind-local.c      |   6 +-
 tools/perf/util/unwind.h                      |   8 -
 46 files changed, 1078 insertions(+), 766 deletions(-)
 create mode 100644 tools/perf/util/perf-regs-arch/Build
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_aarch64.c
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_arm.c
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_csky.c
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_loongarch.c
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_mips.c
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_powerpc.c
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_riscv.c
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_s390.c
 create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_x86.c

Comments

Ian Rogers July 12, 2023, 10:37 p.m. UTC | #1
On Mon, Jun 5, 2023 at 6:46 PM Leo Yan <leo.yan@linaro.org> wrote:
>
> This patch series is to refactor arch related functions for register
> parsing, which follows up the discussion for v1:
> https://lore.kernel.org/lkml/20230520025537.1811986-1-leo.yan@linaro.org/
>
> Compared to patch series v1, this patch series introduces new functions
> perf_arch_reg_{ip|sp}(), so this can allow the tool to support cross
> analysis.
>
> To verify the cross analysis, I used below steps:
>
> - Firstly, I captured perf data on Arm64 machine:
>
>   $ perf record --call-graph fp -- ./test_program
>
>   Or ...
>
>   $ perf record --call-graph dwarf -- ./test_program
>
>   Then, I also archived associated debug data:
>
>   $ perf archive
>
> - Secondly, I copied the perf data file and debug tar file on my x86
>   machine:
>
>   $ scp perf.data perf.data.tar.bz2 leoy@IP_ADDRESS:/target/path/
>
> - On x86 machine, I need to build perf for support multi-arch unwinding:
>
>   $ git clone http://git.savannah.gnu.org/r/libunwind.git
>   $ cd libunwind
>   $ autoreconf -i
>
>   # Build and install libunwind aarch64:
>   $ ./configure prefix=/home/leoy/Work/tools/libunwind/install/ \
>         --target=aarch64-linux-gnu CC=x86_64-linux-gnu-gcc
>   $ make && make install
>
>   # Build and install libunwind x86:
>   $ ./configure prefix=/home/leoy/Work/tools/libunwind/install/ \
>         --target=x86_64-linux-gnu CC=x86_64-linux-gnu-gcc
>   $ make && make install
>
> - Build perf tool for support multi-archs:
>
>   $ cd $LINUX/tools/perf
>   $ make VF=1 DEBUG=1 LIBUNWIND_DIR=/home/leoy/Work/tools/libunwind/install
>
> At the end, I verified the x86 perf tool can do cross analysis for aarch64's
> perf data file.
>
> Note, I still see x86 perf tool cannot display the complete callgraph
> for aarch64, but it should not the issue caused by this series, which
> will be addressed by separate patches.
>
> I also built this patch series on my Arm64 and x86 machines, both can
> compile perf tool successfully; but I have no chance to build other
> archs natively.
>
> Changes from v1:
> - For support cross analysis for IP/SP registers, introduced patch 0002
>   (James Clark, Ian Rogers).
>
>
> Leo Yan (6):
>   perf parse-regs: Refactor arch register parsing functions
>   perf parse-regs: Introduce functions perf_arch_reg_{ip|sp}()
>   perf unwind: Use perf_arch_reg_{ip|sp}() to substitute macros
>   perf parse-regs: Remove unused macros PERF_REG_{IP|SP}
>   perf parse-regs: Remove PERF_REGS_{MAX|MASK} from common code
>   perf parse-regs: Move out arch specific header from util/perf_regs.h

Sorry for the slow review. For the series:
Acked-by: Ian Rogers <irogers@google.com>

Some thoughts:
uint64_t __perf_reg_ip_arm(void)
uint64_t seems like we're giving a lot of space for future register
encodings. I think some of the other functions use this size of value
due to returning a bitmap/mask, but here it isn't clear and just feels
excessive.

Do we need the "__" prefix on all the functions?

In Makefile.config there are NO_PERF_REGS and CONFIG_PERF_REGS then
the define HAVE_PERF_REGS_SUPPORT. Is this still relevant? If we had
an architecture with no support, couldn't it still read a perf.data
file from a supported architecture? It would be nice to remove at
least NO_PERF_REGS and HAVE_PERF_REGS_SUPPORT.

This change is very worthwhile fix and cleanup, it didn't introduce
what is pondered above, hence the acked-by.

Thanks!
Ian

>  tools/perf/arch/arm/include/perf_regs.h       |   3 -
>  tools/perf/arch/arm/util/perf_regs.c          |  11 +
>  tools/perf/arch/arm/util/unwind-libdw.c       |   1 +
>  tools/perf/arch/arm64/include/perf_regs.h     |   3 -
>  tools/perf/arch/arm64/util/machine.c          |   1 +
>  tools/perf/arch/arm64/util/perf_regs.c        |   6 +
>  tools/perf/arch/arm64/util/unwind-libdw.c     |   1 +
>  tools/perf/arch/csky/include/perf_regs.h      |   3 -
>  tools/perf/arch/csky/util/perf_regs.c         |  11 +
>  tools/perf/arch/csky/util/unwind-libdw.c      |   1 +
>  tools/perf/arch/loongarch/include/perf_regs.h |   2 -
>  tools/perf/arch/loongarch/util/perf_regs.c    |  11 +
>  tools/perf/arch/loongarch/util/unwind-libdw.c |   1 +
>  tools/perf/arch/mips/include/perf_regs.h      |   2 -
>  tools/perf/arch/mips/util/perf_regs.c         |  11 +
>  tools/perf/arch/powerpc/include/perf_regs.h   |   3 -
>  tools/perf/arch/powerpc/util/perf_regs.c      |   6 +
>  tools/perf/arch/powerpc/util/unwind-libdw.c   |   1 +
>  tools/perf/arch/riscv/include/perf_regs.h     |   3 -
>  tools/perf/arch/riscv/util/perf_regs.c        |  11 +
>  tools/perf/arch/riscv/util/unwind-libdw.c     |   1 +
>  tools/perf/arch/s390/include/perf_regs.h      |   3 -
>  tools/perf/arch/s390/util/perf_regs.c         |  11 +
>  tools/perf/arch/s390/util/unwind-libdw.c      |   1 +
>  tools/perf/arch/x86/include/perf_regs.h       |   2 -
>  tools/perf/arch/x86/util/perf_regs.c          |   6 +
>  tools/perf/arch/x86/util/unwind-libdw.c       |   1 +
>  tools/perf/util/Build                         |   1 +
>  tools/perf/util/evsel.c                       |   6 +-
>  tools/perf/util/libunwind/arm64.c             |   2 -
>  tools/perf/util/libunwind/x86_32.c            |   2 -
>  tools/perf/util/perf-regs-arch/Build          |   9 +
>  .../util/perf-regs-arch/perf_regs_aarch64.c   |  96 +++
>  .../perf/util/perf-regs-arch/perf_regs_arm.c  |  60 ++
>  .../perf/util/perf-regs-arch/perf_regs_csky.c | 100 +++
>  .../util/perf-regs-arch/perf_regs_loongarch.c |  91 +++
>  .../perf/util/perf-regs-arch/perf_regs_mips.c |  87 ++
>  .../util/perf-regs-arch/perf_regs_powerpc.c   | 145 ++++
>  .../util/perf-regs-arch/perf_regs_riscv.c     |  92 +++
>  .../perf/util/perf-regs-arch/perf_regs_s390.c |  96 +++
>  .../perf/util/perf-regs-arch/perf_regs_x86.c  |  98 +++
>  tools/perf/util/perf_regs.c                   | 772 ++----------------
>  tools/perf/util/perf_regs.h                   |  49 +-
>  tools/perf/util/unwind-libdw.c                |   7 +-
>  tools/perf/util/unwind-libunwind-local.c      |   6 +-
>  tools/perf/util/unwind.h                      |   8 -
>  46 files changed, 1078 insertions(+), 766 deletions(-)
>  create mode 100644 tools/perf/util/perf-regs-arch/Build
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_aarch64.c
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_arm.c
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_csky.c
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_loongarch.c
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_mips.c
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_powerpc.c
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_riscv.c
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_s390.c
>  create mode 100644 tools/perf/util/perf-regs-arch/perf_regs_x86.c
>
> --
> 2.34.1
>
Arnaldo Carvalho de Melo Aug. 15, 2023, 6:24 p.m. UTC | #2
Em Wed, Jul 12, 2023 at 03:37:36PM -0700, Ian Rogers escreveu:
> On Mon, Jun 5, 2023 at 6:46 PM Leo Yan <leo.yan@linaro.org> wrote:
> >
> > This patch series is to refactor arch related functions for register
> > parsing, which follows up the discussion for v1:
> > https://lore.kernel.org/lkml/20230520025537.1811986-1-leo.yan@linaro.org/
> >
> > Compared to patch series v1, this patch series introduces new functions
> > perf_arch_reg_{ip|sp}(), so this can allow the tool to support cross
> > analysis.
> >
> > To verify the cross analysis, I used below steps:
> >
> > - Firstly, I captured perf data on Arm64 machine:
> >
> >   $ perf record --call-graph fp -- ./test_program
> >
> >   Or ...
> >
> >   $ perf record --call-graph dwarf -- ./test_program
> >
> >   Then, I also archived associated debug data:
> >
> >   $ perf archive
> >
> > - Secondly, I copied the perf data file and debug tar file on my x86
> >   machine:
> >
> >   $ scp perf.data perf.data.tar.bz2 leoy@IP_ADDRESS:/target/path/
> >
> > - On x86 machine, I need to build perf for support multi-arch unwinding:
> >
> >   $ git clone http://git.savannah.gnu.org/r/libunwind.git
> >   $ cd libunwind
> >   $ autoreconf -i
> >
> >   # Build and install libunwind aarch64:
> >   $ ./configure prefix=/home/leoy/Work/tools/libunwind/install/ \
> >         --target=aarch64-linux-gnu CC=x86_64-linux-gnu-gcc
> >   $ make && make install
> >
> >   # Build and install libunwind x86:
> >   $ ./configure prefix=/home/leoy/Work/tools/libunwind/install/ \
> >         --target=x86_64-linux-gnu CC=x86_64-linux-gnu-gcc
> >   $ make && make install
> >
> > - Build perf tool for support multi-archs:
> >
> >   $ cd $LINUX/tools/perf
> >   $ make VF=1 DEBUG=1 LIBUNWIND_DIR=/home/leoy/Work/tools/libunwind/install
> >
> > At the end, I verified the x86 perf tool can do cross analysis for aarch64's
> > perf data file.
> >
> > Note, I still see x86 perf tool cannot display the complete callgraph
> > for aarch64, but it should not the issue caused by this series, which
> > will be addressed by separate patches.
> >
> > I also built this patch series on my Arm64 and x86 machines, both can
> > compile perf tool successfully; but I have no chance to build other
> > archs natively.
> >
> > Changes from v1:
> > - For support cross analysis for IP/SP registers, introduced patch 0002
> >   (James Clark, Ian Rogers).
> >
> >
> > Leo Yan (6):
> >   perf parse-regs: Refactor arch register parsing functions
> >   perf parse-regs: Introduce functions perf_arch_reg_{ip|sp}()
> >   perf unwind: Use perf_arch_reg_{ip|sp}() to substitute macros
> >   perf parse-regs: Remove unused macros PERF_REG_{IP|SP}
> >   perf parse-regs: Remove PERF_REGS_{MAX|MASK} from common code
> >   perf parse-regs: Move out arch specific header from util/perf_regs.h
> 
> Sorry for the slow review. For the series:
> Acked-by: Ian Rogers <irogers@google.com>
> 
> Some thoughts:
> uint64_t __perf_reg_ip_arm(void)
> uint64_t seems like we're giving a lot of space for future register
> encodings. I think some of the other functions use this size of value
> due to returning a bitmap/mask, but here it isn't clear and just feels
> excessive.
> 
> Do we need the "__" prefix on all the functions?
> 
> In Makefile.config there are NO_PERF_REGS and CONFIG_PERF_REGS then
> the define HAVE_PERF_REGS_SUPPORT. Is this still relevant? If we had
> an architecture with no support, couldn't it still read a perf.data
> file from a supported architecture? It would be nice to remove at
> least NO_PERF_REGS and HAVE_PERF_REGS_SUPPORT.
> 
> This change is very worthwhile fix and cleanup, it didn't introduce
> what is pondered above, hence the acked-by.

Agreed, applied to perf-tools-next, sorry for the delay.

- Arnaldo
Arnaldo Carvalho de Melo Aug. 15, 2023, 6:45 p.m. UTC | #3
Em Tue, Aug 15, 2023 at 03:24:04PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Jul 12, 2023 at 03:37:36PM -0700, Ian Rogers escreveu:
> > On Mon, Jun 5, 2023 at 6:46 PM Leo Yan <leo.yan@linaro.org> wrote:
> > >
> > > This patch series is to refactor arch related functions for register
> > > parsing, which follows up the discussion for v1:
> > > https://lore.kernel.org/lkml/20230520025537.1811986-1-leo.yan@linaro.org/
> > >
> > > Compared to patch series v1, this patch series introduces new functions
> > > perf_arch_reg_{ip|sp}(), so this can allow the tool to support cross
> > > analysis.
> > >
> > > To verify the cross analysis, I used below steps:
> > >
> > > - Firstly, I captured perf data on Arm64 machine:
> > >
> > >   $ perf record --call-graph fp -- ./test_program
> > >
> > >   Or ...
> > >
> > >   $ perf record --call-graph dwarf -- ./test_program
> > >
> > >   Then, I also archived associated debug data:
> > >
> > >   $ perf archive
> > >
> > > - Secondly, I copied the perf data file and debug tar file on my x86
> > >   machine:
> > >
> > >   $ scp perf.data perf.data.tar.bz2 leoy@IP_ADDRESS:/target/path/
> > >
> > > - On x86 machine, I need to build perf for support multi-arch unwinding:
> > >
> > >   $ git clone http://git.savannah.gnu.org/r/libunwind.git
> > >   $ cd libunwind
> > >   $ autoreconf -i
> > >
> > >   # Build and install libunwind aarch64:
> > >   $ ./configure prefix=/home/leoy/Work/tools/libunwind/install/ \
> > >         --target=aarch64-linux-gnu CC=x86_64-linux-gnu-gcc
> > >   $ make && make install
> > >
> > >   # Build and install libunwind x86:
> > >   $ ./configure prefix=/home/leoy/Work/tools/libunwind/install/ \
> > >         --target=x86_64-linux-gnu CC=x86_64-linux-gnu-gcc
> > >   $ make && make install
> > >
> > > - Build perf tool for support multi-archs:
> > >
> > >   $ cd $LINUX/tools/perf
> > >   $ make VF=1 DEBUG=1 LIBUNWIND_DIR=/home/leoy/Work/tools/libunwind/install
> > >
> > > At the end, I verified the x86 perf tool can do cross analysis for aarch64's
> > > perf data file.
> > >
> > > Note, I still see x86 perf tool cannot display the complete callgraph
> > > for aarch64, but it should not the issue caused by this series, which
> > > will be addressed by separate patches.
> > >
> > > I also built this patch series on my Arm64 and x86 machines, both can
> > > compile perf tool successfully; but I have no chance to build other
> > > archs natively.
> > >
> > > Changes from v1:
> > > - For support cross analysis for IP/SP registers, introduced patch 0002
> > >   (James Clark, Ian Rogers).
> > >
> > >
> > > Leo Yan (6):
> > >   perf parse-regs: Refactor arch register parsing functions
> > >   perf parse-regs: Introduce functions perf_arch_reg_{ip|sp}()
> > >   perf unwind: Use perf_arch_reg_{ip|sp}() to substitute macros
> > >   perf parse-regs: Remove unused macros PERF_REG_{IP|SP}
> > >   perf parse-regs: Remove PERF_REGS_{MAX|MASK} from common code
> > >   perf parse-regs: Move out arch specific header from util/perf_regs.h
> > 
> > Sorry for the slow review. For the series:
> > Acked-by: Ian Rogers <irogers@google.com>
> > 
> > Some thoughts:
> > uint64_t __perf_reg_ip_arm(void)
> > uint64_t seems like we're giving a lot of space for future register
> > encodings. I think some of the other functions use this size of value
> > due to returning a bitmap/mask, but here it isn't clear and just feels
> > excessive.
> > 
> > Do we need the "__" prefix on all the functions?
> > 
> > In Makefile.config there are NO_PERF_REGS and CONFIG_PERF_REGS then
> > the define HAVE_PERF_REGS_SUPPORT. Is this still relevant? If we had
> > an architecture with no support, couldn't it still read a perf.data
> > file from a supported architecture? It would be nice to remove at
> > least NO_PERF_REGS and HAVE_PERF_REGS_SUPPORT.
> > 
> > This change is very worthwhile fix and cleanup, it didn't introduce
> > what is pondered above, hence the acked-by.
> 
> Agreed, applied to perf-tools-next, sorry for the delay.

Had to add this to make 'perf test python' to work. Please run 'perf
test' before sending patches.

- Arnaldo

diff --git a/tools/perf/util/python-ext-sources b/tools/perf/util/python-ext-sources
index d4c9b4cd35efa556..26e1c8d973ea0b95 100644
--- a/tools/perf/util/python-ext-sources
+++ b/tools/perf/util/python-ext-sources
@@ -40,3 +40,12 @@ util/rwsem.c
 util/hashmap.c
 util/perf_regs.c
 util/fncache.c
+util/perf-regs-arch/perf_regs_aarch64.c
+util/perf-regs-arch/perf_regs_arm.c
+util/perf-regs-arch/perf_regs_csky.c
+util/perf-regs-arch/perf_regs_loongarch.c
+util/perf-regs-arch/perf_regs_mips.c
+util/perf-regs-arch/perf_regs_powerpc.c
+util/perf-regs-arch/perf_regs_riscv.c
+util/perf-regs-arch/perf_regs_s390.c
+util/perf-regs-arch/perf_regs_x86.c
Arnaldo Carvalho de Melo Aug. 15, 2023, 6:52 p.m. UTC | #4
Em Tue, Aug 15, 2023 at 03:45:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Agreed, applied to perf-tools-next, sorry for the delay.
> 
> Had to add this to make 'perf test python' to work. Please run 'perf
> test' before sending patches.

One more, please also do a 'make -C tools/perf build-test', with it I
caught this:

         make_no_libunwind_O: cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH

  CC      /tmp/tmp.yeEGyQq2HR/util/expr-flex.o
util/unwind-libdw.c: In function ‘memory_read’:
util/unwind-libdw.c:173:28: error: implicit declaration of function ‘perf_env__arch’ [-Werror=implicit-function-declaration]
  173 |         const char *arch = perf_env__arch(ui->machine->env);
      |                            ^~~~~~~~~~~~~~
util/unwind-libdw.c:173:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
util/unwind-libdw.c: In function ‘unwind__get_entries’:
util/unwind-libdw.c:258:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
  258 |         const char *arch = perf_env__arch(ui_buf.machine->env);
      |                            ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[6]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:98: /tmp/tmp.yeEGyQq2HR/util/unwind-libdw.o] Error 1
make[6]: *** Waiting for unfinished jobs....
make[5]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:150: util] Error 2
make[4]: *** [Makefile.perf:662: /tmp/tmp.yeEGyQq2HR/perf-in.o] Error 2
make[4]: *** Waiting for unfinished jobs....
  CC      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events.o
  LD      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events-in.o
make[3]: *** [Makefile.perf:238: sub-make] Error 2
make[2]: *** [Makefile:70: all] Error 2
make[1]: *** [tests/make:337: make_no_libunwind_O] Error 1
make: *** [Makefile:103: build-test] Error 2
make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'

real	1m29.784s
user	10m41.597s
sys	2m55.948s
⬢[acme@toolbox perf-tools-next]$

I'm trying to fix
Arnaldo Carvalho de Melo Aug. 15, 2023, 6:57 p.m. UTC | #5
Em Tue, Aug 15, 2023 at 03:52:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Aug 15, 2023 at 03:45:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Agreed, applied to perf-tools-next, sorry for the delay.
> > 
> > Had to add this to make 'perf test python' to work. Please run 'perf
> > test' before sending patches.
> 
> One more, please also do a 'make -C tools/perf build-test', with it I
> caught this:
> 
>          make_no_libunwind_O: cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH

+#include "util/env.h"

As now we need it for perf_env__arch(ui->machine->env)
 
>   CC      /tmp/tmp.yeEGyQq2HR/util/expr-flex.o
> util/unwind-libdw.c: In function ‘memory_read’:
> util/unwind-libdw.c:173:28: error: implicit declaration of function ‘perf_env__arch’ [-Werror=implicit-function-declaration]
>   173 |         const char *arch = perf_env__arch(ui->machine->env);
>       |                            ^~~~~~~~~~~~~~
> util/unwind-libdw.c:173:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> util/unwind-libdw.c: In function ‘unwind__get_entries’:
> util/unwind-libdw.c:258:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
>   258 |         const char *arch = perf_env__arch(ui_buf.machine->env);
>       |                            ^~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[6]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:98: /tmp/tmp.yeEGyQq2HR/util/unwind-libdw.o] Error 1
> make[6]: *** Waiting for unfinished jobs....
> make[5]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:150: util] Error 2
> make[4]: *** [Makefile.perf:662: /tmp/tmp.yeEGyQq2HR/perf-in.o] Error 2
> make[4]: *** Waiting for unfinished jobs....
>   CC      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events.o
>   LD      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events-in.o
> make[3]: *** [Makefile.perf:238: sub-make] Error 2
> make[2]: *** [Makefile:70: all] Error 2
> make[1]: *** [tests/make:337: make_no_libunwind_O] Error 1
> make: *** [Makefile:103: build-test] Error 2
> make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> 
> real	1m29.784s
> user	10m41.597s
> sys	2m55.948s
> ⬢[acme@toolbox perf-tools-next]$
> 
> I'm trying to fix
Leo Yan Aug. 16, 2023, 2:07 a.m. UTC | #6
On Tue, Aug 15, 2023 at 03:57:17PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Aug 15, 2023 at 03:52:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Aug 15, 2023 at 03:45:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Agreed, applied to perf-tools-next, sorry for the delay.
> > > 
> > > Had to add this to make 'perf test python' to work. Please run 'perf
> > > test' before sending patches.
> > 
> > One more, please also do a 'make -C tools/perf build-test', with it I
> > caught this:
> > 
> >          make_no_libunwind_O: cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> > cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> 
> +#include "util/env.h"
> 
> As now we need it for perf_env__arch(ui->machine->env)

Sorry for inconvenience.

I saw this patch series has been picked into the branch:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf-tools-next

If want me to follow up, let me know.  Thank you!

> >   CC      /tmp/tmp.yeEGyQq2HR/util/expr-flex.o
> > util/unwind-libdw.c: In function ‘memory_read’:
> > util/unwind-libdw.c:173:28: error: implicit declaration of function ‘perf_env__arch’ [-Werror=implicit-function-declaration]
> >   173 |         const char *arch = perf_env__arch(ui->machine->env);
> >       |                            ^~~~~~~~~~~~~~
> > util/unwind-libdw.c:173:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> > util/unwind-libdw.c: In function ‘unwind__get_entries’:
> > util/unwind-libdw.c:258:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> >   258 |         const char *arch = perf_env__arch(ui_buf.machine->env);
> >       |                            ^~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> > make[6]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:98: /tmp/tmp.yeEGyQq2HR/util/unwind-libdw.o] Error 1
> > make[6]: *** Waiting for unfinished jobs....
> > make[5]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:150: util] Error 2
> > make[4]: *** [Makefile.perf:662: /tmp/tmp.yeEGyQq2HR/perf-in.o] Error 2
> > make[4]: *** Waiting for unfinished jobs....
> >   CC      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events.o
> >   LD      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events-in.o
> > make[3]: *** [Makefile.perf:238: sub-make] Error 2
> > make[2]: *** [Makefile:70: all] Error 2
> > make[1]: *** [tests/make:337: make_no_libunwind_O] Error 1
> > make: *** [Makefile:103: build-test] Error 2
> > make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > 
> > real	1m29.784s
> > user	10m41.597s
> > sys	2m55.948s
> > ⬢[acme@toolbox perf-tools-next]$
> > 
> > I'm trying to fix
> 
> -- 
> 
> - Arnaldo
Arnaldo Carvalho de Melo Aug. 16, 2023, 11:46 a.m. UTC | #7
Em Wed, Aug 16, 2023 at 10:07:15AM +0800, Leo Yan escreveu:
> On Tue, Aug 15, 2023 at 03:57:17PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Aug 15, 2023 at 03:52:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Tue, Aug 15, 2023 at 03:45:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > Agreed, applied to perf-tools-next, sorry for the delay.
> > > > 
> > > > Had to add this to make 'perf test python' to work. Please run 'perf
> > > > test' before sending patches.
> > > 
> > > One more, please also do a 'make -C tools/perf build-test', with it I
> > > caught this:
> > > 
> > >          make_no_libunwind_O: cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> > > cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> > 
> > +#include "util/env.h"
> > 
> > As now we need it for perf_env__arch(ui->machine->env)
> 
> Sorry for inconvenience.
> 
> I saw this patch series has been picked into the branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf-tools-next
> 
> If want me to follow up, let me know.  Thank you!

Right, I'll fix this ones:

[perfbuilder@five ~]$ grep "unused variable" dm.log/*:*
dm.log/ubuntu:18.04-x-m68k:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
dm.log/ubuntu:18.04-x-riscv64:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
dm.log/ubuntu:18.04-x-sh4:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
dm.log/ubuntu:18.04-x-sparc64:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
[perfbuilder@five ~]$

And move that to perf-tools-next, we can go on from there.

The above is because we don't define CONFIG_PERF_REGS for these
architectures and thus that variable ends up not being used, so I'm
fixing up like below, in the cset where you made DWARF_MINIMAL_REGS
receive the arch parameter.

Also I haven't checked how gracefully we react when processing a
perf.data collected in one of those unsupported arches, can you please
check?

- Arnaldo

diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
index 790c1a26bbfe9b4b..de1673057e502de9 100644
--- a/tools/perf/util/perf_regs.h
+++ b/tools/perf/util/perf_regs.h
@@ -32,9 +32,6 @@ extern const struct sample_reg sample_reg_masks[];
 
 #include <perf_regs.h>
 
-#define DWARF_MINIMAL_REGS(arch)	\
-	((1ULL << perf_arch_reg_ip(arch)) | (1ULL << perf_arch_reg_sp(arch)))
-
 const char *perf_reg_name(int id, const char *arch);
 int perf_reg_value(u64 *valp, struct regs_dump *regs, int id);
 uint64_t perf_arch_reg_ip(const char *arch);
@@ -67,11 +64,19 @@ const char *__perf_reg_name_x86(int id);
 uint64_t __perf_reg_ip_x86(void);
 uint64_t __perf_reg_sp_x86(void);
 
+static inline uint64_t DWARF_MINIMAL_REGS(const char *arch)
+{
+	return (1ULL << perf_arch_reg_ip(arch)) | (1ULL << perf_arch_reg_sp(arch));
+}
+
 #else
 #define PERF_REGS_MASK	0
 #define PERF_REGS_MAX	0
 
-#define DWARF_MINIMAL_REGS(arch)	PERF_REGS_MASK
+static inline uint64_t DWARF_MINIMAL_REGS(const char *arch __maybe_unused)
+{
+	return PERF_REGS_MASK;
+}
 
 static inline const char *perf_reg_name(int id __maybe_unused, const char *arch __maybe_unused)
 {
 
> > >   CC      /tmp/tmp.yeEGyQq2HR/util/expr-flex.o
> > > util/unwind-libdw.c: In function ‘memory_read’:
> > > util/unwind-libdw.c:173:28: error: implicit declaration of function ‘perf_env__arch’ [-Werror=implicit-function-declaration]
> > >   173 |         const char *arch = perf_env__arch(ui->machine->env);
> > >       |                            ^~~~~~~~~~~~~~
> > > util/unwind-libdw.c:173:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> > > util/unwind-libdw.c: In function ‘unwind__get_entries’:
> > > util/unwind-libdw.c:258:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> > >   258 |         const char *arch = perf_env__arch(ui_buf.machine->env);
> > >       |                            ^~~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > > make[6]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:98: /tmp/tmp.yeEGyQq2HR/util/unwind-libdw.o] Error 1
> > > make[6]: *** Waiting for unfinished jobs....
> > > make[5]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:150: util] Error 2
> > > make[4]: *** [Makefile.perf:662: /tmp/tmp.yeEGyQq2HR/perf-in.o] Error 2
> > > make[4]: *** Waiting for unfinished jobs....
> > >   CC      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events.o
> > >   LD      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events-in.o
> > > make[3]: *** [Makefile.perf:238: sub-make] Error 2
> > > make[2]: *** [Makefile:70: all] Error 2
> > > make[1]: *** [tests/make:337: make_no_libunwind_O] Error 1
> > > make: *** [Makefile:103: build-test] Error 2
> > > make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > > 
> > > real	1m29.784s
> > > user	10m41.597s
> > > sys	2m55.948s
> > > ⬢[acme@toolbox perf-tools-next]$
> > > 
> > > I'm trying to fix
> > 
> > -- 
> > 
> > - Arnaldo
Arnaldo Carvalho de Melo Aug. 16, 2023, 11:48 a.m. UTC | #8
Em Wed, Aug 16, 2023 at 08:46:23AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Aug 16, 2023 at 10:07:15AM +0800, Leo Yan escreveu:
> > On Tue, Aug 15, 2023 at 03:57:17PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Aug 15, 2023 at 03:52:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Tue, Aug 15, 2023 at 03:45:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > > Agreed, applied to perf-tools-next, sorry for the delay.
> > > > > 
> > > > > Had to add this to make 'perf test python' to work. Please run 'perf
> > > > > test' before sending patches.
> > > > 
> > > > One more, please also do a 'make -C tools/perf build-test', with it I
> > > > caught this:
> > > > 
> > > >          make_no_libunwind_O: cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> > > > cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> > > 
> > > +#include "util/env.h"
> > > 
> > > As now we need it for perf_env__arch(ui->machine->env)
> > 
> > Sorry for inconvenience.
> > 
> > I saw this patch series has been picked into the branch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf-tools-next
> > 
> > If want me to follow up, let me know.  Thank you!
> 
> Right, I'll fix this ones:
> 
> [perfbuilder@five ~]$ grep "unused variable" dm.log/*:*
> dm.log/ubuntu:18.04-x-m68k:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
> dm.log/ubuntu:18.04-x-riscv64:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
> dm.log/ubuntu:18.04-x-sh4:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
> dm.log/ubuntu:18.04-x-sparc64:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
> [perfbuilder@five ~]$
> 
> And move that to perf-tools-next, we can go on from there.
> 
> The above is because we don't define CONFIG_PERF_REGS for these
> architectures and thus that variable ends up not being used, so I'm
> fixing up like below, in the cset where you made DWARF_MINIMAL_REGS
> receive the arch parameter.

I added this to the cset commit message:

Committer notes:

Make DWARF_MINIMAL_REGS() an inline function, so that we can use the
__maybe_unused attribute for the 'arch' parameter, as this will avoid a
build failure when that variable is unused in the callers. That happens
when building on unsupported architectures, the ones without
HAVE_PERF_REGS_SUPPORT defined.

 
> Also I haven't checked how gracefully we react when processing a
> perf.data collected in one of those unsupported arches, can you please
> check?
> 
> - Arnaldo
> 
> diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
> index 790c1a26bbfe9b4b..de1673057e502de9 100644
> --- a/tools/perf/util/perf_regs.h
> +++ b/tools/perf/util/perf_regs.h
> @@ -32,9 +32,6 @@ extern const struct sample_reg sample_reg_masks[];
>  
>  #include <perf_regs.h>
>  
> -#define DWARF_MINIMAL_REGS(arch)	\
> -	((1ULL << perf_arch_reg_ip(arch)) | (1ULL << perf_arch_reg_sp(arch)))
> -
>  const char *perf_reg_name(int id, const char *arch);
>  int perf_reg_value(u64 *valp, struct regs_dump *regs, int id);
>  uint64_t perf_arch_reg_ip(const char *arch);
> @@ -67,11 +64,19 @@ const char *__perf_reg_name_x86(int id);
>  uint64_t __perf_reg_ip_x86(void);
>  uint64_t __perf_reg_sp_x86(void);
>  
> +static inline uint64_t DWARF_MINIMAL_REGS(const char *arch)
> +{
> +	return (1ULL << perf_arch_reg_ip(arch)) | (1ULL << perf_arch_reg_sp(arch));
> +}
> +
>  #else
>  #define PERF_REGS_MASK	0
>  #define PERF_REGS_MAX	0
>  
> -#define DWARF_MINIMAL_REGS(arch)	PERF_REGS_MASK
> +static inline uint64_t DWARF_MINIMAL_REGS(const char *arch __maybe_unused)
> +{
> +	return PERF_REGS_MASK;
> +}
>  
>  static inline const char *perf_reg_name(int id __maybe_unused, const char *arch __maybe_unused)
>  {
>  
> > > >   CC      /tmp/tmp.yeEGyQq2HR/util/expr-flex.o
> > > > util/unwind-libdw.c: In function ‘memory_read’:
> > > > util/unwind-libdw.c:173:28: error: implicit declaration of function ‘perf_env__arch’ [-Werror=implicit-function-declaration]
> > > >   173 |         const char *arch = perf_env__arch(ui->machine->env);
> > > >       |                            ^~~~~~~~~~~~~~
> > > > util/unwind-libdw.c:173:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> > > > util/unwind-libdw.c: In function ‘unwind__get_entries’:
> > > > util/unwind-libdw.c:258:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> > > >   258 |         const char *arch = perf_env__arch(ui_buf.machine->env);
> > > >       |                            ^~~~~~~~~~~~~~
> > > > cc1: all warnings being treated as errors
> > > > make[6]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:98: /tmp/tmp.yeEGyQq2HR/util/unwind-libdw.o] Error 1
> > > > make[6]: *** Waiting for unfinished jobs....
> > > > make[5]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:150: util] Error 2
> > > > make[4]: *** [Makefile.perf:662: /tmp/tmp.yeEGyQq2HR/perf-in.o] Error 2
> > > > make[4]: *** Waiting for unfinished jobs....
> > > >   CC      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events.o
> > > >   LD      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events-in.o
> > > > make[3]: *** [Makefile.perf:238: sub-make] Error 2
> > > > make[2]: *** [Makefile:70: all] Error 2
> > > > make[1]: *** [tests/make:337: make_no_libunwind_O] Error 1
> > > > make: *** [Makefile:103: build-test] Error 2
> > > > make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > > > 
> > > > real	1m29.784s
> > > > user	10m41.597s
> > > > sys	2m55.948s
> > > > ⬢[acme@toolbox perf-tools-next]$
> > > > 
> > > > I'm trying to fix
> > > 
> > > -- 
> > > 
> > > - Arnaldo
> 
> -- 
> 
> - Arnaldo
Leo Yan Aug. 17, 2023, 9:12 a.m. UTC | #9
On Wed, Aug 16, 2023 at 08:46:23AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 16, 2023 at 10:07:15AM +0800, Leo Yan escreveu:
> > On Tue, Aug 15, 2023 at 03:57:17PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Aug 15, 2023 at 03:52:38PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Tue, Aug 15, 2023 at 03:45:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > > Agreed, applied to perf-tools-next, sorry for the delay.
> > > > > 
> > > > > Had to add this to make 'perf test python' to work. Please run 'perf
> > > > > test' before sending patches.
> > > > 
> > > > One more, please also do a 'make -C tools/perf build-test', with it I
> > > > caught this:
> > > > 
> > > >          make_no_libunwind_O: cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> > > > cd . && make NO_LIBUNWIND=1 FEATURES_DUMP=/var/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.yeEGyQq2HR DESTDIR=/tmp/tmp.ITgoO16jjH
> > > 
> > > +#include "util/env.h"
> > > 
> > > As now we need it for perf_env__arch(ui->machine->env)
> > 
> > Sorry for inconvenience.
> > 
> > I saw this patch series has been picked into the branch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf-tools-next
> > 
> > If want me to follow up, let me know.  Thank you!
> 
> Right, I'll fix this ones:
> 
> [perfbuilder@five ~]$ grep "unused variable" dm.log/*:*
> dm.log/ubuntu:18.04-x-m68k:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
> dm.log/ubuntu:18.04-x-riscv64:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
> dm.log/ubuntu:18.04-x-sh4:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
> dm.log/ubuntu:18.04-x-sparc64:util/evsel.c:848:14: error: unused variable 'arch' [-Werror=unused-variable]
> [perfbuilder@five ~]$
> 
> And move that to perf-tools-next, we can go on from there.
> 
> The above is because we don't define CONFIG_PERF_REGS for these
> architectures and thus that variable ends up not being used, so I'm
> fixing up like below, in the cset where you made DWARF_MINIMAL_REGS
> receive the arch parameter.

I reviewed your below amended change, it looks good to me.

And I tested the latest perf-tools-next branch for both native and cross
register parsing with perf Arm64 binaries.  Looks good to me.

Thanks a lot for fixing up!

> Also I haven't checked how gracefully we react when processing a
> perf.data collected in one of those unsupported arches, can you please
> check?

At the first glance, we can add checking arch support of register
parsing in the function callchain_param_setup(), which is invoked by
`perf report` and `perf script`, but we need to handle `perf top`
specifically.  Or we can consider to add checking in the function
__parse_callchain_report_opt().

Anyway, I will look into deails and work out patches.

Thanks,
Leo

> - Arnaldo
> 
> diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h
> index 790c1a26bbfe9b4b..de1673057e502de9 100644
> --- a/tools/perf/util/perf_regs.h
> +++ b/tools/perf/util/perf_regs.h
> @@ -32,9 +32,6 @@ extern const struct sample_reg sample_reg_masks[];
>  
>  #include <perf_regs.h>
>  
> -#define DWARF_MINIMAL_REGS(arch)	\
> -	((1ULL << perf_arch_reg_ip(arch)) | (1ULL << perf_arch_reg_sp(arch)))
> -
>  const char *perf_reg_name(int id, const char *arch);
>  int perf_reg_value(u64 *valp, struct regs_dump *regs, int id);
>  uint64_t perf_arch_reg_ip(const char *arch);
> @@ -67,11 +64,19 @@ const char *__perf_reg_name_x86(int id);
>  uint64_t __perf_reg_ip_x86(void);
>  uint64_t __perf_reg_sp_x86(void);
>  
> +static inline uint64_t DWARF_MINIMAL_REGS(const char *arch)
> +{
> +	return (1ULL << perf_arch_reg_ip(arch)) | (1ULL << perf_arch_reg_sp(arch));
> +}
> +
>  #else
>  #define PERF_REGS_MASK	0
>  #define PERF_REGS_MAX	0
>  
> -#define DWARF_MINIMAL_REGS(arch)	PERF_REGS_MASK
> +static inline uint64_t DWARF_MINIMAL_REGS(const char *arch __maybe_unused)
> +{
> +	return PERF_REGS_MASK;
> +}
>  
>  static inline const char *perf_reg_name(int id __maybe_unused, const char *arch __maybe_unused)
>  {
>  
> > > >   CC      /tmp/tmp.yeEGyQq2HR/util/expr-flex.o
> > > > util/unwind-libdw.c: In function ‘memory_read’:
> > > > util/unwind-libdw.c:173:28: error: implicit declaration of function ‘perf_env__arch’ [-Werror=implicit-function-declaration]
> > > >   173 |         const char *arch = perf_env__arch(ui->machine->env);
> > > >       |                            ^~~~~~~~~~~~~~
> > > > util/unwind-libdw.c:173:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> > > > util/unwind-libdw.c: In function ‘unwind__get_entries’:
> > > > util/unwind-libdw.c:258:28: error: initialization of ‘const char *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> > > >   258 |         const char *arch = perf_env__arch(ui_buf.machine->env);
> > > >       |                            ^~~~~~~~~~~~~~
> > > > cc1: all warnings being treated as errors
> > > > make[6]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:98: /tmp/tmp.yeEGyQq2HR/util/unwind-libdw.o] Error 1
> > > > make[6]: *** Waiting for unfinished jobs....
> > > > make[5]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:150: util] Error 2
> > > > make[4]: *** [Makefile.perf:662: /tmp/tmp.yeEGyQq2HR/perf-in.o] Error 2
> > > > make[4]: *** Waiting for unfinished jobs....
> > > >   CC      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events.o
> > > >   LD      /tmp/tmp.yeEGyQq2HR/pmu-events/pmu-events-in.o
> > > > make[3]: *** [Makefile.perf:238: sub-make] Error 2
> > > > make[2]: *** [Makefile:70: all] Error 2
> > > > make[1]: *** [tests/make:337: make_no_libunwind_O] Error 1
> > > > make: *** [Makefile:103: build-test] Error 2
> > > > make: Leaving directory '/var/home/acme/git/perf-tools-next/tools/perf'
> > > > 
> > > > real	1m29.784s
> > > > user	10m41.597s
> > > > sys	2m55.948s
> > > > ⬢[acme@toolbox perf-tools-next]$
> > > > 
> > > > I'm trying to fix
> > > 
> > > -- 
> > > 
> > > - Arnaldo
> 
> -- 
> 
> - Arnaldo
Leo Yan Aug. 17, 2023, 9:23 a.m. UTC | #10
On Wed, Aug 16, 2023 at 08:48:30AM -0300, Arnaldo Carvalho de Melo wrote:

[...]

> > And move that to perf-tools-next, we can go on from there.
> > 
> > The above is because we don't define CONFIG_PERF_REGS for these
> > architectures and thus that variable ends up not being used, so I'm
> > fixing up like below, in the cset where you made DWARF_MINIMAL_REGS
> > receive the arch parameter.
> 
> I added this to the cset commit message:
> 
> Committer notes:
> 
> Make DWARF_MINIMAL_REGS() an inline function, so that we can use the
> __maybe_unused attribute for the 'arch' parameter, as this will avoid a
> build failure when that variable is unused in the callers. That happens
> when building on unsupported architectures, the ones without
> HAVE_PERF_REGS_SUPPORT defined.

Looks good to me, thanks for fixing.

Leo