From: Bruce Richardson <bruce.richardson@intel.com>
To: 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
Date: Thu, 5 Nov 2015 15:17:49 +0000 [thread overview]
Message-ID: <20151105151749.GA26416@bricha3-MOBL3> (raw)
In-Reply-To: <20151105150918.GV3518@6wind.com>
On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> On Wed, Nov 04, 2015 at 10:39:57AM -0800, Stephen Hemminger wrote:
> > On Wed, 4 Nov 2015 11:24:18 +0100
> > Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> >
> > > On Wed, Nov 04, 2015 at 02:19:36AM +0100, Thomas Monjalon wrote:
> > > > 2015-11-03 12:00, Bruce Richardson:
> > > > > Move the function ptr and port id checking macros to the header file, so
> > > > > that they can be used in the static inline functions there. In doxygen
> > > > > comments, mark them as for internal use only.
> > > > [...]
> > > > > +/**
> > > > > + * @internal
> > > > > + * Macro to print a message if in debugging mode
> > > > > + */
> > > > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > > > > + RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > > > > +#else
> > > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > > > > +#endif
> > > >
> > > > It does not compile because Mellanox drivers are pedantic:
> > > >
> > > > In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
> > > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
> > > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
> > > > #define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > >
> > > I suggest something like the following definitions as a pedantic-proof and
> > > standard compliant method (one drawback being that it cannot be done with a
> > > single macro), see PMD_DRV_LOG() in drivers/net/mlx5/mlx5_utils.h which also
> > > automatically appends a line feed:
> > >
> > > #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > >
> > > #define STRIP(a, b) a
> > > #define OPAREN (
> > > #define CPAREN )
> > > #define COMMA ,
> > >
> > > #define RTE_PMD_DEBUG_TRACE(...) \
> > > RTE_PMD_DEBUG_TRACE_(__VA_ARGS__ STRIP OPAREN, CPAREN)
> > >
> > > #define RTE_PMD_DEBUG_TRACE_(fmt, ...) \
> > > RTE_PMD_DEBUG_TRACE__(fmt COMMA __func__, __VA_ARGS__)
> > >
> > > #define RTE_PMD_DEBUG_TRACE__(...) \
> > > RTE_LOG(ERR, PMD, "%s: " __VA_ARGS__)
> > >
> > > #else /* RTE_LIBRTE_ETHDEV_DEBUG */
> > >
> > > #define RTE_PMD_DEBUG_TRACE(...)
> > >
> > > #endif /* RTE_LIBRTE_ETHDEV_DEBUG */
> > >
> > > STRIP() and other helper macros are used to manage the dangling comma issue
> > > when __VA_ARGS__ is empty as in the first call below:
> > >
> > > RTE_PMD_DEBUG_TRACE("foo\n");
> > > RTE_PMD_DEBUG_TRACE("foo %u\n", 42);
> >
> > That solution is really ugly.
>
> I won't argue against this as it's obviously more complex than the original
> method, however note that users of the RTE_PMD_DEBUG_TRACE() macro do not
> have to modify their code. They shouldn't care about the implementation.
>
> Also note that we can do much cleaner code if we drop the all macros
> implementation using a (much easier to debug) static inline function,
> only perhaps with a wrapper macro that provides __LINE__, __func__ and
> __FILE__ as arguments. Nontrival code shouldn't be done in macros anyway.
>
+1 to this. I was planning to seeing if a static inline could help here, but
haven't had the chance to try it yet.
> > Why not do something that keeps the expected checks.
>
> Sure but it's not the issue, we're discussing errors related to
> -pedantic. I've only made the above suggestion to pass its pedantic
> checks. RTE_LOG_DISABLED can be managed with these macros as well.
>
> > diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> > index ede0dca..f3a3d34 100644
> > --- a/lib/librte_eal/common/include/rte_log.h
> > +++ b/lib/librte_eal/common/include/rte_log.h
> > @@ -99,6 +99,8 @@ extern struct rte_logs rte_logs;
> > #define RTE_LOG_INFO 7U /**< Informational. */
> > #define RTE_LOG_DEBUG 8U /**< Debug-level messages. */
> >
> > +#define RTE_LOG_DISABLED 99U /**< Never printed */
> > +
> > /** The default log stream. */
> > extern FILE *eal_default_log_stream;
> >
> > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> > index eee1194..e431f2e 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -931,6 +931,61 @@ struct rte_eth_dev_callback;
> > /** @internal Structure to keep track of registered callbacks */
> > TAILQ_HEAD(rte_eth_dev_cb_list, rte_eth_dev_callback);
> >
> > +/**
> > + * @internal
> > + * Macro to print a message if in debugging mode
> > + */
> > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > + RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > +#else
> > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > + RTE_LOG(DISABLED, PMD, "%s: " fmt, __func__, ## args)
> > +#endif
>
> My previous message was probably not clear enough about the reason for this
> error. With -pedantic, GCC complains about these bits:
>
> - "args..." causing "error: ISO C does not permit named variadic
> macros", as in C function you cannot put an ellipsis directly behind a
> token without a comma.
>
> - ", ## args" for which I can't recall the error, but pasting a comma and
> args is also nonstandard, especially when args happens to be empty.
> Without -pedantic, GCC silently drops the comma.
>
> Bruce is asking for a consensus about -pedantic, whether we want to do the
> extra effort to support it in DPDK. Since I like checking for -pedantic
> errors, it's enabled for mlx4 and mlx5 when compiling these drivers in
> debugging mode. There is currently no established rule in DPDK against this.
>
> I'm arguing that most C headers (C compiler, libc, most libraries, even the
> Linux kernel in uapi to an extent) provide standards compliant includes
> because they cannot predict or force particular compilation flags on
> user applications.
>
> If we consider DPDK as a system wide library, I think we should do it as
> well in all installed header files. If we choose not to, then we must
> document that our code is not standard, -pedantic is unsupported and I'll
> have to drop it from mlx4 and mlx5.
>
> --
I'm in favour in principle of being standards compliant so long as the cost is
not extremely high. If we have to go through a lot of macro gymnastics to get
things working in order to support pedantic, I'd be of the opinion that the cost
of supporting that particular flag is too high. Each one will probably have his
own opinion of this.
Hopefully a static inline can provide a good compromise solution that everyone
can live with. I'll look at it as soon as I can.
/Bruce
next prev parent reply other threads:[~2015-11-05 15:17 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-12 11:28 [dpdk-dev] [PATCH 0/4] ethdev: Add checks for function support in driver Bruce Richardson
2015-06-12 11:28 ` [dpdk-dev] [PATCH 1/4] ethdev: rename macros to have RTE_ETH prefix Bruce Richardson
2015-06-12 11:28 ` [dpdk-dev] [PATCH 2/4] ethdev: move RTE_ETH_FPTR_OR_ERR macros to header Bruce Richardson
2015-06-12 11:28 ` [dpdk-dev] [PATCH 3/4] ethdev: remove duplicated debug functions Bruce Richardson
2015-06-12 11:28 ` [dpdk-dev] [PATCH 4/4] ethdev: check support for rx_queue_count and descriptor_done fns Bruce Richardson
2015-06-12 17:32 ` Roger B. Melton
2015-06-15 10:14 ` Bruce Richardson
2015-07-06 15:11 ` Thomas Monjalon
2015-07-26 20:44 ` Thomas Monjalon
2015-09-09 15:09 ` [dpdk-dev] [PATCH v2 0/4] ethdev: minor cleanup Bruce Richardson
2015-09-09 15:09 ` [dpdk-dev] [PATCH v2 1/4] ethdev: rename macros to have RTE_ETH prefix Bruce Richardson
2015-09-09 15:09 ` [dpdk-dev] [PATCH v2 2/4] ethdev: move error checking macros to header Bruce Richardson
2015-09-09 15:09 ` [dpdk-dev] [PATCH v2 3/4] ethdev: remove duplicated debug functions Bruce Richardson
2015-09-09 15:09 ` [dpdk-dev] [PATCH v2 4/4] ethdev: check driver support for functions Bruce Richardson
2015-09-28 10:23 ` [dpdk-dev] [PATCH v2 0/4] ethdev: minor cleanup Bruce Richardson
2015-11-03 1:11 ` Thomas Monjalon
2015-11-03 10:06 ` Bruce Richardson
2015-11-03 12:00 ` [dpdk-dev] [PATCH v3 " Bruce Richardson
2015-11-03 12:00 ` [dpdk-dev] [PATCH v3 1/4] ethdev: rename macros to have RTE_ETH prefix Bruce Richardson
2015-11-03 12:00 ` [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header Bruce Richardson
2015-11-04 1:19 ` Thomas Monjalon
2015-11-04 10:24 ` Adrien Mazarguil
2015-11-04 14:10 ` Bruce Richardson
2015-11-04 15:25 ` Adrien Mazarguil
2015-11-04 18:39 ` Stephen Hemminger
2015-11-05 15:09 ` Adrien Mazarguil
2015-11-05 15:17 ` Bruce Richardson [this message]
2015-11-06 11:49 ` Bruce Richardson
2015-11-06 17:10 ` Bruce Richardson
2015-11-06 17:22 ` Bruce Richardson
2015-11-09 13:39 ` Adrien Mazarguil
2015-11-09 13:50 ` Adrien Mazarguil
2015-11-09 14:02 ` Richardson, Bruce
2015-11-10 10:31 ` Declan Doherty
2015-11-10 16:08 ` Adrien Mazarguil
2015-11-10 16:21 ` Richardson, Bruce
2015-11-10 17:12 ` Adrien Mazarguil
2015-11-11 10:51 ` Bruce Richardson
2015-11-03 12:00 ` [dpdk-dev] [PATCH v3 3/4] ethdev: remove duplicated debug functions Bruce Richardson
2015-11-03 12:00 ` [dpdk-dev] [PATCH v3 4/4] ethdev: check driver support for functions Bruce Richardson
2015-11-03 22:00 ` Stephen Hemminger
2015-11-04 14:15 ` Bruce Richardson
2015-11-17 12:21 ` [dpdk-dev] [PATCH v4 0/2] ethdev: debug code cleanup Bruce Richardson
2015-11-17 12:21 ` [dpdk-dev] [PATCH v4 1/2] ethdev: remove duplicated debug functions Bruce Richardson
2015-11-17 12:21 ` [dpdk-dev] [PATCH v4 2/2] ethdev: add sanity checks to functions Bruce Richardson
2015-11-17 15:53 ` Stephen Hemminger
2015-11-24 14:56 ` Bruce Richardson
2015-11-24 15:29 ` Thomas Monjalon
2015-11-24 15:45 ` Bruce Richardson
2015-11-24 15:48 ` Thomas Monjalon
2015-11-24 17:37 ` [dpdk-dev] [PATCH v5 0/2] ethdev: debug code cleanup Bruce Richardson
2015-11-24 17:37 ` [dpdk-dev] [PATCH v5 1/2] ethdev: remove duplicated debug functions Bruce Richardson
2015-11-25 18:14 ` Thomas Monjalon
2015-11-24 17:37 ` [dpdk-dev] [PATCH v5 2/2] ethdev: add sanity checks to functions Bruce Richardson
2015-11-25 18:21 ` [dpdk-dev] [PATCH v5 0/2] ethdev: debug code cleanup Thomas Monjalon
2015-11-06 11:52 [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header Bruce Richardson
2015-11-06 12:25 ` Adrien Mazarguil
2015-11-06 14:39 ` Richardson, Bruce
2015-11-06 14:54 ` Adrien Mazarguil
2015-11-06 15:30 ` Richardson, Bruce
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151105151749.GA26416@bricha3-MOBL3 \
--to=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=stephen@networkplumber.org \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).