diff mbox series

[v3,01/12] perf script: Make printing flags reliable

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

Commit Message

Leo Yan Feb. 17, 2025, 7:58 p.m. UTC
Add a check for the generated string of flags.  Print out the raw number
if the string generation fails.

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(-)

Comments

Adrian Hunter March 3, 2025, 10:44 a.m. UTC | #1
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)
Adrian Hunter March 3, 2025, 3:05 p.m. UTC | #2
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)
>>
Leo Yan March 3, 2025, 4:22 p.m. UTC | #3
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)
>
Adrian Hunter March 3, 2025, 4:56 p.m. UTC | #4
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
Leo Yan March 3, 2025, 6:11 p.m. UTC | #5
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
Leo Yan March 3, 2025, 6:49 p.m. UTC | #6
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 mbox series

Patch

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)