[PATCH 0/4] Branch stack improvements

[PATCH 0/4] Branch stack improvements

[James Clark]

I came across a few generic non-Arm issues when reviewing Anshuman's new
support for branch record buffers on Arm [1].

The first one is a fix to an error message which is misleading if the
feature is unavailable and the remaining ones make the branch type field
visible in perf report and perf script so that it can be debugged or used
by other tools.

Applies to perf/core (56dce86819)

Thanks
James

[1] https://lore.kernel.org/lkml/1642998653-21377-1-git-send-email-anshuman.khandual@arm.com/

James Clark (4):
  perf: Add error message for unsupported branch stack cases
  perf: Print branch stack entry type in --dump-raw-trace
  perf: Refactor perf script branch stack printing
  perf script: Output branch sample type

 tools/perf/builtin-script.c | 28 +++++++++++++---------------
 tools/perf/util/evsel.c     |  4 ++++
 tools/perf/util/session.c   |  5 +++--
 3 files changed, 20 insertions(+), 17 deletions(-)

-- 
2.28.0

[PATCH 1/4] perf: Add error message for unsupported branch stack cases

[James Clark]

EOPNOTSUPP is a possible return value when branch stacks are requested
but they aren't enabled in the kernel or hardware. It's also returned if
they aren't supported on the specific event type. The currently printed
error message about sampling/overflow-interrupts is not correct in this
case.

Add a check for branch stacks before sample_period is checked because
sample_period is also set (to the default value) when using branch
stacks.

Before this change (when branch stacks aren't supported):

  perf record -j any
  Error:
  cycles: PMU Hardware doesn't support sampling/overflow-interrupts. Try 'perf stat'

After this change:

  perf record -j any
  Error:
  cycles: PMU Hardware or event type doesn't support branch stack sampling.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/evsel.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 22d3267ce294..4e10a4ec11c7 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2909,6 +2909,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
 	 "No such device - did you specify an out-of-range profile CPU?");
 		break;
 	case EOPNOTSUPP:
+		if (evsel->core.attr.sample_type & PERF_SAMPLE_BRANCH_STACK)
+			return scnprintf(msg, size,
+	"%s: PMU Hardware or event type doesn't support branch stack sampling.",
+					 evsel__name(evsel));
 		if (evsel->core.attr.aux_output)
 			return scnprintf(msg, size,
 	"%s: PMU Hardware doesn't support 'aux_output' feature",
-- 
2.28.0

[PATCH 2/4] perf: Print branch stack entry type in --dump-raw-trace

[James Clark]

This can help with debugging issues. It only prints when -j save_type
is used otherwise an empty string is printed.

Before the change:

  101603801707130 0xa70 [0x630]: PERF_RECORD_SAMPLE(IP, 0x2): 1108/1108: 0xffff9c1df24c period: 10694 addr: 0
  ... branch stack: nr:64
  .....  0: 0000ffff9c26029c -> 0000ffff9c26f340 0 cycles  P   0
  .....  1: 0000ffff9c2601bc -> 0000ffff9c26f340 0 cycles  P   0

After the change:

  101603801707130 0xa70 [0x630]: PERF_RECORD_SAMPLE(IP, 0x2): 1108/1108: 0xffff9c1df24c period: 10694 addr: 0
  ... branch stack: nr:64
  .....  0: 0000ffff9c26029c -> 0000ffff9c26f340 0 cycles  P   0 CALL
  .....  1: 0000ffff9c2601bc -> 0000ffff9c26f340 0 cycles  P   0 IND_CALL

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/util/session.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index f54282d5c648..3b8dfe603e50 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1159,14 +1159,15 @@ static void branch_stack__printf(struct perf_sample *sample, bool callstack)
 		struct branch_entry *e = &entries[i];
 
 		if (!callstack) {
-			printf("..... %2"PRIu64": %016" PRIx64 " -> %016" PRIx64 " %hu cycles %s%s%s%s %x\n",
+			printf("..... %2"PRIu64": %016" PRIx64 " -> %016" PRIx64 " %hu cycles %s%s%s%s %x %s\n",
 				i, e->from, e->to,
 				(unsigned short)e->flags.cycles,
 				e->flags.mispred ? "M" : " ",
 				e->flags.predicted ? "P" : " ",
 				e->flags.abort ? "A" : " ",
 				e->flags.in_tx ? "T" : " ",
-				(unsigned)e->flags.reserved);
+				(unsigned)e->flags.reserved,
+				e->flags.type ? branch_type_name(e->flags.type) : "");
 		} else {
 			printf("..... %2"PRIu64": %016" PRIx64 "\n",
 				i, i > 0 ? e->from : e->to);
-- 
2.28.0

[PATCH 3/4] perf: Refactor perf script branch stack printing

[James Clark]

Remove duplicate code so that future changes to flags are always made to
all 3 printing variations.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/builtin-script.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 9e032343f1c6..fac2e9470926 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -857,6 +857,15 @@ mispred_str(struct branch_entry *br)
 	return br->flags.predicted ? 'P' : 'M';
 }
 
+static int print_bstack_flags(FILE *fp, struct branch_entry *br)
+{
+	return fprintf(fp, "/%c/%c/%c/%d ",
+		       mispred_str(br),
+		       br->flags.in_tx ? 'X' : '-',
+		       br->flags.abort ? 'A' : '-',
+		       br->flags.cycles);
+}
+
 static int perf_sample__fprintf_brstack(struct perf_sample *sample,
 					struct thread *thread,
 					struct perf_event_attr *attr, FILE *fp)
@@ -895,11 +904,7 @@ static int perf_sample__fprintf_brstack(struct perf_sample *sample,
 			printed += fprintf(fp, ")");
 		}
 
-		printed += fprintf(fp, "/%c/%c/%c/%d ",
-			mispred_str(entries + i),
-			entries[i].flags.in_tx ? 'X' : '-',
-			entries[i].flags.abort ? 'A' : '-',
-			entries[i].flags.cycles);
+		printed += print_bstack_flags(fp, entries + i);
 	}
 
 	return printed;
@@ -941,11 +946,7 @@ static int perf_sample__fprintf_brstacksym(struct perf_sample *sample,
 			printed += map__fprintf_dsoname(alt.map, fp);
 			printed += fprintf(fp, ")");
 		}
-		printed += fprintf(fp, "/%c/%c/%c/%d ",
-			mispred_str(entries + i),
-			entries[i].flags.in_tx ? 'X' : '-',
-			entries[i].flags.abort ? 'A' : '-',
-			entries[i].flags.cycles);
+		printed += print_bstack_flags(fp, entries + i);
 	}
 
 	return printed;
@@ -991,11 +992,7 @@ static int perf_sample__fprintf_brstackoff(struct perf_sample *sample,
 			printed += map__fprintf_dsoname(alt.map, fp);
 			printed += fprintf(fp, ")");
 		}
-		printed += fprintf(fp, "/%c/%c/%c/%d ",
-			mispred_str(entries + i),
-			entries[i].flags.in_tx ? 'X' : '-',
-			entries[i].flags.abort ? 'A' : '-',
-			entries[i].flags.cycles);
+		printed += print_bstack_flags(fp, entries + i);
 	}
 
 	return printed;
-- 
2.28.0

[PATCH 4/4] perf script: Output branch sample type

[James Clark]

The type info is saved when using '-j save_type'. Output this in perf
script so it can be accessed by other tools or for debugging.

It's appended to the end of the list of fields so any existing tools
that split on / and access fields via an index are not affected. Also
output '-' instead of 'N/A' when the branch type isn't saved because /
is used as a field separator.

Entries before this change look like this:

  0xaaaadb350838/0xaaaadb3507a4/P/-/-/0

And afterwards like this:

  0xaaaadb350838/0xaaaadb3507a4/P/-/-/0/CALL

or this if no type info is saved:

  0x7fb57586df6b/0x7fb5758731f0/P/-/-/143/-

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/builtin-script.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index fac2e9470926..5e4a262a6825 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -859,11 +859,12 @@ mispred_str(struct branch_entry *br)
 
 static int print_bstack_flags(FILE *fp, struct branch_entry *br)
 {
-	return fprintf(fp, "/%c/%c/%c/%d ",
+	return fprintf(fp, "/%c/%c/%c/%d/%s ",
 		       mispred_str(br),
 		       br->flags.in_tx ? 'X' : '-',
 		       br->flags.abort ? 'A' : '-',
-		       br->flags.cycles);
+		       br->flags.cycles,
+		       br->flags.type ? branch_type_name(br->flags.type) : "-");
 }
 
 static int perf_sample__fprintf_brstack(struct perf_sample *sample,
-- 
2.28.0

Re: [PATCH 0/4] Branch stack improvements

[Arnaldo Carvalho de Melo]

Em Mon, Mar 07, 2022 at 05:19:13PM +0000, James Clark escreveu:
> I came across a few generic non-Arm issues when reviewing Anshuman's new
> support for branch record buffers on Arm [1].
> 
> The first one is a fix to an error message which is misleading if the
> feature is unavailable and the remaining ones make the branch type field
> visible in perf report and perf script so that it can be debugged or used
> by other tools.
> 
> Applies to perf/core (56dce86819)

Thanks, applied to tmp.perf/core, locally, if someone spots some
problem, there is some time till I run tests and move it to perf/core.

- Arnaldo
 
> Thanks
> James
> 
> [1] https://lore.kernel.org/lkml/1642998653-21377-1-git-send-email-anshuman.khandual@arm.com/
> 
> James Clark (4):
>   perf: Add error message for unsupported branch stack cases
>   perf: Print branch stack entry type in --dump-raw-trace
>   perf: Refactor perf script branch stack printing
>   perf script: Output branch sample type
> 
>  tools/perf/builtin-script.c | 28 +++++++++++++---------------
>  tools/perf/util/evsel.c     |  4 ++++
>  tools/perf/util/session.c   |  5 +++--
>  3 files changed, 20 insertions(+), 17 deletions(-)
> 
> -- 
> 2.28.0

-- 

- Arnaldo

Re: [PATCH 1/4] perf: Add error message for unsupported branch stack cases

[Anshuman Khandual]

Hi James,

On 3/7/22 22:49, James Clark wrote:
> EOPNOTSUPP is a possible return value when branch stacks are requested
> but they aren't enabled in the kernel or hardware. It's also returned if
> they aren't supported on the specific event type. The currently printed

specific event type ? Events carrying certain branch sampling/filter flags
which could not be supported in the kernel on a given platform ?

> error message about sampling/overflow-interrupts is not correct in this
> case.
> 
> Add a check for branch stacks before sample_period is checked because
> sample_period is also set (to the default value) when using branch
> stacks.

Makes sense.

> 
> Before this change (when branch stacks aren't supported):
> 
>   perf record -j any
>   Error:
>   cycles: PMU Hardware doesn't support sampling/overflow-interrupts. Try 'perf stat'
> 
> After this change:
> 
>   perf record -j any
>   Error:
>   cycles: PMU Hardware or event type doesn't support branch stack sampling.

Indeed better in explaining what went wrong.

> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/evsel.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 22d3267ce294..4e10a4ec11c7 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -2909,6 +2909,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target,
>  	 "No such device - did you specify an out-of-range profile CPU?");
>  		break;
>  	case EOPNOTSUPP:
> +		if (evsel->core.attr.sample_type & PERF_SAMPLE_BRANCH_STACK)
> +			return scnprintf(msg, size,
> +	"%s: PMU Hardware or event type doesn't support branch stack sampling.",
> +					 evsel__name(evsel));

As this is being added right at the beginning for returned EOPNOTSUPP error,
previous fall through behaviour for all other cases will be preserved.

>  		if (evsel->core.attr.aux_output)
>  			return scnprintf(msg, size,
>  	"%s: PMU Hardware doesn't support 'aux_output' feature",

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH 2/4] perf: Print branch stack entry type in --dump-raw-trace

[Anshuman Khandual]

On 3/7/22 22:49, James Clark wrote:
> This can help with debugging issues. It only prints when -j save_type
> is used otherwise an empty string is printed.

Specifying events with PERF_SAMPLE_BRANCH_CALL_STACK flag explicitly might
be better along with '-j save_type'.

> 
> Before the change:
> 
>   101603801707130 0xa70 [0x630]: PERF_RECORD_SAMPLE(IP, 0x2): 1108/1108: 0xffff9c1df24c period: 10694 addr: 0
>   ... branch stack: nr:64
>   .....  0: 0000ffff9c26029c -> 0000ffff9c26f340 0 cycles  P   0
>   .....  1: 0000ffff9c2601bc -> 0000ffff9c26f340 0 cycles  P   0
> 
> After the change:
> 
>   101603801707130 0xa70 [0x630]: PERF_RECORD_SAMPLE(IP, 0x2): 1108/1108: 0xffff9c1df24c period: 10694 addr: 0
>   ... branch stack: nr:64
>   .....  0: 0000ffff9c26029c -> 0000ffff9c26f340 0 cycles  P   0 CALL
>   .....  1: 0000ffff9c2601bc -> 0000ffff9c26f340 0 cycles  P   0 IND_CALL
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/util/session.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index f54282d5c648..3b8dfe603e50 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1159,14 +1159,15 @@ static void branch_stack__printf(struct perf_sample *sample, bool callstack)
>  		struct branch_entry *e = &entries[i];
>  
>  		if (!callstack) {
> -			printf("..... %2"PRIu64": %016" PRIx64 " -> %016" PRIx64 " %hu cycles %s%s%s%s %x\n",
> +			printf("..... %2"PRIu64": %016" PRIx64 " -> %016" PRIx64 " %hu cycles %s%s%s%s %x %s\n",
>  				i, e->from, e->to,
>  				(unsigned short)e->flags.cycles,
>  				e->flags.mispred ? "M" : " ",
>  				e->flags.predicted ? "P" : " ",
>  				e->flags.abort ? "A" : " ",
>  				e->flags.in_tx ? "T" : " ",
> -				(unsigned)e->flags.reserved);
> +				(unsigned)e->flags.reserved,
> +				e->flags.type ? branch_type_name(e->flags.type) : "");
>  		} else {
>  			printf("..... %2"PRIu64": %016" PRIx64 "\n",
>  				i, i > 0 ? e->from : e->to);

LGTM but I am wondering whether this might affect existing tools ?

Re: [PATCH 3/4] perf: Refactor perf script branch stack printing

[Anshuman Khandual]

On 3/7/22 22:49, James Clark wrote:
> Remove duplicate code so that future changes to flags are always made to
> all 3 printing variations.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/builtin-script.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 9e032343f1c6..fac2e9470926 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -857,6 +857,15 @@ mispred_str(struct branch_entry *br)
>  	return br->flags.predicted ? 'P' : 'M';
>  }
>  
> +static int print_bstack_flags(FILE *fp, struct branch_entry *br)
> +{
> +	return fprintf(fp, "/%c/%c/%c/%d ",
> +		       mispred_str(br),
> +		       br->flags.in_tx ? 'X' : '-',
> +		       br->flags.abort ? 'A' : '-',
> +		       br->flags.cycles);
> +}
> +
>  static int perf_sample__fprintf_brstack(struct perf_sample *sample,
>  					struct thread *thread,
>  					struct perf_event_attr *attr, FILE *fp)
> @@ -895,11 +904,7 @@ static int perf_sample__fprintf_brstack(struct perf_sample *sample,
>  			printed += fprintf(fp, ")");
>  		}
>  
> -		printed += fprintf(fp, "/%c/%c/%c/%d ",
> -			mispred_str(entries + i),
> -			entries[i].flags.in_tx ? 'X' : '-',
> -			entries[i].flags.abort ? 'A' : '-',
> -			entries[i].flags.cycles);
> +		printed += print_bstack_flags(fp, entries + i);
>  	}
>  
>  	return printed;
> @@ -941,11 +946,7 @@ static int perf_sample__fprintf_brstacksym(struct perf_sample *sample,
>  			printed += map__fprintf_dsoname(alt.map, fp);
>  			printed += fprintf(fp, ")");
>  		}
> -		printed += fprintf(fp, "/%c/%c/%c/%d ",
> -			mispred_str(entries + i),
> -			entries[i].flags.in_tx ? 'X' : '-',
> -			entries[i].flags.abort ? 'A' : '-',
> -			entries[i].flags.cycles);
> +		printed += print_bstack_flags(fp, entries + i);
>  	}
>  
>  	return printed;
> @@ -991,11 +992,7 @@ static int perf_sample__fprintf_brstackoff(struct perf_sample *sample,
>  			printed += map__fprintf_dsoname(alt.map, fp);
>  			printed += fprintf(fp, ")");
>  		}
> -		printed += fprintf(fp, "/%c/%c/%c/%d ",
> -			mispred_str(entries + i),
> -			entries[i].flags.in_tx ? 'X' : '-',
> -			entries[i].flags.abort ? 'A' : '-',
> -			entries[i].flags.cycles);
> +		printed += print_bstack_flags(fp, entries + i);
>  	}
>  
>  	return printed;


LGTM

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH 4/4] perf script: Output branch sample type

[Anshuman Khandual]

On 3/7/22 22:49, James Clark wrote:
> The type info is saved when using '-j save_type'. Output this in perf

Mentioning PERF_SAMPLE_BRANCH_CALL_STACK here as well might be better.

> script so it can be accessed by other tools or for debugging.
> 
> It's appended to the end of the list of fields so any existing tools
> that split on / and access fields via an index are not affected. Also
> output '-' instead of 'N/A' when the branch type isn't saved because /
> is used as a field separator.

Did not get it. Why 'N/A' should have been used anyway ?

> 
> Entries before this change look like this:
> 
>   0xaaaadb350838/0xaaaadb3507a4/P/-/-/0
> 
> And afterwards like this:
> 
>   0xaaaadb350838/0xaaaadb3507a4/P/-/-/0/CALL
> 
> or this if no type info is saved:
> 
>   0x7fb57586df6b/0x7fb5758731f0/P/-/-/143/-

143 ?

> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/builtin-script.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index fac2e9470926..5e4a262a6825 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -859,11 +859,12 @@ mispred_str(struct branch_entry *br)
>  
>  static int print_bstack_flags(FILE *fp, struct branch_entry *br)
>  {
> -	return fprintf(fp, "/%c/%c/%c/%d ",
> +	return fprintf(fp, "/%c/%c/%c/%d/%s ",
>  		       mispred_str(br),
>  		       br->flags.in_tx ? 'X' : '-',
>  		       br->flags.abort ? 'A' : '-',
> -		       br->flags.cycles);
> +		       br->flags.cycles,
> +		       br->flags.type ? branch_type_name(br->flags.type) : "-");
>  }
>  
>  static int perf_sample__fprintf_brstack(struct perf_sample *sample,


LGTM but as mentioned before, I hope this does not affect any existing
parsing tools.

Re: [PATCH 2/4] perf: Print branch stack entry type in --dump-raw-trace

[James Clark]

On 08/03/2022 04:29, Anshuman Khandual wrote:
> 
> 
> On 3/7/22 22:49, James Clark wrote:
>> This can help with debugging issues. It only prints when -j save_type
>> is used otherwise an empty string is printed.
> 
> Specifying events with PERF_SAMPLE_BRANCH_CALL_STACK flag explicitly might
> be better along with '-j save_type'.
> 
>>
>> Before the change:
>>
>>   101603801707130 0xa70 [0x630]: PERF_RECORD_SAMPLE(IP, 0x2): 1108/1108: 0xffff9c1df24c period: 10694 addr: 0
>>   ... branch stack: nr:64
>>   .....  0: 0000ffff9c26029c -> 0000ffff9c26f340 0 cycles  P   0
>>   .....  1: 0000ffff9c2601bc -> 0000ffff9c26f340 0 cycles  P   0
>>
>> After the change:
>>
>>   101603801707130 0xa70 [0x630]: PERF_RECORD_SAMPLE(IP, 0x2): 1108/1108: 0xffff9c1df24c period: 10694 addr: 0
>>   ... branch stack: nr:64
>>   .....  0: 0000ffff9c26029c -> 0000ffff9c26f340 0 cycles  P   0 CALL
>>   .....  1: 0000ffff9c2601bc -> 0000ffff9c26f340 0 cycles  P   0 IND_CALL
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  tools/perf/util/session.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index f54282d5c648..3b8dfe603e50 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -1159,14 +1159,15 @@ static void branch_stack__printf(struct perf_sample *sample, bool callstack)
>>  		struct branch_entry *e = &entries[i];
>>  
>>  		if (!callstack) {
>> -			printf("..... %2"PRIu64": %016" PRIx64 " -> %016" PRIx64 " %hu cycles %s%s%s%s %x\n",
>> +			printf("..... %2"PRIu64": %016" PRIx64 " -> %016" PRIx64 " %hu cycles %s%s%s%s %x %s\n",
>>  				i, e->from, e->to,
>>  				(unsigned short)e->flags.cycles,
>>  				e->flags.mispred ? "M" : " ",
>>  				e->flags.predicted ? "P" : " ",
>>  				e->flags.abort ? "A" : " ",
>>  				e->flags.in_tx ? "T" : " ",
>> -				(unsigned)e->flags.reserved);
>> +				(unsigned)e->flags.reserved,
>> +				e->flags.type ? branch_type_name(e->flags.type) : "");
>>  		} else {
>>  			printf("..... %2"PRIu64": %016" PRIx64 "\n",
>>  				i, i > 0 ? e->from : e->to);
> 
> LGTM but I am wondering whether this might affect existing tools ?

Only humans should be reading the -D output so I don't think so. The format is not very parseable anyway

Re: [PATCH 4/4] perf script: Output branch sample type

[James Clark]

On 08/03/2022 05:22, Anshuman Khandual wrote:
> 
> 
> On 3/7/22 22:49, James Clark wrote:
>> The type info is saved when using '-j save_type'. Output this in perf
> 
> Mentioning PERF_SAMPLE_BRANCH_CALL_STACK here as well might be better.
> 
>> script so it can be accessed by other tools or for debugging.
>>
>> It's appended to the end of the list of fields so any existing tools
>> that split on / and access fields via an index are not affected. Also
>> output '-' instead of 'N/A' when the branch type isn't saved because /
>> is used as a field separator.
> 
> Did not get it. Why 'N/A' should have been used anyway ?

N/A would be printed if branch type isn't saved because then branch type == 0.
N/A is the name that's assigned to the 0 entry of the branch type name list.

> 
>>
>> Entries before this change look like this:
>>
>>   0xaaaadb350838/0xaaaadb3507a4/P/-/-/0
>>
>> And afterwards like this:
>>
>>   0xaaaadb350838/0xaaaadb3507a4/P/-/-/0/CALL
>>
>> or this if no type info is saved:
>>
>>   0x7fb57586df6b/0x7fb5758731f0/P/-/-/143/-
> 
> 143 ?

Just random output from my laptop probably. It's not supposed to match up
with the previous entries, it's a new run and a new set of output.

> 
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  tools/perf/builtin-script.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>> index fac2e9470926..5e4a262a6825 100644
>> --- a/tools/perf/builtin-script.c
>> +++ b/tools/perf/builtin-script.c
>> @@ -859,11 +859,12 @@ mispred_str(struct branch_entry *br)
>>  
>>  static int print_bstack_flags(FILE *fp, struct branch_entry *br)
>>  {
>> -	return fprintf(fp, "/%c/%c/%c/%d ",
>> +	return fprintf(fp, "/%c/%c/%c/%d/%s ",
>>  		       mispred_str(br),
>>  		       br->flags.in_tx ? 'X' : '-',
>>  		       br->flags.abort ? 'A' : '-',
>> -		       br->flags.cycles);
>> +		       br->flags.cycles,
>> +		       br->flags.type ? branch_type_name(br->flags.type) : "-");
>>  }
>>  
>>  static int perf_sample__fprintf_brstack(struct perf_sample *sample,
> 
> 
> LGTM but as mentioned before, I hope this does not affect any existing
> parsing tools.

It's possible for this perf script change. But any parser would have to be splitting on
/ and spaces and indexing into the result so I can't see how it's possible to make a parser
that wouldn't handle an entry appended to the end.

I imagine someone could have an assert that checks the number of results after the split on /.
But if they added that I'm assuming they were thinking of the possibility that extra entries
could be added so handle it properly.