Hello,
On Wed, Jun 26, 2024 at 09:38:28AM +0530, Athira Rajeev wrote:
>
>
> > On 26 Jun 2024, at 12:15 AM, Namhyung Kim <namhy...@kernel.org> wrote:
> >
> > On Tue, Jun 25, 2024 at 06:12:51PM +0530, Athira Rajeev wrote:
> >>
> >>
> >>> On 25 Jun 2024, at 11:09 AM, Namhyung Kim <namhy...@kernel.org> wrote:
> >>>
> >>> On Fri, Jun 14, 2024 at 10:56:20PM +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 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 <atraj...@linux.vnet.ibm.com>
> >>>> ---
[SNIP]
> >>>> +/*
> >>>> + * 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);
> >>>> + 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);
> >>>> + } else
> >>>> + *namep = "";
> >
> > Then can you handle this logic under if (annotate_opts.show_raw_insn)
> > in disasm_line__parse() instead of adding a new function?
> >
> > Thanks,
> > Namhyung
>
> Hi Namhyung,
>
> We discussed to have a per-arch disasm_line_parse() here:
> https://lore.kernel.org/all/cam9d7ci1lda7mot2qdr2qk+dtnlu6zbkmronbdozajuqlqf...@mail.gmail.com/#t
>
> So I added it as a new function : disasm_line__parse_powerpc
> Since it is not used by other archs, we can go with having new function ?
Ok, I thought it'd be quite different from disasm_line__parse() but it
seems that it's mostly similar except for the raw insn. So I think it's
better to add the logic to the generic disasm_line__parse(). Sorry for
the inconvenience.
Thanks,
Namhyung
> >>>> +
> >>>> + tmp_raw_insn = strdup(name_raw_insn);
> >>>> + tmp_raw_insn[11] = '\0';
> >>>> + remove_spaces(tmp_raw_insn);
> >>>> +
> >>>> + dl->raw.raw_insn = strtol(tmp_raw_insn, NULL, 16);
> >>>> + if (objdump)
> >>>> + dl->raw.raw_insn = be32_to_cpu(strtol(tmp_raw_insn, NULL, 16));
> >>>
> >>> Hmm.. can you use a sscanf() instead?
> >>>
> >>> sscanf(line, "%x %x %x %x", &dl->raw.bytes[0], &dl->raw.bytes[1], ...)
> >>>
> >>> Thanks,
> >>> Namhyung
> >>>
> >> Sure will address in V5
> >>
> >> Thanks
> >> Athira
> >>>> +
> >>>> + return 0;
> >>>> +}
