DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
@ 2024-02-02  5:13 Ashish Sadanandan
  2024-02-02  9:18 ` Thomas Monjalon
  0 siblings, 1 reply; 9+ messages in thread
From: Ashish Sadanandan @ 2024-02-02  5:13 UTC (permalink / raw)
  To: dev, Thomas Monjalon; +Cc: Ashish Sadanandan, nelio.laranjeiro, stable

The header was missing the extern "C" directive which causes name
mangling of functions by C++ compilers, leading to linker errors
complaining of undefined references to these functions.

Fixes: 86c743cf9140 ("eal: define generic vector types")
Cc: nelio.laranjeiro@6wind.com
Cc: stable@dpdk.org

Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>
---
 .mailmap                           | 2 +-
 lib/eal/include/generic/rte_vect.h | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index aa569ff456..3938ace307 100644
--- a/.mailmap
+++ b/.mailmap
@@ -140,7 +140,7 @@ Ashijeet Acharya <ashijeet.acharya@6wind.com>
 Ashish Gupta <ashishg@marvell.com> <ashish.gupta@marvell.com> <ashish.gupta@caviumnetworks.com>
 Ashish Jain <ashish.jain@nxp.com>
 Ashish Paul <apaul@juniper.net>
-Ashish Sadanandan <ashish.sadanandan@gmail.com>
+Ashish Sadanandan <ashish.sadanandan@gmail.com> <quic_asadanan@quicinc.com>
 Ashish Shah <ashish.n.shah@intel.com>
 Ashwin Sekhar T K <asekhar@marvell.com> <ashwin.sekhar@caviumnetworks.com>
 Asim Jamshed <asim.jamshed@gmail.com>
diff --git a/lib/eal/include/generic/rte_vect.h b/lib/eal/include/generic/rte_vect.h
index 6540419cd2..3578d8749b 100644
--- a/lib/eal/include/generic/rte_vect.h
+++ b/lib/eal/include/generic/rte_vect.h
@@ -15,6 +15,10 @@
 
 #include <stdint.h>
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 #ifndef RTE_TOOLCHAIN_MSVC
 
 /* Unsigned vector types */
@@ -226,4 +230,8 @@ uint16_t rte_vect_get_max_simd_bitwidth(void);
  */
 int rte_vect_set_max_simd_bitwidth(uint16_t bitwidth);
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* _RTE_VECT_H_ */
-- 
2.31.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-02  5:13 [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h Ashish Sadanandan
@ 2024-02-02  9:18 ` Thomas Monjalon
  2024-02-02  9:40   ` Bruce Richardson
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Monjalon @ 2024-02-02  9:18 UTC (permalink / raw)
  To: Ashish Sadanandan
  Cc: dev, nelio.laranjeiro, stable, bruce.richardson,
	honnappa.nagarahalli, konstantin.v.ananyev, david.marchand,
	ruifeng.wang

02/02/2024 06:13, Ashish Sadanandan:
> The header was missing the extern "C" directive which causes name
> mangling of functions by C++ compilers, leading to linker errors
> complaining of undefined references to these functions.
> 
> Fixes: 86c743cf9140 ("eal: define generic vector types")
> Cc: nelio.laranjeiro@6wind.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>

Thank you for improving C++ compatibility.

I'm not sure what is best to fix it.
You are adding extern "C" in a file which is not directly included
by the user app. The same was done for rte_rwlock.h.
The other way is to make sure this include is in an extern "C" block
in lib/eal/*/include/rte_vect.h (instead of being before the block).

I would like we use the same approach for all files.
Opinions?



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-02  9:18 ` Thomas Monjalon
@ 2024-02-02  9:40   ` Bruce Richardson
  2024-02-02 20:58     ` Ashish Sadanandan
                       ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Bruce Richardson @ 2024-02-02  9:40 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Ashish Sadanandan, dev, nelio.laranjeiro, stable,
	honnappa.nagarahalli, konstantin.v.ananyev, david.marchand,
	ruifeng.wang

On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thomas Monjalon wrote:
> 02/02/2024 06:13, Ashish Sadanandan:
> > The header was missing the extern "C" directive which causes name
> > mangling of functions by C++ compilers, leading to linker errors
> > complaining of undefined references to these functions.
> > 
> > Fixes: 86c743cf9140 ("eal: define generic vector types")
> > Cc: nelio.laranjeiro@6wind.com
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>
> 
> Thank you for improving C++ compatibility.
> 
> I'm not sure what is best to fix it.
> You are adding extern "C" in a file which is not directly included
> by the user app. The same was done for rte_rwlock.h.
> The other way is to make sure this include is in an extern "C" block
> in lib/eal/*/include/rte_vect.h (instead of being before the block).
> 
> I would like we use the same approach for all files.
> Opinions?
> 
I think just having the extern "C" guard in all files is the safest choice,
because it's immediately obvious in each and every file that it is correct.
Taking the other option, to check any indirect include file you need to go
finding what other files include it and check there that a) they have
include guards and b) the include for the indirect header is contained
within it.

Adopting the policy of putting the guard in each and every header is also a
lot easier to do basic automated sanity checks on. If the file ends in .h,
we just use grep to quickly verify it's not missing the guards. [Naturally,
we can do more complete checks than that if we want, but 99% percent of
misses can be picked up by a grep for the 'extern "C"' bit]

/Bruce

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-02  9:40   ` Bruce Richardson
@ 2024-02-02 20:58     ` Ashish Sadanandan
  2024-02-05 17:36     ` Tyler Retzlaff
  2024-02-12 15:42     ` Ferruh Yigit
  2 siblings, 0 replies; 9+ messages in thread
From: Ashish Sadanandan @ 2024-02-02 20:58 UTC (permalink / raw)
  To: Bruce Richardson
  Cc: Thomas Monjalon, dev, nelio.laranjeiro, stable,
	honnappa.nagarahalli, konstantin.v.ananyev, david.marchand,
	ruifeng.wang

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

On Fri, Feb 2, 2024 at 2:41 AM Bruce Richardson <bruce.richardson@intel.com>
wrote:

> On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thomas Monjalon wrote:
> > 02/02/2024 06:13, Ashish Sadanandan:
> > > The header was missing the extern "C" directive which causes name
> > > mangling of functions by C++ compilers, leading to linker errors
> > > complaining of undefined references to these functions.
> > >
> > > Fixes: 86c743cf9140 ("eal: define generic vector types")
> > > Cc: nelio.laranjeiro@6wind.com
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>
> >
> > Thank you for improving C++ compatibility.
> >
> > I'm not sure what is best to fix it.
> > You are adding extern "C" in a file which is not directly included
> > by the user app. The same was done for rte_rwlock.h.
> > The other way is to make sure this include is in an extern "C" block
> > in lib/eal/*/include/rte_vect.h (instead of being before the block).
> >
> > I would like we use the same approach for all files.
> > Opinions?
> >
> I think just having the extern "C" guard in all files is the safest choice,
> because it's immediately obvious in each and every file that it is correct.
> Taking the other option, to check any indirect include file you need to go
> finding what other files include it and check there that a) they have
> include guards and b) the include for the indirect header is contained
> within it.
>
> Adopting the policy of putting the guard in each and every header is also a
> lot easier to do basic automated sanity checks on. If the file ends in .h,
> we just use grep to quickly verify it's not missing the guards. [Naturally,
> we can do more complete checks than that if we want, but 99% percent of
> misses can be picked up by a grep for the 'extern "C"' bit]
>
> /Bruce
>

100% agree with Bruce. It's a valid ideological argument that private
headers
don't need such safeguards, but it's difficult to enforce and easy to break
during refactoring.

- Ashish

[-- Attachment #2: Type: text/html, Size: 2853 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-02  9:40   ` Bruce Richardson
  2024-02-02 20:58     ` Ashish Sadanandan
@ 2024-02-05 17:36     ` Tyler Retzlaff
  2024-02-05 21:07       ` Morten Brørup
  2024-02-12 15:42     ` Ferruh Yigit
  2 siblings, 1 reply; 9+ messages in thread
From: Tyler Retzlaff @ 2024-02-05 17:36 UTC (permalink / raw)
  To: Bruce Richardson
  Cc: Thomas Monjalon, Ashish Sadanandan, dev, nelio.laranjeiro,
	stable, honnappa.nagarahalli, konstantin.v.ananyev,
	david.marchand, ruifeng.wang

On Fri, Feb 02, 2024 at 09:40:59AM +0000, Bruce Richardson wrote:
> On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thomas Monjalon wrote:
> > 02/02/2024 06:13, Ashish Sadanandan:
> > > The header was missing the extern "C" directive which causes name
> > > mangling of functions by C++ compilers, leading to linker errors
> > > complaining of undefined references to these functions.
> > > 
> > > Fixes: 86c743cf9140 ("eal: define generic vector types")
> > > Cc: nelio.laranjeiro@6wind.com
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>
> > 
> > Thank you for improving C++ compatibility.
> > 
> > I'm not sure what is best to fix it.
> > You are adding extern "C" in a file which is not directly included
> > by the user app. The same was done for rte_rwlock.h.
> > The other way is to make sure this include is in an extern "C" block
> > in lib/eal/*/include/rte_vect.h (instead of being before the block).
> > 
> > I would like we use the same approach for all files.
> > Opinions?
> > 
> I think just having the extern "C" guard in all files is the safest choice,
> because it's immediately obvious in each and every file that it is correct.
> Taking the other option, to check any indirect include file you need to go
> finding what other files include it and check there that a) they have
> include guards and b) the include for the indirect header is contained
> within it.
> 
> Adopting the policy of putting the guard in each and every header is also a
> lot easier to do basic automated sanity checks on. If the file ends in .h,
> we just use grep to quickly verify it's not missing the guards. [Naturally,
> we can do more complete checks than that if we want, but 99% percent of
> misses can be picked up by a grep for the 'extern "C"' bit]

so first, i agree with what you say here. but one downside i've seen
is that non-public symbols may end up as extern "C".

i've also been unsatisfied with the inconsistency of either having
includes in or outside of the guards.

a lot of dpdk headers follow this pattern.

// foo.h
#ifdef __cplusplus
extern "C" {
#endif

#include <stdio.h>

...

but some dpdk headers follow this pattern.

// foo.h
#include <stdio.h>

#ifdef __cplusplus
extern "C"
#endif

...

standard C headers include the guards so don't need to be inside the
extern "C" block. one minor annoyance with always including inside the
block is we can't reliably provide a offer a C++-only header without
doing extern "C++".

please bear in mind i do not mean to suggest implementing any dpdk in
C++ with this comment, merely that there are advantages to occasionally
offering C++-only header content to applications should we wever want
to.

in some cases for harmony between C and C++ it may be easier to
interoperate by supplying some basic C++-only headers, this may become
more important as it there appears to be increasing divergance between
the C and C++ standards and interoperability.

for full disclosure i do anticipate having to provide some small bits of
header only C++ for msvc which is why this is top of my mind.

finally, i'll also note that we could again be explicit in our intent to
declare what is extern "C" / exported by instead marking the declared names
(functions and variables) themselves in a more precise manner.

i.e.
__rte_<lib>_export // extern "C" or __declspec(dllexport) extern "C"
void some_public_symbol(void);

you'll recall we had a related discussion about symbol visibility here
which is somewhat a similar problem to being solved to symbol naming. if
we were defaulting visibility to hidden and using a single mechanism to
guarantee extern "C" linkage and public visibility exposure we could
catch all "missed" symbols for C++ without having to build as C++ and
reference the symbols.

https://mails.dpdk.org/archives/dev/2024-January/285109.html

i still intend to put forward an RFC for the discussion resulting from
that thread (just haven't had time yet).

> 
> /Bruce

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-05 17:36     ` Tyler Retzlaff
@ 2024-02-05 21:07       ` Morten Brørup
  2024-02-12 15:43         ` Ferruh Yigit
  0 siblings, 1 reply; 9+ messages in thread
From: Morten Brørup @ 2024-02-05 21:07 UTC (permalink / raw)
  To: Tyler Retzlaff, Bruce Richardson
  Cc: Thomas Monjalon, Ashish Sadanandan, dev, nelio.laranjeiro,
	stable, honnappa.nagarahalli, konstantin.v.ananyev,
	david.marchand, ruifeng.wang

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Monday, 5 February 2024 18.37
> 
> On Fri, Feb 02, 2024 at 09:40:59AM +0000, Bruce Richardson wrote:
> > On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thomas Monjalon wrote:
> > > 02/02/2024 06:13, Ashish Sadanandan:
> > > > The header was missing the extern "C" directive which causes name
> > > > mangling of functions by C++ compilers, leading to linker errors
> > > > complaining of undefined references to these functions.
> > > >
> > > > Fixes: 86c743cf9140 ("eal: define generic vector types")
> > > > Cc: nelio.laranjeiro@6wind.com
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>
> > >
> > > Thank you for improving C++ compatibility.
> > >
> > > I'm not sure what is best to fix it.
> > > You are adding extern "C" in a file which is not directly included
> > > by the user app. The same was done for rte_rwlock.h.
> > > The other way is to make sure this include is in an extern "C"
> block
> > > in lib/eal/*/include/rte_vect.h (instead of being before the
> block).
> > >
> > > I would like we use the same approach for all files.
> > > Opinions?
> > >
> > I think just having the extern "C" guard in all files is the safest
> choice,
> > because it's immediately obvious in each and every file that it is
> correct.
> > Taking the other option, to check any indirect include file you need
> to go
> > finding what other files include it and check there that a) they have
> > include guards and b) the include for the indirect header is
> contained
> > within it.
> >
> > Adopting the policy of putting the guard in each and every header is
> also a
> > lot easier to do basic automated sanity checks on. If the file ends
> in .h,
> > we just use grep to quickly verify it's not missing the guards.
> [Naturally,
> > we can do more complete checks than that if we want, but 99% percent
> of
> > misses can be picked up by a grep for the 'extern "C"' bit]
> 
> so first, i agree with what you say here. but one downside i've seen
> is that non-public symbols may end up as extern "C".
> 
> i've also been unsatisfied with the inconsistency of either having
> includes in or outside of the guards.
> 
> a lot of dpdk headers follow this pattern.
> 
> // foo.h
> #ifdef __cplusplus
> extern "C" {
> #endif
> 
> #include <stdio.h>
> 
> ...
> 
> but some dpdk headers follow this pattern.
> 
> // foo.h
> #include <stdio.h>
> 
> #ifdef __cplusplus
> extern "C"
> #endif
> 
> ...
> 
> standard C headers include the guards so don't need to be inside the
> extern "C" block. one minor annoyance with always including inside the
> block is we can't reliably provide a offer a C++-only header without
> doing extern "C++".

I would say that the first of the two above patterns is not only annoying, it is incorrect.
A DPDK header file should not change the meaning of any other header files it includes.
And although the incorrectness currently only screws up any C++ in those header files, I still consider it a bug.

> 
> please bear in mind i do not mean to suggest implementing any dpdk in
> C++ with this comment, merely that there are advantages to occasionally
> offering C++-only header content to applications should we wever want
> to.
> 
> in some cases for harmony between C and C++ it may be easier to
> interoperate by supplying some basic C++-only headers, this may become
> more important as it there appears to be increasing divergance between
> the C and C++ standards and interoperability.
> 
> for full disclosure i do anticipate having to provide some small bits
> of
> header only C++ for msvc which is why this is top of my mind.
> 
> finally, i'll also note that we could again be explicit in our intent
> to
> declare what is extern "C" / exported by instead marking the declared
> names
> (functions and variables) themselves in a more precise manner.
> 
> i.e.
> __rte_<lib>_export // extern "C" or __declspec(dllexport) extern "C"
> void some_public_symbol(void);
> 
> you'll recall we had a related discussion about symbol visibility here
> which is somewhat a similar problem to being solved to symbol naming.
> if
> we were defaulting visibility to hidden and using a single mechanism to
> guarantee extern "C" linkage and public visibility exposure we could
> catch all "missed" symbols for C++ without having to build as C++ and
> reference the symbols.
> 
> https://mails.dpdk.org/archives/dev/2024-January/285109.html
> 
> i still intend to put forward an RFC for the discussion resulting from
> that thread (just haven't had time yet).

With that RFC, please also mention if function pointers need any special/additional considerations. No need to think about it yet. :-)

> 
> >
> > /Bruce

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-02  9:40   ` Bruce Richardson
  2024-02-02 20:58     ` Ashish Sadanandan
  2024-02-05 17:36     ` Tyler Retzlaff
@ 2024-02-12 15:42     ` Ferruh Yigit
  2 siblings, 0 replies; 9+ messages in thread
From: Ferruh Yigit @ 2024-02-12 15:42 UTC (permalink / raw)
  To: Bruce Richardson, Thomas Monjalon
  Cc: Ashish Sadanandan, dev, nelio.laranjeiro, stable,
	honnappa.nagarahalli, konstantin.v.ananyev, david.marchand,
	ruifeng.wang

On 2/2/2024 9:40 AM, Bruce Richardson wrote:
> On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thomas Monjalon wrote:
>> 02/02/2024 06:13, Ashish Sadanandan:
>>> The header was missing the extern "C" directive which causes name
>>> mangling of functions by C++ compilers, leading to linker errors
>>> complaining of undefined references to these functions.
>>>
>>> Fixes: 86c743cf9140 ("eal: define generic vector types")
>>> Cc: nelio.laranjeiro@6wind.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>
>>
>> Thank you for improving C++ compatibility.
>>
>> I'm not sure what is best to fix it.
>> You are adding extern "C" in a file which is not directly included
>> by the user app. The same was done for rte_rwlock.h.
>> The other way is to make sure this include is in an extern "C" block
>> in lib/eal/*/include/rte_vect.h (instead of being before the block).
>>
>> I would like we use the same approach for all files.
>> Opinions?
>>
> I think just having the extern "C" guard in all files is the safest choice,
> because it's immediately obvious in each and every file that it is correct.
> Taking the other option, to check any indirect include file you need to go
> finding what other files include it and check there that a) they have
> include guards and b) the include for the indirect header is contained
> within it.
> 

I assume you mean all header files exposed to user (ones installed by
meson), in that case +1

> Adopting the policy of putting the guard in each and every header is also a
> lot easier to do basic automated sanity checks on. If the file ends in .h,
> we just use grep to quickly verify it's not missing the guards. [Naturally,
> we can do more complete checks than that if we want, but 99% percent of
> misses can be picked up by a grep for the 'extern "C"' bit]
> 
> /Bruce


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-05 21:07       ` Morten Brørup
@ 2024-02-12 15:43         ` Ferruh Yigit
  2024-02-12 16:02           ` Morten Brørup
  0 siblings, 1 reply; 9+ messages in thread
From: Ferruh Yigit @ 2024-02-12 15:43 UTC (permalink / raw)
  To: Morten Brørup, Tyler Retzlaff, Bruce Richardson
  Cc: Thomas Monjalon, Ashish Sadanandan, dev, nelio.laranjeiro,
	stable, honnappa.nagarahalli, konstantin.v.ananyev,
	david.marchand, ruifeng.wang

On 2/5/2024 9:07 PM, Morten Brørup wrote:
>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>> Sent: Monday, 5 February 2024 18.37
>>
>> On Fri, Feb 02, 2024 at 09:40:59AM +0000, Bruce Richardson wrote:
>>> On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thomas Monjalon wrote:
>>>> 02/02/2024 06:13, Ashish Sadanandan:
>>>>> The header was missing the extern "C" directive which causes name
>>>>> mangling of functions by C++ compilers, leading to linker errors
>>>>> complaining of undefined references to these functions.
>>>>>
>>>>> Fixes: 86c743cf9140 ("eal: define generic vector types")
>>>>> Cc: nelio.laranjeiro@6wind.com
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>
>>>>
>>>> Thank you for improving C++ compatibility.
>>>>
>>>> I'm not sure what is best to fix it.
>>>> You are adding extern "C" in a file which is not directly included
>>>> by the user app. The same was done for rte_rwlock.h.
>>>> The other way is to make sure this include is in an extern "C"
>> block
>>>> in lib/eal/*/include/rte_vect.h (instead of being before the
>> block).
>>>>
>>>> I would like we use the same approach for all files.
>>>> Opinions?
>>>>
>>> I think just having the extern "C" guard in all files is the safest
>> choice,
>>> because it's immediately obvious in each and every file that it is
>> correct.
>>> Taking the other option, to check any indirect include file you need
>> to go
>>> finding what other files include it and check there that a) they have
>>> include guards and b) the include for the indirect header is
>> contained
>>> within it.
>>>
>>> Adopting the policy of putting the guard in each and every header is
>> also a
>>> lot easier to do basic automated sanity checks on. If the file ends
>> in .h,
>>> we just use grep to quickly verify it's not missing the guards.
>> [Naturally,
>>> we can do more complete checks than that if we want, but 99% percent
>> of
>>> misses can be picked up by a grep for the 'extern "C"' bit]
>>
>> so first, i agree with what you say here. but one downside i've seen
>> is that non-public symbols may end up as extern "C".
>>
>> i've also been unsatisfied with the inconsistency of either having
>> includes in or outside of the guards.
>>
>> a lot of dpdk headers follow this pattern.
>>
>> // foo.h
>> #ifdef __cplusplus
>> extern "C" {
>> #endif
>>
>> #include <stdio.h>
>>
>> ...
>>
>> but some dpdk headers follow this pattern.
>>
>> // foo.h
>> #include <stdio.h>
>>
>> #ifdef __cplusplus
>> extern "C"
>> #endif
>>
>> ...
>>
>> standard C headers include the guards so don't need to be inside the
>> extern "C" block. one minor annoyance with always including inside the
>> block is we can't reliably provide a offer a C++-only header without
>> doing extern "C++".
> 
> I would say that the first of the two above patterns is not only annoying, it is incorrect.
> A DPDK header file should not change the meaning of any other header files it includes.
> And although the incorrectness currently only screws up any C++ in those header files, I still consider it a bug.
> 

Should we document the proper extern "C" usage somewhere?


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-12 15:43         ` Ferruh Yigit
@ 2024-02-12 16:02           ` Morten Brørup
  0 siblings, 0 replies; 9+ messages in thread
From: Morten Brørup @ 2024-02-12 16:02 UTC (permalink / raw)
  To: Ferruh Yigit, Tyler Retzlaff, Bruce Richardson
  Cc: Thomas Monjalon, Ashish Sadanandan, dev, nelio.laranjeiro,
	stable, honnappa.nagarahalli, konstantin.v.ananyev,
	david.marchand, ruifeng.wang

> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> Sent: Monday, 12 February 2024 16.43
> 
> On 2/5/2024 9:07 PM, Morten Brørup wrote:
> >> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> >> Sent: Monday, 5 February 2024 18.37
> >>
> >> On Fri, Feb 02, 2024 at 09:40:59AM +0000, Bruce Richardson wrote:
> >>> On Fri, Feb 02, 2024 at 10:18:23AM +0100, Thomas Monjalon wrote:
> >>>> 02/02/2024 06:13, Ashish Sadanandan:
> >>>>> The header was missing the extern "C" directive which causes name
> >>>>> mangling of functions by C++ compilers, leading to linker errors
> >>>>> complaining of undefined references to these functions.
> >>>>>
> >>>>> Fixes: 86c743cf9140 ("eal: define generic vector types")
> >>>>> Cc: nelio.laranjeiro@6wind.com
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com>
> >>>>
> >>>> Thank you for improving C++ compatibility.
> >>>>
> >>>> I'm not sure what is best to fix it.
> >>>> You are adding extern "C" in a file which is not directly included
> >>>> by the user app. The same was done for rte_rwlock.h.
> >>>> The other way is to make sure this include is in an extern "C"
> >> block
> >>>> in lib/eal/*/include/rte_vect.h (instead of being before the
> >> block).
> >>>>
> >>>> I would like we use the same approach for all files.
> >>>> Opinions?
> >>>>
> >>> I think just having the extern "C" guard in all files is the safest
> >> choice,
> >>> because it's immediately obvious in each and every file that it is
> >> correct.
> >>> Taking the other option, to check any indirect include file you
> need
> >> to go
> >>> finding what other files include it and check there that a) they
> have
> >>> include guards and b) the include for the indirect header is
> >> contained
> >>> within it.
> >>>
> >>> Adopting the policy of putting the guard in each and every header
> is
> >> also a
> >>> lot easier to do basic automated sanity checks on. If the file ends
> >> in .h,
> >>> we just use grep to quickly verify it's not missing the guards.
> >> [Naturally,
> >>> we can do more complete checks than that if we want, but 99%
> percent
> >> of
> >>> misses can be picked up by a grep for the 'extern "C"' bit]
> >>
> >> so first, i agree with what you say here. but one downside i've seen
> >> is that non-public symbols may end up as extern "C".
> >>
> >> i've also been unsatisfied with the inconsistency of either having
> >> includes in or outside of the guards.
> >>
> >> a lot of dpdk headers follow this pattern.
> >>
> >> // foo.h
> >> #ifdef __cplusplus
> >> extern "C" {
> >> #endif
> >>
> >> #include <stdio.h>
> >>
> >> ...
> >>
> >> but some dpdk headers follow this pattern.
> >>
> >> // foo.h
> >> #include <stdio.h>
> >>
> >> #ifdef __cplusplus
> >> extern "C"
> >> #endif
> >>
> >> ...
> >>
> >> standard C headers include the guards so don't need to be inside the
> >> extern "C" block. one minor annoyance with always including inside
> the
> >> block is we can't reliably provide a offer a C++-only header without
> >> doing extern "C++".
> >
> > I would say that the first of the two above patterns is not only
> annoying, it is incorrect.
> > A DPDK header file should not change the meaning of any other header
> files it includes.
> > And although the incorrectness currently only screws up any C++ in
> those header files, I still consider it a bug.
> >
> 
> Should we document the proper extern "C" usage somewhere?

Good point!

It could be added to § 1.4.2. Header File Guards in the Coding Style chapter of the Contributor's Guidelines.

BTW, that paragraph (and its example) should be updated to reflect that alphabetical order is preferred.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-02-12 16:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-02  5:13 [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h Ashish Sadanandan
2024-02-02  9:18 ` Thomas Monjalon
2024-02-02  9:40   ` Bruce Richardson
2024-02-02 20:58     ` Ashish Sadanandan
2024-02-05 17:36     ` Tyler Retzlaff
2024-02-05 21:07       ` Morten Brørup
2024-02-12 15:43         ` Ferruh Yigit
2024-02-12 16:02           ` Morten Brørup
2024-02-12 15:42     ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).