Message ID | 20151104103957.4cabd090@xeon-e3 (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 389DF8E9A; Wed, 4 Nov 2015 19:39:50 +0100 (CET) Received: from mail-pa0-f41.google.com (mail-pa0-f41.google.com [209.85.220.41]) by dpdk.org (Postfix) with ESMTP id AF8B28E93 for <dev@dpdk.org>; Wed, 4 Nov 2015 19:39:48 +0100 (CET) Received: by pacdm15 with SMTP id dm15so35987992pac.3 for <dev@dpdk.org>; Wed, 04 Nov 2015 10:39:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber_org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=c3VIwaADjHksVGtieM01t8nlIYfwL6XMMcOgKbchmyQ=; b=T3Kp+IkjqlDbitYcLq+kl/ff+Sk9ahh9vL2ZZSPonbZ2OAnZGsDcEWoFfr4PUsJiNL dhD6ZO6EkJRsA5Lk7K+xp/ZcJiC3XlI7Y608agDXuX7nv0dH3vrGDZchr2kyoUMcXBms W3P40BVnBvwT6hZODjdi1LTRP/r36EkzhTYhREY5gERQwDLWXyA1LCpnTvTEsAd+RHFU d0xvJYbybIpTkVJ983CEl9bDKq8WgE/DVrtdrOKCwIJGCGqHOUsaOYxOAdhUWcrh1JZy 3Se63YDgcQDVaNAHOYACgrFiBnCdarSQhw7UAfpCsZPtJNU9rAGtdqU2D4f87zfpPQ// 7EXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-type:content-transfer-encoding; bh=c3VIwaADjHksVGtieM01t8nlIYfwL6XMMcOgKbchmyQ=; b=gyGS7PbtCiBCJllJHJh5Vu07io+cUtuTEnqRxW5cK32ei81m8uXVmXciZh183NL3gf BM0PArLvnRvCQxxkXuK/DyuPJp0+expavl1PQgxOJB0/rv4/uS2cju/3GawTlNKMGdWu EReYqhXYdjkS0w2ayJSKWYpiSk24s3i6kkQBNEUmGmApsMx4KpCnHBdyEIhWgbMBnMnT 1AHf5wI0DMbMXFmLLM2SgLmYZMCGUcXuDcpPfV5tmQePze8vtdKYL7V6P4jXyNcq9nQ4 bIxKZQdFtyE29zMYdh4Ic49hNM6rMY4IVuUN+sooViLZtCoZ0AvvV6kdQ7HDXL4ZV0r3 rBxA== X-Gm-Message-State: ALoCoQmj4qSGmvV3JWCD8YqLBpG+3T4mOPN3ryDJ5LTAt+WIbsJRJDe2on46pRg2BqvdbfdLH5T3 X-Received: by 10.68.162.35 with SMTP id xx3mr3691292pbb.79.1446662388034; Wed, 04 Nov 2015 10:39:48 -0800 (PST) Received: from xeon-e3 (static-50-53-82-155.bvtn.or.frontiernet.net. [50.53.82.155]) by smtp.gmail.com with ESMTPSA id ha1sm3359785pbc.54.2015.11.04.10.39.45 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Nov 2015 10:39:47 -0800 (PST) Date: Wed, 4 Nov 2015 10:39:57 -0800 From: Stephen Hemminger <stephen@networkplumber.org> To: Adrien Mazarguil <adrien.mazarguil@6wind.com> Message-ID: <20151104103957.4cabd090@xeon-e3> In-Reply-To: <20151104102418.GN3518@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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Stephen Hemminger
Nov. 4, 2015, 6:39 p.m. UTC
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.
Comments
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
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