On Wed, 11 Oct 2023 20:50:32 -0700
Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> The get_dwarf_regnum() returns a DWARF register number from a register
> name string according to the psABI. Also add two pseudo encodings of
> DWARF_REG_PC which is a register that are used by PC-relative addressing
> and DWARF_REG_FB which is a frame base register. They need to be
> handled in a special way.
>
I have some comments below.
> Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
> tools/perf/arch/x86/util/dwarf-regs.c | 38 +++++++++++++++++++++++++++
> tools/perf/util/dwarf-regs.c | 33 +++++++++++++++++++++++
> tools/perf/util/include/dwarf-regs.h | 11 ++++++++
> 3 files changed, 82 insertions(+)
>
> diff --git a/tools/perf/arch/x86/util/dwarf-regs.c b/tools/perf/arch/x86/util/dwarf-regs.c
> index 530934805710..79835b897cae 100644
> --- a/tools/perf/arch/x86/util/dwarf-regs.c
> +++ b/tools/perf/arch/x86/util/dwarf-regs.c
> @@ -113,3 +113,41 @@ int regs_query_register_offset(const char *name)
> return roff->offset;
> return -EINVAL;
> }
> +
> +struct dwarf_regs_idx {
> + const char *name;
> + int idx;
> +};
> +
> +static const struct dwarf_regs_idx x86_regidx_table[] = {
> + { "rax", 0 }, { "eax", 0 }, { "ax", 0 }, { "al", 0 },
> + { "rdx", 1 }, { "edx", 1 }, { "dx", 1 }, { "dl", 1 },
> + { "rcx", 2 }, { "ecx", 2 }, { "cx", 2 }, { "cl", 2 },
> + { "rbx", 3 }, { "edx", 3 }, { "bx", 3 }, { "bl", 3 },
> + { "rsi", 4 }, { "esi", 4 }, { "si", 4 }, { "sil", 4 },
> + { "rdi", 5 }, { "edi", 5 }, { "di", 5 }, { "dil", 5 },
> + { "rbp", 6 }, { "ebp", 6 }, { "bp", 6 }, { "bpl", 6 },
> + { "rsp", 7 }, { "esp", 7 }, { "sp", 7 }, { "spl", 7 },
> + { "r8", 8 }, { "r8d", 8 }, { "r8w", 8 }, { "r8b", 8 },
> + { "r9", 9 }, { "r9d", 9 }, { "r9w", 9 }, { "r9b", 9 },
> + { "r10", 10 }, { "r10d", 10 }, { "r10w", 10 }, { "r10b", 10 },
> + { "r11", 11 }, { "r11d", 11 }, { "r11w", 11 }, { "r11b", 11 },
> + { "r12", 12 }, { "r12d", 12 }, { "r12w", 12 }, { "r12b", 12 },
> + { "r13", 13 }, { "r13d", 13 }, { "r13w", 13 }, { "r13b", 13 },
> + { "r14", 14 }, { "r14d", 14 }, { "r14w", 14 }, { "r14b", 14 },
> + { "r15", 15 }, { "r15d", 15 }, { "r15w", 15 }, { "r15b", 15 },
> + { "rip", DWARF_REG_PC },
> +};
> +
> +int get_arch_regnum(const char *name)
> +{
> + unsigned int i;
> +
> + if (*name != '%')
> + return -1;
Isn't it better to return an error code? e.g. -EINVAL.
> +
> + for (i = 0; i < ARRAY_SIZE(x86_regidx_table); i++)
> + if (!strcmp(x86_regidx_table[i].name, name + 1))
> + return x86_regidx_table[i].idx;
> + return -1;
And -ENOENT.
> +}
> diff --git a/tools/perf/util/dwarf-regs.c b/tools/perf/util/dwarf-regs.c
> index 69cfaa5953bf..28d786c7df55 100644
> --- a/tools/perf/util/dwarf-regs.c
> +++ b/tools/perf/util/dwarf-regs.c
> @@ -5,6 +5,8 @@
> * Written by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> */
>
> +#include <stdlib.h>
> +#include <string.h>
> #include <debug.h>
> #include <dwarf-regs.h>
> #include <elf.h>
> @@ -68,3 +70,34 @@ const char *get_dwarf_regstr(unsigned int n, unsigned int machine)
> }
> return NULL;
> }
> +
> +__weak int get_arch_regnum(const char *name __maybe_unused)
> +{
> + return -1;
And -EOPNOTSUPP.
> +}
> +
> +/* Return DWARF register number from architecture register name */
> +int get_dwarf_regnum(const char *name, unsigned int machine)
> +{
> + char *regname = strdup(name);
> + int reg = -1;
> + char *p;
> +
> + if (regname == NULL)
> + return -1;
Here, -EINVAL.
> +
> + /* For convenience, remove trailing characters */
> + p = strpbrk(regname, " ,)");
> + if (p)
> + *p = '\0';
> +
> + switch (machine) {
> + case EM_NONE: /* Generic arch - use host arch */
> + reg = get_arch_regnum(regname);
> + break;
> + default:
> + pr_err("ELF MACHINE %x is not supported.\n", machine);
> + }
> + free(regname);
> + return reg;
> +}
> diff --git a/tools/perf/util/include/dwarf-regs.h b/tools/perf/util/include/dwarf-regs.h
> index 7d99a084e82d..b515f694f55e 100644
> --- a/tools/perf/util/include/dwarf-regs.h
> +++ b/tools/perf/util/include/dwarf-regs.h
> @@ -2,6 +2,9 @@
> #ifndef _PERF_DWARF_REGS_H_
> #define _PERF_DWARF_REGS_H_
>
> +#define DWARF_REG_PC 0xd3af9c /* random number */
> +#define DWARF_REG_FB 0xd3affb /* random number */
> +
Is this for other patch in the series?
> #ifdef HAVE_DWARF_SUPPORT
> const char *get_arch_regstr(unsigned int n);
> /*
> @@ -10,6 +13,14 @@ const char *get_arch_regstr(unsigned int n);
> * machine: ELF machine signature (EM_*)
> */
> const char *get_dwarf_regstr(unsigned int n, unsigned int machine);
> +
> +int get_arch_regnum(const char *name);
> +/*
> + * get_dwarf_regnum - Returns DWARF regnum from register name
> + * name: architecture register name
> + * machine: ELF machine signature (EM_*)
> + */
> +int get_dwarf_regnum(const char *name, unsigned int machine);
> #endif
>
> #ifdef HAVE_ARCH_REGS_QUERY_REGISTER_OFFSET
> --
> 2.42.0.655.g421f12c284-goog
>
Thank you,
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
|