Messages in this thread | | | Date | Tue, 8 Mar 2022 12:09:31 +0000 | Subject | Re: [PATCH 4/4] perf script: Output branch sample type | From | 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.
| |