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

[Leo Yan]

On Tue, Feb 09, 2021 at 02:18:26PM +0000, John Garry wrote:
> On 09/02/2021 12:17, Leo Yan wrote:
> > Hi Jianlin,
> > 
> > On Tue, Feb 09, 2021 at 07:33:57PM +0800, 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 by replacing the return
> > > value NULL of perf_reg_name with "unknown".
> > > 
> > > Signed-off-by: Jianlin Lv <Jianlin.Lv@xxxxxxx>
> > > ---
> > >   tools/perf/arch/arm64/include/perf_regs.h | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/perf/arch/arm64/include/perf_regs.h b/tools/perf/arch/arm64/include/perf_regs.h
> > > index baaa5e64a3fb..901419f907c0 100644
> > > --- a/tools/perf/arch/arm64/include/perf_regs.h
> > > +++ b/tools/perf/arch/arm64/include/perf_regs.h
> > > @@ -85,10 +85,10 @@ static inline const char *perf_reg_name(int id)
> > >   	case PERF_REG_ARM64_PC:
> > >   		return "pc";
> > >   	default:
> > > -		return NULL;
> > > +		return "unknown";
> > >   	}
> > > -	return NULL;
> > > +	return "unknown";
> > 
> > This issue is a common issue crossing all archs.  So it's better to
> > change the code in the places where calls perf_reg_name(), e.g. in
> > util/session.c:
> > 
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -1135,12 +1135,14 @@ static void branch_stack__printf(struct perf_sample *sample, bool callstack)
> >   static void regs_dump__printf(u64 mask, u64 *regs)
> >   {
> >          unsigned rid, i = 0;
> > +       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);
> >          }
> >   }
> > 
> > And another potential issue is the format specifier "%-5s", it prints
> > out maximum to 5 chars,
> 
> Doesn't the width field specify the min, not max, number of characters?

Thanks for correction, John.

I wrongly understood it and sorry for confusion.  Wiki says [1]:

"The Width field specifies a minimum number of characters to output,
and is typically used to pad fixed-width fields in tabulated output,
where the fields would otherwise be smaller, although it does not
cause truncation of oversized fields."

Thanks,
Leo

[1] https://en.wikipedia.org/wiki/Printf_format_string#Width_field