All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Fix s390x compile error on F32 utils/stat-display.c
@ 2020-07-22  9:20 Thomas Richter
  2020-08-12 11:27 ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Richter @ 2020-07-22  9:20 UTC (permalink / raw)
  To: linux-kernel, linux-perf-users, acme
  Cc: svens, gor, sumanthk, heiko.carstens, Thomas Richter

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.

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

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);
 		}
 		break;
 	case AGGR_THREAD:
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix s390x compile error on F32 utils/stat-display.c
  2020-07-22  9:20 [PATCH] Fix s390x compile error on F32 utils/stat-display.c Thomas Richter
@ 2020-08-12 11:27 ` Arnaldo Carvalho de Melo
  2020-08-24 14:44   ` Thomas Richter
  2020-08-24 20:22   ` Jiri Olsa
  0 siblings, 2 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-08-12 11:27 UTC (permalink / raw)
  To: Jin Yao, Thomas Richter
  Cc: Jiri Olsa, Namhyung Kim, linux-kernel, linux-perf-users, svens,
	gor, sumanthk, heiko.carstens

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]#
> 
> 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);
>  		}
>  		break;
>  	case AGGR_THREAD:
> -- 
> 2.26.2
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix s390x compile error on F32 utils/stat-display.c
  2020-08-12 11:27 ` Arnaldo Carvalho de Melo
@ 2020-08-24 14:44   ` Thomas Richter
  2020-08-24 20:22   ` Jiri Olsa
  1 sibling, 0 replies; 5+ messages in thread
From: Thomas Richter @ 2020-08-24 14:44 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jin Yao
  Cc: Jiri Olsa, Namhyung Kim, linux-kernel, linux-perf-users, svens,
	gor, sumanthk, heiko.carstens

Hi Arnaldo, Jin, Jiri

any update on this issue?

Thanks.

On 8/12/20 1:27 PM, 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]#
>>
>> 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);
>>  		}
>>  		break;
>>  	case AGGR_THREAD:
>> -- 
>> 2.26.2
>>
> 


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix s390x compile error on F32 utils/stat-display.c
  2020-08-12 11:27 ` Arnaldo Carvalho de Melo
  2020-08-24 14:44   ` Thomas Richter
@ 2020-08-24 20:22   ` Jiri Olsa
  2020-08-25  6:15     ` Thomas Richter
  1 sibling, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2020-08-24 20:22 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jin Yao, Thomas Richter, Jiri Olsa, Namhyung Kim, linux-kernel,
	linux-perf-users, svens, gor, sumanthk, heiko.carstens

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

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix s390x compile error on F32 utils/stat-display.c
  2020-08-24 20:22   ` Jiri Olsa
@ 2020-08-25  6:15     ` Thomas Richter
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Richter @ 2020-08-25  6:15 UTC (permalink / raw)
  To: Jiri Olsa, Arnaldo Carvalho de Melo
  Cc: Jin Yao, Jiri Olsa, Namhyung Kim, linux-kernel, linux-perf-users,
	svens, gor, sumanthk, heiko.carstens

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-08-25  6:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22  9:20 [PATCH] Fix s390x compile error on F32 utils/stat-display.c Thomas Richter
2020-08-12 11:27 ` Arnaldo Carvalho de Melo
2020-08-24 14:44   ` Thomas Richter
2020-08-24 20:22   ` Jiri Olsa
2020-08-25  6:15     ` Thomas Richter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.