Re: [V4 05/16] tools/perf: Add disasm_line__parse to parse raw instruction for powerpc

Athira Rajeev
2024-06-27T09:28:47Z

> On 27 Jun 2024, at 2:47 AM, Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Wed, Jun 26, 2024 at 09:38:28AM +0530, Athira Rajeev wrote:
>>
>>
>>> On 26 Jun 2024, at 12:15 AM, Namhyung Kim <namhyung@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 <namhyung@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= --stop-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 <atrajeev@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+DTNLU6ZBkmROnbdozAjuQLQfNog@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

Sure

Thanks
Athira
>
>>>>>> +
>>>>>> + 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;
>>>>>> +}