[PATCH V3 05/14] tools/perf: Add disasm_line__parse to parse raw instruction for powerpc

Christophe Leroy christophe.leroy at csgroup.eu
Sat Jun 8 18:58:16 AEST 2024



Le 06/06/2024 à 08:33, Namhyung Kim a écrit :
> Hello,
> 
> On Sat, Jun 01, 2024 at 11:39:32AM +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 ins" and "struct ins_operands" to save "opcode" and
>> binary code. With the change, function captures:
>>
>> line -> "38 01 81 e8     ld      r4,312(r1)"
>> opcode and 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 ins_operands"
>> and "struct ins" is updated to carry opcode and raw instruction binary
>> code (raw_insn). Function "disasm_line__parse_powerpc fills the raw
>> instruction hex value and opcode in newly added fields. 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 at linux.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/disasm.c                      | 63 ++++++++++++++++++-
>>   tools/perf/util/disasm.h                      |  7 +++
>>   6 files changed, 94 insertions(+), 1 deletion(-)
>>
>> 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/disasm.c b/tools/perf/util/disasm.c
>> index 3cd187f08193..61f0f1656f82 100644
>> --- a/tools/perf/util/disasm.c
>> +++ b/tools/perf/util/disasm.c
>> @@ -45,6 +45,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)
>>   {
>> @@ -844,6 +845,63 @@ 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_opcode, *name_opcode = skip_spaces(line);
>> +	char *name = skip_spaces(name_opcode + 11);
>> +	int objdump = 0;
>> +
>> +	if (strlen(line) > 11)
>> +		objdump = 1;
>> +
>> +	if (name_opcode[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 = "";
>> +
>> +	tmp_opcode = strdup(name_opcode);
>> +	tmp_opcode[11] = '\0';
>> +	remove_spaces(tmp_opcode);
>> +
>> +	dl->ins.opcode = PPC_OP(strtol(tmp_opcode, NULL, 16));
>> +	if (objdump)
>> +		dl->ins.opcode = PPC_OP(be32_to_cpu(strtol(tmp_opcode, NULL, 16)));
>> +	dl->ops.opcode = dl->ins.opcode;
>> +
>> +	dl->ops.raw_insn = strtol(tmp_opcode, NULL, 16);
>> +	if (objdump)
>> +		dl->ops.raw_insn = be32_to_cpu(strtol(tmp_opcode, NULL, 16));
>> +	return 0;
>> +}
>> +
>>   static void annotation_line__init(struct annotation_line *al,
>>   				  struct annotate_args *args,
>>   				  int nr)
>> @@ -897,7 +955,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);
>> diff --git a/tools/perf/util/disasm.h b/tools/perf/util/disasm.h
>> index 718177fa4775..a391e1bb81f7 100644
>> --- a/tools/perf/util/disasm.h
>> +++ b/tools/perf/util/disasm.h
>> @@ -43,14 +43,19 @@ struct arch {
>>   
>>   struct ins {
>>   	const char     *name;
>> +	int	opcode;
> 
> I don't think this is the right place as 'ins' can be shared for
> different opcodes.  IIUC it's like a class and disasm_line should
> have a pointer instead of a copy of the arch instructions.  So I'd
> like to keep a single instance if they behave in the same way.  But
> this is a separate change.
> 
> I guess we can move it to struct disasm_line and use helper macros when
> we need to access the opcode.  This will be helpful for other arches.
> 
>    struct disasm_line {
>        struct ins *ins;
>        struct ins_operands ops;
>        union {
>            u8 bytes[4];
>            u32 opcode;
>        } raw;
>        struct annotation_line al;
>    };
>    
>    #define PPC_OP(dl)  (((dl)->raw.bytes[0] >> 2) & 0x3F)

We already have a definition for PPC_OP(), see arch/powerpc/xmon/ppc.h:

/* A macro to extract the major opcode from an instruction.  */
#define PPC_OP(i) (((i) >> 26) & 0x3f)

By the way why do you want to split off instructions in bytes ? On 
powerpc an instruction is one (sometimes two) u32, nothing else, and if 
you start breaking that into bytes you will likely unnecessarily 
increase complexity when a param has bits on different bytes, and maybe 
also with the byte order depending whether you are running on a little 
or big endian PPC.
See for instance arch_decode_instruction() in 
tools/objtool/arch/powerpc/decode.c

By the way, why are we spreading different decoding functions in 
different tools ? Wouldn't it make sense to try and share decoding 
functions between objtool and perf for instance ?

Christophe



> 
> Thanks,
> Namhyung
> 
>>
>>   	struct ins_ops *ops;
>>   };
>>   
>>   struct ins_operands {
>>   	char	*raw;
>> +	int	raw_insn;
>> +	int	opcode;
>>   	struct {
>>   		char	*raw;
>>   		char	*name;
>> +		int	opcode;
>> +		int	raw_insn;
>>   		struct symbol *sym;
>>   		u64	addr;
>>   		s64	offset;
>> @@ -62,6 +67,8 @@ struct ins_operands {
>>   		struct {
>>   			char	*raw;
>>   			char	*name;
>> +			int	opcode;
>> +			int	raw_insn;
>>   			u64	addr;
>>   			bool	multi_regs;
>>   		} source;
>> -- 
>> 2.43.0
>>


More information about the Linuxppc-dev mailing list