[PATCH 1/2] tools/perf: Add field to annotation options to determine disassembler used

[PATCH 1/2] tools/perf: Add field to annotation options to determine disassembler used

[Athira Rajeev]

When doing "perf annotate", perf tool provides option to
use specific disassembler like llvm/objdump/capstone. The
order picked is to use llvm first and if that fails fallback
to objdump ie to use PERF_DISASM_LLVM, PERF_DISASM_CAPSTONE
and PERF_DISASM_OBJDUMP

In powerpc, when using "data type" sort keys, first preferred
approach is to read the raw instruction from the DSO. In objdump
is specified in "--objdump" option, it picks the symbol disassemble
using objdump. Currently disasm_line__parse_powerpc() function
uses length of the "line" to determine if objdump is used.
But there are few cases, where if objdump doesn't recognise the
instruction, the disassembled string will be empty.

Example:

     134cdc:	c4 05 82 41 	beq     1352a0 <getcwd+0x6e0>
     134ce0:	ac 00 8e 40 	bne     cr3,134d8c <getcwd+0x1cc>
     134ce4:	0f 00 10 04 	pld     r9,1028308
====>134ce8:	d4 b0 20 e5
     134cec:	16 00 40 39 	li      r10,22
     134cf0:	48 01 21 ea 	ld      r17,328(r1)

So depending on length of line will give bad results.

Add a new filed to annotation options structure,
"struct annotation_options" to save the disassembler used.
Use this info to determine if disassembly is done while
parsing the disasm line.

Reported-by: Tejas Manhas <Tejas.Manhas1@ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
 tools/perf/util/annotate.h |  1 +
 tools/perf/util/disasm.c   | 22 +++++++++++++---------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 98db1b88daf4..30a344afd91a 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -58,6 +58,7 @@ struct annotation_options {
 	     full_addr;
 	u8   offset_level;
 	u8   disassemblers[MAX_DISASSEMBLERS];
+	u8   disassembler_used;
 	int  min_pcnt;
 	int  max_lines;
 	int  context;
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 50c5c206b70e..a53e8c4e5bca 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -48,7 +48,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 int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_args *args);
 static char *expand_tabs(char *line, char **storage, size_t *storage_len);
 
 static __attribute__((constructor)) void symbol__init_regexpr(void)
@@ -968,24 +968,24 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
 #define	PPC_OP(op)	(((op) >> 26) & 0x3F)
 #define	RAW_BYTES	11
 
-static int disasm_line__parse_powerpc(struct disasm_line *dl)
+static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_args *args)
 {
 	char *line = dl->al.line;
 	const char **namep = &dl->ins.name;
 	char **rawp = &dl->ops.raw;
 	char *tmp_raw_insn, *name_raw_insn = skip_spaces(line);
 	char *name = skip_spaces(name_raw_insn + RAW_BYTES);
-	int objdump = 0;
+	int disasm = 0;
 
-	if (strlen(line) > RAW_BYTES)
-		objdump = 1;
+	if (args->options->disassembler_used)
+		disasm = 1;
 
 	if (name_raw_insn[0] == '\0')
 		return -1;
 
-	if (objdump) {
+	if (disasm)
 		disasm_line__parse(name, namep, rawp);
-	} else
+	else
 		*namep = "";
 
 	tmp_raw_insn = strndup(name_raw_insn, 11);
@@ -995,7 +995,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl)
 	remove_spaces(tmp_raw_insn);
 
 	sscanf(tmp_raw_insn, "%x", &dl->raw.raw_insn);
-	if (objdump)
+	if (disasm)
 		dl->raw.raw_insn = be32_to_cpu(dl->raw.raw_insn);
 
 	return 0;
@@ -1054,7 +1054,7 @@ struct disasm_line *disasm_line__new(struct annotate_args *args)
 
 	if (args->offset != -1) {
 		if (arch__is(args->arch, "powerpc")) {
-			if (disasm_line__parse_powerpc(dl) < 0)
+			if (disasm_line__parse_powerpc(dl, args) < 0)
 				goto out_free_line;
 		} else if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
 			goto out_free_line;
@@ -2289,16 +2289,20 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
 
 		switch (dis) {
 		case PERF_DISASM_LLVM:
+			args->options->disassembler_used = PERF_DISASM_LLVM;
 			err = symbol__disassemble_llvm(symfs_filename, sym, args);
 			break;
 		case PERF_DISASM_CAPSTONE:
+			args->options->disassembler_used = PERF_DISASM_CAPSTONE;
 			err = symbol__disassemble_capstone(symfs_filename, sym, args);
 			break;
 		case PERF_DISASM_OBJDUMP:
+			args->options->disassembler_used = PERF_DISASM_OBJDUMP;
 			err = symbol__disassemble_objdump(symfs_filename, sym, args);
 			break;
 		case PERF_DISASM_UNKNOWN: /* End of disassemblers. */
 		default:
+			args->options->disassembler_used = PERF_DISASM_UNKNOWN;
 			goto out_remove_tmp;
 		}
 		if (err == 0)
-- 
2.43.5

[PATCH 2/2] tools/perf/powerpc: Use return code from disasm_line__parse

[Athira Rajeev]

In disasm_line__parse_powerpc() , return code from function
disasm_line__parse() is ignored. This will result in bad results
if the disasm_line__parse fails to disasm the line. Use
the return code to fix this.

Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
 tools/perf/util/disasm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index a53e8c4e5bca..8f0eb56c6fc6 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -976,6 +976,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar
 	char *tmp_raw_insn, *name_raw_insn = skip_spaces(line);
 	char *name = skip_spaces(name_raw_insn + RAW_BYTES);
 	int disasm = 0;
+	int ret = 0;
 
 	if (args->options->disassembler_used)
 		disasm = 1;
@@ -984,7 +985,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar
 		return -1;
 
 	if (disasm)
-		disasm_line__parse(name, namep, rawp);
+		ret = disasm_line__parse(name, namep, rawp);
 	else
 		*namep = "";
 
@@ -998,7 +999,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar
 	if (disasm)
 		dl->raw.raw_insn = be32_to_cpu(dl->raw.raw_insn);
 
-	return 0;
+	return ret;
 }
 
 static void annotation_line__init(struct annotation_line *al,
-- 
2.43.5

Re: [PATCH 2/2] tools/perf/powerpc: Use return code from disasm_line__parse

[Venkat Rao Bagalkote]

I was able to reproduce the issue without this patch.

Applied this patch on next-20250227 kernel and tested the patch and 
issue is fixed.

Please add below tag.

Tested-By: Venkat Rao Bagalkote <venkat88@linux.ibm.com>

On 04/03/25 9:11 pm, Athira Rajeev wrote:
> In disasm_line__parse_powerpc() , return code from function
> disasm_line__parse() is ignored. This will result in bad results
> if the disasm_line__parse fails to disasm the line. Use
> the return code to fix this.
>
> Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
> ---
>   tools/perf/util/disasm.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index a53e8c4e5bca..8f0eb56c6fc6 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -976,6 +976,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar
>   	char *tmp_raw_insn, *name_raw_insn = skip_spaces(line);
>   	char *name = skip_spaces(name_raw_insn + RAW_BYTES);
>   	int disasm = 0;
> +	int ret = 0;
>   
>   	if (args->options->disassembler_used)
>   		disasm = 1;
> @@ -984,7 +985,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar
>   		return -1;
>   
>   	if (disasm)
> -		disasm_line__parse(name, namep, rawp);
> +		ret = disasm_line__parse(name, namep, rawp);
>   	else
>   		*namep = "";
>   
> @@ -998,7 +999,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_ar
>   	if (disasm)
>   		dl->raw.raw_insn = be32_to_cpu(dl->raw.raw_insn);
>   
> -	return 0;
> +	return ret;
>   }
>   
>   static void annotation_line__init(struct annotation_line *al,

Re: [PATCH 1/2] tools/perf: Add field to annotation options to determine disassembler used

[Venkat Rao Bagalkote]

Applied this patch on next-20250227, and issue is fixed.

./perf -v

perf version 6.14.rc4.g2a432e11ebbb

./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err
./perf annotate --stdio 1> out 2>err


Please add below tag.

Tested-By: Venkat Rao Bagalkote <venkat88@linux.ibm.com>

Regards,

Venkat.

On 04/03/25 9:11 pm, Athira Rajeev wrote:
> When doing "perf annotate", perf tool provides option to
> use specific disassembler like llvm/objdump/capstone. The
> order picked is to use llvm first and if that fails fallback
> to objdump ie to use PERF_DISASM_LLVM, PERF_DISASM_CAPSTONE
> and PERF_DISASM_OBJDUMP
>
> In powerpc, when using "data type" sort keys, first preferred
> approach is to read the raw instruction from the DSO. In objdump
> is specified in "--objdump" option, it picks the symbol disassemble
> using objdump. Currently disasm_line__parse_powerpc() function
> uses length of the "line" to determine if objdump is used.
> But there are few cases, where if objdump doesn't recognise the
> instruction, the disassembled string will be empty.
>
> Example:
>
>       134cdc:	c4 05 82 41 	beq     1352a0 <getcwd+0x6e0>
>       134ce0:	ac 00 8e 40 	bne     cr3,134d8c <getcwd+0x1cc>
>       134ce4:	0f 00 10 04 	pld     r9,1028308
> ====>134ce8:	d4 b0 20 e5
>       134cec:	16 00 40 39 	li      r10,22
>       134cf0:	48 01 21 ea 	ld      r17,328(r1)
>
> So depending on length of line will give bad results.
>
> Add a new filed to annotation options structure,
> "struct annotation_options" to save the disassembler used.
> Use this info to determine if disassembly is done while
> parsing the disasm line.
>
> Reported-by: Tejas Manhas <Tejas.Manhas1@ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
> ---
>   tools/perf/util/annotate.h |  1 +
>   tools/perf/util/disasm.c   | 22 +++++++++++++---------
>   2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 98db1b88daf4..30a344afd91a 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -58,6 +58,7 @@ struct annotation_options {
>   	     full_addr;
>   	u8   offset_level;
>   	u8   disassemblers[MAX_DISASSEMBLERS];
> +	u8   disassembler_used;
>   	int  min_pcnt;
>   	int  max_lines;
>   	int  context;
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 50c5c206b70e..a53e8c4e5bca 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -48,7 +48,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 int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_args *args);
>   static char *expand_tabs(char *line, char **storage, size_t *storage_len);
>   
>   static __attribute__((constructor)) void symbol__init_regexpr(void)
> @@ -968,24 +968,24 @@ static int disasm_line__parse(char *line, const char **namep, char **rawp)
>   #define	PPC_OP(op)	(((op) >> 26) & 0x3F)
>   #define	RAW_BYTES	11
>   
> -static int disasm_line__parse_powerpc(struct disasm_line *dl)
> +static int disasm_line__parse_powerpc(struct disasm_line *dl, struct annotate_args *args)
>   {
>   	char *line = dl->al.line;
>   	const char **namep = &dl->ins.name;
>   	char **rawp = &dl->ops.raw;
>   	char *tmp_raw_insn, *name_raw_insn = skip_spaces(line);
>   	char *name = skip_spaces(name_raw_insn + RAW_BYTES);
> -	int objdump = 0;
> +	int disasm = 0;
>   
> -	if (strlen(line) > RAW_BYTES)
> -		objdump = 1;
> +	if (args->options->disassembler_used)
> +		disasm = 1;
>   
>   	if (name_raw_insn[0] == '\0')
>   		return -1;
>   
> -	if (objdump) {
> +	if (disasm)
>   		disasm_line__parse(name, namep, rawp);
> -	} else
> +	else
>   		*namep = "";
>   
>   	tmp_raw_insn = strndup(name_raw_insn, 11);
> @@ -995,7 +995,7 @@ static int disasm_line__parse_powerpc(struct disasm_line *dl)
>   	remove_spaces(tmp_raw_insn);
>   
>   	sscanf(tmp_raw_insn, "%x", &dl->raw.raw_insn);
> -	if (objdump)
> +	if (disasm)
>   		dl->raw.raw_insn = be32_to_cpu(dl->raw.raw_insn);
>   
>   	return 0;
> @@ -1054,7 +1054,7 @@ struct disasm_line *disasm_line__new(struct annotate_args *args)
>   
>   	if (args->offset != -1) {
>   		if (arch__is(args->arch, "powerpc")) {
> -			if (disasm_line__parse_powerpc(dl) < 0)
> +			if (disasm_line__parse_powerpc(dl, args) < 0)
>   				goto out_free_line;
>   		} else if (disasm_line__parse(dl->al.line, &dl->ins.name, &dl->ops.raw) < 0)
>   			goto out_free_line;
> @@ -2289,16 +2289,20 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
>   
>   		switch (dis) {
>   		case PERF_DISASM_LLVM:
> +			args->options->disassembler_used = PERF_DISASM_LLVM;
>   			err = symbol__disassemble_llvm(symfs_filename, sym, args);
>   			break;
>   		case PERF_DISASM_CAPSTONE:
> +			args->options->disassembler_used = PERF_DISASM_CAPSTONE;
>   			err = symbol__disassemble_capstone(symfs_filename, sym, args);
>   			break;
>   		case PERF_DISASM_OBJDUMP:
> +			args->options->disassembler_used = PERF_DISASM_OBJDUMP;
>   			err = symbol__disassemble_objdump(symfs_filename, sym, args);
>   			break;
>   		case PERF_DISASM_UNKNOWN: /* End of disassemblers. */
>   		default:
> +			args->options->disassembler_used = PERF_DISASM_UNKNOWN;
>   			goto out_remove_tmp;
>   		}
>   		if (err == 0)

Re: [PATCH 1/2] tools/perf: Add field to annotation options to determine disassembler used

[Namhyung Kim]

On Tue, 04 Mar 2025 21:11:13 +0530, Athira Rajeev wrote:
> When doing "perf annotate", perf tool provides option to
> use specific disassembler like llvm/objdump/capstone. The
> order picked is to use llvm first and if that fails fallback
> to objdump ie to use PERF_DISASM_LLVM, PERF_DISASM_CAPSTONE
> and PERF_DISASM_OBJDUMP
> 
> In powerpc, when using "data type" sort keys, first preferred
> approach is to read the raw instruction from the DSO. In objdump
> is specified in "--objdump" option, it picks the symbol disassemble
> using objdump. Currently disasm_line__parse_powerpc() function
> uses length of the "line" to determine if objdump is used.
> But there are few cases, where if objdump doesn't recognise the
> instruction, the disassembled string will be empty.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung