[PATCH] perf: use C LC_NUMERIC locale for json output in perf stat

[PATCH] perf: use C LC_NUMERIC locale for json output in perf stat

[David Alvarez]

When using perf stat, the tool will use the system locale to format
decimal values. This is incorrect for json output, since the user locale
may lead to incorrect json formatting (which requires a dot as decimal
separator). The following example is perf's output under a ca_ES.UTF-8
locale:

{"interval" : 1.005100134, "counter-value" : "0,486755", "unit" : "Joules", "event" : "power/energy-pkg/", "event-runtime" : 100240331, "pcnt-running" : 100,00, "metric-value" : "0,000000", "metric-unit" : "(null)"}

Solve this issue by setting the LC_NUMERIC locale to "C" when enabling
json output (after parsing arguments).

Signed-off-by: David Alvarez <david.alvarez@bsc.es>
---
 tools/perf/builtin-stat.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 689a3d43c258..b9ebb06b254d 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2840,6 +2840,15 @@ int cmd_stat(int argc, const char **argv)
 	if (evlist__alloc_stats(&stat_config, evsel_list, interval))
 		goto out;
 
+	/*
+	 * For JSON output, we cannot use the user's numeric
+	 * locale since decimal separators must be dots.
+	 * Set the locale to "C" instead now that we've already parsed
+	 * all of the arguments using the user's locale.
+	 */
+	if (stat_config.json_output)
+		setlocale(LC_NUMERIC, "C");
+
 	/*
 	 * Set sample_type to PERF_SAMPLE_IDENTIFIER, which should be harmless
 	 * while avoiding that older tools show confusing messages.
-- 
2.47.0

Re: [PATCH] perf: use C LC_NUMERIC locale for json output in perf stat

[Ian Rogers]

On Mon, Nov 18, 2024 at 10:45 AM David Alvarez <david.alvarez@bsc.es> wrote:
>
> When using perf stat, the tool will use the system locale to format
> decimal values. This is incorrect for json output, since the user locale
> may lead to incorrect json formatting (which requires a dot as decimal
> separator). The following example is perf's output under a ca_ES.UTF-8
> locale:
>
> {"interval" : 1.005100134, "counter-value" : "0,486755", "unit" : "Joules", "event" : "power/energy-pkg/", "event-runtime" : 100240331, "pcnt-running" : 100,00, "metric-value" : "0,000000", "metric-unit" : "(null)"}
>
> Solve this issue by setting the LC_NUMERIC locale to "C" when enabling
> json output (after parsing arguments).
>
> Signed-off-by: David Alvarez <david.alvarez@bsc.es>

Thanks for the report but ugh, this is broken everywhere. For example,
for CSV output the printf format is created here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/stat-shadow.c#n199
Before being used:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/stat-display.c#n417
On BSD there is a locale specific printf with printf_l, but it looks
to be missing with glibc and no doubt missing on all the obscure libcs
the maintainers test with. Given this I think this is the right fix
but wonder, should we just set the locale globally and not for json
output?

We could also change tests like:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/stat+json_output.sh
to test both with the default and locales like ca_ES.UTF-8 (export
LC_NUMERIC=...). We could test other forms of output like CSV
similarly:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/stat+csv_output.sh

Thanks,
Ian

> ---
>  tools/perf/builtin-stat.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 689a3d43c258..b9ebb06b254d 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2840,6 +2840,15 @@ int cmd_stat(int argc, const char **argv)
>         if (evlist__alloc_stats(&stat_config, evsel_list, interval))
>                 goto out;
>
> +       /*
> +        * For JSON output, we cannot use the user's numeric
> +        * locale since decimal separators must be dots.
> +        * Set the locale to "C" instead now that we've already parsed
> +        * all of the arguments using the user's locale.
> +        */
> +       if (stat_config.json_output)
> +               setlocale(LC_NUMERIC, "C");
> +
>         /*
>          * Set sample_type to PERF_SAMPLE_IDENTIFIER, which should be harmless
>          * while avoiding that older tools show confusing messages.
> --
> 2.47.0
>

Re: [PATCH] perf: use C LC_NUMERIC locale for json output in perf stat

[Namhyung Kim]

On Mon, Nov 18, 2024 at 01:40:34PM -0800, Ian Rogers wrote:
> On Mon, Nov 18, 2024 at 10:45 AM David Alvarez <david.alvarez@bsc.es> wrote:
> >
> > When using perf stat, the tool will use the system locale to format
> > decimal values. This is incorrect for json output, since the user locale
> > may lead to incorrect json formatting (which requires a dot as decimal
> > separator). The following example is perf's output under a ca_ES.UTF-8
> > locale:
> >
> > {"interval" : 1.005100134, "counter-value" : "0,486755", "unit" : "Joules", "event" : "power/energy-pkg/", "event-runtime" : 100240331, "pcnt-running" : 100,00, "metric-value" : "0,000000", "metric-unit" : "(null)"}
> >
> > Solve this issue by setting the LC_NUMERIC locale to "C" when enabling
> > json output (after parsing arguments).
> >
> > Signed-off-by: David Alvarez <david.alvarez@bsc.es>
> 
> Thanks for the report but ugh, this is broken everywhere. For example,
> for CSV output the printf format is created here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/stat-shadow.c#n199
> Before being used:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/stat-display.c#n417
> On BSD there is a locale specific printf with printf_l, but it looks
> to be missing with glibc and no doubt missing on all the obscure libcs
> the maintainers test with. Given this I think this is the right fix
> but wonder, should we just set the locale globally and not for json
> output?

+1.

This might break something but it looks like a proper fix.

Thanks,
Namhyung

> 
> We could also change tests like:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/stat+json_output.sh
> to test both with the default and locales like ca_ES.UTF-8 (export
> LC_NUMERIC=...). We could test other forms of output like CSV
> similarly:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/stat+csv_output.sh
> 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/builtin-stat.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 689a3d43c258..b9ebb06b254d 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -2840,6 +2840,15 @@ int cmd_stat(int argc, const char **argv)
> >         if (evlist__alloc_stats(&stat_config, evsel_list, interval))
> >                 goto out;
> >
> > +       /*
> > +        * For JSON output, we cannot use the user's numeric
> > +        * locale since decimal separators must be dots.
> > +        * Set the locale to "C" instead now that we've already parsed
> > +        * all of the arguments using the user's locale.
> > +        */
> > +       if (stat_config.json_output)
> > +               setlocale(LC_NUMERIC, "C");
> > +
> >         /*
> >          * Set sample_type to PERF_SAMPLE_IDENTIFIER, which should be harmless
> >          * while avoiding that older tools show confusing messages.
> > --
> > 2.47.0
> >