mbox series

[v2,0/3] Support register names of all architectures

Message ID 20211207180653.1147374-1-german.gomez@arm.com (mailing list archive)
Headers show
Series Support register names of all architectures | expand

Message

German Gomez Dec. 7, 2021, 6:06 p.m. UTC
The following changeset applies some corrections to the way system
registers are processed and presented when reading perf.data files using
the various perf tools.

The commit message from [3/3] shows how register names aren't correctly
presented when performing x-arch analysis of perf.data files (recording
in one arch, then reading the file from a different arch).

  - [PATCH 1/3] Fixes a potential out-of-bounds access when reading the
    values of the registers in the perf.data file.
  - [PATCH 2/3] Fixes an issue of ARM and ARM64 registers having the
    same enum name.
  - [PATCH 3/3] Refactors the function "perf_reg_name" declared in the
   "tools/perf/util/perf_regs.h" header, in order to support every arch.

Thanks,
German

--
Changes since v1

  - Added "Reported-by" tags.
  - Removed [PATCH 2/4] because it's not needed (suggested by Athira
    Rajeev).
  - Removed [PATCH 3/4] which created additional header files with the
    register names of every arch.
  - Introduced [PATCH 2/3] to deal with ARM and ARM64 registers having the
    same enum name across "/tools/perf/".
  - Reworked the refactor of "perf_reg_name" function (now implemented in
    perf_regs.c, rather than in the header file) in [PATCH 3/3].

German Gomez (3):
  perf tools: Prevent out-of-bounds access to registers
  perf tools: Rename perf_event_arm_regs for ARM64 registers
  perf tools: Support register names from all archs

 tools/perf/arch/arm/include/perf_regs.h       |  42 --
 tools/perf/arch/arm64/include/perf_regs.h     |  78 +-
 tools/perf/arch/csky/include/perf_regs.h      |  82 ---
 tools/perf/arch/mips/include/perf_regs.h      |  69 --
 tools/perf/arch/powerpc/include/perf_regs.h   |  66 --
 tools/perf/arch/riscv/include/perf_regs.h     |  74 --
 tools/perf/arch/s390/include/perf_regs.h      |  78 --
 tools/perf/arch/x86/include/perf_regs.h       |  82 ---
 tools/perf/builtin-script.c                   |  18 +-
 tools/perf/util/event.h                       |   5 +-
 tools/perf/util/libunwind/arm64.c             |   2 +
 tools/perf/util/perf_regs.c                   | 671 +++++++++++++++++-
 tools/perf/util/perf_regs.h                   |  10 +-
 .../scripting-engines/trace-event-python.c    |  10 +-
 tools/perf/util/session.c                     |  25 +-
 15 files changed, 709 insertions(+), 603 deletions(-)

Comments

Athira Rajeev Dec. 14, 2021, 8:50 a.m. UTC | #1
> On 07-Dec-2021, at 11:36 PM, German Gomez <german.gomez@arm.com> wrote:
> 
> The following changeset applies some corrections to the way system
> registers are processed and presented when reading perf.data files using
> the various perf tools.
> 
> The commit message from [3/3] shows how register names aren't correctly
> presented when performing x-arch analysis of perf.data files (recording
> in one arch, then reading the file from a different arch).
> 
>  - [PATCH 1/3] Fixes a potential out-of-bounds access when reading the
>    values of the registers in the perf.data file.
>  - [PATCH 2/3] Fixes an issue of ARM and ARM64 registers having the
>    same enum name.
>  - [PATCH 3/3] Refactors the function "perf_reg_name" declared in the
>   "tools/perf/util/perf_regs.h" header, in order to support every arch.
> 
> Thanks,
> German

Looks good to me. Tested this patchset in powerpc by capturing regs in powerpc and doing
perf report to read the data from x86.

Reviewed-and-Tested-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> 
> --
> Changes since v1
> 
>  - Added "Reported-by" tags.
>  - Removed [PATCH 2/4] because it's not needed (suggested by Athira
>    Rajeev).
>  - Removed [PATCH 3/4] which created additional header files with the
>    register names of every arch.
>  - Introduced [PATCH 2/3] to deal with ARM and ARM64 registers having the
>    same enum name across "/tools/perf/".
>  - Reworked the refactor of "perf_reg_name" function (now implemented in
>    perf_regs.c, rather than in the header file) in [PATCH 3/3].
> 
> German Gomez (3):
>  perf tools: Prevent out-of-bounds access to registers
>  perf tools: Rename perf_event_arm_regs for ARM64 registers
>  perf tools: Support register names from all archs
> 
> tools/perf/arch/arm/include/perf_regs.h       |  42 --
> tools/perf/arch/arm64/include/perf_regs.h     |  78 +-
> tools/perf/arch/csky/include/perf_regs.h      |  82 ---
> tools/perf/arch/mips/include/perf_regs.h      |  69 --
> tools/perf/arch/powerpc/include/perf_regs.h   |  66 --
> tools/perf/arch/riscv/include/perf_regs.h     |  74 --
> tools/perf/arch/s390/include/perf_regs.h      |  78 --
> tools/perf/arch/x86/include/perf_regs.h       |  82 ---
> tools/perf/builtin-script.c                   |  18 +-
> tools/perf/util/event.h                       |   5 +-
> tools/perf/util/libunwind/arm64.c             |   2 +
> tools/perf/util/perf_regs.c                   | 671 +++++++++++++++++-
> tools/perf/util/perf_regs.h                   |  10 +-
> .../scripting-engines/trace-event-python.c    |  10 +-
> tools/perf/util/session.c                     |  25 +-
> 15 files changed, 709 insertions(+), 603 deletions(-)
> 
> -- 
> 2.25.1
>
Arnaldo Carvalho de Melo Dec. 14, 2021, 1:26 p.m. UTC | #2
Em Tue, Dec 14, 2021 at 02:20:26PM +0530, Athira Rajeev escreveu:
> 
> 
> > On 07-Dec-2021, at 11:36 PM, German Gomez <german.gomez@arm.com> wrote:
> > 
> > The following changeset applies some corrections to the way system
> > registers are processed and presented when reading perf.data files using
> > the various perf tools.
> > 
> > The commit message from [3/3] shows how register names aren't correctly
> > presented when performing x-arch analysis of perf.data files (recording
> > in one arch, then reading the file from a different arch).
> > 
> >  - [PATCH 1/3] Fixes a potential out-of-bounds access when reading the
> >    values of the registers in the perf.data file.
> >  - [PATCH 2/3] Fixes an issue of ARM and ARM64 registers having the
> >    same enum name.
> >  - [PATCH 3/3] Refactors the function "perf_reg_name" declared in the
> >   "tools/perf/util/perf_regs.h" header, in order to support every arch.
> > 
> > Thanks,
> > German
> 
> Looks good to me. Tested this patchset in powerpc by capturing regs in powerpc and doing
> perf report to read the data from x86.
> 
> Reviewed-and-Tested-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>

Thanks, added to the commit log message.

- Arnaldo

> > --
> > Changes since v1
> > 
> >  - Added "Reported-by" tags.
> >  - Removed [PATCH 2/4] because it's not needed (suggested by Athira
> >    Rajeev).
> >  - Removed [PATCH 3/4] which created additional header files with the
> >    register names of every arch.
> >  - Introduced [PATCH 2/3] to deal with ARM and ARM64 registers having the
> >    same enum name across "/tools/perf/".
> >  - Reworked the refactor of "perf_reg_name" function (now implemented in
> >    perf_regs.c, rather than in the header file) in [PATCH 3/3].
> > 
> > German Gomez (3):
> >  perf tools: Prevent out-of-bounds access to registers
> >  perf tools: Rename perf_event_arm_regs for ARM64 registers
> >  perf tools: Support register names from all archs
> > 
> > tools/perf/arch/arm/include/perf_regs.h       |  42 --
> > tools/perf/arch/arm64/include/perf_regs.h     |  78 +-
> > tools/perf/arch/csky/include/perf_regs.h      |  82 ---
> > tools/perf/arch/mips/include/perf_regs.h      |  69 --
> > tools/perf/arch/powerpc/include/perf_regs.h   |  66 --
> > tools/perf/arch/riscv/include/perf_regs.h     |  74 --
> > tools/perf/arch/s390/include/perf_regs.h      |  78 --
> > tools/perf/arch/x86/include/perf_regs.h       |  82 ---
> > tools/perf/builtin-script.c                   |  18 +-
> > tools/perf/util/event.h                       |   5 +-
> > tools/perf/util/libunwind/arm64.c             |   2 +
> > tools/perf/util/perf_regs.c                   | 671 +++++++++++++++++-
> > tools/perf/util/perf_regs.h                   |  10 +-
> > .../scripting-engines/trace-event-python.c    |  10 +-
> > tools/perf/util/session.c                     |  25 +-
> > 15 files changed, 709 insertions(+), 603 deletions(-)
> > 
> > -- 
> > 2.25.1
> >