[v4,1/7] eal: introduce portable format attribute
Checks
Commit Message
When using __attribute__((format(...)) on functions, GCC on Windows
assumes MS-specific format string by default, even if the underlying
stdio implementation is ANSI-compliant (either MS Unicersal CRT
or MinGW implementation). Wrap attribute into a macro that forces
GNU-specific format string when using GCC.
Use this new attribute for logging and panic messages in EAL
and for output strings in cmdline library.
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
lib/librte_cmdline/cmdline.h | 4 +++-
lib/librte_eal/common/include/rte_common.h | 17 ++++++++++++++++-
lib/librte_eal/common/include/rte_debug.h | 2 +-
lib/librte_eal/common/include/rte_devargs.h | 2 +-
lib/librte_eal/common/include/rte_log.h | 4 ++--
5 files changed, 23 insertions(+), 6 deletions(-)
Comments
27/02/2020 05:25, Dmitry Kozlyuk:
> When using __attribute__((format(...)) on functions, GCC on Windows
> assumes MS-specific format string by default, even if the underlying
> stdio implementation is ANSI-compliant (either MS Unicersal CRT
> or MinGW implementation). Wrap attribute into a macro that forces
> GNU-specific format string when using GCC.
>
> Use this new attribute for logging and panic messages in EAL
> and for output strings in cmdline library.
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> +/**
> + * Check format string and its arguments at compile-time.
> + *
> + * GCC on Windows assumes MS-specific format string by default,
> + * even if the underlying stdio implementation is ANSI-compliant,
> + * so this must be overridden.
> + */
> +#if defined(RTE_TOOLCHAIN_GCC)
> +#define __rte_format_printf(format_index, first_arg) \
> + __attribute__((format(gnu_printf, format_index, first_arg)))
> +#else
> +#define __rte_format_printf(format_index, first_arg) \
> + __attribute__((format(printf, format_index, first_arg)))
> +#endif
It does not work when compiling pmdinfogen with clang and drivers with gcc.
I suggest this change (I can send a patch fixing the issue in other .h files):
+/*
+ * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
+ * while a host application (like pmdinfogen) may have another compiler.
+ * RTE_CC_IS_GNU is true if the file is compiled with GCC,
+ * no matter it is a target or host application.
+ */
+#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
+#define RTE_CC_IS_GNU
+#endif
+
+#ifdef RTE_CC_IS_GNU
-/** Define GCC_VERSION **/
-#ifdef RTE_TOOLCHAIN_GCC
#define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
__GNUC_PATCHLEVEL__)
#endif
@@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
* even if the underlying stdio implementation is ANSI-compliant,
* so this must be overridden.
*/
-#if defined(RTE_TOOLCHAIN_GCC)
+#ifdef RTE_CC_IS_GNU
#define __rte_format_printf(format_index, first_arg) \
__attribute__((format(gnu_printf, format_index, first_arg)))
#else
> I suggest this change (I can send a patch fixing the issue in other .h files):
>
> +/*
> + * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
> + * while a host application (like pmdinfogen) may have another compiler.
> + * RTE_CC_IS_GNU is true if the file is compiled with GCC,
> + * no matter it is a target or host application.
> + */
> +#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
> +#define RTE_CC_IS_GNU
> +#endif
> +
> +#ifdef RTE_CC_IS_GNU
> -/** Define GCC_VERSION **/
> -#ifdef RTE_TOOLCHAIN_GCC
> #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
> __GNUC_PATCHLEVEL__)
> #endif
> @@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
> * even if the underlying stdio implementation is ANSI-compliant,
> * so this must be overridden.
> */
> -#if defined(RTE_TOOLCHAIN_GCC)
> +#ifdef RTE_CC_IS_GNU
> #define __rte_format_printf(format_index, first_arg) \
> __attribute__((format(gnu_printf, format_index, first_arg)))
> #else
The code you propose LGTM itself. If you think it's a better solution than
the one proposed below, I see no problem going with it.
What I wonder is whether pmdinfogen should include the problematic code in the
first place. The errors come from declarations in rte_debug.h, but pmdinfogen
really can't use them, because definitions are compiled for different
machine. Pmdinfogen pulls rte_debug.h via rte_pci.h, which is only needed for
struct rte_pci_id. Shouldn't we instead break this bogus dependency chain by
moving struct rte_pci_id to a separate header?
EAL defines are tied to the target toolchain and are consistent with each
other, from this point of view RTE_CC_IS_GNU looks like a workaround. Another
reason why pmdinfogen should not depend on EAL is that its code will differ
considerably for POSIX and Windows (ELF vs COFF, mmap vs Win32 API).
14/03/2020 00:38, Dmitry Kozlyuk:
> > I suggest this change (I can send a patch fixing the issue in other .h files):
> >
> > +/*
> > + * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
> > + * while a host application (like pmdinfogen) may have another compiler.
> > + * RTE_CC_IS_GNU is true if the file is compiled with GCC,
> > + * no matter it is a target or host application.
> > + */
> > +#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
> > +#define RTE_CC_IS_GNU
> > +#endif
> > +
> > +#ifdef RTE_CC_IS_GNU
> > -/** Define GCC_VERSION **/
> > -#ifdef RTE_TOOLCHAIN_GCC
> > #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
> > __GNUC_PATCHLEVEL__)
> > #endif
> > @@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
> > * even if the underlying stdio implementation is ANSI-compliant,
> > * so this must be overridden.
> > */
> > -#if defined(RTE_TOOLCHAIN_GCC)
> > +#ifdef RTE_CC_IS_GNU
> > #define __rte_format_printf(format_index, first_arg) \
> > __attribute__((format(gnu_printf, format_index, first_arg)))
> > #else
>
> The code you propose LGTM itself. If you think it's a better solution than
> the one proposed below, I see no problem going with it.
>
> What I wonder is whether pmdinfogen should include the problematic code in the
> first place. The errors come from declarations in rte_debug.h, but pmdinfogen
> really can't use them, because definitions are compiled for different
> machine. Pmdinfogen pulls rte_debug.h via rte_pci.h, which is only needed for
> struct rte_pci_id. Shouldn't we instead break this bogus dependency chain by
> moving struct rte_pci_id to a separate header?
Splitting headers to avoid EAL dependency looks to be a bad precedent to me.
> EAL defines are tied to the target toolchain and are consistent with each
> other, from this point of view RTE_CC_IS_GNU looks like a workaround.
Yes there are some target-arch-dependent code in EAL.
But a host application is not supposed to use arch-dependent code.
> Another
> reason why pmdinfogen should not depend on EAL is that its code will differ
> considerably for POSIX and Windows (ELF vs COFF, mmap vs Win32 API).
I believe managing some OS differences should be helped with EAL.
On Sun, Mar 15, 2020 at 09:36:11AM +0100, Thomas Monjalon wrote:
> 14/03/2020 00:38, Dmitry Kozlyuk:
> > > I suggest this change (I can send a patch fixing the issue in other .h files):
> > >
> > > +/*
> > > + * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
> > > + * while a host application (like pmdinfogen) may have another compiler.
> > > + * RTE_CC_IS_GNU is true if the file is compiled with GCC,
> > > + * no matter it is a target or host application.
> > > + */
> > > +#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
> > > +#define RTE_CC_IS_GNU
> > > +#endif
> > > +
> > > +#ifdef RTE_CC_IS_GNU
> > > -/** Define GCC_VERSION **/
> > > -#ifdef RTE_TOOLCHAIN_GCC
> > > #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
> > > __GNUC_PATCHLEVEL__)
> > > #endif
> > > @@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
> > > * even if the underlying stdio implementation is ANSI-compliant,
> > > * so this must be overridden.
> > > */
> > > -#if defined(RTE_TOOLCHAIN_GCC)
> > > +#ifdef RTE_CC_IS_GNU
> > > #define __rte_format_printf(format_index, first_arg) \
> > > __attribute__((format(gnu_printf, format_index, first_arg)))
> > > #else
> >
> > The code you propose LGTM itself. If you think it's a better solution than
> > the one proposed below, I see no problem going with it.
> >
> > What I wonder is whether pmdinfogen should include the problematic code in the
> > first place. The errors come from declarations in rte_debug.h, but pmdinfogen
> > really can't use them, because definitions are compiled for different
> > machine. Pmdinfogen pulls rte_debug.h via rte_pci.h, which is only needed for
> > struct rte_pci_id. Shouldn't we instead break this bogus dependency chain by
> > moving struct rte_pci_id to a separate header?
>
> Splitting headers to avoid EAL dependency looks to be a bad precedent to me.
>
Rather than splitting, we can still fix this by breaking the dependency by
just not having rte_debug.h included in rte_pci.h. From what I see, there
is no need for that include to be there, and DPDK pretty much compiles fine
with it removed - the only other change I had to make was add an extra
include into mlx5_common.h to compensate for it not getting pulled in via
rte_pci.h.
Generally, while we should ensure that each header includes any other
headers it depends upon, we must be careful that headers don't include
other headers that they don't directly use.
/Bruce
On Mon, Mar 16, 2020 at 10:54:09AM +0000, Bruce Richardson wrote:
> On Sun, Mar 15, 2020 at 09:36:11AM +0100, Thomas Monjalon wrote:
> > 14/03/2020 00:38, Dmitry Kozlyuk:
> > > > I suggest this change (I can send a patch fixing the issue in other .h files):
> > > >
> > > > +/*
> > > > + * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
> > > > + * while a host application (like pmdinfogen) may have another compiler.
> > > > + * RTE_CC_IS_GNU is true if the file is compiled with GCC,
> > > > + * no matter it is a target or host application.
> > > > + */
> > > > +#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
> > > > +#define RTE_CC_IS_GNU
> > > > +#endif
> > > > +
> > > > +#ifdef RTE_CC_IS_GNU
> > > > -/** Define GCC_VERSION **/
> > > > -#ifdef RTE_TOOLCHAIN_GCC
> > > > #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
> > > > __GNUC_PATCHLEVEL__)
> > > > #endif
> > > > @@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
> > > > * even if the underlying stdio implementation is ANSI-compliant,
> > > > * so this must be overridden.
> > > > */
> > > > -#if defined(RTE_TOOLCHAIN_GCC)
> > > > +#ifdef RTE_CC_IS_GNU
> > > > #define __rte_format_printf(format_index, first_arg) \
> > > > __attribute__((format(gnu_printf, format_index, first_arg)))
> > > > #else
> > >
> > > The code you propose LGTM itself. If you think it's a better solution than
> > > the one proposed below, I see no problem going with it.
> > >
> > > What I wonder is whether pmdinfogen should include the problematic code in the
> > > first place. The errors come from declarations in rte_debug.h, but pmdinfogen
> > > really can't use them, because definitions are compiled for different
> > > machine. Pmdinfogen pulls rte_debug.h via rte_pci.h, which is only needed for
> > > struct rte_pci_id. Shouldn't we instead break this bogus dependency chain by
> > > moving struct rte_pci_id to a separate header?
> >
> > Splitting headers to avoid EAL dependency looks to be a bad precedent to me.
> >
>
> Rather than splitting, we can still fix this by breaking the dependency by
> just not having rte_debug.h included in rte_pci.h. From what I see, there
> is no need for that include to be there, and DPDK pretty much compiles fine
> with it removed - the only other change I had to make was add an extra
> include into mlx5_common.h to compensate for it not getting pulled in via
> rte_pci.h.
>
And by adding rte_interrupts.h to drivers/bus/ifpga/rte_bus_ifpga.h we can
remove the following headers from rte_pci.h also:
stdio.h
errno.h
stdint.h
rte_interrupts.h
This means that we go from 9 includes in the file (including rte_debug.h) to
just 4.
Regards,
/Bruce
16/03/2020 12:02, Bruce Richardson:
> On Mon, Mar 16, 2020 at 10:54:09AM +0000, Bruce Richardson wrote:
> > On Sun, Mar 15, 2020 at 09:36:11AM +0100, Thomas Monjalon wrote:
> > > 14/03/2020 00:38, Dmitry Kozlyuk:
> > > > > I suggest this change (I can send a patch fixing the issue in other .h files):
> > > > >
> > > > > +/*
> > > > > + * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
> > > > > + * while a host application (like pmdinfogen) may have another compiler.
> > > > > + * RTE_CC_IS_GNU is true if the file is compiled with GCC,
> > > > > + * no matter it is a target or host application.
> > > > > + */
> > > > > +#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
> > > > > +#define RTE_CC_IS_GNU
> > > > > +#endif
> > > > > +
> > > > > +#ifdef RTE_CC_IS_GNU
> > > > > -/** Define GCC_VERSION **/
> > > > > -#ifdef RTE_TOOLCHAIN_GCC
> > > > > #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
> > > > > __GNUC_PATCHLEVEL__)
> > > > > #endif
> > > > > @@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
> > > > > * even if the underlying stdio implementation is ANSI-compliant,
> > > > > * so this must be overridden.
> > > > > */
> > > > > -#if defined(RTE_TOOLCHAIN_GCC)
> > > > > +#ifdef RTE_CC_IS_GNU
> > > > > #define __rte_format_printf(format_index, first_arg) \
> > > > > __attribute__((format(gnu_printf, format_index, first_arg)))
> > > > > #else
> > > >
> > > > The code you propose LGTM itself. If you think it's a better solution than
> > > > the one proposed below, I see no problem going with it.
> > > >
> > > > What I wonder is whether pmdinfogen should include the problematic code in the
> > > > first place. The errors come from declarations in rte_debug.h, but pmdinfogen
> > > > really can't use them, because definitions are compiled for different
> > > > machine. Pmdinfogen pulls rte_debug.h via rte_pci.h, which is only needed for
> > > > struct rte_pci_id. Shouldn't we instead break this bogus dependency chain by
> > > > moving struct rte_pci_id to a separate header?
> > >
> > > Splitting headers to avoid EAL dependency looks to be a bad precedent to me.
> > >
> >
> > Rather than splitting, we can still fix this by breaking the dependency by
> > just not having rte_debug.h included in rte_pci.h. From what I see, there
> > is no need for that include to be there, and DPDK pretty much compiles fine
> > with it removed - the only other change I had to make was add an extra
> > include into mlx5_common.h to compensate for it not getting pulled in via
> > rte_pci.h.
> >
>
> And by adding rte_interrupts.h to drivers/bus/ifpga/rte_bus_ifpga.h we can
> remove the following headers from rte_pci.h also:
>
> stdio.h
> errno.h
> stdint.h
> rte_interrupts.h
>
> This means that we go from 9 includes in the file (including rte_debug.h) to
> just 4.
Thank you Bruce
Are you sending the patch or you prefer I do it?
On Mon, Mar 16, 2020 at 12:14:51PM +0100, Thomas Monjalon wrote:
> 16/03/2020 12:02, Bruce Richardson:
> > On Mon, Mar 16, 2020 at 10:54:09AM +0000, Bruce Richardson wrote:
> > > On Sun, Mar 15, 2020 at 09:36:11AM +0100, Thomas Monjalon wrote:
> > > > 14/03/2020 00:38, Dmitry Kozlyuk:
> > > > > > I suggest this change (I can send a patch fixing the issue in other .h files):
> > > > > >
> > > > > > +/*
> > > > > > + * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
> > > > > > + * while a host application (like pmdinfogen) may have another compiler.
> > > > > > + * RTE_CC_IS_GNU is true if the file is compiled with GCC,
> > > > > > + * no matter it is a target or host application.
> > > > > > + */
> > > > > > +#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
> > > > > > +#define RTE_CC_IS_GNU
> > > > > > +#endif
> > > > > > +
> > > > > > +#ifdef RTE_CC_IS_GNU
> > > > > > -/** Define GCC_VERSION **/
> > > > > > -#ifdef RTE_TOOLCHAIN_GCC
> > > > > > #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
> > > > > > __GNUC_PATCHLEVEL__)
> > > > > > #endif
> > > > > > @@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
> > > > > > * even if the underlying stdio implementation is ANSI-compliant,
> > > > > > * so this must be overridden.
> > > > > > */
> > > > > > -#if defined(RTE_TOOLCHAIN_GCC)
> > > > > > +#ifdef RTE_CC_IS_GNU
> > > > > > #define __rte_format_printf(format_index, first_arg) \
> > > > > > __attribute__((format(gnu_printf, format_index, first_arg)))
> > > > > > #else
> > > > >
> > > > > The code you propose LGTM itself. If you think it's a better solution than
> > > > > the one proposed below, I see no problem going with it.
> > > > >
> > > > > What I wonder is whether pmdinfogen should include the problematic code in the
> > > > > first place. The errors come from declarations in rte_debug.h, but pmdinfogen
> > > > > really can't use them, because definitions are compiled for different
> > > > > machine. Pmdinfogen pulls rte_debug.h via rte_pci.h, which is only needed for
> > > > > struct rte_pci_id. Shouldn't we instead break this bogus dependency chain by
> > > > > moving struct rte_pci_id to a separate header?
> > > >
> > > > Splitting headers to avoid EAL dependency looks to be a bad precedent to me.
> > > >
> > >
> > > Rather than splitting, we can still fix this by breaking the dependency by
> > > just not having rte_debug.h included in rte_pci.h. From what I see, there
> > > is no need for that include to be there, and DPDK pretty much compiles fine
> > > with it removed - the only other change I had to make was add an extra
> > > include into mlx5_common.h to compensate for it not getting pulled in via
> > > rte_pci.h.
> > >
> >
> > And by adding rte_interrupts.h to drivers/bus/ifpga/rte_bus_ifpga.h we can
> > remove the following headers from rte_pci.h also:
> >
> > stdio.h
> > errno.h
> > stdint.h
> > rte_interrupts.h
> >
> > This means that we go from 9 includes in the file (including rte_debug.h) to
> > just 4.
>
> Thank you Bruce
>
> Are you sending the patch or you prefer I do it?
>
Just running through test-meson-builds.sh tests with it, and then I'll send
it on.
/Bruce
On Mon, Mar 16, 2020 at 12:14:51PM +0100, Thomas Monjalon wrote:
> 16/03/2020 12:02, Bruce Richardson:
> > On Mon, Mar 16, 2020 at 10:54:09AM +0000, Bruce Richardson wrote:
> > > On Sun, Mar 15, 2020 at 09:36:11AM +0100, Thomas Monjalon wrote:
> > > > 14/03/2020 00:38, Dmitry Kozlyuk:
> > > > > > I suggest this change (I can send a patch fixing the issue in other .h files):
> > > > > >
> > > > > > +/*
> > > > > > + * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
> > > > > > + * while a host application (like pmdinfogen) may have another compiler.
> > > > > > + * RTE_CC_IS_GNU is true if the file is compiled with GCC,
> > > > > > + * no matter it is a target or host application.
> > > > > > + */
> > > > > > +#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
> > > > > > +#define RTE_CC_IS_GNU
> > > > > > +#endif
> > > > > > +
> > > > > > +#ifdef RTE_CC_IS_GNU
> > > > > > -/** Define GCC_VERSION **/
> > > > > > -#ifdef RTE_TOOLCHAIN_GCC
> > > > > > #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
> > > > > > __GNUC_PATCHLEVEL__)
> > > > > > #endif
> > > > > > @@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
> > > > > > * even if the underlying stdio implementation is ANSI-compliant,
> > > > > > * so this must be overridden.
> > > > > > */
> > > > > > -#if defined(RTE_TOOLCHAIN_GCC)
> > > > > > +#ifdef RTE_CC_IS_GNU
> > > > > > #define __rte_format_printf(format_index, first_arg) \
> > > > > > __attribute__((format(gnu_printf, format_index, first_arg)))
> > > > > > #else
> > > > >
> > > > > The code you propose LGTM itself. If you think it's a better solution than
> > > > > the one proposed below, I see no problem going with it.
> > > > >
> > > > > What I wonder is whether pmdinfogen should include the problematic code in the
> > > > > first place. The errors come from declarations in rte_debug.h, but pmdinfogen
> > > > > really can't use them, because definitions are compiled for different
> > > > > machine. Pmdinfogen pulls rte_debug.h via rte_pci.h, which is only needed for
> > > > > struct rte_pci_id. Shouldn't we instead break this bogus dependency chain by
> > > > > moving struct rte_pci_id to a separate header?
> > > >
> > > > Splitting headers to avoid EAL dependency looks to be a bad precedent to me.
> > > >
> > >
> > > Rather than splitting, we can still fix this by breaking the dependency by
> > > just not having rte_debug.h included in rte_pci.h. From what I see, there
> > > is no need for that include to be there, and DPDK pretty much compiles fine
> > > with it removed - the only other change I had to make was add an extra
> > > include into mlx5_common.h to compensate for it not getting pulled in via
> > > rte_pci.h.
> > >
> >
> > And by adding rte_interrupts.h to drivers/bus/ifpga/rte_bus_ifpga.h we can
> > remove the following headers from rte_pci.h also:
> >
> > stdio.h
> > errno.h
> > stdint.h
> > rte_interrupts.h
> >
> > This means that we go from 9 includes in the file (including rte_debug.h) to
> > just 4.
>
> Thank you Bruce
>
> Are you sending the patch or you prefer I do it?
>
For completeness, patch now at: http://patches.dpdk.org/patch/66701/
14/03/2020 00:38, Dmitry Kozlyuk:
> > I suggest this change (I can send a patch fixing the issue in other .h files):
> >
> > +/*
> > + * RTE_TOOLCHAIN_GCC is true if the target is built with GCC,
> > + * while a host application (like pmdinfogen) may have another compiler.
> > + * RTE_CC_IS_GNU is true if the file is compiled with GCC,
> > + * no matter it is a target or host application.
> > + */
> > +#if defined __GNUC__ && !defined __clang__ && !defined __INTEL_COMPILER
> > +#define RTE_CC_IS_GNU
> > +#endif
> > +
> > +#ifdef RTE_CC_IS_GNU
> > -/** Define GCC_VERSION **/
> > -#ifdef RTE_TOOLCHAIN_GCC
> > #define GCC_VERSION (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + \
> > __GNUC_PATCHLEVEL__)
> > #endif
> > @@ -96,7 +105,7 @@ typedef uint16_t unaligned_uint16_t;
> > * even if the underlying stdio implementation is ANSI-compliant,
> > * so this must be overridden.
> > */
> > -#if defined(RTE_TOOLCHAIN_GCC)
> > +#ifdef RTE_CC_IS_GNU
> > #define __rte_format_printf(format_index, first_arg) \
> > __attribute__((format(gnu_printf, format_index, first_arg)))
> > #else
>
> The code you propose LGTM itself. If you think it's a better solution than
> the one proposed below, I see no problem going with it.
>
> What I wonder is whether pmdinfogen should include the problematic code in the
> first place. The errors come from declarations in rte_debug.h,
No the error comes from rte_common.h included by pmdinfogen.c
Please review this patch:
https://patches.dpdk.org/patch/66702/
@@ -7,6 +7,8 @@
#ifndef _CMDLINE_H_
#define _CMDLINE_H_
+#include <rte_common.h>
+
#include <termios.h>
#include <cmdline_rdline.h>
#include <cmdline_parse.h>
@@ -34,7 +36,7 @@ struct cmdline *cmdline_new(cmdline_parse_ctx_t *ctx, const char *prompt, int s_
void cmdline_set_prompt(struct cmdline *cl, const char *prompt);
void cmdline_free(struct cmdline *cl);
void cmdline_printf(const struct cmdline *cl, const char *fmt, ...)
- __attribute__((format(printf,2,3)));
+ __rte_format_printf(2, 3);
int cmdline_in(struct cmdline *cl, const char *buf, int size);
int cmdline_write_char(struct rdline *rdl, char c);
@@ -89,6 +89,21 @@ typedef uint16_t unaligned_uint16_t;
*/
#define RTE_SET_USED(x) (void)(x)
+/**
+ * Check format string and its arguments at compile-time.
+ *
+ * GCC on Windows assumes MS-specific format string by default,
+ * even if the underlying stdio implementation is ANSI-compliant,
+ * so this must be overridden.
+ */
+#if defined(RTE_TOOLCHAIN_GCC)
+#define __rte_format_printf(format_index, first_arg) \
+ __attribute__((format(gnu_printf, format_index, first_arg)))
+#else
+#define __rte_format_printf(format_index, first_arg) \
+ __attribute__((format(printf, format_index, first_arg)))
+#endif
+
#define RTE_PRIORITY_LOG 101
#define RTE_PRIORITY_BUS 110
#define RTE_PRIORITY_CLASS 120
@@ -784,7 +799,7 @@ rte_str_to_size(const char *str)
void
rte_exit(int exit_code, const char *format, ...)
__attribute__((noreturn))
- __attribute__((format(printf, 2, 3)));
+ __rte_format_printf(2, 3);
#ifdef __cplusplus
}
@@ -73,7 +73,7 @@ void __rte_panic(const char *funcname , const char *format, ...)
#endif
#endif
__attribute__((noreturn))
- __attribute__((format(printf, 2, 3)));
+ __rte_format_printf(2, 3);
#ifdef __cplusplus
}
@@ -137,7 +137,7 @@ rte_devargs_parse(struct rte_devargs *da, const char *dev);
int
rte_devargs_parsef(struct rte_devargs *da,
const char *format, ...)
-__attribute__((format(printf, 2, 0)));
+__rte_format_printf(2, 0);
/**
* Insert an rte_devargs in the global list.
@@ -282,7 +282,7 @@ int rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
__attribute__((cold))
#endif
#endif
- __attribute__((format(printf, 3, 4)));
+ __rte_format_printf(3, 4);
/**
* Generates a log message.
@@ -311,7 +311,7 @@ int rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
* - Negative on error.
*/
int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
- __attribute__((format(printf,3,0)));
+ __rte_format_printf(3, 0);
/**
* Generates a log message.