| Message ID | 20250217195908.176207-2-leo.yan@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | perf script: Refactor branch flags for Arm SPE | expand |
On 17/02/25 21:58, Leo Yan wrote: > Add a check for the generated string of flags. Print out the raw number > if the string generation fails. How does it fail? > > In another case, if the string length is longer than the aligned size, > allow the completed string to be printed. > > Reviewed-by: Ian Rogers <irogers@google.com> > Reviewed-by: James Clark <james.clark@linaro.org> > Signed-off-by: Leo Yan <leo.yan@arm.com> > --- > tools/perf/builtin-script.c | 10 ++++++++-- > tools/perf/util/trace-event.h | 2 ++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c > index d797cec4f054..2c4b1fb7dc72 100644 > --- a/tools/perf/builtin-script.c > +++ b/tools/perf/builtin-script.c > @@ -1709,9 +1709,15 @@ static int perf_sample__fprintf_bts(struct perf_sample *sample, > static int perf_sample__fprintf_flags(u32 flags, FILE *fp) > { > char str[SAMPLE_FLAGS_BUF_SIZE]; > + int ret; > + > + ret = perf_sample__sprintf_flags(flags, str, sizeof(str)); > + if (ret < 0) AFAICT ret is always >= 0 > + return fprintf(fp, " raw flags:0x%-*x ", > + SAMPLE_FLAGS_STR_ALIGNED_SIZE - 12, flags); > > - perf_sample__sprintf_flags(flags, str, sizeof(str)); > - return fprintf(fp, " %-21s ", str); > + ret = max(ret, SAMPLE_FLAGS_STR_ALIGNED_SIZE); > + return fprintf(fp, " %-*s ", ret, str); -21 means the field width is 21 and left-justified. It should not truncate the string. > } > > struct printer_data { > diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h > index ac9fde2f980c..71e680bc3d4b 100644 > --- a/tools/perf/util/trace-event.h > +++ b/tools/perf/util/trace-event.h > @@ -145,6 +145,8 @@ int common_flags(struct scripting_context *context); > int common_lock_depth(struct scripting_context *context); > > #define SAMPLE_FLAGS_BUF_SIZE 64 > +#define SAMPLE_FLAGS_STR_ALIGNED_SIZE 21 > + > int perf_sample__sprintf_flags(u32 flags, char *str, size_t sz); > > #if defined(LIBTRACEEVENT_VERSION) && LIBTRACEEVENT_VERSION >= MAKE_LIBTRACEEVENT_VERSION(1, 5, 0)
On 3/03/25 18:22, Leo Yan wrote: > Hi Adrian, > > On Mon, Mar 03, 2025 at 12:44:33PM +0200, Adrian Hunter wrote: >> On 17/02/25 21:58, Leo Yan wrote: >>> Add a check for the generated string of flags. Print out the raw number >>> if the string generation fails. >> >> How does it fail? > > In practice, I agreed perf_sample__sprintf_flags() will not fail. This > bases on a careful calculation for every invoking snprintf(). > > Please see comment below. > >>> In another case, if the string length is longer than the aligned size, >>> allow the completed string to be printed. >>> >>> Reviewed-by: Ian Rogers <irogers@google.com> >>> Reviewed-by: James Clark <james.clark@linaro.org> >>> Signed-off-by: Leo Yan <leo.yan@arm.com> >>> --- >>> tools/perf/builtin-script.c | 10 ++++++++-- >>> tools/perf/util/trace-event.h | 2 ++ >>> 2 files changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c >>> index d797cec4f054..2c4b1fb7dc72 100644 >>> --- a/tools/perf/builtin-script.c >>> +++ b/tools/perf/builtin-script.c >>> @@ -1709,9 +1709,15 @@ static int perf_sample__fprintf_bts(struct perf_sample *sample, >>> static int perf_sample__fprintf_flags(u32 flags, FILE *fp) >>> { >>> char str[SAMPLE_FLAGS_BUF_SIZE]; >>> + int ret; >>> + >>> + ret = perf_sample__sprintf_flags(flags, str, sizeof(str)); >>> + if (ret < 0) >> >> AFAICT ret is always >= 0 > > Since I refactored perf_sample__sprintf_flags() in the sequential > patches, for easier capturing and debugging, here checks the return > value to detect any potential issues. > > Later when we review a perf log, a printed raw number for error cases > can remind there must be something wrong for printing flags. > >>> + return fprintf(fp, " raw flags:0x%-*x ", >>> + SAMPLE_FLAGS_STR_ALIGNED_SIZE - 12, flags); >>> >>> - perf_sample__sprintf_flags(flags, str, sizeof(str)); >>> - return fprintf(fp, " %-21s ", str); >>> + ret = max(ret, SAMPLE_FLAGS_STR_ALIGNED_SIZE); >>> + return fprintf(fp, " %-*s ", ret, str); >> >> -21 means the field width is 21 and left-justified. It should not >> truncate the string. > > No, it does not truncate the string. > > It calculates a maximum value between the returned length and 21 ( > defined in SAMPLE_FLAGS_STR_ALIGNED_SIZE). It keeps left-justified and > can printing a complete string if the string length is bigger than 21. Maybe I am missing something, but that isn't that what return fprintf(fp, " %-21s ", str); does anyway? Why change it to something more complicated. > > >> >>> } >>> >>> struct printer_data { >>> diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h >>> index ac9fde2f980c..71e680bc3d4b 100644 >>> --- a/tools/perf/util/trace-event.h >>> +++ b/tools/perf/util/trace-event.h >>> @@ -145,6 +145,8 @@ int common_flags(struct scripting_context *context); >>> int common_lock_depth(struct scripting_context *context); >>> >>> #define SAMPLE_FLAGS_BUF_SIZE 64 >>> +#define SAMPLE_FLAGS_STR_ALIGNED_SIZE 21 >>> + >>> int perf_sample__sprintf_flags(u32 flags, char *str, size_t sz); >>> >>> #if defined(LIBTRACEEVENT_VERSION) && LIBTRACEEVENT_VERSION >= MAKE_LIBTRACEEVENT_VERSION(1, 5, 0) >>
Hi Adrian, On Mon, Mar 03, 2025 at 12:44:33PM +0200, Adrian Hunter wrote: > On 17/02/25 21:58, Leo Yan wrote: > > Add a check for the generated string of flags. Print out the raw number > > if the string generation fails. > > How does it fail? In practice, I agreed perf_sample__sprintf_flags() will not fail. This bases on a careful calculation for every invoking snprintf(). Please see comment below. > > In another case, if the string length is longer than the aligned size, > > allow the completed string to be printed. > > > > Reviewed-by: Ian Rogers <irogers@google.com> > > Reviewed-by: James Clark <james.clark@linaro.org> > > Signed-off-by: Leo Yan <leo.yan@arm.com> > > --- > > tools/perf/builtin-script.c | 10 ++++++++-- > > tools/perf/util/trace-event.h | 2 ++ > > 2 files changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c > > index d797cec4f054..2c4b1fb7dc72 100644 > > --- a/tools/perf/builtin-script.c > > +++ b/tools/perf/builtin-script.c > > @@ -1709,9 +1709,15 @@ static int perf_sample__fprintf_bts(struct perf_sample *sample, > > static int perf_sample__fprintf_flags(u32 flags, FILE *fp) > > { > > char str[SAMPLE_FLAGS_BUF_SIZE]; > > + int ret; > > + > > + ret = perf_sample__sprintf_flags(flags, str, sizeof(str)); > > + if (ret < 0) > > AFAICT ret is always >= 0 Since I refactored perf_sample__sprintf_flags() in the sequential patches, for easier capturing and debugging, here checks the return value to detect any potential issues. Later when we review a perf log, a printed raw number for error cases can remind there must be something wrong for printing flags. > > + return fprintf(fp, " raw flags:0x%-*x ", > > + SAMPLE_FLAGS_STR_ALIGNED_SIZE - 12, flags); > > > > - perf_sample__sprintf_flags(flags, str, sizeof(str)); > > - return fprintf(fp, " %-21s ", str); > > + ret = max(ret, SAMPLE_FLAGS_STR_ALIGNED_SIZE); > > + return fprintf(fp, " %-*s ", ret, str); > > -21 means the field width is 21 and left-justified. It should not > truncate the string. No, it does not truncate the string. It calculates a maximum value between the returned length and 21 ( defined in SAMPLE_FLAGS_STR_ALIGNED_SIZE). It keeps left-justified and can printing a complete string if the string length is bigger than 21. Thanks, Leo > > > } > > > > struct printer_data { > > diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h > > index ac9fde2f980c..71e680bc3d4b 100644 > > --- a/tools/perf/util/trace-event.h > > +++ b/tools/perf/util/trace-event.h > > @@ -145,6 +145,8 @@ int common_flags(struct scripting_context *context); > > int common_lock_depth(struct scripting_context *context); > > > > #define SAMPLE_FLAGS_BUF_SIZE 64 > > +#define SAMPLE_FLAGS_STR_ALIGNED_SIZE 21 > > + > > int perf_sample__sprintf_flags(u32 flags, char *str, size_t sz); > > > > #if defined(LIBTRACEEVENT_VERSION) && LIBTRACEEVENT_VERSION >= MAKE_LIBTRACEEVENT_VERSION(1, 5, 0) >
On 3/03/25 20:11, Leo Yan wrote: > On Mon, Mar 03, 2025 at 05:05:13PM +0200, Adrian Hunter wrote: > > [...] > >>>>> + ret = max(ret, SAMPLE_FLAGS_STR_ALIGNED_SIZE); >>>>> + return fprintf(fp, " %-*s ", ret, str); >>>> >>>> -21 means the field width is 21 and left-justified. It should not >>>> truncate the string. >>> >>> No, it does not truncate the string. >>> >>> It calculates a maximum value between the returned length and 21 ( >>> defined in SAMPLE_FLAGS_STR_ALIGNED_SIZE). It keeps left-justified and >>> can printing a complete string if the string length is bigger than 21. >> >> Maybe I am missing something, but that isn't that what >> >> return fprintf(fp, " %-21s ", str); >> >> does anyway? Why change it to something more complicated. > > You are right. I should have done an experiment for this. > > I will remove the max() sentence and update the last line: > > return fprintf(fp, " %-*s ", SAMPLE_FLAGS_STR_ALIGNED_SIZE, str); > > Another option is to drop this patch. But I prefer to keep it, the > reason is except the benefit for the debugging log, an extra reason is > the SAMPLE_FLAGS_STR_ALIGNED_SIZE macro is used to replace the opened > value '21'. The macro also will be used by a later patch for > right-alignment printing. How about you think? Sure
On Mon, Mar 03, 2025 at 05:05:13PM +0200, Adrian Hunter wrote: [...] > >>> + ret = max(ret, SAMPLE_FLAGS_STR_ALIGNED_SIZE); > >>> + return fprintf(fp, " %-*s ", ret, str); > >> > >> -21 means the field width is 21 and left-justified. It should not > >> truncate the string. > > > > No, it does not truncate the string. > > > > It calculates a maximum value between the returned length and 21 ( > > defined in SAMPLE_FLAGS_STR_ALIGNED_SIZE). It keeps left-justified and > > can printing a complete string if the string length is bigger than 21. > > Maybe I am missing something, but that isn't that what > > return fprintf(fp, " %-21s ", str); > > does anyway? Why change it to something more complicated. You are right. I should have done an experiment for this. I will remove the max() sentence and update the last line: return fprintf(fp, " %-*s ", SAMPLE_FLAGS_STR_ALIGNED_SIZE, str); Another option is to drop this patch. But I prefer to keep it, the reason is except the benefit for the debugging log, an extra reason is the SAMPLE_FLAGS_STR_ALIGNED_SIZE macro is used to replace the opened value '21'. The macro also will be used by a later patch for right-alignment printing. How about you think? Thanks, Leo
On Mon, Mar 03, 2025 at 06:56:42PM +0200, Adrian Hunter wrote: > On 3/03/25 20:11, Leo Yan wrote: > > On Mon, Mar 03, 2025 at 05:05:13PM +0200, Adrian Hunter wrote: > > > > [...] > > > >>>>> + ret = max(ret, SAMPLE_FLAGS_STR_ALIGNED_SIZE); > >>>>> + return fprintf(fp, " %-*s ", ret, str); > >>>> > >>>> -21 means the field width is 21 and left-justified. It should not > >>>> truncate the string. > >>> > >>> No, it does not truncate the string. > >>> > >>> It calculates a maximum value between the returned length and 21 ( > >>> defined in SAMPLE_FLAGS_STR_ALIGNED_SIZE). It keeps left-justified and > >>> can printing a complete string if the string length is bigger than 21. > >> > >> Maybe I am missing something, but that isn't that what > >> > >> return fprintf(fp, " %-21s ", str); > >> > >> does anyway? Why change it to something more complicated. > > > > You are right. I should have done an experiment for this. > > > > I will remove the max() sentence and update the last line: > > > > return fprintf(fp, " %-*s ", SAMPLE_FLAGS_STR_ALIGNED_SIZE, str); > > > > Another option is to drop this patch. But I prefer to keep it, the > > reason is except the benefit for the debugging log, an extra reason is > > the SAMPLE_FLAGS_STR_ALIGNED_SIZE macro is used to replace the opened > > value '21'. The macro also will be used by a later patch for > > right-alignment printing. How about you think? > > Sure Thanks a lot for confirmation and review! I will send new version in my tomorrow. Leo
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c index d797cec4f054..2c4b1fb7dc72 100644 --- a/tools/perf/builtin-script.c +++ b/tools/perf/builtin-script.c @@ -1709,9 +1709,15 @@ static int perf_sample__fprintf_bts(struct perf_sample *sample, static int perf_sample__fprintf_flags(u32 flags, FILE *fp) { char str[SAMPLE_FLAGS_BUF_SIZE]; + int ret; + + ret = perf_sample__sprintf_flags(flags, str, sizeof(str)); + if (ret < 0) + return fprintf(fp, " raw flags:0x%-*x ", + SAMPLE_FLAGS_STR_ALIGNED_SIZE - 12, flags); - perf_sample__sprintf_flags(flags, str, sizeof(str)); - return fprintf(fp, " %-21s ", str); + ret = max(ret, SAMPLE_FLAGS_STR_ALIGNED_SIZE); + return fprintf(fp, " %-*s ", ret, str); } struct printer_data { diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h index ac9fde2f980c..71e680bc3d4b 100644 --- a/tools/perf/util/trace-event.h +++ b/tools/perf/util/trace-event.h @@ -145,6 +145,8 @@ int common_flags(struct scripting_context *context); int common_lock_depth(struct scripting_context *context); #define SAMPLE_FLAGS_BUF_SIZE 64 +#define SAMPLE_FLAGS_STR_ALIGNED_SIZE 21 + int perf_sample__sprintf_flags(u32 flags, char *str, size_t sz); #if defined(LIBTRACEEVENT_VERSION) && LIBTRACEEVENT_VERSION >= MAKE_LIBTRACEEVENT_VERSION(1, 5, 0)