From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind_com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to; bh=akeYE7LiUvoNLXXj3RdK0fE9cXPnVI66ziX+29Vu+88=; b=InufQYlQc/ilC5MEsdwUIPECVfTB+WtM7w/kcVsV6k+y2zegxnZjQLboW5+lB8QpPm hfZ/hHjTbccAaw7PdM13Ph7IFYOFrUVivdFxQeJ/DDdi67LV3anKHpxv6I4gJyePI98n M0w6Tt5tJQrLmfaoMcYNhDe6h/+q5U7aVwrJiKwPpEbM2zRubqHlwl2pH5TCa4UJh7Fv IpWznioaV8KnjjxAF1WtsypQSttEmQ85Tkn+x1EOnEBloK0/X3ueJzKuduUzcrsqZPlb sC3VL7q45jKG+LU0rWBGzES2udaY/242kOhHXjE/QE7KsyOihsmbucegRwjMDhLzAqvG uqnw== 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 :mail-followup-to:references:mime-version:content-type :content-disposition:in-reply-to; bh=akeYE7LiUvoNLXXj3RdK0fE9cXPnVI66ziX+29Vu+88=; b=AUcbbsw7cmQ+tKWfjRQGPsKmOco7NfrT4+c7vLSHf9rZM9t3vtgR2Id0Spaope/Xjm Toj/lBm0pC25XlpAYdACk+YMCCdPRvjl2nta8Xe3KfJmJswisB0C8JsAIzbug7sckX4P 9yRWXf+vXPTAV0rwUoFNk6bqRRLNFqIHB59w+426C9TjT7kBuCLXEiyfC8QD9aXj2ZNu /AdfKQ9rxxCGC+j2EjMMZoD0JCyeJFceXgvULYDi2bOCueyMel3mBK4klJNJlplxXk2n SZDgtHeCPen1Txkttsx/FkbsaaVFPK7S7/J1Vd8ESSGLxejEtJ5jHb/ZXj4QJjks8oSM Od4A== X-Gm-Message-State: ALoCoQmXfwOfawv6frYGDjMuGdFpNeWPKD+U3KcG+E+63YcqWVPHPxIKNoDdv2MzpvdccSj+Uko1 X-Received: by 10.28.109.193 with SMTP id b62mr4535991wmi.58.1446736179065; Thu, 05 Nov 2015 07:09:39 -0800 (PST) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id m135sm3551729wmb.0.2015.11.05.07.09.37 (version=TLSv1 cipher=RC4-SHA bits=128/128); Thu, 05 Nov 2015 07:09:38 -0800 (PST) Date: Thu, 5 Nov 2015 16:09:18 +0100 From: Adrien Mazarguil To: Stephen Hemminger Message-ID: <20151105150918.GV3518@6wind.com> Mail-Followup-To: Stephen Hemminger , Thomas Monjalon , dev@dpdk.org 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 Content-Disposition: inline In-Reply-To: <20151104103957.4cabd090@xeon-e3> 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Nov 2015 15:09:39 -0000 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