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

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

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?
  

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
  

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
  

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
  

> 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
  

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
  

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?
  

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

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
  

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.
  

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
  

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.
  

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.