[PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

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

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Thomas Monjalon]

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?

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Bruce Richardson]

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

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Ashish Sadanandan]

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

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Tyler Retzlaff]

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

RE: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Morten Brørup]

> 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

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Ferruh Yigit]

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

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Ferruh Yigit]

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?

RE: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Morten Brørup]

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

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Ashish Sadanandan]

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

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Thomas Monjalon]

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.

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Ashish Sadanandan]

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

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Stephen Hemminger]

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.

Re: [PATCH 1/1] eal: add C++ include guard in generic/rte_vect.h

[Tyler Retzlaff]

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.

[PATCH v2 1/1] eal: add C++ include guard in generic/rte_vect.h

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

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

[PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

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

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

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Ashish Sadanandan]

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

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Bruce Richardson]

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>

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Tyler Retzlaff]

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>

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Thomas Monjalon]

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.

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Ashish Sadanandan]

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

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Stephen Hemminger]

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.

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Ferruh Yigit]

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.

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Bruce Richardson]

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

Re: [PATCH v3 1/1] eal: add C++ include guard in generic/rte_vect.h

[Tyler Retzlaff]

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.