patches for DPDK stable branches
 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
                   ` (2 more replies)
  0 siblings, 3 replies; 26+ 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] 26+ 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
  2024-03-18  2:40 ` [PATCH v2 " Ashish Sadanandan
  2024-03-18  2:44 ` [PATCH v3 " Ashish Sadanandan
  2 siblings, 1 reply; 26+ 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] 26+ 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; 26+ 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] 26+ 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-03-13 23:45       ` Stephen Hemminger
  2024-02-05 17:36     ` Tyler Retzlaff
  2024-02-12 15:42     ` Ferruh Yigit
  2 siblings, 1 reply; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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
  2024-03-13 20:26             ` Ashish Sadanandan
  0 siblings, 1 reply; 26+ 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] 26+ messages in thread

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

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

On Mon, Feb 12, 2024 at 9:02 AM Morten Brørup <mb@smartsharesystems.com>
wrote:

> > 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.
>

Was the intent of this comment that I should include this update in my
patch? I'm happy to do it, but IMO the guideline update should be a
separate commit.

It's been a month since the last activity on this thread, does someone need
to sign off on this change before it can be merged?

Thanks,
Ashish

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

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

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

13/03/2024 21:26, Ashish Sadanandan:
> On Mon, Feb 12, 2024 at 9:02 AM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> 
> > > 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.
> >
> 
> Was the intent of this comment that I should include this update in my
> patch? I'm happy to do it, but IMO the guideline update should be a
> separate commit.
> 
> It's been a month since the last activity on this thread, does someone need
> to sign off on this change before it can be merged?

Instead of doing one specific change, it would be better to change all files for consistency.
So the guideline change can be in the same commit applying the new guideline.





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

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

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

On Wed, Mar 13, 2024 at 2:45 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 13/03/2024 21:26, Ashish Sadanandan:
> > On Mon, Feb 12, 2024 at 9:02 AM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> >
> > > > 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.
> > >
> >
> > Was the intent of this comment that I should include this update in my
> > patch? I'm happy to do it, but IMO the guideline update should be a
> > separate commit.
> >
> > It's been a month since the last activity on this thread, does someone
> need
> > to sign off on this change before it can be merged?
>
> Instead of doing one specific change, it would be better to change all
> files for consistency.
> So the guideline change can be in the same commit applying the new
> guideline.
>
>
>  Fair enough, I'll take a crack at it tonight.

- Ashish

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

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

* Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-02-02 20:58     ` Ashish Sadanandan
@ 2024-03-13 23:45       ` Stephen Hemminger
  2024-03-14  3:45         ` Tyler Retzlaff
  0 siblings, 1 reply; 26+ messages in thread
From: Stephen Hemminger @ 2024-03-13 23:45 UTC (permalink / raw)
  To: Ashish Sadanandan
  Cc: Bruce Richardson, Thomas Monjalon, dev, nelio.laranjeiro, stable,
	honnappa.nagarahalli, konstantin.v.ananyev, david.marchand,
	ruifeng.wang

On Fri, 2 Feb 2024 13:58:19 -0700
Ashish Sadanandan <ashish.sadanandan@gmail.com> wrote:

> > 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

But splashing this across all the internal driver headers is bad idea.
It should only apply to header files that exported in final package.

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

* Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-03-13 23:45       ` Stephen Hemminger
@ 2024-03-14  3:45         ` Tyler Retzlaff
  0 siblings, 0 replies; 26+ messages in thread
From: Tyler Retzlaff @ 2024-03-14  3:45 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Ashish Sadanandan, Bruce Richardson, Thomas Monjalon, dev,
	nelio.laranjeiro, stable, honnappa.nagarahalli,
	konstantin.v.ananyev, david.marchand, ruifeng.wang

On Wed, Mar 13, 2024 at 04:45:36PM -0700, Stephen Hemminger wrote:
> On Fri, 2 Feb 2024 13:58:19 -0700
> Ashish Sadanandan <ashish.sadanandan@gmail.com> wrote:
> 
> > > 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
> 
> But splashing this across all the internal driver headers is bad idea.
> It should only apply to header files that exported in final package.

while we don't provide api/abi stability promises for driver headers we
do optionally install them with -Denable_driver_sdk=true.

the driver sdk allows drivers to be developed outside of the dpdk source
tree, many such drivers are explicitly authored in C++ and are outside of
the dpdk source tree because dpdk does not allow C++ drivers in tree.

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

* [PATCH v2 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-03-18  2:40 ` Ashish Sadanandan
  2024-03-18  2:44 ` [PATCH v3 " Ashish Sadanandan
  2 siblings, 0 replies; 26+ messages in thread
From: Ashish Sadanandan @ 2024-03-18  2:40 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.

Also updated the coding style contribution guideline with a new
"Language Linkage" section stating `extern "C"` block should be added to
public headers.

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 +-
 doc/guides/contributing/coding_style.rst | 21 +++++++++++++++++++++
 lib/eal/include/generic/rte_vect.h       |  8 ++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 50726e1232..24de59ba89 100644
--- a/.mailmap
+++ b/.mailmap
@@ -142,7 +142,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/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
index 1ebc79ca3c..da57cc1f80 100644
--- a/doc/guides/contributing/coding_style.rst
+++ b/doc/guides/contributing/coding_style.rst
@@ -106,6 +106,27 @@ Headers should be protected against multiple inclusion with the usual:
 
    #endif /* _FILE_H_ */
 
+Language Linkage
+~~~~~~~~~~~~~~~~
+
+Public headers should enclose all function and variable declarations and defintions in an ``extern "C"`` block to facilitate interoperability with C++.
+
+.. code-block:: c
+
+   #ifndef _FILE_H_
+   #define _FILE_H_
+
+   #ifdef __cplusplus
+   extern "C" {
+   #endif
+
+   /* Code */
+
+   #ifdef __cplusplus
+   }
+   #endif
+
+   #endif /* _FILE_H_ */
 
 Macros
 ~~~~~~
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] 26+ messages in thread

* [PATCH v3 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-03-18  2:40 ` [PATCH v2 " Ashish Sadanandan
@ 2024-03-18  2:44 ` Ashish Sadanandan
  2024-04-02 16:03   ` Ashish Sadanandan
                     ` (2 more replies)
  2 siblings, 3 replies; 26+ messages in thread
From: Ashish Sadanandan @ 2024-03-18  2:44 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.

Also updated the coding style contribution guideline with a new
"Language Linkage" section stating `extern "C"` block should be added to
public headers.

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 +-
 doc/guides/contributing/coding_style.rst | 21 +++++++++++++++++++++
 lib/eal/include/generic/rte_vect.h       |  8 ++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 50726e1232..24de59ba89 100644
--- a/.mailmap
+++ b/.mailmap
@@ -142,7 +142,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/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
index 1ebc79ca3c..27c947fcec 100644
--- a/doc/guides/contributing/coding_style.rst
+++ b/doc/guides/contributing/coding_style.rst
@@ -106,6 +106,27 @@ Headers should be protected against multiple inclusion with the usual:
 
    #endif /* _FILE_H_ */
 
+Language Linkage
+~~~~~~~~~~~~~~~~
+
+Public headers should enclose all function and variable declarations and definitions in an ``extern "C"`` block to facilitate interoperability with C++.
+
+.. code-block:: c
+
+   #ifndef _FILE_H_
+   #define _FILE_H_
+
+   #ifdef __cplusplus
+   extern "C" {
+   #endif
+
+   /* Code */
+
+   #ifdef __cplusplus
+   }
+   #endif
+
+   #endif /* _FILE_H_ */
 
 Macros
 ~~~~~~
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] 26+ messages in thread

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-03-18  2:44 ` [PATCH v3 " Ashish Sadanandan
@ 2024-04-02 16:03   ` Ashish Sadanandan
  2024-04-03 14:52     ` Thomas Monjalon
  2024-04-02 16:10   ` Bruce Richardson
  2024-04-02 16:19   ` Tyler Retzlaff
  2 siblings, 1 reply; 26+ messages in thread
From: Ashish Sadanandan @ 2024-04-02 16:03 UTC (permalink / raw)
  To: dev, Thomas Monjalon; +Cc: nelio.laranjeiro, stable

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

Hi everyone,
I've made the updates as suggested. Could someone please review the latest
patchset? Not sure if I followed the new patchset instructions correctly,
I've always had trouble with that part.

Thanks,
Ashish.

On Sun, Mar 17, 2024 at 8:44 PM Ashish Sadanandan <
ashish.sadanandan@gmail.com> wrote:

> 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.
>
> Also updated the coding style contribution guideline with a new
> "Language Linkage" section stating `extern "C"` block should be added to
> public headers.
>
> 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 +-
>  doc/guides/contributing/coding_style.rst | 21 +++++++++++++++++++++
>  lib/eal/include/generic/rte_vect.h       |  8 ++++++++
>  3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index 50726e1232..24de59ba89 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -142,7 +142,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/doc/guides/contributing/coding_style.rst
> b/doc/guides/contributing/coding_style.rst
> index 1ebc79ca3c..27c947fcec 100644
> --- a/doc/guides/contributing/coding_style.rst
> +++ b/doc/guides/contributing/coding_style.rst
> @@ -106,6 +106,27 @@ Headers should be protected against multiple
> inclusion with the usual:
>
>     #endif /* _FILE_H_ */
>
> +Language Linkage
> +~~~~~~~~~~~~~~~~
> +
> +Public headers should enclose all function and variable declarations and
> definitions in an ``extern "C"`` block to facilitate interoperability with
> C++.
> +
> +.. code-block:: c
> +
> +   #ifndef _FILE_H_
> +   #define _FILE_H_
> +
> +   #ifdef __cplusplus
> +   extern "C" {
> +   #endif
> +
> +   /* Code */
> +
> +   #ifdef __cplusplus
> +   }
> +   #endif
> +
> +   #endif /* _FILE_H_ */
>
>  Macros
>  ~~~~~~
> 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
>
>

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

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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-03-18  2:44 ` [PATCH v3 " Ashish Sadanandan
  2024-04-02 16:03   ` Ashish Sadanandan
@ 2024-04-02 16:10   ` Bruce Richardson
  2024-04-02 16:19   ` Tyler Retzlaff
  2 siblings, 0 replies; 26+ messages in thread
From: Bruce Richardson @ 2024-04-02 16:10 UTC (permalink / raw)
  To: Ashish Sadanandan; +Cc: dev, Thomas Monjalon, nelio.laranjeiro, stable

On Sun, Mar 17, 2024 at 08:44:15PM -0600, Ashish Sadanandan wrote:
> 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.
> 
> Also updated the coding style contribution guideline with a new
> "Language Linkage" section stating `extern "C"` block should be added to
> public headers.
> 
> 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>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-03-18  2:44 ` [PATCH v3 " Ashish Sadanandan
  2024-04-02 16:03   ` Ashish Sadanandan
  2024-04-02 16:10   ` Bruce Richardson
@ 2024-04-02 16:19   ` Tyler Retzlaff
  2024-10-07 20:20     ` Stephen Hemminger
  2 siblings, 1 reply; 26+ messages in thread
From: Tyler Retzlaff @ 2024-04-02 16:19 UTC (permalink / raw)
  To: Ashish Sadanandan; +Cc: dev, Thomas Monjalon, nelio.laranjeiro, stable

On Sun, Mar 17, 2024 at 08:44:15PM -0600, Ashish Sadanandan wrote:
> 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.
> 
> Also updated the coding style contribution guideline with a new
> "Language Linkage" section stating `extern "C"` block should be added to
> public headers.
> 
> 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>
> ---
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>


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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-04-02 16:03   ` Ashish Sadanandan
@ 2024-04-03 14:52     ` Thomas Monjalon
  2024-04-07  1:30       ` Ashish Sadanandan
  0 siblings, 1 reply; 26+ messages in thread
From: Thomas Monjalon @ 2024-04-03 14:52 UTC (permalink / raw)
  To: Ashish Sadanandan; +Cc: dev, nelio.laranjeiro, stable

02/04/2024 18:03, Ashish Sadanandan:
> Hi everyone,
> I've made the updates as suggested. Could someone please review the latest
> patchset? Not sure if I followed the new patchset instructions correctly,
> I've always had trouble with that part.

I remember we were discussing about aligning all files.
I was waiting for a patch applying the rule we discussed.


> On Sun, Mar 17, 2024 at 8:44 PM Ashish Sadanandan <
> ashish.sadanandan@gmail.com> wrote:
> 
> > 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.
> >
> > Also updated the coding style contribution guideline with a new
> > "Language Linkage" section stating `extern "C"` block should be added to
> > public headers.
> >
> > 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 +-
> >  doc/guides/contributing/coding_style.rst | 21 +++++++++++++++++++++
> >  lib/eal/include/generic/rte_vect.h       |  8 ++++++++
> >  3 files changed, 30 insertions(+), 1 deletion(-)
> >
> > --- a/doc/guides/contributing/coding_style.rst
> > +++ b/doc/guides/contributing/coding_style.rst
> > +Language Linkage
> > +~~~~~~~~~~~~~~~~
> > +
> > +Public headers should enclose all function and variable declarations and
> > definitions in an ``extern "C"`` block to facilitate interoperability with
> > C++.
> > +
> > +.. code-block:: c
> > +
> > +   #ifndef _FILE_H_
> > +   #define _FILE_H_
> > +
> > +   #ifdef __cplusplus
> > +   extern "C" {
> > +   #endif
> > +
> > +   /* Code */
> > +
> > +   #ifdef __cplusplus
> > +   }
> > +   #endif
> > +
> > +   #endif /* _FILE_H_ */

This is not describing where the includes should be placed.



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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-04-03 14:52     ` Thomas Monjalon
@ 2024-04-07  1:30       ` Ashish Sadanandan
  2024-04-07 17:04         ` Stephen Hemminger
                           ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Ashish Sadanandan @ 2024-04-07  1:30 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, nelio.laranjeiro, stable

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

On Wed, Apr 3, 2024 at 8:52 AM Thomas Monjalon <thomas@monjalon.net> wrote:

> 02/04/2024 18:03, Ashish Sadanandan:
> > Hi everyone,
> > I've made the updates as suggested. Could someone please review the
> latest
> > patchset? Not sure if I followed the new patchset instructions correctly,
> > I've always had trouble with that part.
>
> I remember we were discussing about aligning all files.
> I was waiting for a patch applying the rule we discussed.
>
> I missed the part where people were volunteering me for additional work :)

The consensus seems to be that the extern "C" directives should only be in
public headers, not private ones. Can you please tell me if there's an easy
way to get a list of public headers?


> > On Sun, Mar 17, 2024 at 8:44 PM Ashish Sadanandan <
> > ashish.sadanandan@gmail.com> wrote:
> >
> > > 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.
> > >
> > > Also updated the coding style contribution guideline with a new
> > > "Language Linkage" section stating `extern "C"` block should be added
> to
> > > public headers.
> > >
> > > 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 +-
> > >  doc/guides/contributing/coding_style.rst | 21 +++++++++++++++++++++
> > >  lib/eal/include/generic/rte_vect.h       |  8 ++++++++
> > >  3 files changed, 30 insertions(+), 1 deletion(-)
> > >
> > > --- a/doc/guides/contributing/coding_style.rst
> > > +++ b/doc/guides/contributing/coding_style.rst
> > > +Language Linkage
> > > +~~~~~~~~~~~~~~~~
> > > +
> > > +Public headers should enclose all function and variable declarations
> and
> > > definitions in an ``extern "C"`` block to facilitate interoperability
> with
> > > C++.
> > > +
> > > +.. code-block:: c
> > > +
> > > +   #ifndef _FILE_H_
> > > +   #define _FILE_H_
> > > +
> > > +   #ifdef __cplusplus
> > > +   extern "C" {
> > > +   #endif
> > > +
> > > +   /* Code */
> > > +
> > > +   #ifdef __cplusplus
> > > +   }
> > > +   #endif
> > > +
> > > +   #endif /* _FILE_H_ */
>
> This is not describing where the includes should be placed.
>

Will address this along with the rest of the headers.

Regards,
Ashish.

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

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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-04-07  1:30       ` Ashish Sadanandan
@ 2024-04-07 17:04         ` Stephen Hemminger
  2024-04-08  8:50         ` Ferruh Yigit
  2024-04-08 15:29         ` Tyler Retzlaff
  2 siblings, 0 replies; 26+ messages in thread
From: Stephen Hemminger @ 2024-04-07 17:04 UTC (permalink / raw)
  To: Ashish Sadanandan; +Cc: Thomas Monjalon, dev, nelio.laranjeiro, stable

On Sat, 6 Apr 2024 19:30:38 -0600
Ashish Sadanandan <ashish.sadanandan@gmail.com> wrote:

> On Wed, Apr 3, 2024 at 8:52 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 02/04/2024 18:03, Ashish Sadanandan:  
> > > Hi everyone,
> > > I've made the updates as suggested. Could someone please review the  
> > latest  
> > > patchset? Not sure if I followed the new patchset instructions correctly,
> > > I've always had trouble with that part.  
> >
> > I remember we were discussing about aligning all files.
> > I was waiting for a patch applying the rule we discussed.
> >
> > I missed the part where people were volunteering me for additional work :)  
> 
> The consensus seems to be that the extern "C" directives should only be in
> public headers, not private ones. Can you please tell me if there's an easy
> way to get a list of public headers?

The issue for private headers that Tyler mentioned is that MS has private
DPDK PMD's that are written in C++. Not sure if this needs to be considered
or supported.

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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-04-07  1:30       ` Ashish Sadanandan
  2024-04-07 17:04         ` Stephen Hemminger
@ 2024-04-08  8:50         ` Ferruh Yigit
  2024-04-08  9:04           ` Bruce Richardson
  2024-04-08 15:29         ` Tyler Retzlaff
  2 siblings, 1 reply; 26+ messages in thread
From: Ferruh Yigit @ 2024-04-08  8:50 UTC (permalink / raw)
  To: Ashish Sadanandan, Thomas Monjalon; +Cc: dev, nelio.laranjeiro, stable

On 4/7/2024 2:30 AM, Ashish Sadanandan wrote:
> 
> 
> On Wed, Apr 3, 2024 at 8:52 AM Thomas Monjalon <thomas@monjalon.net
> <mailto:thomas@monjalon.net>> wrote:
> 
>     02/04/2024 18:03, Ashish Sadanandan:
>     > Hi everyone,
>     > I've made the updates as suggested. Could someone please review
>     the latest
>     > patchset? Not sure if I followed the new patchset instructions
>     correctly,
>     > I've always had trouble with that part.
> 
>     I remember we were discussing about aligning all files.
>     I was waiting for a patch applying the rule we discussed.
> 
> I missed the part where people were volunteering me for additional work :)
> 
> The consensus seems to be that the extern "C" directives should only be
> in public headers, not private ones. Can you please tell me if there's
> an easy way to get a list of public headers? 
> 

Public headers are installed as part of ninja install target, so one
option is you can install a build to a custom folder and get the list
from there.
Or they are listed in meson.build files in 'headers' variable, you can
parse them.


Don't get confused with 'driver_sdk_headers' variable in meson files,
that is the list of headers required for driver development, and
installed if sdk ('enable_driver_sdk') meson option is enabled.

> 
>     > On Sun, Mar 17, 2024 at 8:44 PM Ashish Sadanandan <
>     > ashish.sadanandan@gmail.com <mailto:ashish.sadanandan@gmail.com>>
>     wrote:
>     >
>     > > 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.
>     > >
>     > > Also updated the coding style contribution guideline with a new
>     > > "Language Linkage" section stating `extern "C"` block should be
>     added to
>     > > public headers.
>     > >
>     > > Fixes: 86c743cf9140 ("eal: define generic vector types")
>     > > Cc: nelio.laranjeiro@6wind.com <mailto:nelio.laranjeiro@6wind.com>
>     > > Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>     > >
>     > > Signed-off-by: Ashish Sadanandan <ashish.sadanandan@gmail.com
>     <mailto:ashish.sadanandan@gmail.com>>
>     > > ---
>     > >  .mailmap                                 |  2 +-
>     > >  doc/guides/contributing/coding_style.rst | 21 +++++++++++++++++++++
>     > >  lib/eal/include/generic/rte_vect.h       |  8 ++++++++
>     > >  3 files changed, 30 insertions(+), 1 deletion(-)
>     > >
>     > > --- a/doc/guides/contributing/coding_style.rst
>     > > +++ b/doc/guides/contributing/coding_style.rst
>     > > +Language Linkage
>     > > +~~~~~~~~~~~~~~~~
>     > > +
>     > > +Public headers should enclose all function and variable
>     declarations and
>     > > definitions in an ``extern "C"`` block to facilitate
>     interoperability with
>     > > C++.
>     > > +
>     > > +.. code-block:: c
>     > > +
>     > > +   #ifndef _FILE_H_
>     > > +   #define _FILE_H_
>     > > +
>     > > +   #ifdef __cplusplus
>     > > +   extern "C" {
>     > > +   #endif
>     > > +
>     > > +   /* Code */
>     > > +
>     > > +   #ifdef __cplusplus
>     > > +   }
>     > > +   #endif
>     > > +
>     > > +   #endif /* _FILE_H_ */
> 
>     This is not describing where the includes should be placed.
>      
> 
> Will address this along with the rest of the headers.
> 
> Regards,
> Ashish.


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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-04-08  8:50         ` Ferruh Yigit
@ 2024-04-08  9:04           ` Bruce Richardson
  0 siblings, 0 replies; 26+ messages in thread
From: Bruce Richardson @ 2024-04-08  9:04 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Ashish Sadanandan, Thomas Monjalon, dev, nelio.laranjeiro, stable

On Mon, Apr 08, 2024 at 09:50:51AM +0100, Ferruh Yigit wrote:
> On 4/7/2024 2:30 AM, Ashish Sadanandan wrote:
> > 
> > 
> > On Wed, Apr 3, 2024 at 8:52 AM Thomas Monjalon <thomas@monjalon.net
> > <mailto:thomas@monjalon.net>> wrote:
> > 
> >     02/04/2024 18:03, Ashish Sadanandan:
> >     > Hi everyone,
> >     > I've made the updates as suggested. Could someone please review
> >     the latest
> >     > patchset? Not sure if I followed the new patchset instructions
> >     correctly,
> >     > I've always had trouble with that part.
> > 
> >     I remember we were discussing about aligning all files.
> >     I was waiting for a patch applying the rule we discussed.
> > 
> > I missed the part where people were volunteering me for additional work :)
> > 
> > The consensus seems to be that the extern "C" directives should only be
> > in public headers, not private ones. Can you please tell me if there's
> > an easy way to get a list of public headers? 
> > 
> 
> Public headers are installed as part of ninja install target, so one
> option is you can install a build to a custom folder and get the list
> from there.
> Or they are listed in meson.build files in 'headers' variable, you can
> parse them.
> 
> 
> Don't get confused with 'driver_sdk_headers' variable in meson files,
> that is the list of headers required for driver development, and
> installed if sdk ('enable_driver_sdk') meson option is enabled.
> 

I still don't see what the major issue is with just adding the guards to
all headers in DPDK. It would avoid any complications of which headers get
installed or when. They're stripped out on pre-processing, so it's not like
they are going to have a performance impact or anything.

/Bruce

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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-04-07  1:30       ` Ashish Sadanandan
  2024-04-07 17:04         ` Stephen Hemminger
  2024-04-08  8:50         ` Ferruh Yigit
@ 2024-04-08 15:29         ` Tyler Retzlaff
  2 siblings, 0 replies; 26+ messages in thread
From: Tyler Retzlaff @ 2024-04-08 15:29 UTC (permalink / raw)
  To: Ashish Sadanandan; +Cc: Thomas Monjalon, dev, nelio.laranjeiro, stable

On Sat, Apr 06, 2024 at 07:30:38PM -0600, Ashish Sadanandan wrote:
> On Wed, Apr 3, 2024 at 8:52 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 02/04/2024 18:03, Ashish Sadanandan:
> > > Hi everyone,
> > > I've made the updates as suggested. Could someone please review the
> > latest
> > > patchset? Not sure if I followed the new patchset instructions correctly,
> > > I've always had trouble with that part.
> >
> > I remember we were discussing about aligning all files.
> > I was waiting for a patch applying the rule we discussed.
> >
> > I missed the part where people were volunteering me for additional work :)
> 
> The consensus seems to be that the extern "C" directives should only be in
> public headers, not private ones. Can you please tell me if there's an easy
> way to get a list of public headers?

build with driver_sdk_headers enabled.
run the meson install target.

all headers that are installed need the guard.

> 
> 
> > > On Sun, Mar 17, 2024 at 8:44 PM Ashish Sadanandan <
> > > ashish.sadanandan@gmail.com> wrote:
> > >
> > > > 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.
> > > >
> > > > Also updated the coding style contribution guideline with a new
> > > > "Language Linkage" section stating `extern "C"` block should be added
> > to
> > > > public headers.
> > > >
> > > > 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 +-
> > > >  doc/guides/contributing/coding_style.rst | 21 +++++++++++++++++++++
> > > >  lib/eal/include/generic/rte_vect.h       |  8 ++++++++
> > > >  3 files changed, 30 insertions(+), 1 deletion(-)
> > > >
> > > > --- a/doc/guides/contributing/coding_style.rst
> > > > +++ b/doc/guides/contributing/coding_style.rst
> > > > +Language Linkage
> > > > +~~~~~~~~~~~~~~~~
> > > > +
> > > > +Public headers should enclose all function and variable declarations
> > and
> > > > definitions in an ``extern "C"`` block to facilitate interoperability
> > with
> > > > C++.
> > > > +
> > > > +.. code-block:: c
> > > > +
> > > > +   #ifndef _FILE_H_
> > > > +   #define _FILE_H_
> > > > +
> > > > +   #ifdef __cplusplus
> > > > +   extern "C" {
> > > > +   #endif
> > > > +
> > > > +   /* Code */
> > > > +
> > > > +   #ifdef __cplusplus
> > > > +   }
> > > > +   #endif
> > > > +
> > > > +   #endif /* _FILE_H_ */
> >
> > This is not describing where the includes should be placed.
> >
> 
> Will address this along with the rest of the headers.
> 
> Regards,
> Ashish.

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

* Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h
  2024-04-02 16:19   ` Tyler Retzlaff
@ 2024-10-07 20:20     ` Stephen Hemminger
  0 siblings, 0 replies; 26+ messages in thread
From: Stephen Hemminger @ 2024-10-07 20:20 UTC (permalink / raw)
  To: Tyler Retzlaff
  Cc: Ashish Sadanandan, dev, Thomas Monjalon, nelio.laranjeiro, stable

On Tue, 2 Apr 2024 09:19:23 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Sun, Mar 17, 2024 at 08:44:15PM -0600, Ashish Sadanandan wrote:
> > 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.
> > 
> > Also updated the coding style contribution guideline with a new
> > "Language Linkage" section stating `extern "C"` block should be added to
> > public headers.
> > 
> > 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>
> > ---  
> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

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

end of thread, other threads:[~2024-10-07 20:20 UTC | newest]

Thread overview: 26+ 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-03-13 23:45       ` Stephen Hemminger
2024-03-14  3:45         ` Tyler Retzlaff
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-03-13 20:26             ` Ashish Sadanandan
2024-03-13 20:45               ` Thomas Monjalon
2024-03-13 22:11                 ` Ashish Sadanandan
2024-02-12 15:42     ` Ferruh Yigit
2024-03-18  2:40 ` [PATCH v2 " Ashish Sadanandan
2024-03-18  2:44 ` [PATCH v3 " Ashish Sadanandan
2024-04-02 16:03   ` Ashish Sadanandan
2024-04-03 14:52     ` Thomas Monjalon
2024-04-07  1:30       ` Ashish Sadanandan
2024-04-07 17:04         ` Stephen Hemminger
2024-04-08  8:50         ` Ferruh Yigit
2024-04-08  9:04           ` Bruce Richardson
2024-04-08 15:29         ` Tyler Retzlaff
2024-04-02 16:10   ` Bruce Richardson
2024-04-02 16:19   ` Tyler Retzlaff
2024-10-07 20:20     ` Stephen Hemminger

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).