[v2,1/5] mk/icc: disable treatment of warnings as errors

  Remove -Werror-all flag in ICC configuration file to stop treating ICC
warnings as errors in DPDK due to many false positives. We are using
GCC and Clang as a benchmark for warnings anyway for simplification.

Suggested-by: Thomas Monjalon <thomas@monjalon.net>
Signed-off-by: Alexander Kozyrev <akozyrev@mellanox.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 mk/toolchain/icc/rte.vars.mk | 4 ----
 1 file changed, 4 deletions(-)
  

On 1/23/2020 6:20 PM, Alexander Kozyrev wrote:
> Remove -Werror-all flag in ICC configuration file to stop treating ICC
> warnings as errors in DPDK due to many false positives. We are using
> GCC and Clang as a benchmark for warnings anyway for simplification.
> 
> Suggested-by: Thomas Monjalon <thomas@monjalon.net>
> Signed-off-by: Alexander Kozyrev <akozyrev@mellanox.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  mk/toolchain/icc/rte.vars.mk | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk
> index 8aa87aa..1729f3d 100644
> --- a/mk/toolchain/icc/rte.vars.mk
> +++ b/mk/toolchain/icc/rte.vars.mk
> @@ -47,10 +47,6 @@ WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527
>  WERROR_FLAGS += -diag-disable 188
>  WERROR_FLAGS += -diag-disable 11074 -diag-disable 11076 -Wdeprecated
>  
> -ifeq ($(RTE_DEVEL_BUILD),y)
> -WERROR_FLAGS += -Werror-all
> -endif
> -

Not sure about removing this globally, as of now the ICC builds fine. If this is
for the coming changes in mlx, why not disable warnings in mlx driver only?
  

24/01/2020 17:36, Ferruh Yigit:
> On 1/23/2020 6:20 PM, Alexander Kozyrev wrote:
> > Remove -Werror-all flag in ICC configuration file to stop treating ICC
> > warnings as errors in DPDK due to many false positives. We are using
> > GCC and Clang as a benchmark for warnings anyway for simplification.
> > 
> > Suggested-by: Thomas Monjalon <thomas@monjalon.net>
> > Signed-off-by: Alexander Kozyrev <akozyrev@mellanox.com>
> > Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> >  mk/toolchain/icc/rte.vars.mk | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk
> > index 8aa87aa..1729f3d 100644
> > --- a/mk/toolchain/icc/rte.vars.mk
> > +++ b/mk/toolchain/icc/rte.vars.mk
> > @@ -47,10 +47,6 @@ WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527
> >  WERROR_FLAGS += -diag-disable 188
> >  WERROR_FLAGS += -diag-disable 11074 -diag-disable 11076 -Wdeprecated
> >  
> > -ifeq ($(RTE_DEVEL_BUILD),y)
> > -WERROR_FLAGS += -Werror-all
> > -endif
> 
> Not sure about removing this globally, as of now the ICC builds fine. If this is
> for the coming changes in mlx, why not disable warnings in mlx driver only?

Adding special handling for ICC in the driver means it is kind of supported.
ICC support is on best-effort basis as a favor to Intel and its CI.

I don't see any good reason to spend time disabling ICC warnings each time
we hit a false positive.
And I would like we stop doing strange things in the code to make ICC happy.
We do not need to support ICC, or at least we do not need to make ICC build
warning-free.

This patch will solve all future annoying issues with ICC in the future.

Now let me ask the reverse question: why the flag -Werror-all is set for ICC?
We set this flag for gcc and clang because we use gcc and clang as static
code analyzers (like coverity) and we want to be sure to catch any issue.
ICC is not used as code analyzer, this is just an optimization for some
Intel projects. I won't elaborate on the packaging and licensing of ICC...

I hope you will be convinced to acknowledge this change.
  

On 1/24/2020 7:37 PM, Thomas Monjalon wrote:
> 24/01/2020 17:36, Ferruh Yigit:
>> On 1/23/2020 6:20 PM, Alexander Kozyrev wrote:
>>> Remove -Werror-all flag in ICC configuration file to stop treating ICC
>>> warnings as errors in DPDK due to many false positives. We are using
>>> GCC and Clang as a benchmark for warnings anyway for simplification.
>>>
>>> Suggested-by: Thomas Monjalon <thomas@monjalon.net>
>>> Signed-off-by: Alexander Kozyrev <akozyrev@mellanox.com>
>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>>> ---
>>>  mk/toolchain/icc/rte.vars.mk | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk
>>> index 8aa87aa..1729f3d 100644
>>> --- a/mk/toolchain/icc/rte.vars.mk
>>> +++ b/mk/toolchain/icc/rte.vars.mk
>>> @@ -47,10 +47,6 @@ WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527
>>>  WERROR_FLAGS += -diag-disable 188
>>>  WERROR_FLAGS += -diag-disable 11074 -diag-disable 11076 -Wdeprecated
>>>  
>>> -ifeq ($(RTE_DEVEL_BUILD),y)
>>> -WERROR_FLAGS += -Werror-all
>>> -endif
>>
>> Not sure about removing this globally, as of now the ICC builds fine. If this is
>> for the coming changes in mlx, why not disable warnings in mlx driver only?
> 
> Adding special handling for ICC in the driver means it is kind of supported.
> ICC support is on best-effort basis as a favor to Intel and its CI.
> 
> I don't see any good reason to spend time disabling ICC warnings each time
> we hit a false positive.
> And I would like we stop doing strange things in the code to make ICC happy.
> We do not need to support ICC, or at least we do not need to make ICC build
> warning-free.
> 
> This patch will solve all future annoying issues with ICC in the future.
> 
> Now let me ask the reverse question: why the flag -Werror-all is set for ICC?
> We set this flag for gcc and clang because we use gcc and clang as static
> code analyzers (like coverity) and we want to be sure to catch any issue.
> ICC is not used as code analyzer, this is just an optimization for some
> Intel projects. I won't elaborate on the packaging and licensing of ICC...
> 
> I hope you will be convinced to acknowledge this change.
> 

To support the ICC or not is a valid question, but for me it is better to
support more compilers in case different ones catch an additional issues that is
a benefit for us, although I agree false positives are annoying, which is not
limited to icc.

"-Werror-all" is forced in DEVEL_BUILD, in this cause I was assuming to force
developer to fix the warning to increase the code quality, independent from
compiler.

As of now, all DPDK compiles with icc without warning. But we are allowing the
icc warnings just because the warnings with the change in the mlx driver. And
when we let is once, it is for sure warnings will increase by time.

If we support ICC, I am for keeping this flag and support it properly.
  

27/01/2020 16:37, Ferruh Yigit:
> On 1/24/2020 7:37 PM, Thomas Monjalon wrote:
> > 24/01/2020 17:36, Ferruh Yigit:
> >> On 1/23/2020 6:20 PM, Alexander Kozyrev wrote:
> >>> Remove -Werror-all flag in ICC configuration file to stop treating ICC
> >>> warnings as errors in DPDK due to many false positives. We are using
> >>> GCC and Clang as a benchmark for warnings anyway for simplification.
> >>>
> >>> Suggested-by: Thomas Monjalon <thomas@monjalon.net>
> >>> Signed-off-by: Alexander Kozyrev <akozyrev@mellanox.com>
> >>> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> >>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> >>> ---
> >>>  mk/toolchain/icc/rte.vars.mk | 4 ----
> >>>  1 file changed, 4 deletions(-)
> >>>
> >>> diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk
> >>> index 8aa87aa..1729f3d 100644
> >>> --- a/mk/toolchain/icc/rte.vars.mk
> >>> +++ b/mk/toolchain/icc/rte.vars.mk
> >>> @@ -47,10 +47,6 @@ WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527
> >>>  WERROR_FLAGS += -diag-disable 188
> >>>  WERROR_FLAGS += -diag-disable 11074 -diag-disable 11076 -Wdeprecated
> >>>  
> >>> -ifeq ($(RTE_DEVEL_BUILD),y)
> >>> -WERROR_FLAGS += -Werror-all
> >>> -endif
> >>
> >> Not sure about removing this globally, as of now the ICC builds fine. If this is
> >> for the coming changes in mlx, why not disable warnings in mlx driver only?
> > 
> > Adding special handling for ICC in the driver means it is kind of supported.
> > ICC support is on best-effort basis as a favor to Intel and its CI.
> > 
> > I don't see any good reason to spend time disabling ICC warnings each time
> > we hit a false positive.
> > And I would like we stop doing strange things in the code to make ICC happy.
> > We do not need to support ICC, or at least we do not need to make ICC build
> > warning-free.
> > 
> > This patch will solve all future annoying issues with ICC in the future.
> > 
> > Now let me ask the reverse question: why the flag -Werror-all is set for ICC?
> > We set this flag for gcc and clang because we use gcc and clang as static
> > code analyzers (like coverity) and we want to be sure to catch any issue.
> > ICC is not used as code analyzer, this is just an optimization for some
> > Intel projects. I won't elaborate on the packaging and licensing of ICC...
> > 
> > I hope you will be convinced to acknowledge this change.
> > 
> 
> To support the ICC or not is a valid question, but for me it is better to
> support more compilers in case different ones catch an additional issues that is
> a benefit for us, although I agree false positives are annoying, which is not
> limited to icc.
> 
> "-Werror-all" is forced in DEVEL_BUILD, in this cause I was assuming to force
> developer to fix the warning to increase the code quality, independent from
> compiler.

I doubt that ICC can help to increase code quality.
And it is really annoying to fix any ICC warning,
because it is not a truly open compiler.

> As of now, all DPDK compiles with icc without warning.

There is no warning currently because we avoid some known patterns
that ICC does not like. I don't see this fact as an advantage.

> But we are allowing the
> icc warnings just because the warnings with the change in the mlx driver. And
> when we let is once, it is for sure warnings will increase by time.

Yes, this is the intent of this patch: let the ICC warnings grow.

> If we support ICC, I am for keeping this flag and support it properly.

We do not really support ICC.
See the first item of the requirements:
	http://doc.dpdk.org/guides/linux_gsg/sys_reqs.html#compilation-of-the-dpdk
	"supported C compiler such as gcc (version 4.9+) or clang (version 3.4+)"
and the consensus in the techboard:
	http://mails.dpdk.org/archives/dev/2019-June/135847.html
	"Agreement that all DPDK PMDs themselves must be possible to
	 compile with a DPDK supported compiler - GCC and/or clang"

I add the Technical Board as Cc, in case a confirmation is required.