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

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> 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@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.
> 

+1 to this. I was planning to seeing if a static inline could help here, but
haven't had the chance to try it yet.

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

I'm in favour in principle of being standards compliant so long as the cost is
not extremely high. If we have to go through a lot of macro gymnastics to get
things working in order to support pedantic, I'd be of the opinion that the cost
of supporting that particular flag is too high. Each one will probably have his
own opinion of this.

Hopefully a static inline can provide a good compromise solution that everyone
can live with. I'll look at it as soon as I can.

/Bruce