[dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Wed, Nov 04, 2015 at 10:39:57AM -0800, Stephen Hemminger wrote:
> On Wed, 4 Nov 2015 11:24:18 +0100
> Adrien Mazarguil <adrien.mazarguil at 6wind.com> wrote:
> 
> > On Wed, Nov 04, 2015 at 02:19:36AM +0100, Thomas Monjalon wrote:
> > > 2015-11-03 12:00, Bruce Richardson:
> > > > Move the function ptr and port id checking macros to the header file, so
> > > > that they can be used in the static inline functions there. In doxygen
> > > > comments, mark them as for internal use only.
> > > [...]
> > > > +/**
> > > > + * @internal
> > > > + *  Macro to print a message if in debugging mode
> > > > + */
> > > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > > > +		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > > > +#else
> > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > > > +#endif
> > > 
> > > It does not compile because Mellanox drivers are pedantic:
> > > 
> > > In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
> > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
> > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
> > >  #define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > 
> > I suggest something like the following definitions as a pedantic-proof and
> > standard compliant method (one drawback being that it cannot be done with a
> > single macro), see PMD_DRV_LOG() in drivers/net/mlx5/mlx5_utils.h which also
> > automatically appends a line feed:
> > 
> >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > 
> >  #define STRIP(a, b) a
> >  #define OPAREN (
> >  #define CPAREN )
> >  #define COMMA ,
> > 
> >  #define RTE_PMD_DEBUG_TRACE(...) \
> >          RTE_PMD_DEBUG_TRACE_(__VA_ARGS__ STRIP OPAREN, CPAREN)
> > 
> >  #define RTE_PMD_DEBUG_TRACE_(fmt, ...) \
> >          RTE_PMD_DEBUG_TRACE__(fmt COMMA __func__, __VA_ARGS__)
> > 
> >  #define RTE_PMD_DEBUG_TRACE__(...) \
> >          RTE_LOG(ERR, PMD, "%s: " __VA_ARGS__)
> > 
> >  #else /* RTE_LIBRTE_ETHDEV_DEBUG */
> > 
> >  #define RTE_PMD_DEBUG_TRACE(...)
> > 
> >  #endif /* RTE_LIBRTE_ETHDEV_DEBUG */
> > 
> > STRIP() and other helper macros are used to manage the dangling comma issue
> > when __VA_ARGS__ is empty as in the first call below:
> > 
> >  RTE_PMD_DEBUG_TRACE("foo\n");
> >  RTE_PMD_DEBUG_TRACE("foo %u\n", 42);
> 
> That solution is really ugly.

I won't argue against this as it's obviously more complex than the original
method, however note that users of the RTE_PMD_DEBUG_TRACE() macro do not
have to modify their code. They shouldn't care about the implementation.

Also note that we can do much cleaner code if we drop the all macros
implementation using a (much easier to debug) static inline function,
only perhaps with a wrapper macro that provides __LINE__, __func__ and
__FILE__ as arguments. Nontrival code shouldn't be done in macros anyway.

> Why not do something that keeps the expected checks.

Sure but it's not the issue, we're discussing errors related to
-pedantic. I've only made the above suggestion to pass its pedantic
checks. RTE_LOG_DISABLED can be managed with these macros as well.

> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index ede0dca..f3a3d34 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -99,6 +99,8 @@ extern struct rte_logs rte_logs;
>  #define RTE_LOG_INFO     7U  /**< Informational.                    */
>  #define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
>  
> +#define RTE_LOG_DISABLED 99U /**< Never printed			    */
> +
>  /** The default log stream. */
>  extern FILE *eal_default_log_stream;
>  
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index eee1194..e431f2e 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -931,6 +931,61 @@ struct rte_eth_dev_callback;
>  /** @internal Structure to keep track of registered callbacks */
>  TAILQ_HEAD(rte_eth_dev_cb_list, rte_eth_dev_callback);
>  
> +/**
> + * @internal
> + *  Macro to print a message if in debugging mode
> + */
> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> +	RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> +#else
> +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> +	RTE_LOG(DISABLED, PMD, "%s: " fmt, __func__, ## args)
> +#endif

My previous message was probably not clear enough about the reason for this
error. With -pedantic, GCC complains about these bits:

- "args..." causing "error: ISO C does not permit named variadic
  macros", as in C function you cannot put an ellipsis directly behind a
  token without a comma.

- ", ## args" for which I can't recall the error, but pasting a comma and
  args is also nonstandard, especially when args happens to be empty.
  Without -pedantic, GCC silently drops the comma.

Bruce is asking for a consensus about -pedantic, whether we want to do the
extra effort to support it in DPDK. Since I like checking for -pedantic
errors, it's enabled for mlx4 and mlx5 when compiling these drivers in
debugging mode. There is currently no established rule in DPDK against this.

I'm arguing that most C headers (C compiler, libc, most libraries, even the
Linux kernel in uapi to an extent) provide standards compliant includes
because they cannot predict or force particular compilation flags on
user applications.

If we consider DPDK as a system wide library, I think we should do it as
well in all installed header files. If we choose not to, then we must
document that our code is not standard, -pedantic is unsupported and I'll
have to drop it from mlx4 and mlx5.

-- 
Adrien Mazarguil
6WIND