Re: [PATCH v6 1/6] dpdk: do not force C linkage on include file dependencies

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-09-16 14:05, David Marchand wrote:
> Hello,
> 
> On Tue, Sep 10, 2024 at 10:41 AM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>> diff --git a/lib/acl/rte_acl_osdep.h b/lib/acl/rte_acl_osdep.h
>> index 3c1dc402ca..e4c7d07c69 100644
>> --- a/lib/acl/rte_acl_osdep.h
>> +++ b/lib/acl/rte_acl_osdep.h
>> @@ -5,10 +5,6 @@
>>   #ifndef _RTE_ACL_OSDEP_H_
>>   #define _RTE_ACL_OSDEP_H_
>>
>> -#ifdef __cplusplus
>> -extern "C" {
>> -#endif
>> -
>>   /**
>>    * @file
>>    *
>> @@ -49,6 +45,10 @@ extern "C" {
>>   #include <rte_cpuflags.h>
>>   #include <rte_debug.h>
>>
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>>   #ifdef __cplusplus
>>   }
>>   #endif
> 
> This part is a NOOP, so we can just drop it.
> 

I did try to drop such NOOPs, but then something called 
sanitycheckcpp.exe failed the build because it required 'extern "C"' in 
those header files.

Isn't that check superfluous? A missing 'extern "C"' would be detected 
at a later stage, when the dummy C++ programs are compiled against the 
public header files.

If we agree santifycheckcpp.exe should be fixed, is that a separate 
patch or need it be a part of this patch set?

> I found this occurence in other files of the patch.
> 
> $ git show lib/ | grep -E '^ .*__cplusplus|diff' | grep -B1
> __cplusplus | sed -ne 's/^diff --git a\/\(.*\) b\/.*$/\1/p' | while
> read file; do git show -- $file | tr '\n' ' ' | grep -q ' +#ifdef
> __cplusplus +extern "C" { +#endif +  #ifdef __cplusplus' && echo
> $file; done
> lib/acl/rte_acl_osdep.h
> lib/eal/arm/include/rte_cpuflags_32.h
> lib/eal/arm/include/rte_cpuflags_64.h
> lib/eal/arm/include/rte_power_intrinsics.h
> lib/eal/loongarch/include/rte_cpuflags.h
> lib/eal/loongarch/include/rte_power_intrinsics.h
> lib/eal/ppc/include/rte_cpuflags.h
> lib/eal/ppc/include/rte_power_intrinsics.h
> lib/eal/riscv/include/rte_cpuflags.h
> lib/eal/riscv/include/rte_power_intrinsics.h
> lib/eal/x86/include/rte_cpuflags.h
> lib/eal/x86/include/rte_power_intrinsics.h
> lib/ipsec/rte_ipsec.h
> lib/pdcp/rte_pdcp.h
> lib/ring/rte_ring_elem.h
> 
> 
>> diff --git a/lib/eal/arm/include/rte_io.h b/lib/eal/arm/include/rte_io.h
>> index f4e66e6bad..658768697c 100644
>> --- a/lib/eal/arm/include/rte_io.h
>> +++ b/lib/eal/arm/include/rte_io.h
>> @@ -5,14 +5,14 @@
>>   #ifndef _RTE_IO_ARM_H_
>>   #define _RTE_IO_ARM_H_
>>
>> -#ifdef __cplusplus
>> -extern "C" {
>> -#endif
>> -
>>   #ifdef RTE_ARCH_64
>>   #include "rte_io_64.h"
>>   #else
>>   #include "generic/rte_io.h"
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>>   #endif
> 
> I suspect it is the reason for the CI build error on ARM.
> This block should be out of the #endif, but then with the next lines,
> it ends up as a noop.
> 
>>
>>   #ifdef __cplusplus
> 
> 
>> diff --git a/lib/eal/arm/include/rte_pause.h b/lib/eal/arm/include/rte_pause.h
>> index 6c7002ad98..8f35d60a6e 100644
>> --- a/lib/eal/arm/include/rte_pause.h
>> +++ b/lib/eal/arm/include/rte_pause.h
>> @@ -5,14 +5,14 @@
>>   #ifndef _RTE_PAUSE_ARM_H_
>>   #define _RTE_PAUSE_ARM_H_
>>
>> -#ifdef __cplusplus
>> -extern "C" {
>> -#endif
>> -
>>   #ifdef RTE_ARCH_64
>>   #include <rte_pause_64.h>
>>   #else
>>   #include <rte_pause_32.h>
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>>   #endif
> 
> Idem, probably breaking build for ARM.
> 
> 
>> diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
>> index f859707744..0a4f3f8528 100644
>> --- a/lib/eal/include/generic/rte_atomic.h
>> +++ b/lib/eal/include/generic/rte_atomic.h
>> @@ -17,6 +17,10 @@
>>   #include <rte_common.h>
>>   #include <rte_stdatomic.h>
>>
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif
>> +
>>   #ifdef __DOXYGEN__
>>
>>   /** @name Memory Barrier
>> @@ -1156,4 +1160,8 @@ rte_atomic128_cmp_exchange(rte_int128_t *dst,
>>
>>   #endif /* __DOXYGEN__ */
>>
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> +
> 
> I would move under #ifdef DOXYGEN.
> 

Why? The pattern now is "almost always directly after the #includes". 
That is better than before, but not ideal. C linkage should only be 
covering functions and global variables declared, I think.

> The rest looks good to me.
> 
> 

Thanks for the help!