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 <tho...@monjalon.net>
> >>> Signed-off-by: Alexander Kozyrev <akozy...@mellanox.com>
> >>> Acked-by: Viacheslav Ovsiienko <viachesl...@mellanox.com>
> >>> Acked-by: Thomas Monjalon <tho...@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.
