[v4,1/7] eal: introduce portable format attribute
diff mbox series

Message ID 20200227042537.187459-2-dmitry.kozliuk@gmail.com
State Accepted
Delegated to: Thomas Monjalon
Headers show
Series
  • MinGW-w64 support
Related show

Checks

Context Check Description
ci/iol-testing warning Testing issues
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk Feb. 27, 2020, 4:25 a.m. UTC
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

Thomas Monjalon March 12, 2020, 10:33 p.m. UTC | #1
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
Dmitry Kozlyuk March 13, 2020, 11:38 p.m. UTC | #2
> 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).
Thomas Monjalon March 15, 2020, 8:36 a.m. UTC | #3
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.
Bruce Richardson March 16, 2020, 10:54 a.m. UTC | #4
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
Bruce Richardson March 16, 2020, 11:02 a.m. UTC | #5
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
Thomas Monjalon March 16, 2020, 11:14 a.m. UTC | #6
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?
Bruce Richardson March 16, 2020, 11:18 a.m. UTC | #7
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
Bruce Richardson March 16, 2020, 11:35 a.m. UTC | #8
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/
Thomas Monjalon March 16, 2020, 2:27 p.m. UTC | #9
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/

Patch
diff mbox series

diff --git a/lib/librte_cmdline/cmdline.h b/lib/librte_cmdline/cmdline.h
index 27d2effdf..243f99d20 100644
--- a/lib/librte_cmdline/cmdline.h
+++ b/lib/librte_cmdline/cmdline.h
@@ -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);
 
diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 4b5f3a31f..226c1a011 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -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
 }
diff --git a/lib/librte_eal/common/include/rte_debug.h b/lib/librte_eal/common/include/rte_debug.h
index 748d32c80..7edd4b89c 100644
--- a/lib/librte_eal/common/include/rte_debug.h
+++ b/lib/librte_eal/common/include/rte_debug.h
@@ -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
 }
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 882dfa0ab..898efa0d6 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -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.
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 1bb0e6694..a0d1f4837 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -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.