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

  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.

Why not do something that keeps the expected checks.
  

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.

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

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
  

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

Hi Adrien,

I'm trying to dig into this a bit more now, and try out using a static inline
function, but I'm having trouble getting DPDK to compile with the mlx drivers
turned on in the config. I'm trying to follow the instructions here:
http://dpdk.org/doc/guides/nics/mlx4.html, but it's not clearly called out what
requirements are for compilation vs requirements for running the PMD.

I'm running Fedora 23, and installed the libibverbs-devel package, but when I
compile I get the following error:

== Build drivers/net/mlx4
  CC mlx4.o
  /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c: In function ‘txq_cleanup’:
  /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c:886:37: error: storage size of ‘params’ isn’t known
    struct ibv_exp_release_intf_params params;
                                       ^
compilation terminated due to -Wfatal-errors.

Any suggestions on the fix for this?

Thanks,
/Bruce
  

On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> 
> 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.
> 
Getting something working with __FILE__ and probably __LINE__ would be easy enough
with a helper macro, but __func__ is not so easy as it's not a preprocessor symbol
[since the pre-processor has no idea what function you are in].

However, using func, here is the best I've come up with so far. It's not that
pretty, but it's probably easier to work with than the macro version.

 #ifdef RTE_LIBRTE_ETHDEV_DEBUG
 -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
 -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
 +#define RTE_PMD_DEBUG_TRACE(...) \
 +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
 +
 +static inline void
 +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
 +{
 +       static __thread char buffer[128];
 +       char *out_buf = buffer;
 +       unsigned count;
 +       va_list ap;
 +
 +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name, fmt);
 +       if (count >= sizeof(buffer)) { // truncated output
 +               char *new_buf = malloc(count + 1);
 +               if (new_buf == NULL) // no memory, just print 128 chars
 +                       goto print_buffer;
 +               snprintf(new_buf, count + 1, "%s: %s", func_name, fmt);
 +               va_start(ap, fmt);
 +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
 +               va_end(ap);
 +               free(new_buf);
 +               return;
 +       }
 +
 +print_buffer:
 +       va_start(ap, fmt);
 +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
 +       va_end(ap);
 +}
  #else
  #define RTE_PMD_DEBUG_TRACE(fmt, args...)
  #endif

Comments or improvements?

/Bruce
  

On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
> On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > 
> > 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.
> > 
> Getting something working with __FILE__ and probably __LINE__ would be easy enough
> with a helper macro, but __func__ is not so easy as it's not a preprocessor symbol
> [since the pre-processor has no idea what function you are in].
> 
> However, using func, here is the best I've come up with so far. It's not that
> pretty, but it's probably easier to work with than the macro version.
> 
>  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
>  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
>  +#define RTE_PMD_DEBUG_TRACE(...) \
>  +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
>  +
>  +static inline void
>  +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>  +{
>  +       static __thread char buffer[128];
>  +       char *out_buf = buffer;
>  +       unsigned count;
>  +       va_list ap;
>  +
>  +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name, fmt);
>  +       if (count >= sizeof(buffer)) { // truncated output
>  +               char *new_buf = malloc(count + 1);
>  +               if (new_buf == NULL) // no memory, just print 128 chars
>  +                       goto print_buffer;
>  +               snprintf(new_buf, count + 1, "%s: %s", func_name, fmt);
>  +               va_start(ap, fmt);
>  +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
>  +               va_end(ap);
>  +               free(new_buf);
>  +               return;
>  +       }
>  +
>  +print_buffer:
>  +       va_start(ap, fmt);
>  +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
>  +       va_end(ap);
>  +}
>   #else
>   #define RTE_PMD_DEBUG_TRACE(fmt, args...)
>   #endif
> 
> Comments or improvements?
> 
> /Bruce

And here's the version if we are happy to have file and line number instead of
function name. I think this might be the best option.

/Bruce

 #ifdef RTE_LIBRTE_ETHDEV_DEBUG
 -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
 -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
 +#define RTE_PMD_DEBUG_TRACE(...) \
 +       RTE_LOG(ERR, PMD, __FILE__", " RTE_STR(__LINE__) ": " __VA_ARGS__)
  #else
 -#define RTE_PMD_DEBUG_TRACE(fmt, args...)
 +#define RTE_PMD_DEBUG_TRACE(...)
  #endif
  

On Fri, Nov 06, 2015 at 05:22:27PM +0000, Bruce Richardson wrote:
> On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
> > On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > > 
> > > 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.
> > > 
> > Getting something working with __FILE__ and probably __LINE__ would be easy enough
> > with a helper macro, but __func__ is not so easy as it's not a preprocessor symbol
> > [since the pre-processor has no idea what function you are in].
> > 
> > However, using func, here is the best I've come up with so far. It's not that
> > pretty, but it's probably easier to work with than the macro version.
> > 
> >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> >  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> >  +#define RTE_PMD_DEBUG_TRACE(...) \
> >  +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
> >  +
> >  +static inline void
> >  +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> >  +{
> >  +       static __thread char buffer[128];
> >  +       char *out_buf = buffer;
> >  +       unsigned count;
> >  +       va_list ap;
> >  +
> >  +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name, fmt);
> >  +       if (count >= sizeof(buffer)) { // truncated output
> >  +               char *new_buf = malloc(count + 1);
> >  +               if (new_buf == NULL) // no memory, just print 128 chars
> >  +                       goto print_buffer;
> >  +               snprintf(new_buf, count + 1, "%s: %s", func_name, fmt);
> >  +               va_start(ap, fmt);
> >  +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
> >  +               va_end(ap);
> >  +               free(new_buf);
> >  +               return;
> >  +       }
> >  +
> >  +print_buffer:
> >  +       va_start(ap, fmt);
> >  +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
> >  +       va_end(ap);
> >  +}
> >   #else
> >   #define RTE_PMD_DEBUG_TRACE(fmt, args...)
> >   #endif
> > 
> > Comments or improvements?

Such a function shouldn't malloc() anything. The entire line should fit on
the stack (crashing is fine if it does not, then it's probably a bug). We
did something in two passes along these lines in mlx5_defs.h (not pretty but
quite useful):

 /* Allocate a buffer on the stack and fill it with a printf format string. */
 #define MKSTR(name, ...) \
         char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \
         \
         snprintf(name, sizeof(name), __VA_ARGS__)

Untested but I guess modifying that function accordingly would look like:

 static inline void
 rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
 {
         va_list ap;
         va_start(ap, fmt);

         static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];

         va_end(ap);
	 va_start(ap, fmt);
         vsnprintf(buffer, sizeof(buffer), fmt, ap);
	 va_end(ap);
         rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name, buffer);
 }

> And here's the version if we are happy to have file and line number instead of
> function name. I think this might be the best option.
> 
> /Bruce
> 
>  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
>  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
>  +#define RTE_PMD_DEBUG_TRACE(...) \
>  +       RTE_LOG(ERR, PMD, __FILE__", " RTE_STR(__LINE__) ": " __VA_ARGS__)
>   #else
>  -#define RTE_PMD_DEBUG_TRACE(fmt, args...)
>  +#define RTE_PMD_DEBUG_TRACE(...)
>   #endif

Much cleaner indeed, however __func__ might be useful when comparing log
outputs from different source code versions. I think we should keep it.
  

On Mon, Nov 09, 2015 at 02:39:05PM +0100, Adrien Mazarguil wrote:
> On Fri, Nov 06, 2015 at 05:22:27PM +0000, Bruce Richardson wrote:
> > On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
> > > On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > > > 
> > > > 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.
> > > > 
> > > Getting something working with __FILE__ and probably __LINE__ would be easy enough
> > > with a helper macro, but __func__ is not so easy as it's not a preprocessor symbol
> > > [since the pre-processor has no idea what function you are in].
> > > 
> > > However, using func, here is the best I've come up with so far. It's not that
> > > pretty, but it's probably easier to work with than the macro version.
> > > 
> > >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > >  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > >  +#define RTE_PMD_DEBUG_TRACE(...) \
> > >  +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
> > >  +
> > >  +static inline void
> > >  +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> > >  +{
> > >  +       static __thread char buffer[128];
> > >  +       char *out_buf = buffer;
> > >  +       unsigned count;
> > >  +       va_list ap;
> > >  +
> > >  +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name, fmt);
> > >  +       if (count >= sizeof(buffer)) { // truncated output
> > >  +               char *new_buf = malloc(count + 1);
> > >  +               if (new_buf == NULL) // no memory, just print 128 chars
> > >  +                       goto print_buffer;
> > >  +               snprintf(new_buf, count + 1, "%s: %s", func_name, fmt);
> > >  +               va_start(ap, fmt);
> > >  +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
> > >  +               va_end(ap);
> > >  +               free(new_buf);
> > >  +               return;
> > >  +       }
> > >  +
> > >  +print_buffer:
> > >  +       va_start(ap, fmt);
> > >  +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
> > >  +       va_end(ap);
> > >  +}
> > >   #else
> > >   #define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > >   #endif
> > > 
> > > Comments or improvements?
> 
> Such a function shouldn't malloc() anything. The entire line should fit on
> the stack (crashing is fine if it does not, then it's probably a bug). We
> did something in two passes along these lines in mlx5_defs.h (not pretty but
> quite useful):
> 
>  /* Allocate a buffer on the stack and fill it with a printf format string. */
>  #define MKSTR(name, ...) \
>          char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \
>          \
>          snprintf(name, sizeof(name), __VA_ARGS__)
> 
> Untested but I guess modifying that function accordingly would look like:
> 
>  static inline void
>  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>  {
>          va_list ap;
>          va_start(ap, fmt);
> 
>          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];

Of course this line should read:

         static __thread char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];

>          va_end(ap);
>          va_start(ap, fmt);
>          vsnprintf(buffer, sizeof(buffer), fmt, ap);
>          va_end(ap);
>          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name, buffer);
>  }
> 
> > And here's the version if we are happy to have file and line number instead of
> > function name. I think this might be the best option.
> > 
> > /Bruce
> > 
> >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> >  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> >  +#define RTE_PMD_DEBUG_TRACE(...) \
> >  +       RTE_LOG(ERR, PMD, __FILE__", " RTE_STR(__LINE__) ": " __VA_ARGS__)
> >   #else
> >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> >  +#define RTE_PMD_DEBUG_TRACE(...)
> >   #endif
> 
> Much cleaner indeed, however __func__ might be useful when comparing log
> outputs from different source code versions. I think we should keep it.
  

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Monday, November 9, 2015 1:39 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
> to header
> 
> On Fri, Nov 06, 2015 at 05:22:27PM +0000, Bruce Richardson wrote:
> > On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
> > > On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > > >
> > > > 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.
> > > >
> > > Getting something working with __FILE__ and probably __LINE__ would be
> easy enough
> > > with a helper macro, but __func__ is not so easy as it's not a
> preprocessor symbol
> > > [since the pre-processor has no idea what function you are in].
> > >
> > > However, using func, here is the best I've come up with so far. It's
> not that
> > > pretty, but it's probably easier to work with than the macro version.
> > >
> > >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > >  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > >  +#define RTE_PMD_DEBUG_TRACE(...) \
> > >  +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
> > >  +
> > >  +static inline void
> > >  +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> > >  +{
> > >  +       static __thread char buffer[128];
> > >  +       char *out_buf = buffer;
> > >  +       unsigned count;
> > >  +       va_list ap;
> > >  +
> > >  +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name,
> fmt);
> > >  +       if (count >= sizeof(buffer)) { // truncated output
> > >  +               char *new_buf = malloc(count + 1);
> > >  +               if (new_buf == NULL) // no memory, just print 128
> chars
> > >  +                       goto print_buffer;
> > >  +               snprintf(new_buf, count + 1, "%s: %s", func_name,
> fmt);
> > >  +               va_start(ap, fmt);
> > >  +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
> > >  +               va_end(ap);
> > >  +               free(new_buf);
> > >  +               return;
> > >  +       }
> > >  +
> > >  +print_buffer:
> > >  +       va_start(ap, fmt);
> > >  +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
> > >  +       va_end(ap);
> > >  +}
> > >   #else
> > >   #define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > >   #endif
> > >
> > > Comments or improvements?
> 
> Such a function shouldn't malloc() anything. The entire line should fit on
> the stack (crashing is fine if it does not, then it's probably a bug). We
> did something in two passes along these lines in mlx5_defs.h (not pretty
> but
> quite useful):
> 
>  /* Allocate a buffer on the stack and fill it with a printf format
> string. */
>  #define MKSTR(name, ...) \
>          char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \
>          \
>          snprintf(name, sizeof(name), __VA_ARGS__)
> 
> Untested but I guess modifying that function accordingly would look like:
> 
>  static inline void
>  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>  {
>          va_list ap;
>          va_start(ap, fmt);
> 
>          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> 
>          va_end(ap);
> 	 va_start(ap, fmt);
>          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> 	 va_end(ap);
>          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> buffer);
>  }
> 

Looks a much better option.

From this, though, I assume then that we are only looking to support the -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic with the pre-gcc-5 versions won't allow that to work though, as variably sized arrays only came in with c99, and were gnu extensions before that.

Regards,
/Bruce
  

On 09/11/15 14:02, Richardson, Bruce wrote:
>
>
>> -----Original Message-----
>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
>> Sent: Monday, November 9, 2015 1:39 PM
>> To: Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
>> <thomas.monjalon@6wind.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
>> to header
>>
>> On Fri, Nov 06, 2015 at 05:22:27PM +0000, Bruce Richardson wrote:
>>> On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
>>>> On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
>>>>>
>>>>> 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.
>>>>>
>>>> Getting something working with __FILE__ and probably __LINE__ would be
>> easy enough
>>>> with a helper macro, but __func__ is not so easy as it's not a
>> preprocessor symbol
>>>> [since the pre-processor has no idea what function you are in].
>>>>
>>>> However, using func, here is the best I've come up with so far. It's
>> not that
>>>> pretty, but it's probably easier to work with than the macro version.
>>>>
>>>>   #ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>>   -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
>>>>   -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
>>>>   +#define RTE_PMD_DEBUG_TRACE(...) \
>>>>   +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
>>>>   +
>>>>   +static inline void
>>>>   +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>>>>   +{
>>>>   +       static __thread char buffer[128];
>>>>   +       char *out_buf = buffer;
>>>>   +       unsigned count;
>>>>   +       va_list ap;
>>>>   +
>>>>   +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name,
>> fmt);
>>>>   +       if (count >= sizeof(buffer)) { // truncated output
>>>>   +               char *new_buf = malloc(count + 1);
>>>>   +               if (new_buf == NULL) // no memory, just print 128
>> chars
>>>>   +                       goto print_buffer;
>>>>   +               snprintf(new_buf, count + 1, "%s: %s", func_name,
>> fmt);
>>>>   +               va_start(ap, fmt);
>>>>   +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
>>>>   +               va_end(ap);
>>>>   +               free(new_buf);
>>>>   +               return;
>>>>   +       }
>>>>   +
>>>>   +print_buffer:
>>>>   +       va_start(ap, fmt);
>>>>   +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
>>>>   +       va_end(ap);
>>>>   +}
>>>>    #else
>>>>    #define RTE_PMD_DEBUG_TRACE(fmt, args...)
>>>>    #endif
>>>>
>>>> Comments or improvements?
>>
>> Such a function shouldn't malloc() anything. The entire line should fit on
>> the stack (crashing is fine if it does not, then it's probably a bug). We
>> did something in two passes along these lines in mlx5_defs.h (not pretty
>> but
>> quite useful):
>>
>>   /* Allocate a buffer on the stack and fill it with a printf format
>> string. */
>>   #define MKSTR(name, ...) \
>>           char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \
>>           \
>>           snprintf(name, sizeof(name), __VA_ARGS__)
>>
>> Untested but I guess modifying that function accordingly would look like:
>>
>>   static inline void
>>   rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>>   {
>>           va_list ap;
>>           va_start(ap, fmt);
>>
>>           static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
>>
>>           va_end(ap);
>> 	 va_start(ap, fmt);
>>           vsnprintf(buffer, sizeof(buffer), fmt, ap);
>> 	 va_end(ap);
>>           rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
>> buffer);
>>   }
>>
>
> Looks a much better option.
>
>  From this, though, I assume then that we are only looking to support the -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic with the pre-gcc-5 versions won't allow that to work though, as variably sized arrays only came in with c99, and were gnu extensions before that.
>
> Regards,
> /Bruce
>

I had made some similar changes in the cryptodev patch set to allow 
these macros to be shared between the ethdev and cryptodev, but I wasn't 
aware of the -pedantic build issues. I've incorporate the changes from 
patch 1 & 2 discussed here into the cryptodev patch set. See patch 2/10 
(http://dpdk.org/ml/archives/dev/2015-November/027888.html) for the 
implementation of the rte_pmf_debug_trace function. Any comments or 
ack's are welcome :)

Declan
  

On Mon, Nov 09, 2015 at 02:02:28PM +0000, Richardson, Bruce wrote:
[...]
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
[...]
> > Untested but I guess modifying that function accordingly would look like:
> > 
> >  static inline void
> >  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> >  {
> >          va_list ap;
> >          va_start(ap, fmt);
> > 
> >          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> > 
> >          va_end(ap);
> > 	 va_start(ap, fmt);
> >          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > 	 va_end(ap);
> >          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> > buffer);
> >  }
> > 
> 
> Looks a much better option.
> 
> From this, though, I assume then that we are only looking to support the -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic with the pre-gcc-5 versions won't allow that to work though, as variably sized arrays only came in with c99, and were gnu extensions before that.

Right, -pedantic must follow a given standard such as -std=gnu99 otherwise
it's meaningless.

However pre-GCC 5 is fine for most if not all features we use, see:

 https://gcc.gnu.org/c99status.html

Mixed code and declarations are supported since GCC 3.0, __VA_ARGS__ in
macros since GCC 2.95 and variable length arrays since GCC 0.9, so as long
as we use a version that implements -std=gnu99 (or -std=c99 to be really
pedantic), it's fine.

Besides DPDK already uses C99 extensively, even a few C11 features (such as
embedded anonymous struct definitions) currently supported in C99 mode as
compiler extensions. I think we can safely ignore compilers that don't
support common C99 features.
  

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, November 10, 2015 4:08 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
> to header
> 
> On Mon, Nov 09, 2015 at 02:02:28PM +0000, Richardson, Bruce wrote:
> [...]
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> [...]
> > > Untested but I guess modifying that function accordingly would look
> like:
> > >
> > >  static inline void
> > >  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)  {
> > >          va_list ap;
> > >          va_start(ap, fmt);
> > >
> > >          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> > >
> > >          va_end(ap);
> > > 	 va_start(ap, fmt);
> > >          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > > 	 va_end(ap);
> > >          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> > > buffer);  }
> > >
> >
> > Looks a much better option.
> >
> > From this, though, I assume then that we are only looking to support the
> -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic
> with the pre-gcc-5 versions won't allow that to work though, as variably
> sized arrays only came in with c99, and were gnu extensions before that.
> 
> Right, -pedantic must follow a given standard such as -std=gnu99 otherwise
> it's meaningless.
> 
> However pre-GCC 5 is fine for most if not all features we use, see:
> 
>  https://gcc.gnu.org/c99status.html
> 
> Mixed code and declarations are supported since GCC 3.0, __VA_ARGS__ in
> macros since GCC 2.95 and variable length arrays since GCC 0.9, so as long
> as we use a version that implements -std=gnu99 (or -std=c99 to be really
> pedantic), it's fine.
> 
> Besides DPDK already uses C99 extensively, even a few C11 features (such
> as
> embedded anonymous struct definitions) currently supported in C99 mode as
> compiler extensions. I think we can safely ignore compilers that don't
> support common C99 features.
> 
> --
> Adrien Mazarguil
> 6WIND

Actually my point was slightly different. 
If we are supporting "-pedantic" in header files because "we don't know what compiler flags users are going to pass when", then we need to support it in C90 mode to do the job properly. If you take gcc 4.8 and compile some code with "-pedantic" as the only C-flag you are going to get lots of errors because it will default to C90 mode with pedantic, which means no varargs macros at all. 
This limits the usefulness of supporting pedantic flag at all in our header files, since we only support it in certain situations with non-latest compilers.

/Bruce
  

On Tue, Nov 10, 2015 at 04:21:10PM +0000, Richardson, Bruce wrote:
> 
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Tuesday, November 10, 2015 4:08 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
> > <thomas.monjalon@6wind.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
> > to header
> > 
> > On Mon, Nov 09, 2015 at 02:02:28PM +0000, Richardson, Bruce wrote:
> > [...]
> > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > [...]
> > > > Untested but I guess modifying that function accordingly would look
> > like:
> > > >
> > > >  static inline void
> > > >  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)  {
> > > >          va_list ap;
> > > >          va_start(ap, fmt);
> > > >
> > > >          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> > > >
> > > >          va_end(ap);
> > > > 	 va_start(ap, fmt);
> > > >          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > > > 	 va_end(ap);
> > > >          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> > > > buffer);  }
> > > >
> > >
> > > Looks a much better option.
> > >
> > > From this, though, I assume then that we are only looking to support the
> > -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic
> > with the pre-gcc-5 versions won't allow that to work though, as variably
> > sized arrays only came in with c99, and were gnu extensions before that.
> > 
> > Right, -pedantic must follow a given standard such as -std=gnu99 otherwise
> > it's meaningless.
> > 
> > However pre-GCC 5 is fine for most if not all features we use, see:
> > 
> >  https://gcc.gnu.org/c99status.html
> > 
> > Mixed code and declarations are supported since GCC 3.0, __VA_ARGS__ in
> > macros since GCC 2.95 and variable length arrays since GCC 0.9, so as long
> > as we use a version that implements -std=gnu99 (or -std=c99 to be really
> > pedantic), it's fine.
> > 
> > Besides DPDK already uses C99 extensively, even a few C11 features (such
> > as
> > embedded anonymous struct definitions) currently supported in C99 mode as
> > compiler extensions. I think we can safely ignore compilers that don't
> > support common C99 features.
> > 
> > --
> > Adrien Mazarguil
> > 6WIND
> 
> Actually my point was slightly different. 
> If we are supporting "-pedantic" in header files because "we don't know what compiler flags users are going to pass when", then we need to support it in C90 mode to do the job properly. If you take gcc 4.8 and compile some code with "-pedantic" as the only C-flag you are going to get lots of errors because it will default to C90 mode with pedantic, which means no varargs macros at all. 

Agreed, exported headers should actually be C90 compliant for these reasons
but C99 would be a start. I didn't know GCC 5 switched to C99 by default
(don't worry, I do not intend to go back to C90).

> This limits the usefulness of supporting pedantic flag at all in our header files, since we only support it in certain situations with non-latest compilers.

I won't deny this, as the goal is partly to appease pedantic people like
myself. Using standard methods for doing things should be preferred over
extensions for compatibility with the unknown, unless there is no other way.
  

On Tue, Nov 10, 2015 at 06:12:28PM +0100, Adrien Mazarguil wrote:
> On Tue, Nov 10, 2015 at 04:21:10PM +0000, Richardson, Bruce wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > Sent: Tuesday, November 10, 2015 4:08 PM
> > > To: Richardson, Bruce <bruce.richardson@intel.com>
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
> > > <thomas.monjalon@6wind.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
> > > to header
> > > 
> > > On Mon, Nov 09, 2015 at 02:02:28PM +0000, Richardson, Bruce wrote:
> > > [...]
> > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > [...]
> > > > > Untested but I guess modifying that function accordingly would look
> > > like:
> > > > >
> > > > >  static inline void
> > > > >  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)  {
> > > > >          va_list ap;
> > > > >          va_start(ap, fmt);
> > > > >
> > > > >          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> > > > >
> > > > >          va_end(ap);
> > > > > 	 va_start(ap, fmt);
> > > > >          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > > > > 	 va_end(ap);
> > > > >          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> > > > > buffer);  }
> > > > >
> > > >
> > > > Looks a much better option.
> > > >
> > > > From this, though, I assume then that we are only looking to support the
> > > -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic
> > > with the pre-gcc-5 versions won't allow that to work though, as variably
> > > sized arrays only came in with c99, and were gnu extensions before that.
> > > 
> > > Right, -pedantic must follow a given standard such as -std=gnu99 otherwise
> > > it's meaningless.
> > > 
> > > However pre-GCC 5 is fine for most if not all features we use, see:
> > > 
> > >  https://gcc.gnu.org/c99status.html
> > > 
> > > Mixed code and declarations are supported since GCC 3.0, __VA_ARGS__ in
> > > macros since GCC 2.95 and variable length arrays since GCC 0.9, so as long
> > > as we use a version that implements -std=gnu99 (or -std=c99 to be really
> > > pedantic), it's fine.
> > > 
> > > Besides DPDK already uses C99 extensively, even a few C11 features (such
> > > as
> > > embedded anonymous struct definitions) currently supported in C99 mode as
> > > compiler extensions. I think we can safely ignore compilers that don't
> > > support common C99 features.
> > > 
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> > 
> > Actually my point was slightly different. 
> > If we are supporting "-pedantic" in header files because "we don't know what compiler flags users are going to pass when", then we need to support it in C90 mode to do the job properly. If you take gcc 4.8 and compile some code with "-pedantic" as the only C-flag you are going to get lots of errors because it will default to C90 mode with pedantic, which means no varargs macros at all. 
> 
> Agreed, exported headers should actually be C90 compliant for these reasons
> but C99 would be a start. I didn't know GCC 5 switched to C99 by default
> (don't worry, I do not intend to go back to C90).
> 
Actually, it's even better than C99, the default is now C11 (or gnu11 to be pedantic about it :-) )

https://gcc.gnu.org/gcc-5/changes.html

Top line item is: 
	"The default mode for C is now -std=gnu11 instead of -std=gnu89"

I believe clang is making a similar change to a c11-based default. \o/

/Bruce