RE: [PATCH v2] perf tools: Fix arm64 build error with gcc-11

[Jianlin Lv]

> -----Original Message-----
> From: John Garry <john.garry@xxxxxxxxxx>
> Sent: Wednesday, February 10, 2021 5:38 PM
> To: Jianlin Lv <Jianlin.Lv@xxxxxxx>; will@xxxxxxxxxx;
> mathieu.poirier@xxxxxxxxxx; leo.yan@xxxxxxxxxx; peterz@xxxxxxxxxxxxx;
> mingo@xxxxxxxxxx; acme@xxxxxxxxxx; Mark Rutland
> <Mark.Rutland@xxxxxxx>; alexander.shishkin@xxxxxxxxxxxxxxx;
> jolsa@xxxxxxxxxx; namhyung@xxxxxxxxxx; irogers@xxxxxxxxxx;
> agerstmayr@xxxxxxxxxx; kan.liang@xxxxxxxxxxxxxxx;
> adrian.hunter@xxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> > ---
> > 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.