linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] perf tools: Fix arm64 build error with gcc-11
@ 2021-02-10  3:24 Jianlin Lv
  2021-02-10  9:38 ` John Garry
  0 siblings, 1 reply; 4+ messages in thread
From: Jianlin Lv @ 2021-02-10  3:24 UTC (permalink / raw)
  To: john.garry, will, mathieu.poirier, leo.yan, peterz, mingo, acme,
	mark.rutland, alexander.shishkin, jolsa, namhyung, irogers,
	agerstmayr, kan.liang, adrian.hunter
  Cc: Jianlin.Lv, linux-kernel

gcc version: 11.0.0 20210208 (experimental) (GCC)

Following build error on arm64:

.......
In function ‘printf’,
    inlined from ‘regs_dump__printf’ at util/session.c:1141:3,
    inlined from ‘regs__printf’ at util/session.c:1169:2:
/usr/include/aarch64-linux-gnu/bits/stdio2.h:107:10: \
  error: ‘%-5s’ directive argument is null [-Werror=format-overflow=]

107 |   return __printf_chk (__USE_FORTIFY_LEVEL - 1, __fmt, \
                __va_arg_pack ());

......
In function ‘fprintf’,
  inlined from ‘perf_sample__fprintf_regs.isra’ at \
    builtin-script.c:622:14:
/usr/include/aarch64-linux-gnu/bits/stdio2.h:100:10: \
    error: ‘%5s’ directive argument is null [-Werror=format-overflow=]
  100 |   return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
  101 |                         __va_arg_pack ());

cc1: all warnings being treated as errors
.......

This patch fixes Wformat-overflow warnings. Add ternary operator,
The statement evaluates to "Unknown" if reg_name==NULL is met.

Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
---
v2: Add ternary operator to avoid similar errors in other arch.
---
 tools/perf/builtin-script.c                            | 4 +++-
 tools/perf/util/scripting-engines/trace-event-python.c | 4 +++-
 tools/perf/util/session.c                              | 5 +++--
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 42dad4a0f8cf..d59da3a063d0 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -635,6 +635,7 @@ static int perf_sample__fprintf_regs(struct regs_dump *regs, uint64_t mask,
 {
 	unsigned i = 0, r;
 	int printed = 0;
+	const char *reg_name;
 
 	if (!regs || !regs->regs)
 		return 0;
@@ -643,7 +644,8 @@ static int perf_sample__fprintf_regs(struct regs_dump *regs, uint64_t mask,
 
 	for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
 		u64 val = regs->regs[i++];
-		printed += fprintf(fp, "%5s:0x%"PRIx64" ", perf_reg_name(r), val);
+		reg_name = perf_reg_name(r);
+		printed += fprintf(fp, "%5s:0x%"PRIx64" ", reg_name ?: "Unknown", val);
 	}
 
 	return printed;
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index c83c2c6564e0..e1222cc6a699 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -691,6 +691,7 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
 {
 	unsigned int i = 0, r;
 	int printed = 0;
+	const char *reg_name;
 
 	bf[0] = 0;
 
@@ -700,9 +701,10 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
 	for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
 		u64 val = regs->regs[i++];
 
+		reg_name = perf_reg_name(r);
 		printed += scnprintf(bf + printed, size - printed,
 				     "%5s:0x%" PRIx64 " ",
-				     perf_reg_name(r), val);
+				     reg_name ?: "Unknown", val);
 	}
 
 	return printed;
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 25adbcce0281..1058d8487e98 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1135,12 +1135,13 @@ static void branch_stack__printf(struct perf_sample *sample, bool callstack)
 static void regs_dump__printf(u64 mask, u64 *regs)
 {
 	unsigned rid, i = 0;
+	const char *reg_name;
 
 	for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
 		u64 val = regs[i++];
-
+		reg_name = perf_reg_name(rid);
 		printf(".... %-5s 0x%016" PRIx64 "\n",
-		       perf_reg_name(rid), val);
+		       reg_name ?: "Unknown", val);
 	}
 }
 
-- 
2.25.1


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

* Re: [PATCH v2] perf tools: Fix arm64 build error with gcc-11
  2021-02-10  3:24 [PATCH v2] perf tools: Fix arm64 build error with gcc-11 Jianlin Lv
@ 2021-02-10  9:38 ` John Garry
  2021-02-10 14:40   ` Jianlin Lv
  2021-02-10 15:20   ` David Laight
  0 siblings, 2 replies; 4+ messages in thread
From: John Garry @ 2021-02-10  9:38 UTC (permalink / raw)
  To: Jianlin Lv, will, mathieu.poirier, leo.yan, peterz, mingo, acme,
	mark.rutland, alexander.shishkin, jolsa, namhyung, irogers,
	agerstmayr, kan.liang, adrian.hunter
  Cc: linux-kernel

On 10/02/2021 03:24, Jianlin Lv wrote:
> gcc version: 11.0.0 20210208 (experimental) (GCC)
> 
> Following build error on arm64:
> 
> .......
> In function ‘printf’,
>      inlined from ‘regs_dump__printf’ at util/session.c:1141:3,
>      inlined from ‘regs__printf’ at util/session.c:1169:2:
> /usr/include/aarch64-linux-gnu/bits/stdio2.h:107:10: \
>    error: ‘%-5s’ directive argument is null [-Werror=format-overflow=]
> 
> 107 |   return __printf_chk (__USE_FORTIFY_LEVEL - 1, __fmt, \
>                  __va_arg_pack ());
> 
> ......
> In function ‘fprintf’,
>    inlined from ‘perf_sample__fprintf_regs.isra’ at \
>      builtin-script.c:622:14:
> /usr/include/aarch64-linux-gnu/bits/stdio2.h:100:10: \
>      error: ‘%5s’ directive argument is null [-Werror=format-overflow=]
>    100 |   return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
>    101 |                         __va_arg_pack ());
> 
> cc1: all warnings being treated as errors
> .......
> 
> This patch fixes Wformat-overflow warnings. Add ternary operator,
> The statement evaluates to "Unknown" if reg_name==NULL is met.
> 
> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
> ---
> v2: Add ternary operator to avoid similar errors in other arch.
> ---
>   tools/perf/builtin-script.c                            | 4 +++-
>   tools/perf/util/scripting-engines/trace-event-python.c | 4 +++-
>   tools/perf/util/session.c                              | 5 +++--
>   3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 42dad4a0f8cf..d59da3a063d0 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -635,6 +635,7 @@ static int perf_sample__fprintf_regs(struct regs_dump *regs, uint64_t mask,
>   {
>   	unsigned i = 0, r;
>   	int printed = 0;
> +	const char *reg_name;
>   
>   	if (!regs || !regs->regs)
>   		return 0;
> @@ -643,7 +644,8 @@ static int perf_sample__fprintf_regs(struct regs_dump *regs, uint64_t mask,
>   
>   	for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
>   		u64 val = regs->regs[i++];
> -		printed += fprintf(fp, "%5s:0x%"PRIx64" ", perf_reg_name(r), val);
> +		reg_name = perf_reg_name(r);
> +		printed += fprintf(fp, "%5s:0x%"PRIx64" ", reg_name ?: "Unknown", val);

is it possible not have to do this check for NULL and reassignment 
everywhere?

>   	}
>   
>   	return printed;
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index c83c2c6564e0..e1222cc6a699 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -691,6 +691,7 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
>   {
>   	unsigned int i = 0, r;
>   	int printed = 0;
> +	const char *reg_name;
>   
>   	bf[0] = 0;
>   
> @@ -700,9 +701,10 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
>   	for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {

a good practice is to limit scope of variables as much as possible, so 
reg_name could be declared here
	
>   		u64 val = regs->regs[i++];
>   
> +		reg_name = perf_reg_name(r);
>   		printed += scnprintf(bf + printed, size - printed,
>   				     "%5s:0x%" PRIx64 " ",
> -				     perf_reg_name(r), val);
> +				     reg_name ?: "Unknown", val);
>   	}
>   
>   	return printed;
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 25adbcce0281..1058d8487e98 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1135,12 +1135,13 @@ static void branch_stack__printf(struct perf_sample *sample, bool callstack)
>   static void regs_dump__printf(u64 mask, u64 *regs)
>   {
>   	unsigned rid, i = 0;
> +	const char *reg_name;
>   
>   	for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
>   		u64 val = regs[i++];
> -
> +		reg_name = perf_reg_name(rid);
>   		printf(".... %-5s 0x%016" PRIx64 "\n",
> -		       perf_reg_name(rid), val);
> +		       reg_name ?: "Unknown", val);

super nit: it's "unknown" in util/perf_regs.h::perf_reg_name(), so nice 
to be consistent


>   	}
>   }
>   
> 

thanks,
John

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

* RE: [PATCH v2] perf tools: Fix arm64 build error with gcc-11
  2021-02-10  9:38 ` John Garry
@ 2021-02-10 14:40   ` Jianlin Lv
  2021-02-10 15:20   ` David Laight
  1 sibling, 0 replies; 4+ messages in thread
From: Jianlin Lv @ 2021-02-10 14:40 UTC (permalink / raw)
  To: John Garry, will, mathieu.poirier, leo.yan, peterz, mingo, acme,
	Mark Rutland, alexander.shishkin, jolsa, namhyung, irogers,
	agerstmayr, kan.liang, adrian.hunter
  Cc: linux-kernel



> -----Original Message-----
> From: John Garry <john.garry@huawei.com>
> Sent: Wednesday, February 10, 2021 5:38 PM
> To: Jianlin Lv <Jianlin.Lv@arm.com>; will@kernel.org;
> mathieu.poirier@linaro.org; leo.yan@linaro.org; peterz@infradead.org;
> mingo@redhat.com; acme@kernel.org; Mark Rutland
> <Mark.Rutland@arm.com>; alexander.shishkin@linux.intel.com;
> jolsa@redhat.com; namhyung@kernel.org; irogers@google.com;
> agerstmayr@redhat.com; kan.liang@linux.intel.com;
> adrian.hunter@intel.com
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] perf tools: Fix arm64 build error with gcc-11
>
> On 10/02/2021 03:24, Jianlin Lv wrote:
> > gcc version: 11.0.0 20210208 (experimental) (GCC)
> >
> > Following build error on arm64:
> >
> > .......
> > In function ‘printf’,
> >      inlined from ‘regs_dump__printf’ at util/session.c:1141:3,
> >      inlined from ‘regs__printf’ at util/session.c:1169:2:
> > /usr/include/aarch64-linux-gnu/bits/stdio2.h:107:10: \
> >    error: ‘%-5s’ directive argument is null [-Werror=format-overflow=]
> >
> > 107 |   return __printf_chk (__USE_FORTIFY_LEVEL - 1, __fmt, \
> >                  __va_arg_pack ());
> >
> > ......
> > In function ‘fprintf’,
> >    inlined from ‘perf_sample__fprintf_regs.isra’ at \
> >      builtin-script.c:622:14:
> > /usr/include/aarch64-linux-gnu/bits/stdio2.h:100:10: \
> >      error: ‘%5s’ directive argument is null [-Werror=format-overflow=]
> >    100 |   return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
> >    101 |                         __va_arg_pack ());
> >
> > cc1: all warnings being treated as errors .......
> >
> > This patch fixes Wformat-overflow warnings. Add ternary operator, The
> > statement evaluates to "Unknown" if reg_name==NULL is met.
> >
> > Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
> > ---
> > v2: Add ternary operator to avoid similar errors in other arch.
> > ---
> >   tools/perf/builtin-script.c                            | 4 +++-
> >   tools/perf/util/scripting-engines/trace-event-python.c | 4 +++-
> >   tools/perf/util/session.c                              | 5 +++--
> >   3 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > index 42dad4a0f8cf..d59da3a063d0 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -635,6 +635,7 @@ static int perf_sample__fprintf_regs(struct
> regs_dump *regs, uint64_t mask,
> >   {
> >   unsigned i = 0, r;
> >   int printed = 0;
> > +const char *reg_name;
> >
> >   if (!regs || !regs->regs)
> >   return 0;
> > @@ -643,7 +644,8 @@ static int perf_sample__fprintf_regs(struct
> > regs_dump *regs, uint64_t mask,
> >
> >   for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
> >   u64 val = regs->regs[i++];
> > -printed += fprintf(fp, "%5s:0x%"PRIx64" ", perf_reg_name(r),
> val);
> > +reg_name = perf_reg_name(r);
> > +printed += fprintf(fp, "%5s:0x%"PRIx64" ", reg_name ?:
> "Unknown",
> > +val);
>
> is it possible not have to do this check for NULL and reassignment
> everywhere?
>

In fact, Wformat-overflow warning only occurs during the compilation of
the two files util/session.c and builtin-script.c.

util/scripting-engines/trace-event-python.c can be compiled. In order to
prevent unexpected exceptions, I changed it in the same way.

To be honest, I did not fully understand this comment. Do you mean to
adopt other ways to solve this issue? Could you give me more tips?

In addition, other comments are of great help to me, thank you.

Jianlin

> >   }
> >
> >   return printed;
> > diff --git a/tools/perf/util/scripting-engines/trace-event-python.c
> > b/tools/perf/util/scripting-engines/trace-event-python.c
> > index c83c2c6564e0..e1222cc6a699 100644
> > --- a/tools/perf/util/scripting-engines/trace-event-python.c
> > +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> > @@ -691,6 +691,7 @@ static int regs_map(struct regs_dump *regs,
> uint64_t mask, char *bf, int size)
> >   {
> >   unsigned int i = 0, r;
> >   int printed = 0;
> > +const char *reg_name;
> >
> >   bf[0] = 0;
> >
> > @@ -700,9 +701,10 @@ static int regs_map(struct regs_dump *regs,
> uint64_t mask, char *bf, int size)
> >   for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
>
> a good practice is to limit scope of variables as much as possible, so
> reg_name could be declared here
>
> >   u64 val = regs->regs[i++];
> >
> > +reg_name = perf_reg_name(r);
> >   printed += scnprintf(bf + printed, size - printed,
> >        "%5s:0x%" PRIx64 " ",
> > -     perf_reg_name(r), val);
> > +     reg_name ?: "Unknown", val);
> >   }
> >
> >   return printed;
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 25adbcce0281..1058d8487e98 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -1135,12 +1135,13 @@ static void branch_stack__printf(struct
> perf_sample *sample, bool callstack)
> >   static void regs_dump__printf(u64 mask, u64 *regs)
> >   {
> >   unsigned rid, i = 0;
> > +const char *reg_name;
> >
> >   for_each_set_bit(rid, (unsigned long *) &mask, sizeof(mask) * 8) {
> >   u64 val = regs[i++];
> > -
> > +reg_name = perf_reg_name(rid);
> >   printf(".... %-5s 0x%016" PRIx64 "\n",
> > -       perf_reg_name(rid), val);
> > +       reg_name ?: "Unknown", val);
>
> super nit: it's "unknown" in util/perf_regs.h::perf_reg_name(), so nice to be
> consistent
>
>
> >   }
> >   }
> >
> >
>
> thanks,
> John
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

* RE: [PATCH v2] perf tools: Fix arm64 build error with gcc-11
  2021-02-10  9:38 ` John Garry
  2021-02-10 14:40   ` Jianlin Lv
@ 2021-02-10 15:20   ` David Laight
  1 sibling, 0 replies; 4+ messages in thread
From: David Laight @ 2021-02-10 15:20 UTC (permalink / raw)
  To: 'John Garry',
	Jianlin Lv, will, mathieu.poirier, leo.yan, peterz, mingo, acme,
	mark.rutland, alexander.shishkin, jolsa, namhyung, irogers,
	agerstmayr, kan.liang, adrian.hunter
  Cc: linux-kernel

...
> > @@ -691,6 +691,7 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
> >   {
> >   	unsigned int i = 0, r;
> >   	int printed = 0;
> > +	const char *reg_name;
> >
> >   	bf[0] = 0;
> >
> > @@ -700,9 +701,10 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
> >   	for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
> 
> a good practice is to limit scope of variables as much as possible, so
> reg_name could be declared here

I'd go for either function scope or a very small inner block
(like this one).

Otherwise it gets very difficult for the average human (or other
intelligent being) to locate the definition.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

end of thread, other threads:[~2021-02-10 15:23 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10  3:24 [PATCH v2] perf tools: Fix arm64 build error with gcc-11 Jianlin Lv
2021-02-10  9:38 ` John Garry
2021-02-10 14:40   ` Jianlin Lv
2021-02-10 15:20   ` David Laight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).