Re: [PATCH] Fix s390x compile error on F32 utils/stat-display.c

[Thomas Richter <tmricht@linux.ibm.com>]

On 8/24/20 10:22 PM, Jiri Olsa wrote:
> On Wed, Aug 12, 2020 at 08:27:08AM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Jul 22, 2020 at 11:20:53AM +0200, Thomas Richter escreveu:
>>> Fix a compile error on F32 and gcc version 10.1 on s390 in file
>>> utils/stat-display.c.  The error does not show up with make DEBUG=y.
>>> In fact the issue shows up when using both compiler options
>>> -O6 and -D_FORTIFY_SOURCE=2 (which are omitted with DEBUG=Y).
>>>
>>> This is the offending call chain:
>>> print_counter_aggr()
>>>   printout(config, -1, 0, ...)  with 2nd parm id set to -1
>>>     aggr_printout(config, x, id --> -1, ...) which leads to this code:
>>> 		case AGGR_NONE:
>>>                 if (evsel->percore && !config->percore_show_thread) {
>>>                         ....
>>>                 } else {
>>>                         fprintf(config->output, "CPU%*d%s",
>>>                                 config->csv_output ? 0 : -7,
>>>                                 evsel__cpus(evsel)->map[id],
>>> 				                        ^^ id is -1 !!!!
>>>                                 config->csv_sep);
>>>                 }
>>>
>>> This is a compiler inlining issue which is detected on s390 but not on
>>> other plattforms.
>>
>> What is the sequence of events that gets to this? I.e. is it valid to
>> get a config->aggr_mode == AGGR_NONE, then have evsel not be percore and
>> config->percore_show_thread to be false?
>>
>> I wonder if this won't be papering over some bug :-\
>>
>> Jin?
>>
>> This is where this came from:
>> commit 4fc4d8dfa056dfd48afe73b9ea3b7570ceb80b9c (tag: perf-core-for-mingo-5.2-20190517)
>> Author: Jin Yao <yao.jin@linux.intel.com>
>> Date:   Fri Apr 12 21:59:49 2019 +0800
>>
>>     perf stat: Support 'percore' event qualifier
>>
>>     With this patch, we can use the 'percore' event qualifier in perf-stat.
>>
>> ---
>>
>> Also please add at least Jiri and Namhyung on the CC list, having the
>> person that added that array usage also helps.
>>
>> [acme@quaco perf]$ scripts/get_maintainer.pl tools/perf | grep reviewer
>> Mark Rutland <mark.rutland@arm.com> (reviewer:PERFORMANCE EVENTS SUBSYSTEM)
>> Alexander Shishkin <alexander.shishkin@linux.intel.com> (reviewer:PERFORMANCE EVENTS SUBSYSTEM)
>> Jiri Olsa <jolsa@redhat.com> (reviewer:PERFORMANCE EVENTS SUBSYSTEM)
>> Namhyung Kim <namhyung@kernel.org> (reviewer:PERFORMANCE EVENTS SUBSYSTEM)
>> [acme@quaco perf]$
>>
>> Thanks,
>>
>> - Arnaldo
>>  
>>> Output before:
>>>  # make util/stat-display.o
>>>     .....
>>>
>>>   util/stat-display.c: In function ‘perf_evlist__print_counters’:
>>>   util/stat-display.c:121:4: error: array subscript -1 is below array
>>>       bounds of ‘int[]’ [-Werror=array-bounds]
>>>   121 |    fprintf(config->output, "CPU%*d%s",
>>>       |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   122 |     config->csv_output ? 0 : -7,
>>>       |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   123 |     evsel__cpus(evsel)->map[id],
>>>       |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   124 |     config->csv_sep);
>>>       |     ~~~~~~~~~~~~~~~~
>>>   In file included from util/evsel.h:13,
>>>                  from util/evlist.h:13,
>>>                  from util/stat-display.c:9:
>>>   /root/linux/tools/lib/perf/include/internal/cpumap.h:10:7:
>>>   note: while referencing ‘map’
>>>    10 |  int  map[];
>>>       |       ^~~
>>>   cc1: all warnings being treated as errors
>>>   mv: cannot stat 'util/.stat-display.o.tmp': No such file or directory
>>>   make[3]: *** [/root/linux/tools/build/Makefile.build:97: util/stat-display.o]
>>>   Error 1
>>>   make[2]: *** [Makefile.perf:716: util/stat-display.o] Error 2
>>>   make[1]: *** [Makefile.perf:231: sub-make] Error 2
>>>   make: *** [Makefile:110: util/stat-display.o] Error 2
>>>   [root@t35lp46 perf]#
> 
> I tested on s390 with rhel8, but did not get this error,
> f32's gcc must be smarter
> 

i Jiri,

that is correct. I have installed Fedora 32 on my LPAR and the gcc version
is:
[root@t35lp46 ~]# gcc --version
gcc (GCC) 10.1.1 20200507 (Red Hat 10.1.1-1)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[root@t35lp46 ~]# cat /etc/redhat-release
Fedora release 32 (Thirty Two)
[root@t35lp46 ~]#

Same commands on a RHEL 8.2 installation reveal
[root@m35lp76 ~]# gcc --version
gcc (GCC) 8.3.1 20191121 (Red Hat 8.3.1-5)
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[root@m35lp76 ~]# cat /etc/redhat-release
Red Hat Enterprise Linux release 8.2 Beta (Ootpa)
[root@m35lp76 ~]# 

So there are 2 major revisions difference between a F32 and a RHEL8
gcc compiler installation.

>>>
>>> Output after:
>>>   # make util/stat-display.o
>>>     .....
>>>   CC       util/stat-display.o
>>>   [root@t35lp46 perf]#
>>>
>>> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
>>> ---
>>>  tools/perf/util/stat-display.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
>>> index 57d0706e1330..e49e544188e4 100644
>>> --- a/tools/perf/util/stat-display.c
>>> +++ b/tools/perf/util/stat-display.c
>>> @@ -118,10 +118,11 @@ static void aggr_printout(struct perf_stat_config *config,
>>>  				config->csv_output ? 0 : -3,
>>>  				cpu_map__id_to_cpu(id), config->csv_sep);
>>>  		} else {
>>> -			fprintf(config->output, "CPU%*d%s",
>>> -				config->csv_output ? 0 : -7,
>>> -				evsel__cpus(evsel)->map[id],
>>> -				config->csv_sep);
>>> +			if (id > -1)
>>> +				fprintf(config->output, "CPU%*d%s",
>>> +					config->csv_output ? 0 : -7,
>>> +					evsel__cpus(evsel)->map[id],
>>> +					config->csv_sep);
> 
> -1 is only through print_counter_aggr which is called for AGGR_GLOBAL
> case as you described, but I guess we can have this check as precaution
> 
> you could put it together with above else
> 
>     } else if (id > -1)
> 
> thanks,
> jirka
> 

As suggested I will send a new patch.

-- 
Thomas Richter, Dept 3252, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294