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

[Namhyung Kim <namhyung@kernel.org>]

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