[dpdk-dev,01/10] mk: adjust gcc flags for new gcc 7 warnings

  There are two new warnings in GCC 7 that cause problems in the DPDK
compile.

1. GCC now warns if you have a switch fall-through without a suitable
comment indicating that it was intentional. The compiler supports a number
of levels of warning which are triggered depending on the type of message
used, with level 3 being the default. To accept a wider range of possible
fall-through messages, we adjust this down to level 2.

2. GCC also warns about an snprintf where there may be truncation and the
return value is not checked. Given that we often use snprintf in DPDK in
place of strncpy, and in many cases where truncation is not a problem, we
can just disable this particular warning.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 mk/toolchain/gcc/rte.vars.mk | 7 +++++++
 1 file changed, 7 insertions(+)
  

On Thu,  4 May 2017 16:38:13 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> There are two new warnings in GCC 7 that cause problems in the DPDK
> compile.
> 
> 1. GCC now warns if you have a switch fall-through without a suitable
> comment indicating that it was intentional. The compiler supports a number
> of levels of warning which are triggered depending on the type of message
> used, with level 3 being the default. To accept a wider range of possible
> fall-through messages, we adjust this down to level 2.
> 
> 2. GCC also warns about an snprintf where there may be truncation and the
> return value is not checked. Given that we often use snprintf in DPDK in
> place of strncpy, and in many cases where truncation is not a problem, we
> can just disable this particular warning.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  mk/toolchain/gcc/rte.vars.mk | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
> index 5caa600..3834e00 100644
> --- a/mk/toolchain/gcc/rte.vars.mk
> +++ b/mk/toolchain/gcc/rte.vars.mk
> @@ -99,5 +99,12 @@ ifeq ($(shell test $(GCC_VERSION) -lt 47 && echo 1), 1)
>  WERROR_FLAGS += -Wno-uninitialized
>  endif
>  
> +ifeq ($(shell test $(GCC_VERSION) -gt 70 && echo 1), 1)
> +# Tell GCC only to error for switch fallthroughs without a suitable comment
> +WERROR_FLAGS += -Wimplicit-fallthrough=2
> +# Ignore errors for snprintf truncation
> +WERROR_FLAGS += -Wno-format-truncation
> +endif
> +
>  export CC AS AR LD OBJCOPY OBJDUMP STRIP READELF
>  export TOOLCHAIN_CFLAGS TOOLCHAIN_LDFLAGS TOOLCHAIN_ASFLAGS

Please fix the code not neuter  warnings
  

On Thu, May 04, 2017 at 09:38:08AM -0700, Stephen Hemminger wrote:
> On Thu,  4 May 2017 16:38:13 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > There are two new warnings in GCC 7 that cause problems in the DPDK
> > compile.
> > 
> > 1. GCC now warns if you have a switch fall-through without a suitable
> > comment indicating that it was intentional. The compiler supports a number
> > of levels of warning which are triggered depending on the type of message
> > used, with level 3 being the default. To accept a wider range of possible
> > fall-through messages, we adjust this down to level 2.
> > 
> > 2. GCC also warns about an snprintf where there may be truncation and the
> > return value is not checked. Given that we often use snprintf in DPDK in
> > place of strncpy, and in many cases where truncation is not a problem, we
> > can just disable this particular warning.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  mk/toolchain/gcc/rte.vars.mk | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
> > index 5caa600..3834e00 100644
> > --- a/mk/toolchain/gcc/rte.vars.mk
> > +++ b/mk/toolchain/gcc/rte.vars.mk
> > @@ -99,5 +99,12 @@ ifeq ($(shell test $(GCC_VERSION) -lt 47 && echo 1), 1)
> >  WERROR_FLAGS += -Wno-uninitialized
> >  endif
> >  
> > +ifeq ($(shell test $(GCC_VERSION) -gt 70 && echo 1), 1)
> > +# Tell GCC only to error for switch fallthroughs without a suitable comment
> > +WERROR_FLAGS += -Wimplicit-fallthrough=2
> > +# Ignore errors for snprintf truncation
> > +WERROR_FLAGS += -Wno-format-truncation
> > +endif
> > +
> >  export CC AS AR LD OBJCOPY OBJDUMP STRIP READELF
> >  export TOOLCHAIN_CFLAGS TOOLCHAIN_LDFLAGS TOOLCHAIN_ASFLAGS
> 
> Please fix the code not neuter  warnings

As much as is possible I agree. However, for these two warnings:

1. the implicit fallthrough warning is not disabled, it's just adjusted
to allow a wider range of comments for fall-through to be accepted. The
rest of the set implements fixes for a number of these warnings in the
code.

2. for the format truncation warning, ideally, yes we should fix the
code, except that I don't believe this is feasible in the short term,
and I also don't believe it is desirable. We extensively use snprintf
because it has sane/safe truncation, and in many cases we don't care if
it is being truncated. Therefore disabling the warning seems the best
approach to me. Furthermore, if we want 17.05 to compile with GCC 7,
this is the best option within that timeframe.

Regards,
/Bruce
  

In this series, there are some fixes for fall-through comments,
missing break and missing initializers.
I think there is no discussion about accepting them in 17.05.
The last item to discuss it the new snprintf warning:

05/05/2017 11:42, Bruce Richardson:
> On Thu, May 04, 2017 at 09:38:08AM -0700, Stephen Hemminger wrote:
> > On Thu,  4 May 2017 16:38:13 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > 2. GCC also warns about an snprintf where there may be truncation and the
> > > return value is not checked. Given that we often use snprintf in DPDK in
> > > place of strncpy, and in many cases where truncation is not a problem, we
> > > can just disable this particular warning.
[...]
> > > --- a/mk/toolchain/gcc/rte.vars.mk
> > > +++ b/mk/toolchain/gcc/rte.vars.mk
> > > +# Ignore errors for snprintf truncation
> > > +WERROR_FLAGS += -Wno-format-truncation
[...]
> 2. for the format truncation warning, ideally, yes we should fix the
> code, except that I don't believe this is feasible in the short term,
> and I also don't believe it is desirable. We extensively use snprintf
> because it has sane/safe truncation, and in many cases we don't care if
> it is being truncated. Therefore disabling the warning seems the best
> approach to me. Furthermore, if we want 17.05 to compile with GCC 7,
> this is the best option within that timeframe.

We could imagine an explicit ignore of the return code.
However, do we really want this new coding rule for every snprintf?
It is a common call in DPDK:
	git grep '\<snprintf\>' | wc -l
	774
And probably almost never checked:
	git grep '^[[:space:]]*\<snprintf\>' | wc -l
	660

I suggest to disable this new warning in GCC 7.
Any opinions?
  

On Fri, May 05, 2017 at 12:02:44PM +0200, Thomas Monjalon wrote:
> In this series, there are some fixes for fall-through comments,
> missing break and missing initializers.
> I think there is no discussion about accepting them in 17.05.
> The last item to discuss it the new snprintf warning:
> 
> 05/05/2017 11:42, Bruce Richardson:
> > On Thu, May 04, 2017 at 09:38:08AM -0700, Stephen Hemminger wrote:
> > > On Thu,  4 May 2017 16:38:13 +0100
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > > 2. GCC also warns about an snprintf where there may be truncation and the
> > > > return value is not checked. Given that we often use snprintf in DPDK in
> > > > place of strncpy, and in many cases where truncation is not a problem, we
> > > > can just disable this particular warning.
> [...]
> > > > --- a/mk/toolchain/gcc/rte.vars.mk
> > > > +++ b/mk/toolchain/gcc/rte.vars.mk
> > > > +# Ignore errors for snprintf truncation
> > > > +WERROR_FLAGS += -Wno-format-truncation
> [...]
> > 2. for the format truncation warning, ideally, yes we should fix the
> > code, except that I don't believe this is feasible in the short term,
> > and I also don't believe it is desirable. We extensively use snprintf
> > because it has sane/safe truncation, and in many cases we don't care if
> > it is being truncated. Therefore disabling the warning seems the best
> > approach to me. Furthermore, if we want 17.05 to compile with GCC 7,
> > this is the best option within that timeframe.
> 
> We could imagine an explicit ignore of the return code.
> However, do we really want this new coding rule for every snprintf?
> It is a common call in DPDK:
> 	git grep '\<snprintf\>' | wc -l
> 	774
> And probably almost never checked:
> 	git grep '^[[:space:]]*\<snprintf\>' | wc -l
> 	660
> 
> I suggest to disable this new warning in GCC 7.
> Any opinions?

I'd suggest that even fewer than that are actually recorded, let alone
checked:

git grep '= *snprintf\>' | wc -l
89

So I'm (obviously) +1 for dropping this warning check.

/Bruce
  

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Friday, May 5, 2017 11:03 AM
> To: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Subject: Re: [dpdk-dev] [PATCH 01/10] mk: adjust gcc flags for new gcc 7 warnings
> 
> In this series, there are some fixes for fall-through comments,
> missing break and missing initializers.
> I think there is no discussion about accepting them in 17.05.
> The last item to discuss it the new snprintf warning:
> 
> 05/05/2017 11:42, Bruce Richardson:
> > On Thu, May 04, 2017 at 09:38:08AM -0700, Stephen Hemminger wrote:
> > > On Thu,  4 May 2017 16:38:13 +0100
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > > 2. GCC also warns about an snprintf where there may be truncation and the
> > > > return value is not checked. Given that we often use snprintf in DPDK in
> > > > place of strncpy, and in many cases where truncation is not a problem, we
> > > > can just disable this particular warning.
> [...]
> > > > --- a/mk/toolchain/gcc/rte.vars.mk
> > > > +++ b/mk/toolchain/gcc/rte.vars.mk
> > > > +# Ignore errors for snprintf truncation
> > > > +WERROR_FLAGS += -Wno-format-truncation
> [...]
> > 2. for the format truncation warning, ideally, yes we should fix the
> > code, except that I don't believe this is feasible in the short term,
> > and I also don't believe it is desirable. We extensively use snprintf
> > because it has sane/safe truncation, and in many cases we don't care if
> > it is being truncated. Therefore disabling the warning seems the best
> > approach to me. Furthermore, if we want 17.05 to compile with GCC 7,
> > this is the best option within that timeframe.
> 
> We could imagine an explicit ignore of the return code.
> However, do we really want this new coding rule for every snprintf?
> It is a common call in DPDK:
> 	git grep '\<snprintf\>' | wc -l
> 	774
> And probably almost never checked:
> 	git grep '^[[:space:]]*\<snprintf\>' | wc -l
> 	660
> 
> I suggest to disable this new warning in GCC 7.
> Any opinions?

+1 to disable, it seems a pragmatic solution in this case.