Re: [PATCH] perf script python: Fix buffer size to report iregs in perf script

[Paul A. Clarke]

On Mon, Jun 28, 2021 at 11:53:41AM +0530, Kajol Jain wrote:
> Commit 48a1f565261d ("perf script python: Add more PMU fields
> to event handler dict") added functionality to report fields like
> weight, iregs, uregs etc via perf report.
> That commit predefined buffer size to 512 bytes to print those fields.
> 
> But incase of powerpc, since we added extended regs support
> in commits:
> 
> Commit 068aeea3773a ("perf powerpc: Support exposing Performance Monitor
> Counter SPRs as part of extended regs")
> Commit d735599a069f ("powerpc/perf: Add extended regs support for
> power10 platform")
> 
> Now iregs can carry more bytes of data and this predefined buffer size
> can result to data loss in perf script output.
> 
> Patch resolve this issue by making buffer size dynamic based on number
> of registers needed to print. It also changed return type for function
> "regs_map" from int to void, as the return value is not being used by
> the caller function "set_regs_in_dict".
> 
> Fixes: 068aeea3773a ("perf powerpc: Support exposing Performance Monitor
> Counter SPRs as part of extended regs")
> Signed-off-by: Kajol Jain <kjain@xxxxxxxxxxxxx>
> ---
>  .../util/scripting-engines/trace-event-python.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index 4e4aa4c97ac5..c8c9706b4643 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
[...]
> @@ -713,7 +711,16 @@ static void set_regs_in_dict(PyObject *dict,
>  			     struct evsel *evsel)
>  {
>  	struct perf_event_attr *attr = &evsel->core.attr;
> -	char bf[512];
> +
> +	/*
> +	 * Here value 28 is a constant size which can be used to print
> +	 * one register value and its corresponds to:
> +	 * 16 chars is to specify 64 bit register in hexadecimal.
> +	 * 2 chars is for appending "0x" to the hexadecimal value and
> +	 * 10 chars is for register name.
> +	 */
> +	int size = __sw_hweight64(attr->sample_regs_intr) * 28;
> +	char bf[size];

I propose using a template rather than a magic number here. Something like:
const char reg_name_tmpl[] = "10 chars  ";
const char reg_value_tmpl[] = "0x0123456789abcdef";
const int size = __sw_hweight64(attr->sample_regs_intr) +
                 sizeof reg_name_tmpl + sizeof reg_value_tmpl;

Pardon my ignorance, but is there no separation/whitespace between the name
and the value? And is there some significance to 10 characters for the
register name, or is that a magic number?

PC