Re: [dpdk-dev] [PATCH v2 1/5] mk/icc: disable treatment of warnings as errors

[Thomas Monjalon <thomas@monjalon.net>]

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.