Message-Id: <A7397A0E-A76D-4B3E-AA06-29C21BEF3E64@linux.vnet.ibm.com>
Date: Wed, 3 Jul 2024 18:41:12 +0530
From: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Ian Rogers <irogers@...gle.com>,
        Segher Boessenkool <segher@...nel.crashing.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org, akanksha@...ux.ibm.com,
        maddy@...ux.ibm.com, kjain@...ux.ibm.com, disgoel@...ux.vnet.ibm.com
Subject: Re: [PATCH V5 04/17] tools/perf: Add disasm_line__parse to parse raw
 instruction for powerpc



> On 3 Jul 2024, at 6:46 AM, Namhyung Kim <namhyung@...nel.org> wrote:
> 
> On Mon, Jul 01, 2024 at 10:04:17AM +0530, Athira Rajeev wrote:
>> Currently, the perf tool infrastructure disasm_line__parse function to
>> parse disassembled line.
>> 
>> Example snippet from objdump:
>> objdump  --start-address=<address> --stop-address=<address>  -d --no-show-raw-insn -C <vmlinux>
>> 
>> c0000000010224b4: lwz     r10,0(r9)
>> 
>> This line "lwz r10,0(r9)" is parsed to extract instruction name,
>> registers names and offset. In powerpc, the approach for data type
>> profiling uses raw instruction instead of result from objdump to identify
>> the instruction category and extract the source/target registers.
>> 
>> Example: 38 01 81 e8     ld      r4,312(r1)
>> 
>> Here "38 01 81 e8" is the raw instruction representation. Add function
>> "disasm_line__parse_powerpc" to handle parsing of raw instruction.
>> Also update "struct disasm_line" to save the binary code/
>> With the change, function captures:
>> 
>> line -> "38 01 81 e8     ld      r4,312(r1)"
>> raw instruction "38 01 81 e8"
>> 
>> Raw instruction is used later to extract the reg/offset fields. Macros
>> are added to extract opcode and register fields. "struct disasm_line"
>> is updated to carry union of "bytes" and "raw_insn" of 32 bit to carry raw
>> code (raw). Function "disasm_line__parse_powerpc fills the raw
>> instruction hex value and can use macros to get opcode. There is no
>> changes in existing code paths, which parses the disassembled code.
>> The size of raw instruction depends on architecture. In case of powerpc,
>> the parsing the disasm line needs to handle cases for reading binary code
>> directly from DSO as well as parsing the objdump result. Hence adding
>> the logic into separate function instead of updating "disasm_line__parse".
>> The architecture using the instruction name and present approach is
>> not altered. Since this approach targets powerpc, the macro
>> implementation is added for powerpc as of now.
>> 
>> Since the disasm_line__parse is used in other cases (perf annotate) and
>> not only data tye profiling, the powerpc callback includes changes to
>> work with binary code as well as mneumonic representation. Also in case
>> if the DSO read fails and libcapstone is not supported, the approach
>> fallback to use objdump as option. Hence as option, patch has changes to
>> ensure objdump option also works well.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
>> ---
>> tools/include/linux/string.h                  |  2 +
>> tools/lib/string.c                            | 13 ++++
>> .../perf/arch/powerpc/annotate/instructions.c |  1 +
>> tools/perf/arch/powerpc/util/dwarf-regs.c     |  9 +++
>> tools/perf/util/annotate.h                    |  5 +-
>> tools/perf/util/disasm.c                      | 59 ++++++++++++++++++-
>> 6 files changed, 87 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/include/linux/string.h b/tools/include/linux/string.h
>> index db5c99318c79..0acb1fc14e19 100644
>> --- a/tools/include/linux/string.h
>> +++ b/tools/include/linux/string.h
>> @@ -46,5 +46,7 @@ extern char * __must_check skip_spaces(const char *);
>> 
>> extern char *strim(char *);
>> 
>> +extern void remove_spaces(char *s);
>> +
>> extern void *memchr_inv(const void *start, int c, size_t bytes);
>> #endif /* _TOOLS_LINUX_STRING_H_ */
>> diff --git a/tools/lib/string.c b/tools/lib/string.c
>> index 8b6892f959ab..3126d2cff716 100644
>> --- a/tools/lib/string.c
>> +++ b/tools/lib/string.c
>> @@ -153,6 +153,19 @@ char *strim(char *s)
>> return skip_spaces(s);
>> }
>> 
>> +/*
>> + * remove_spaces - Removes whitespaces from @s
>> + */
>> +void remove_spaces(char *s)
>> +{
>> + char *d = s;
>> +
>> + do {
>> + while (*d == ' ')
>> + ++d;
>> + } while ((*s++ = *d++));
>> +}
>> +
>> /**
>>  * strreplace - Replace all occurrences of character in string.
>>  * @s: The string to operate on.
>> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c b/tools/perf/arch/powerpc/annotate/instructions.c
>> index a3f423c27cae..d57fd023ef9c 100644
>> --- a/tools/perf/arch/powerpc/annotate/instructions.c
>> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
>> @@ -55,6 +55,7 @@ static int powerpc__annotate_init(struct arch *arch, char *cpuid __maybe_unused)
>> arch->initialized = true;
>> arch->associate_instruction_ops = powerpc__associate_instruction_ops;
>> arch->objdump.comment_char      = '#';
>> + annotate_opts.show_asm_raw = true;
>> }
>> 
>> return 0;
>> diff --git a/tools/perf/arch/powerpc/util/dwarf-regs.c b/tools/perf/arch/powerpc/util/dwarf-regs.c
>> index 0c4f4caf53ac..430623ca5612 100644
>> --- a/tools/perf/arch/powerpc/util/dwarf-regs.c
>> +++ b/tools/perf/arch/powerpc/util/dwarf-regs.c
>> @@ -98,3 +98,12 @@ int regs_query_register_offset(const char *name)
>> return roff->ptregs_offset;
>> return -EINVAL;
>> }
>> +
>> +#define PPC_OP(op) (((op) >> 26) & 0x3F)
>> +#define PPC_RA(a) (((a) >> 16) & 0x1f)
>> +#define PPC_RT(t) (((t) >> 21) & 0x1f)
>> +#define PPC_RB(b) (((b) >> 11) & 0x1f)
>> +#define PPC_D(D) ((D) & 0xfffe)
>> +#define PPC_DS(DS) ((DS) & 0xfffc)
>> +#define OP_LD 58
>> +#define OP_STD 62
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index d5c821c22f79..9ba772f46270 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -113,7 +113,10 @@ struct annotation_line {
>> struct disasm_line {
>> struct ins  ins;
>> struct ins_operands  ops;
>> -
>> + union {
>> + u8 bytes[4];
>> + u32 raw_insn;
>> + } raw;
>> /* This needs to be at the end. */
>> struct annotation_line  al;
>> };
>> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
>> index d2723ba024bf..ddb861a0b043 100644
>> --- a/tools/perf/util/disasm.c
>> +++ b/tools/perf/util/disasm.c
>> @@ -44,6 +44,7 @@ static int call__scnprintf(struct ins *ins, char *bf, size_t size,
>> 
>> static void ins__sort(struct arch *arch);
>> static int disasm_line__parse(char *line, const char **namep, char **rawp);
>> +static int disasm_line__parse_powerpc(struct disasm_line *dl);
>> 
>> static __attribute__((constructor)) void symbol__init_regexpr(void)
>> {
>> @@ -845,6 +846,59 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
>> return -1;
>> }
>> 
>> +/*
>> + * Parses the result captured from symbol__disassemble_*
>> + * Example, line read from DSO file in powerpc:
>> + * line:    38 01 81 e8
>> + * opcode: fetched from arch specific get_opcode_insn
>> + * rawp_insn: e8810138
>> + *
>> + * rawp_insn is used later to extract the reg/offset fields
>> + */
>> +#define PPC_OP(op) (((op) >> 26) & 0x3F)
>> +
>> +static int disasm_line__parse_powerpc(struct disasm_line *dl)
>> +{
>> + char *line = dl->al.line;
>> + const char **namep = &dl->ins.name;
>> + char **rawp = &dl->ops.raw;
>> + char tmp, *tmp_raw_insn, *name_raw_insn = skip_spaces(line);
>> + char *name = skip_spaces(name_raw_insn + 11);
> 
> What does 11 mean?  Can you add a macro for it instead?
> I guess it's the width of the raw codes and spaces.
> 
Yes, I will add macro
> 
>> + int objdump = 0;
>> +
>> + if (strlen(line) > 11)
>> + objdump = 1;
>> +
>> + if (name_raw_insn[0] == '\0')
>> + return -1;
>> +
>> + if (objdump) {
>> + *rawp = name + 1;
>> + while ((*rawp)[0] != '\0' && !isspace((*rawp)[0]))
>> + ++*rawp;
>> + tmp = (*rawp)[0];
>> + (*rawp)[0] = '\0';
>> +
>> + *namep = strdup(name);
>> + if (*namep == NULL)
>> + return -1;
>> +
>> + (*rawp)[0] = tmp;
>> + *rawp = strim(*rawp);
> 
> Can we just call disasm_line__parse()?
Yes, 
> 
> 
>> + } else
>> + *namep = "";
>> +
>> + tmp_raw_insn = strdup(name_raw_insn);
>> + tmp_raw_insn[11] = '\0';
> 
> You can use strndup().
Ok

Will send next version with these changes

Thanks
Athira
> 
> Thanks,
> Namhyung
> 
> 
>> + remove_spaces(tmp_raw_insn);
>> +
>> + sscanf(tmp_raw_insn, "%x", &dl->raw.raw_insn);
>> + if (objdump)
>> + dl->raw.raw_insn = be32_to_cpu(dl->raw.raw_insn);
>> +
>> + return 0;
>> +}
>> +
>> static void annotation_line__init(struct annotation_line *al,
>>   struct annotate_args *args,
>>   int nr)
>> @@ -898,7 +952,10 @@ struct disasm_line *disasm_line__new(struct annotate_args *args)
>> goto out_delete;
>> 
>> if (args->offset != -1) {
>> - if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
>> + if (arch__is(args->arch, "powerpc")) {
>> + if (disasm_line__parse_powerpc(dl) < 0)
>> + goto out_free_line;
>> + } else if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
>> goto out_free_line;
>> 
>> disasm_line__init_ins(dl, args->arch, &args->ms);
>> -- 
>> 2.43.0

Powered by blists - more mailing lists