Re: [dpdk-dev] [PATCH v2] x86/eal: gcc 10 ignore stringop-overflow warnings

[Kevin Traynor <ktraynor@redhat.com>]

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.