diff mbox series

[v2] x86/eal: gcc 10 ignore stringop-overflow warnings

Message ID 20200416184549.10747-1-ktraynor@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series [v2] x86/eal: gcc 10 ignore stringop-overflow warnings |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/travis-robot warning Travis build: failed
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Kevin Traynor April 16, 2020, 6:45 p.m. UTC 
  stringop-overflow warns when it sees a possible overflow
in a string operation.

In the rte_memcpy functions different branches are taken
depending on the size. stringop-overflow is raised for the
branches in the function where it sees the static size of the
src could be overflowed.

However, in reality a correct size argument and in some cases
dynamic allocation would ensure that this does not happen.

For example, in the case below for key, the correct path will be
chosen in rte_memcpy_generic at runtime based on the size argument
but as some paths in the function could lead to a cast to 32 bytes
a warning is raised.

In function ‘_mm256_storeu_si256’,
inlined from ‘rte_memcpy_generic’
at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:315:2,
inlined from ‘iavf_configure_rss_key’
at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:869:10:

/usr/lib/gcc/x86_64-redhat-linux/10/include/avxintrin.h:928:8:
warning: writing 32 bytes into a region of size 1 [-Wstringop-overflow=]
  928 |   *__P = __A;
      |   ~~~~~^~~~~
In file included
from ../drivers/net/iavf/../../common/iavf/iavf_prototype.h:10,
from ../drivers/net/iavf/iavf.h:9,
from ../drivers/net/iavf/iavf_vchnl.c:22:

../drivers/net/iavf/iavf_vchnl.c:
In function ‘iavf_configure_rss_key’:

../drivers/net/iavf/../../common/iavf/virtchnl.h:508:5:
note: at offset 0 to object ‘key’ with size 1 declared here
  508 |  u8 key[1];         /* RSS hash key, packed bytes */
      |     ^~~

Ignore the stringop-overflow warnings for rte_memcpy.h functions.

Bugzilla ID: 394
Bugzilla ID: 421

Signed-off-by: Kevin Traynor <ktraynor@redhat.com>

---

v2: Change from a global disable to just disabling for x86/rte_memcpy.h
---
 lib/librte_eal/x86/include/rte_memcpy.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Bruce Richardson April 17, 2020, 9:33 a.m. UTC | #1 
On Thu, Apr 16, 2020 at 07:45:49PM +0100, Kevin Traynor wrote:
> stringop-overflow warns when it sees a possible overflow
> in a string operation.
> 
> In the rte_memcpy functions different branches are taken
> depending on the size. stringop-overflow is raised for the
> branches in the function where it sees the static size of the
> src could be overflowed.
> 
> However, in reality a correct size argument and in some cases
> dynamic allocation would ensure that this does not happen.
> 
> For example, in the case below for key, the correct path will be
> chosen in rte_memcpy_generic at runtime based on the size argument
> but as some paths in the function could lead to a cast to 32 bytes
> a warning is raised.
> 
> In function ‘_mm256_storeu_si256’,
> inlined from ‘rte_memcpy_generic’
> at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:315:2,
> inlined from ‘iavf_configure_rss_key’
> at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:869:10:
> 
> /usr/lib/gcc/x86_64-redhat-linux/10/include/avxintrin.h:928:8:
> warning: writing 32 bytes into a region of size 1 [-Wstringop-overflow=]
>   928 |   *__P = __A;
>       |   ~~~~~^~~~~
> In file included
> from ../drivers/net/iavf/../../common/iavf/iavf_prototype.h:10,
> from ../drivers/net/iavf/iavf.h:9,
> from ../drivers/net/iavf/iavf_vchnl.c:22:
> 
> ../drivers/net/iavf/iavf_vchnl.c:
> In function ‘iavf_configure_rss_key’:
> 
> ../drivers/net/iavf/../../common/iavf/virtchnl.h:508:5:
> note: at offset 0 to object ‘key’ with size 1 declared here
>   508 |  u8 key[1];         /* RSS hash key, packed bytes */
>       |     ^~~
> 
> Ignore the stringop-overflow warnings for rte_memcpy.h functions.
> 
> Bugzilla ID: 394
> Bugzilla ID: 421
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> 
> ---
> 
> v2: Change from a global disable to just disabling for x86/rte_memcpy.h
> ---
>  lib/librte_eal/x86/include/rte_memcpy.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_eal/x86/include/rte_memcpy.h b/lib/librte_eal/x86/include/rte_memcpy.h
> index ba44c4a32..283fb79ba 100644
> --- a/lib/librte_eal/x86/include/rte_memcpy.h
> +++ b/lib/librte_eal/x86/include/rte_memcpy.h
> @@ -23,4 +23,8 @@ extern "C" {
>  #endif
>  
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100000)
> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
> +#endif
> +
>  /**
>   * Copy bytes from one location to another. The locations must not overlap.

Does this permanently need to be disabled for all compilation units
including rte_memcpy.h, or can it be used with a push/pop set of pragmas to
only disable for the required functions?
Thomas Monjalon April 17, 2020, 10:13 a.m. UTC | #2 
17/04/2020 11:33, Bruce Richardson:
> On Thu, Apr 16, 2020 at 07:45:49PM +0100, Kevin Traynor wrote:
> > stringop-overflow warns when it sees a possible overflow
> > in a string operation.
> > 
> > In the rte_memcpy functions different branches are taken
> > depending on the size. stringop-overflow is raised for the
> > branches in the function where it sees the static size of the
> > src could be overflowed.
> > 
> > However, in reality a correct size argument and in some cases
> > dynamic allocation would ensure that this does not happen.
> > 
> > For example, in the case below for key, the correct path will be
> > chosen in rte_memcpy_generic at runtime based on the size argument
> > but as some paths in the function could lead to a cast to 32 bytes
> > a warning is raised.
> > 
> > In function ‘_mm256_storeu_si256’,
> > inlined from ‘rte_memcpy_generic’
> > at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:315:2,
> > inlined from ‘iavf_configure_rss_key’
> > at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:869:10:
> > 
> > /usr/lib/gcc/x86_64-redhat-linux/10/include/avxintrin.h:928:8:
> > warning: writing 32 bytes into a region of size 1 [-Wstringop-overflow=]
> >   928 |   *__P = __A;
> >       |   ~~~~~^~~~~
> > In file included
> > from ../drivers/net/iavf/../../common/iavf/iavf_prototype.h:10,
> > from ../drivers/net/iavf/iavf.h:9,
> > from ../drivers/net/iavf/iavf_vchnl.c:22:
> > 
> > ../drivers/net/iavf/iavf_vchnl.c:
> > In function ‘iavf_configure_rss_key’:
> > 
> > ../drivers/net/iavf/../../common/iavf/virtchnl.h:508:5:
> > note: at offset 0 to object ‘key’ with size 1 declared here
> >   508 |  u8 key[1];         /* RSS hash key, packed bytes */
> >       |     ^~~
> > 
> > Ignore the stringop-overflow warnings for rte_memcpy.h functions.
> > 
> > Bugzilla ID: 394
> > Bugzilla ID: 421
> > 
> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
[...]
> > --- a/lib/librte_eal/x86/include/rte_memcpy.h
> > +++ b/lib/librte_eal/x86/include/rte_memcpy.h
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100000)
> > +#pragma GCC diagnostic ignored "-Wstringop-overflow"
> > +#endif
> 
> Does this permanently need to be disabled for all compilation units
> including rte_memcpy.h, or can it be used with a push/pop set of pragmas to
> only disable for the required functions?

Even better, isn't there a solution in memcpy code?
Kevin Traynor April 17, 2020, 12:40 p.m. UTC | #3 
On 17/04/2020 10:33, Bruce Richardson wrote:
> On Thu, Apr 16, 2020 at 07:45:49PM +0100, Kevin Traynor wrote:
>> stringop-overflow warns when it sees a possible overflow
>> in a string operation.
>>
>> In the rte_memcpy functions different branches are taken
>> depending on the size. stringop-overflow is raised for the
>> branches in the function where it sees the static size of the
>> src could be overflowed.
>>
>> However, in reality a correct size argument and in some cases
>> dynamic allocation would ensure that this does not happen.
>>
>> For example, in the case below for key, the correct path will be
>> chosen in rte_memcpy_generic at runtime based on the size argument
>> but as some paths in the function could lead to a cast to 32 bytes
>> a warning is raised.
>>
>> In function ‘_mm256_storeu_si256’,
>> inlined from ‘rte_memcpy_generic’
>> at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:315:2,
>> inlined from ‘iavf_configure_rss_key’
>> at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:869:10:
>>
>> /usr/lib/gcc/x86_64-redhat-linux/10/include/avxintrin.h:928:8:
>> warning: writing 32 bytes into a region of size 1 [-Wstringop-overflow=]
>>   928 |   *__P = __A;
>>       |   ~~~~~^~~~~
>> In file included
>> from ../drivers/net/iavf/../../common/iavf/iavf_prototype.h:10,
>> from ../drivers/net/iavf/iavf.h:9,
>> from ../drivers/net/iavf/iavf_vchnl.c:22:
>>
>> ../drivers/net/iavf/iavf_vchnl.c:
>> In function ‘iavf_configure_rss_key’:
>>
>> ../drivers/net/iavf/../../common/iavf/virtchnl.h:508:5:
>> note: at offset 0 to object ‘key’ with size 1 declared here
>>   508 |  u8 key[1];         /* RSS hash key, packed bytes */
>>       |     ^~~
>>
>> Ignore the stringop-overflow warnings for rte_memcpy.h functions.
>>
>> Bugzilla ID: 394
>> Bugzilla ID: 421
>>
>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>>
>> ---
>>
>> v2: Change from a global disable to just disabling for x86/rte_memcpy.h
>> ---
>>  lib/librte_eal/x86/include/rte_memcpy.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/lib/librte_eal/x86/include/rte_memcpy.h b/lib/librte_eal/x86/include/rte_memcpy.h
>> index ba44c4a32..283fb79ba 100644
>> --- a/lib/librte_eal/x86/include/rte_memcpy.h
>> +++ b/lib/librte_eal/x86/include/rte_memcpy.h
>> @@ -23,4 +23,8 @@ extern "C" {
>>  #endif
>>  
>> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100000)
>> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
>> +#endif
>> +
>>  /**
>>   * Copy bytes from one location to another. The locations must not overlap.
> 
> Does this permanently need to be disabled for all compilation units
> including rte_memcpy.h, or can it be used with a push/pop set of pragmas to
> only disable for the required functions?
> 

Good point about compilation units. I'm not sure it makes sense to do
per function as the only ones that won't need it are the mov64/128/256
that are just wrappers for mov16/32 etc. Every function in rte_memcpy.h
that uses intrinsics and the aligned/generic will likely need it, which
is almost all of them.

With the GCC version conditional wrappers along with multiple
implementations depending on CPUFLAGS per function becomes messy and
harder to test. Considering that adding push/pop for the file only
increases the scope to those wrapper functions, I think it is better to
push/pop for the file.
Kevin Traynor April 17, 2020, 2:50 p.m. UTC | #4 
On 17/04/2020 11:13, Thomas Monjalon wrote:
> 17/04/2020 11:33, Bruce Richardson:
>> On Thu, Apr 16, 2020 at 07:45:49PM +0100, Kevin Traynor wrote:
>>> stringop-overflow warns when it sees a possible overflow
>>> in a string operation.
>>>
>>> In the rte_memcpy functions different branches are taken
>>> depending on the size. stringop-overflow is raised for the
>>> branches in the function where it sees the static size of the
>>> src could be overflowed.
>>>
>>> However, in reality a correct size argument and in some cases
>>> dynamic allocation would ensure that this does not happen.
>>>
>>> For example, in the case below for key, the correct path will be
>>> chosen in rte_memcpy_generic at runtime based on the size argument
>>> but as some paths in the function could lead to a cast to 32 bytes
>>> a warning is raised.
>>>
>>> In function ‘_mm256_storeu_si256’,
>>> inlined from ‘rte_memcpy_generic’
>>> at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:315:2,
>>> inlined from ‘iavf_configure_rss_key’
>>> at ../lib/librte_eal/common/include/arch/x86/rte_memcpy.h:869:10:
>>>
>>> /usr/lib/gcc/x86_64-redhat-linux/10/include/avxintrin.h:928:8:
>>> warning: writing 32 bytes into a region of size 1 [-Wstringop-overflow=]
>>>   928 |   *__P = __A;
>>>       |   ~~~~~^~~~~
>>> In file included
>>> from ../drivers/net/iavf/../../common/iavf/iavf_prototype.h:10,
>>> from ../drivers/net/iavf/iavf.h:9,
>>> from ../drivers/net/iavf/iavf_vchnl.c:22:
>>>
>>> ../drivers/net/iavf/iavf_vchnl.c:
>>> In function ‘iavf_configure_rss_key’:
>>>
>>> ../drivers/net/iavf/../../common/iavf/virtchnl.h:508:5:
>>> note: at offset 0 to object ‘key’ with size 1 declared here
>>>   508 |  u8 key[1];         /* RSS hash key, packed bytes */
>>>       |     ^~~
>>>
>>> Ignore the stringop-overflow warnings for rte_memcpy.h functions.
>>>
>>> Bugzilla ID: 394
>>> Bugzilla ID: 421
>>>
>>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
> [...]
>>> --- a/lib/librte_eal/x86/include/rte_memcpy.h
>>> +++ b/lib/librte_eal/x86/include/rte_memcpy.h
>>> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100000)
>>> +#pragma GCC diagnostic ignored "-Wstringop-overflow"
>>> +#endif
>>
>> Does this permanently need to be disabled for all compilation units
>> including rte_memcpy.h, or can it be used with a push/pop set of pragmas to
>> only disable for the required functions?
> 
> Even better, isn't there a solution in memcpy code?
> 
> 

There may be, it could do with fresh eyes - Bruce/Konstantin?

In the meantime I will send a v3 with push/pop so it is available as an
option.
diff mbox series

Patch

diff --git a/lib/librte_eal/x86/include/rte_memcpy.h b/lib/librte_eal/x86/include/rte_memcpy.h
index ba44c4a32..283fb79ba 100644
--- a/lib/librte_eal/x86/include/rte_memcpy.h
+++ b/lib/librte_eal/x86/include/rte_memcpy.h
@@ -23,4 +23,8 @@  extern "C" {
 #endif
 
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 100000)
+#pragma GCC diagnostic ignored "-Wstringop-overflow"
+#endif
+
 /**
  * Copy bytes from one location to another. The locations must not overlap.