From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v3 2/4] ethdev: move error checking macros to header Date: Thu, 5 Nov 2015 16:09:18 +0100 Message-ID: <20151105150918.GV3518@6wind.com> References: <1441811374-28984-1-git-send-email-bruce.richardson@intel.com> <1446552059-5446-1-git-send-email-bruce.richardson@intel.com> <1446552059-5446-3-git-send-email-bruce.richardson@intel.com> <4698587.GS9blBozDC@xps13> <20151104102418.GN3518@6wind.com> <20151104103957.4cabd090@xeon-e3> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org To: Stephen Hemminger Return-path: Received: from mail-wm0-f52.google.com (mail-wm0-f52.google.com [74.125.82.52]) by dpdk.org (Postfix) with ESMTP id 3DB0E8DAC for ; Thu, 5 Nov 2015 16:09:39 +0100 (CET) Received: by wmeg8 with SMTP id g8so16952271wme.0 for ; Thu, 05 Nov 2015 07:09:39 -0800 (PST) Content-Disposition: inline In-Reply-To: <20151104103957.4cabd090@xeon-e3> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Nov 04, 2015 at 10:39:57AM -0800, Stephen Hemminger wrote: > On Wed, 4 Nov 2015 11:24:18 +0100 > Adrien Mazarguil 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