Re: [PATCH] cryptodev: fix stringop-overflow build failure with gcc 10

[Luca Boccassi <bluca@debian.org>]

On Fri, 2021-12-17 at 12:09 +0000, Richardson, Bruce wrote:
> 
> > -----Original Message-----
> > From: Luca Boccassi <bluca@debian.org>
> > Sent: Friday, December 17, 2021 12:01 PM
> > To: christian.ehrhardt@canonical.com; stable@dpdk.org
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>; ktraynor@redhat.com;
> > Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>
> > Subject: Re: [PATCH] cryptodev: fix stringop-overflow build failure with
> > gcc 10
> > 
> > On Fri, 2021-12-17 at 10:51 +0100, christian.ehrhardt@canonical.com
> > wrote:
> > > From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > > 
> > > 19.11 LTS releases started to fail to build in some newer releases
> > > due to newer gcc-10 compilers available. Those are mostly false
> > > positives [1][2] and to some extend covered by the already present
> > > backports of "4990929c60 eal/ppc: ignore GCC 10 stringop-overflow
> > > warnings" and "df17bcb43b eal/x86: ignore gcc 10 stringop-overflow
> > > warnings".
> > > 
> > > But the combination of 19.11 and gcc 10.3.0 and LTO this can still
> > > exposes the issue.
> > > 
> > > [  549s] In function ‘_mm_storeu_si128’,
> > > [  549s]     inlined from ‘rte_memcpy_generic’
> > >   at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:508:2,
> > > [  549s]     inlined from ‘rte_cryptodev_sym_session_set_user_data’
> > >   at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:882:10:
> > > [  549s] /usr/lib/gcc/x86_64-linux-gnu/10/include/emmintrin.h:727:8:
> > >   error: writing 16 bytes into a region of size 0
> > >   [-Werror=stringop-overflow=]
> > > [  549s]   727 |   *__P = __B;
> > > [  549s]       |        ^
> > > [  549s] ../lib/librte_cryptodev/rte_cryptodev.c:
> > >   In function ‘rte_cryptodev_sym_session_set_user_data’:
> > > [  549s] ../lib/librte_cryptodev/rte_cryptodev.h:984:4: note:
> > >   at offset 0 to object ‘sess_data’ with size 0 declared here
> > > [  549s]   984 |  } sess_data[0];
> > > [  549s]       |    ^
> > > 
> > > The problem is that in this combination only at link time (LTO)
> > > the compiler will (false-)detect the problematic size of the target
> > > structure and break. To make things worse [3] fixed this problem
> > > for the same issue at actual build time, but the pragma has no
> > > effect at link time nor are any werror settings passed to the
> > > linker.
> > > 
> > > For affected compilers set -Wno-stringop-overflow globally for
> > > the linker stage to avoid this breaking the build in any place.
> > > 
> > > Buzilla ID: 908
> > > 
> > > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94335
> > > [2]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92955
> > > [3]: https://github.com/DPDK/dpdk/commit/b5b3ea803e47
> > > 
> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > > ---
> > >  config/meson.build | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/config/meson.build b/config/meson.build
> > > index 4007b8f37c..25dec320ba 100644
> > > --- a/config/meson.build
> > > +++ b/config/meson.build
> > > @@ -143,6 +143,14 @@ if numa_dep.found() and cc.has_header('numaif.h')
> > >  	dpdk_extra_ldflags += '-lnuma'
> > >  endif
> > > 
> > > 
> > > +# Some false positives can trigger at LTO stage, so this warning needs
> > to be
> > > +# disabled on the linker as well for gcc 10.
> > > +# Fixed in gcc-11 and not present <gcc-10
> > > +# https://bugs.dpdk.org/show_bug.cgi?id=908
> > > +if cc.get_id() == 'gcc' and cc.version().version_compare('>=10.0') and
> > cc.version().version_compare('<11.0')
> > > +	add_project_link_arguments('-Wno-stringop-overflow', language: 'c')
> > > +endif
> > > +
> > >  has_libfdt = 0
> > >  fdt_dep = cc.find_library('libfdt', required: false)
> > >  if fdt_dep.found() and cc.has_header('fdt.h')
> > 
> > Sounds reasonable to me. Is it possible to detect that LTO is used, to
> > further restrict when this is applied?
> > 
> 
> Does it really matter that much? This is for backport only, so won't apply for main tree, meaning that all backported patches will have been already checked with the setting enabled on the mainline. Therefore, I would look to not bother trying to limit the impact of the flag much, and just enable it in the most convenient manner.
> 
> /Bruce

Just thinking about coverage for future backports, but it's not too
important

-- 
Kind regards,
Luca Boccassi