[dpdk-dev] [RFC] toolchain specific macro expansion

[dpdk-dev] [RFC] toolchain specific macro expansion

[Tyler Retzlaff]

today rte_common.h defines common macros for use by dpdk and consuming
applications. most expansions are specific to the gcc toolchain.

example
// lib/eal/include/rte_common.h

/**
 * Hint never returning function
 */
#define __rte_noreturn __attribute__((noreturn))

there is an anticipated need rte_common.h to expose alternate
expansions for non-gcc toolchains in the future and would like
comments on how to achieve this in an agreeable manner.


option 1 add conditional compilation directly to rte_common.h

example
// lib/eal/include/rte_common.h

/**
 * Hint never returning function
 */
#ifdef RTE_TOOLCHAIN_GCC
#define __rte_noreturn __attribute__((noreturn))
#endif

#ifdef RTE_TOOLCHAIN_FOO
#define __rte_noreturn __foo_expansion_for_noreturn
#endif

represents the simplest approach technically but may be tedious to
maintain due to the number of macros and number of conditional
expansions per macro.


option 2 add toolchain specific files (follow existing platform/os
         includes pattern)

example:
// lib/eal/gcc/rte_toolchain_common.h
#define __rte_noreturn __attribute__((noreturn))

// lib/eal/include/rte_common.h
#include <rte_toolchain_common.h>

// meson.build (illustrative change)
host_toolchain = cc.get_id() # e.g. gcc

global_inc = include_directories('.', 'config',
    'lib/eal/include',
    'lib/eal/@0@/include'.format(host_machine.system()),
    'lib/eal/@0@/include'.format(arch_subdir),
    'lib/eal/@0@/include'.format(host_toolchain),
)

results in the introduction of more files that need to be
published/installed for applications but separate files per
implementation allow for easier maintainability.


we are interested in hearing about alternatives not listed here but in
the absence of other options we would propose to submit a patch
following option 2.

comments and alternatives welcome.

thanks

Re: [dpdk-dev] [RFC] toolchain specific macro expansion

[Thomas Monjalon]

23/06/2021 20:26, Tyler Retzlaff:
> today rte_common.h defines common macros for use by dpdk and consuming
> applications. most expansions are specific to the gcc toolchain.
> 
> example
> // lib/eal/include/rte_common.h
> 
> /**
>  * Hint never returning function
>  */
> #define __rte_noreturn __attribute__((noreturn))
> 
> there is an anticipated need rte_common.h to expose alternate
> expansions for non-gcc toolchains in the future and would like
> comments on how to achieve this in an agreeable manner.
> 
> 
> option 1 add conditional compilation directly to rte_common.h
> 
> example
> // lib/eal/include/rte_common.h
> 
> /**
>  * Hint never returning function
>  */
> #ifdef RTE_TOOLCHAIN_GCC
> #define __rte_noreturn __attribute__((noreturn))
> #endif
> 
> #ifdef RTE_TOOLCHAIN_FOO
> #define __rte_noreturn __foo_expansion_for_noreturn
> #endif
> 
> represents the simplest approach technically but may be tedious to
> maintain due to the number of macros and number of conditional
> expansions per macro.

Macros are simple so the option 1 is not that bad.


> option 2 add toolchain specific files (follow existing platform/os
>          includes pattern)
> 
> example:
> // lib/eal/gcc/rte_toolchain_common.h
> #define __rte_noreturn __attribute__((noreturn))

We should keep a macro in rte_common.h which triggers an explicit error
if not overriden for the toolchain.
That way we can have a single doxygen comment in rte_common.h
like it is done in lib/eal/include/generic/

> // lib/eal/include/rte_common.h
> #include <rte_toolchain_common.h>

The include may be done in the reverse order:
the toolchain-specific .h in lib/eal/gcc/include/
includes generic/rte_common.h in lib/eal/include/generic/.
If both have the same name, we keep #include <rte_common.h>

> // meson.build (illustrative change)
> host_toolchain = cc.get_id() # e.g. gcc
> 
> global_inc = include_directories('.', 'config',
>     'lib/eal/include',
>     'lib/eal/@0@/include'.format(host_machine.system()),
>     'lib/eal/@0@/include'.format(arch_subdir),
>     'lib/eal/@0@/include'.format(host_toolchain),
> )
> 
> results in the introduction of more files that need to be
> published/installed for applications but separate files per
> implementation allow for easier maintainability.

Re: [dpdk-dev] [RFC] toolchain specific macro expansion

[Tyler Retzlaff]

On Thu, Jun 24, 2021 at 08:54:49AM +0200, Thomas Monjalon wrote:
> 23/06/2021 20:26, Tyler Retzlaff:
> > today rte_common.h defines common macros for use by dpdk and consuming
> > applications. most expansions are specific to the gcc toolchain.
> > 
> > example
> > // lib/eal/include/rte_common.h
> > 
> > /**
> >  * Hint never returning function
> >  */
> > #define __rte_noreturn __attribute__((noreturn))
> > 
> > there is an anticipated need rte_common.h to expose alternate
> > expansions for non-gcc toolchains in the future and would like
> > comments on how to achieve this in an agreeable manner.
> > 
> > 
> > option 1 add conditional compilation directly to rte_common.h
> > 
> > example
> > // lib/eal/include/rte_common.h
> > 
> > /**
> >  * Hint never returning function
> >  */
> > #ifdef RTE_TOOLCHAIN_GCC
> > #define __rte_noreturn __attribute__((noreturn))
> > #endif
> > 
> > #ifdef RTE_TOOLCHAIN_FOO
> > #define __rte_noreturn __foo_expansion_for_noreturn
> > #endif
> > 
> > represents the simplest approach technically but may be tedious to
> > maintain due to the number of macros and number of conditional
> > expansions per macro.
> 
> Macros are simple so the option 1 is not that bad.

our preference for option 2 is based mainly on experience in platform
conditional compile where reviewers of patches ask us to reduce
or attempt to hide as much visible conditional compilation.  the testpmd
patch is a recent example where we get asked to reorganize code to avoid
frequency of conditional compile for windows execenv.

either way if the community prefers option 1 we'll do it that way we
look forward to seeing additional responses.

> 
> 
> > option 2 add toolchain specific files (follow existing platform/os
> >          includes pattern)
> > 
> > example:
> > // lib/eal/gcc/rte_toolchain_common.h
> > #define __rte_noreturn __attribute__((noreturn))
> 
> We should keep a macro in rte_common.h which triggers an explicit error

i think that's relatively trivial to do. rte_common.h could after
toolchain specific include do a simple test.

#ifndef __rte_no_return
#error no __rte_no_return defined for toolchain
#endif

what is harder to catch is if there is an addition of a new macro to a
toolchain specific header where there is no corresponding test added to
rte_common.h this is somewhat mitigated by standing up more ci
automation that covers the set of toolchains that may be used and to
some degree would need help from patch reviewers to catch but on balance
it is no worse than what we already have.

Re: [dpdk-dev] [RFC] toolchain specific macro expansion

[Thomas Monjalon]

24/06/2021 18:02, Tyler Retzlaff:
> On Thu, Jun 24, 2021 at 08:54:49AM +0200, Thomas Monjalon wrote:
> > 23/06/2021 20:26, Tyler Retzlaff:
> > > // lib/eal/gcc/rte_toolchain_common.h
> > > #define __rte_noreturn __attribute__((noreturn))
> > 
> > We should keep a macro in rte_common.h which triggers an explicit error
> 
> i think that's relatively trivial to do. rte_common.h could after
> toolchain specific include do a simple test.
> 
> #ifndef __rte_no_return
> #error no __rte_no_return defined for toolchain
> #endif

No I was thinking of:

/** Doxygen comment for the attribute below */
#define __rte_no_return RTE_ATTR_NOT_SUPPORTED

This way we have a documentation in a single place for the macro,
and compilation fails if it is not implemented for the toolchain.

Re: [dpdk-dev] [RFC] toolchain specific macro expansion

[Tyler Retzlaff]

On Thu, Jun 24, 2021 at 06:29:20PM +0200, Thomas Monjalon wrote:
> 24/06/2021 18:02, Tyler Retzlaff:
> > On Thu, Jun 24, 2021 at 08:54:49AM +0200, Thomas Monjalon wrote:
> > > 23/06/2021 20:26, Tyler Retzlaff:
> > > > // lib/eal/gcc/rte_toolchain_common.h
> > > > #define __rte_noreturn __attribute__((noreturn))
> > > 
> > > We should keep a macro in rte_common.h which triggers an explicit error
> > 
> > i think that's relatively trivial to do. rte_common.h could after
> > toolchain specific include do a simple test.
> > 
> > #ifndef __rte_no_return
> > #error no __rte_no_return defined for toolchain
> > #endif
> 
> No I was thinking of:
> 
> /** Doxygen comment for the attribute below */
> #define __rte_no_return RTE_ATTR_NOT_SUPPORTED

oh, didn't know about this. it sounds better.

> 
> This way we have a documentation in a single place for the macro,
> and compilation fails if it is not implemented for the toolchain.

yes, i was thinking about this.  i'm glad you suggested it because
a signle source of documentation in rte_common.h would be better than
having to maintain redundant copies.

Re: [dpdk-dev] [RFC] toolchain specific macro expansion

[Tyler Retzlaff]

On Thu, Jun 24, 2021 at 06:29:20PM +0200, Thomas Monjalon wrote:
> 24/06/2021 18:02, Tyler Retzlaff:
> > On Thu, Jun 24, 2021 at 08:54:49AM +0200, Thomas Monjalon wrote:
> > > 23/06/2021 20:26, Tyler Retzlaff:
> > > > // lib/eal/gcc/rte_toolchain_common.h
> > > > #define __rte_noreturn __attribute__((noreturn))
> > > 
> > > We should keep a macro in rte_common.h which triggers an explicit error
> > 
> > i think that's relatively trivial to do. rte_common.h could after
> > toolchain specific include do a simple test.
> > 
> > #ifndef __rte_no_return
> > #error no __rte_no_return defined for toolchain
> > #endif
> 
> No I was thinking of:
> 
> /** Doxygen comment for the attribute below */
> #define __rte_no_return RTE_ATTR_NOT_SUPPORTED

when you suggested this i thought it was something already established.
i see now that's not the case.  would you mind elaborating a little with
a complete example of the toolchain specific override and the generic
include?  i'd just like to understand completely what you were
proposing so i can try it out.

thanks

> 
> This way we have a documentation in a single place for the macro,
> and compilation fails if it is not implemented for the toolchain.
> 

Re: [dpdk-dev] [RFC] toolchain specific macro expansion

[Thomas Monjalon]

07/07/2021 22:23, Tyler Retzlaff:
> On Thu, Jun 24, 2021 at 06:29:20PM +0200, Thomas Monjalon wrote:
> > 24/06/2021 18:02, Tyler Retzlaff:
> > > On Thu, Jun 24, 2021 at 08:54:49AM +0200, Thomas Monjalon wrote:
> > > > 23/06/2021 20:26, Tyler Retzlaff:
> > > > > // lib/eal/gcc/rte_toolchain_common.h
> > > > > #define __rte_noreturn __attribute__((noreturn))
> > > > 
> > > > We should keep a macro in rte_common.h which triggers an explicit error
> > > 
> > > i think that's relatively trivial to do. rte_common.h could after
> > > toolchain specific include do a simple test.
> > > 
> > > #ifndef __rte_no_return
> > > #error no __rte_no_return defined for toolchain
> > > #endif
> > 
> > No I was thinking of:
> > 
> > /** Doxygen comment for the attribute below */
> > #define __rte_no_return RTE_ATTR_NOT_SUPPORTED
> 
> when you suggested this i thought it was something already established.
> i see now that's not the case.  would you mind elaborating a little with
> a complete example of the toolchain specific override and the generic
> include?  i'd just like to understand completely what you were
> proposing so i can try it out.

Sorry I don't have time currently to elaborate,
and not sure how to implement it.
I may look at it next week if that's the way we want to go.
 
> > This way we have a documentation in a single place for the macro,
> > and compilation fails if it is not implemented for the toolchain.

Re: [dpdk-dev] [RFC] toolchain specific macro expansion

[Tyler Retzlaff]

On Wed, Jul 07, 2021 at 10:35:56PM +0200, Thomas Monjalon wrote:
> 07/07/2021 22:23, Tyler Retzlaff:
> > On Thu, Jun 24, 2021 at 06:29:20PM +0200, Thomas Monjalon wrote:
> > > 24/06/2021 18:02, Tyler Retzlaff:
> > > > On Thu, Jun 24, 2021 at 08:54:49AM +0200, Thomas Monjalon wrote:
> > > > > 23/06/2021 20:26, Tyler Retzlaff:
> > > > > > // lib/eal/gcc/rte_toolchain_common.h
> > > > > > #define __rte_noreturn __attribute__((noreturn))
> > > > > 
> > > > > We should keep a macro in rte_common.h which triggers an explicit error
> > > > 
> > > > i think that's relatively trivial to do. rte_common.h could after
> > > > toolchain specific include do a simple test.
> > > > 
> > > > #ifndef __rte_no_return
> > > > #error no __rte_no_return defined for toolchain
> > > > #endif
> > > 
> > > No I was thinking of:
> > > 
> > > /** Doxygen comment for the attribute below */
> > > #define __rte_no_return RTE_ATTR_NOT_SUPPORTED
> > 
> > when you suggested this i thought it was something already established.
> > i see now that's not the case.  would you mind elaborating a little with
> > a complete example of the toolchain specific override and the generic
> > include?  i'd just like to understand completely what you were
> > proposing so i can try it out.
> 
> Sorry I don't have time currently to elaborate,
> and not sure how to implement it.
> I may look at it next week if that's the way we want to go.

i think the community generally preferences not having obvious
conditional compilation on a per-macro basis so i think it is the way we
want to go.

i'll take a look at the existing uses of include/generic/foo.h and
<arch>/include/foo.h where the arch specific includes generic for
inspiration. but i look forward to seeing what you come up with when you
get a chance to focus on the topic.