Hi Ian,
Ian Munsie wrote:
> From: Ian Munsie <imun...@au.ibm.com>
>
> The perf userspace tool included some architecture specific code to map
> registers from the DWARF register number into the names used by the regs
> and stack access API.
>
> This patch moves the architecture specific code out into a seperate
> arch/x86 directory along with the infrastructure required to use it.
Nice! :)
I have just some comments on it.
>
> Signed-off-by: Ian Munsie <imun...@au.ibm.com>
> ---
> tools/perf/Makefile | 18 ++++++-
> tools/perf/arch/x86/Makefile | 1 +
> tools/perf/arch/x86/include/arch_dwarf-regs.h | 6 ++
> tools/perf/arch/x86/util/dwarf-regs.c | 75
> +++++++++++++++++++++++++
> tools/perf/util/include/dwarf-regs.h | 8 +++
> tools/perf/util/probe-finder.c | 55 ++----------------
> 6 files changed, 113 insertions(+), 50 deletions(-)
> create mode 100644 tools/perf/arch/x86/Makefile
> create mode 100644 tools/perf/arch/x86/include/arch_dwarf-regs.h
> create mode 100644 tools/perf/arch/x86/util/dwarf-regs.c
> create mode 100644 tools/perf/util/include/dwarf-regs.h
>
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index f578b05..07a6ee2 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -172,6 +172,20 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo
> not')
> uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
> uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
>
> +ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/i386/ -e s/sun4u/sparc64/ \
> + -e s/arm.*/arm/ -e s/sa110/arm/ \
> + -e s/s390x/s390/ -e s/parisc64/parisc/ \
> + -e s/ppc.*/powerpc/ -e s/mips.*/mips/ \
> + -e s/sh[234].*/sh/ )
> +
> +# Additional ARCH settings for x86
> +ifeq ($(ARCH),i386)
> + ARCH := x86
> +endif
> +ifeq ($(ARCH),x86_64)
> + ARCH := x86
> +endif
> +
> # CFLAGS and LDFLAGS are for the users to override from the command line.
>
> #
> @@ -284,7 +298,7 @@ endif
> # Those must not be GNU-specific; they are shared with perl/ which may
> # be built by a different compiler. (Note that this is an artifact now
> # but it still might be nice to keep that distinction.)
> -BASIC_CFLAGS = -Iutil/include
> +BASIC_CFLAGS = -Iutil/include -Iarch/$(ARCH)/include
> BASIC_LDFLAGS =
>
> # Guard against environment variables
> @@ -366,6 +380,7 @@ LIB_H += util/include/asm/byteorder.h
> LIB_H += util/include/asm/swab.h
> LIB_H += util/include/asm/system.h
> LIB_H += util/include/asm/uaccess.h
> +LIB_H += util/include/dwarf-regs.h
> LIB_H += perf.h
> LIB_H += util/cache.h
> LIB_H += util/callchain.h
> @@ -484,6 +499,7 @@ PERFLIBS = $(LIB_FILE)
>
> -include config.mak.autogen
> -include config.mak
> +-include arch/$(ARCH)/Makefile
>
> ifeq ($(uname_S),Darwin)
> ifndef NO_FINK
Could you add a check whether the get_arch_regstr() is defined
(or dwarf-regs.h is exist) in Makefile?
If it is not defined, we'd better drop dwarf support(so set NO_DWARF),
because it means we haven't ported perf probe on that architecture yet. :)
> diff --git a/tools/perf/arch/x86/Makefile b/tools/perf/arch/x86/Makefile
> new file mode 100644
> index 0000000..1191403
> --- /dev/null
> +++ b/tools/perf/arch/x86/Makefile
> @@ -0,0 +1 @@
> +LIB_OBJS += $(OUTPUT)arch/$(ARCH)/util/dwarf-regs.o
> diff --git a/tools/perf/arch/x86/include/arch_dwarf-regs.h
> b/tools/perf/arch/x86/include/arch_dwarf-regs.h
> new file mode 100644
> index 0000000..9e8da6a
> --- /dev/null
> +++ b/tools/perf/arch/x86/include/arch_dwarf-regs.h
> @@ -0,0 +1,6 @@
> +#ifndef _PREF_ARCH_PPC_DWARF_REGS_H
> +#define _PREF_ARCH_PPC_DWARF_REGS_H
_PREF_ARCH_X86_DWARF_REGS_H ?
> +
> +#define get_arch_regstr(n) get_arch_regstr(n)
If we checked above dwarf support, we don't need this odd macro.
> +
> +#endif
> diff --git a/tools/perf/arch/x86/util/dwarf-regs.c
> b/tools/perf/arch/x86/util/dwarf-regs.c
> new file mode 100644
> index 0000000..a794d30
> --- /dev/null
> +++ b/tools/perf/arch/x86/util/dwarf-regs.c
> @@ -0,0 +1,75 @@
> +/*
> + * dwarf-regs.c : Mapping of DWARF debug register numbers into register
> names.
> + * Extracted from probe-finder.c
> + *
> + * Written by Masami Hiramatsu <mhira...@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + */
> +
> +#include <libio.h>
> +#include <dwarf-regs.h>
> +
> +/*
> + * Generic dwarf analysis helpers
> + */
> +
> +#define X86_32_MAX_REGS 8
> +const char *x86_32_regs_table[X86_32_MAX_REGS] = {
> + "%ax",
> + "%cx",
> + "%dx",
> + "%bx",
> + "$stack", /* Stack address instead of %sp */
> + "%bp",
> + "%si",
> + "%di",
> +};
> +
> +#define X86_64_MAX_REGS 16
> +const char *x86_64_regs_table[X86_64_MAX_REGS] = {
> + "%ax",
> + "%dx",
> + "%cx",
> + "%bx",
> + "%si",
> + "%di",
> + "%bp",
> + "%sp",
> + "%r8",
> + "%r9",
> + "%r10",
> + "%r11",
> + "%r12",
> + "%r13",
> + "%r14",
> + "%r15",
> +};
> +
> +/* TODO: switching by dwarf address size */
> +#ifdef __x86_64__
> +#define ARCH_MAX_REGS X86_64_MAX_REGS
> +#define arch_regs_table x86_64_regs_table
> +#else
> +#define ARCH_MAX_REGS X86_32_MAX_REGS
> +#define arch_regs_table x86_32_regs_table
> +#endif
> +
> +/* Return architecture dependent register string (for kprobe-tracer) */
> +const char *get_arch_regstr(unsigned int n)
> +{
> + return (n <= ARCH_MAX_REGS) ? arch_regs_table[n] : NULL;
> +}
> diff --git a/tools/perf/util/include/dwarf-regs.h
> b/tools/perf/util/include/dwarf-regs.h
> new file mode 100644
> index 0000000..2a1cf6f
> --- /dev/null
> +++ b/tools/perf/util/include/dwarf-regs.h
> @@ -0,0 +1,8 @@
> +#ifndef _PERF_DWARF_REGS_H_
> +#define _PERF_DWARF_REGS_H_
> +
> +#include <arch_dwarf-regs.h>
> +
> +const char *get_arch_regstr(unsigned int n);
> +
> +#endif
> diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> index a851377..27020fe 100644
> --- a/tools/perf/util/probe-finder.c
> +++ b/tools/perf/util/probe-finder.c
> @@ -31,6 +31,7 @@
> #include <string.h>
> #include <stdarg.h>
> #include <ctype.h>
> +#include <dwarf-regs.h>
>
> #include "string.h"
> #include "event.h"
> @@ -38,57 +39,13 @@
> #include "util.h"
> #include "probe-finder.h"
>
> -
> -/*
> - * Generic dwarf analysis helpers
> - */
> -
> -#define X86_32_MAX_REGS 8
> -const char *x86_32_regs_table[X86_32_MAX_REGS] = {
> - "%ax",
> - "%cx",
> - "%dx",
> - "%bx",
> - "$stack", /* Stack address instead of %sp */
> - "%bp",
> - "%si",
> - "%di",
> -};
> -
> -#define X86_64_MAX_REGS 16
> -const char *x86_64_regs_table[X86_64_MAX_REGS] = {
> - "%ax",
> - "%dx",
> - "%cx",
> - "%bx",
> - "%si",
> - "%di",
> - "%bp",
> - "%sp",
> - "%r8",
> - "%r9",
> - "%r10",
> - "%r11",
> - "%r12",
> - "%r13",
> - "%r14",
> - "%r15",
> -};
> -
> -/* TODO: switching by dwarf address size */
> -#ifdef __x86_64__
> -#define ARCH_MAX_REGS X86_64_MAX_REGS
> -#define arch_regs_table x86_64_regs_table
> -#else
> -#define ARCH_MAX_REGS X86_32_MAX_REGS
> -#define arch_regs_table x86_32_regs_table
> -#endif
> -
> /* Return architecture dependent register string (for kprobe-tracer) */
> -static const char *get_arch_regstr(unsigned int n)
> +#ifndef get_arch_regstr
> +const char *get_arch_regstr(unsigned int n __attribute__((unused)))
> {
> - return (n <= ARCH_MAX_REGS) ? arch_regs_table[n] : NULL;
> + return NULL;
> }
> +#endif
Again, if we add a check in Makefile, we can remove this completely.
>
> /*
> * Compare the tail of two strings.
> @@ -397,7 +354,7 @@ static void convert_location(Dwarf_Op *op, struct
> probe_finder *pf)
>
> regs = get_arch_regstr(regn);
> if (!regs)
> - die("%u exceeds max register number.", regn);
> + die("Mapping for DWARF register number %u missing on this
> architecture.", regn);
>
> tvar->value = xstrdup(regs);
> if (ref) {
Thank you,
--
Masami Hiramatsu
e-mail: mhira...@redhat.com
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
