* [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
@ 2020-02-27 4:25 ` Dmitry Kozlyuk
2020-03-12 22:33 ` Thomas Monjalon
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 2/7] eal/windows: use lowercase filenames for system headers Dmitry Kozlyuk
` (7 subsequent siblings)
8 siblings, 1 reply; 32+ messages in thread
From: Dmitry Kozlyuk @ 2020-02-27 4:25 UTC (permalink / raw)
To: dev; +Cc: Dmitry Kozlyuk, Olivier Matz
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(-)
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.
--
2.25.1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute Dmitry Kozlyuk
@ 2020-03-12 22:33 ` Thomas Monjalon
2020-03-13 23:38 ` Dmitry Kozlyuk
0 siblings, 1 reply; 32+ messages in thread
From: Thomas Monjalon @ 2020-03-12 22:33 UTC (permalink / raw)
To: Dmitry Kozlyuk; +Cc: dev, Olivier Matz
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-03-12 22:33 ` Thomas Monjalon
@ 2020-03-13 23:38 ` Dmitry Kozlyuk
2020-03-15 8:36 ` Thomas Monjalon
2020-03-16 14:27 ` Thomas Monjalon
0 siblings, 2 replies; 32+ messages in thread
From: Dmitry Kozlyuk @ 2020-03-13 23:38 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, Olivier Matz
> 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).
--
Dmitry Kozlyuk
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-03-13 23:38 ` Dmitry Kozlyuk
@ 2020-03-15 8:36 ` Thomas Monjalon
2020-03-16 10:54 ` Bruce Richardson
2020-03-16 14:27 ` Thomas Monjalon
1 sibling, 1 reply; 32+ messages in thread
From: Thomas Monjalon @ 2020-03-15 8:36 UTC (permalink / raw)
To: Dmitry Kozlyuk; +Cc: dev, Olivier Matz
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-03-15 8:36 ` Thomas Monjalon
@ 2020-03-16 10:54 ` Bruce Richardson
2020-03-16 11:02 ` Bruce Richardson
0 siblings, 1 reply; 32+ messages in thread
From: Bruce Richardson @ 2020-03-16 10:54 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Dmitry Kozlyuk, dev, Olivier Matz
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-03-16 10:54 ` Bruce Richardson
@ 2020-03-16 11:02 ` Bruce Richardson
2020-03-16 11:14 ` Thomas Monjalon
0 siblings, 1 reply; 32+ messages in thread
From: Bruce Richardson @ 2020-03-16 11:02 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Dmitry Kozlyuk, dev, Olivier Matz
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-03-16 11:02 ` Bruce Richardson
@ 2020-03-16 11:14 ` Thomas Monjalon
2020-03-16 11:18 ` Bruce Richardson
2020-03-16 11:35 ` Bruce Richardson
0 siblings, 2 replies; 32+ messages in thread
From: Thomas Monjalon @ 2020-03-16 11:14 UTC (permalink / raw)
To: Bruce Richardson; +Cc: Dmitry Kozlyuk, dev, Olivier Matz
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?
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-03-16 11:14 ` Thomas Monjalon
@ 2020-03-16 11:18 ` Bruce Richardson
2020-03-16 11:35 ` Bruce Richardson
1 sibling, 0 replies; 32+ messages in thread
From: Bruce Richardson @ 2020-03-16 11:18 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Dmitry Kozlyuk, dev, Olivier Matz
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-03-16 11:14 ` Thomas Monjalon
2020-03-16 11:18 ` Bruce Richardson
@ 2020-03-16 11:35 ` Bruce Richardson
1 sibling, 0 replies; 32+ messages in thread
From: Bruce Richardson @ 2020-03-16 11:35 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Dmitry Kozlyuk, dev, Olivier Matz
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/
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute
2020-03-13 23:38 ` Dmitry Kozlyuk
2020-03-15 8:36 ` Thomas Monjalon
@ 2020-03-16 14:27 ` Thomas Monjalon
1 sibling, 0 replies; 32+ messages in thread
From: Thomas Monjalon @ 2020-03-16 14:27 UTC (permalink / raw)
To: Dmitry Kozlyuk; +Cc: dev, Olivier Matz
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/
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH v4 2/7] eal/windows: use lowercase filenames for system headers
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute Dmitry Kozlyuk
@ 2020-02-27 4:25 ` Dmitry Kozlyuk
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 3/7] eal/windows: support builing with MinGW-w64 Dmitry Kozlyuk
` (6 subsequent siblings)
8 siblings, 0 replies; 32+ messages in thread
From: Dmitry Kozlyuk @ 2020-02-27 4:25 UTC (permalink / raw)
To: dev
Cc: Dmitry Kozlyuk, Narcisa Ana Maria Vasile, William Tu,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon
Mixed case in Windows header names causes errors when cross-compiling
from Linux with case-sensitive filesystem using MinGW, because MinGW
distribution provides all platform SDK headers in lowercase. The change
does not affect Windows native builds on case-insensitive filesystems
(NTFS default).
Reported-by: Narcisa Ana Maria Vasile <Narcisa.Vasile@microsoft.com>
Reported-by: William Tu <u9012063@gmail.com>
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
lib/librte_eal/windows/eal/include/pthread.h | 2 +-
lib/librte_eal/windows/eal/include/rte_os.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/librte_eal/windows/eal/include/pthread.h b/lib/librte_eal/windows/eal/include/pthread.h
index 4ac24de0a..b9dd18e56 100644
--- a/lib/librte_eal/windows/eal/include/pthread.h
+++ b/lib/librte_eal/windows/eal/include/pthread.h
@@ -14,7 +14,7 @@
extern "C" {
#endif
-#include <Windows.h>
+#include <windows.h>
#define PTHREAD_BARRIER_SERIAL_THREAD TRUE
diff --git a/lib/librte_eal/windows/eal/include/rte_os.h b/lib/librte_eal/windows/eal/include/rte_os.h
index c76be1216..95a19b2d3 100644
--- a/lib/librte_eal/windows/eal/include/rte_os.h
+++ b/lib/librte_eal/windows/eal/include/rte_os.h
@@ -15,8 +15,8 @@
extern "C" {
#endif
-#include <Windows.h>
-#include <BaseTsd.h>
+#include <windows.h>
+#include <basetsd.h>
#include <pthread.h>
#include <stdio.h>
--
2.25.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [dpdk-dev] [PATCH v4 3/7] eal/windows: support builing with MinGW-w64
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 1/7] eal: introduce portable format attribute Dmitry Kozlyuk
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 2/7] eal/windows: use lowercase filenames for system headers Dmitry Kozlyuk
@ 2020-02-27 4:25 ` Dmitry Kozlyuk
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 4/7] build: MinGW-w64 support for Meson Dmitry Kozlyuk
` (5 subsequent siblings)
8 siblings, 0 replies; 32+ messages in thread
From: Dmitry Kozlyuk @ 2020-02-27 4:25 UTC (permalink / raw)
To: dev
Cc: Dmitry Kozlyuk, Harini Ramakrishnan, Omar Cardona, Pallavi Kadam,
Ranjit Menon
Disable bundled getopt implementation and GNU extensions shim when
building in GNU environment.
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
lib/librte_eal/meson.build | 3 +++
lib/librte_eal/windows/eal/eal.c | 6 ++++--
lib/librte_eal/windows/eal/include/getopt.h | 4 ++++
lib/librte_eal/windows/eal/include/rte_os.h | 2 ++
4 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
index 4be5118ce..1730d603f 100644
--- a/lib/librte_eal/meson.build
+++ b/lib/librte_eal/meson.build
@@ -20,6 +20,9 @@ endif
if cc.has_function('getentropy', prefix : '#include <unistd.h>')
cflags += '-DRTE_LIBEAL_USE_GETENTROPY'
endif
+if cc.has_header('getopt.h')
+ cflags += ['-DHAVE_GETOPT_H', '-DHAVE_GETOPT', '-DHAVE_GETOPT_LONG']
+endif
sources = common_sources + env_sources
objs = common_objs + env_objs
headers = common_headers + env_headers
diff --git a/lib/librte_eal/windows/eal/eal.c b/lib/librte_eal/windows/eal/eal.c
index 34852d42c..e4b50df3b 100644
--- a/lib/librte_eal/windows/eal/eal.c
+++ b/lib/librte_eal/windows/eal/eal.c
@@ -2,9 +2,11 @@
* Copyright(c) 2019 Intel Corporation
*/
-#include <sys/stat.h>
-#include <io.h>
#include <fcntl.h>
+#include <io.h>
+#include <share.h>
+#include <sys/stat.h>
+
#include <rte_debug.h>
#include <rte_eal.h>
#include <eal_memcfg.h>
diff --git a/lib/librte_eal/windows/eal/include/getopt.h b/lib/librte_eal/windows/eal/include/getopt.h
index 2eebe54e3..6f57af454 100644
--- a/lib/librte_eal/windows/eal/include/getopt.h
+++ b/lib/librte_eal/windows/eal/include/getopt.h
@@ -26,7 +26,11 @@
#ifndef NEED_USUAL_GETOPT
/* Use system getopt */
+#ifdef RTE_TOOLCHAIN_GCC
+#include_next <getopt.h>
+#else
#include <getopt.h>
+#endif
#else /* NEED_USUAL_GETOPT */
diff --git a/lib/librte_eal/windows/eal/include/rte_os.h b/lib/librte_eal/windows/eal/include/rte_os.h
index 95a19b2d3..e1e0378e6 100644
--- a/lib/librte_eal/windows/eal/include/rte_os.h
+++ b/lib/librte_eal/windows/eal/include/rte_os.h
@@ -57,6 +57,7 @@ int eal_thread_create(pthread_t *thread);
*/
void eal_create_cpu_map(void);
+#ifndef RTE_TOOLCHAIN_GCC
static inline int
asprintf(char **buffer, const char *format, ...)
{
@@ -83,6 +84,7 @@ asprintf(char **buffer, const char *format, ...)
}
return ret;
}
+#endif /* RTE_TOOLCHAIN_GCC */
/* cpu_set macros implementation */
#define RTE_CPU_AND(dst, src1, src2) CPU_AND(dst, src1, src2)
--
2.25.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [dpdk-dev] [PATCH v4 4/7] build: MinGW-w64 support for Meson
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
` (2 preceding siblings ...)
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 3/7] eal/windows: support builing with MinGW-w64 Dmitry Kozlyuk
@ 2020-02-27 4:25 ` Dmitry Kozlyuk
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 5/7] build: add cross-file for MinGW-w64 Dmitry Kozlyuk
` (4 subsequent siblings)
8 siblings, 0 replies; 32+ messages in thread
From: Dmitry Kozlyuk @ 2020-02-27 4:25 UTC (permalink / raw)
To: dev; +Cc: Dmitry Kozlyuk, Thomas Monjalon
MinGW-w64 linker does not mimic MS linker options, so the build system
must differentiate between linkers on Windows. Use GNU linker options
with GCC and MS linker options with Clang.
MinGW-w64 by default uses MSVCRT stdio, which does not comply to ANSI,
most notably its formatting and string handling functions. MinGW-w64
support for the Universal CRT (UCRT) is ongoing, but the toolchain
provides its own standard-complying implementation of stdio. The latter
is used in the patch to support formatting in DPDK.
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
config/meson.build | 14 ++++++++++++++
lib/meson.build | 8 ++++++--
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/config/meson.build b/config/meson.build
index d3d2370ce..68aeb8470 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -14,6 +14,10 @@ foreach env:supported_exec_envs
set_variable('is_' + env, exec_env == env)
endforeach
+# MS linker requires special treatment.
+# TODO: use cc.get_linker_id() with Meson >= 0.54
+is_ms_linker = is_windows and (cc.get_id() == 'clang')
+
# set the major version, which might be used by drivers and libraries
# depending on the configuration options
pver = meson.project_version().split('.')
@@ -247,6 +251,16 @@ if is_freebsd
add_project_arguments('-D__BSD_VISIBLE', language: 'c')
endif
+if is_windows
+ # Minimum supported API is Windows 7.
+ add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')
+
+ # Use MinGW-w64 stdio, because DPDK assumes ANSI-compliant formatting.
+ if cc.get_id() == 'gcc'
+ add_project_arguments('-D__USE_MINGW_ANSI_STDIO', language: 'c')
+ endif
+endif
+
if get_option('b_lto')
if cc.has_argument('-ffat-lto-objects')
add_project_arguments('-ffat-lto-objects', language: 'c')
diff --git a/lib/meson.build b/lib/meson.build
index 0af3efab2..9c3cc55d5 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -148,12 +148,16 @@ foreach l:libraries
command: [map_to_def_cmd, '@INPUT@', '@OUTPUT@'],
input: version_map,
output: 'rte_@0@_exports.def'.format(name))
- lk_deps = [version_map, def_file]
- if is_windows
+
+ if is_ms_linker
lk_args = ['-Wl,/def:' + def_file.full_path(),
'-Wl,/implib:lib\\' + implib]
else
lk_args = ['-Wl,--version-script=' + version_map]
+ endif
+
+ lk_deps = [version_map, def_file]
+ if not is_windows
# on unix systems check the output of the
# experimental syms script, using it as a
# dependency of the .so build
--
2.25.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [dpdk-dev] [PATCH v4 5/7] build: add cross-file for MinGW-w64
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
` (3 preceding siblings ...)
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 4/7] build: MinGW-w64 support for Meson Dmitry Kozlyuk
@ 2020-02-27 4:25 ` Dmitry Kozlyuk
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 6/7] doc: guide for Windows build using MinGW-w64 Dmitry Kozlyuk
` (3 subsequent siblings)
8 siblings, 0 replies; 32+ messages in thread
From: Dmitry Kozlyuk @ 2020-02-27 4:25 UTC (permalink / raw)
To: dev; +Cc: Dmitry Kozlyuk, Thomas Monjalon
Add Meson configuration to cross-compile for Windows using MinGW-w64.
It may require adjustments in some cases, but at least it provides
the foundation.
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
config/x86/meson_mingw.txt | 13 +++++++++++++
1 file changed, 13 insertions(+)
create mode 100644 config/x86/meson_mingw.txt
diff --git a/config/x86/meson_mingw.txt b/config/x86/meson_mingw.txt
new file mode 100644
index 000000000..fac33cfcd
--- /dev/null
+++ b/config/x86/meson_mingw.txt
@@ -0,0 +1,13 @@
+[binaries]
+c = '/usr/bin/x86_64-w64-mingw32-gcc'
+cpp = '/usr/bin/x86_64-w64-mingw32-g++'
+ld = '/usr/bin/x86_64-w64-mingw32-ld'
+ar = '/usr/bin/x86_64-w64-mingw32-ar'
+strip = '/usr/bin/x86_64-w64-mingw32-strip'
+pkgconfig = '/usr/bin/x86_64-w64-mingw32-pkg-config'
+
+[host_machine]
+system = 'windows'
+cpu_family = 'x86_64'
+cpu = 'native'
+endian = 'little'
--
2.25.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [dpdk-dev] [PATCH v4 6/7] doc: guide for Windows build using MinGW-w64
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
` (4 preceding siblings ...)
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 5/7] build: add cross-file for MinGW-w64 Dmitry Kozlyuk
@ 2020-02-27 4:25 ` Dmitry Kozlyuk
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 7/7] build: fix linker warnings with Clang on Windows Dmitry Kozlyuk
` (2 subsequent siblings)
8 siblings, 0 replies; 32+ messages in thread
From: Dmitry Kozlyuk @ 2020-02-27 4:25 UTC (permalink / raw)
To: dev
Cc: Dmitry Kozlyuk, Harini Ramakrishnan, Omar Cardona, Pallavi Kadam,
Ranjit Menon, John McNamara, Marko Kovacevic
Instructions for different toolchains presented as options on the
corresponging steps of the guide, so that common parts may be reused.
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
doc/guides/windows_gsg/build_dpdk.rst | 62 +++++++++++++++++++++++----
1 file changed, 53 insertions(+), 9 deletions(-)
diff --git a/doc/guides/windows_gsg/build_dpdk.rst b/doc/guides/windows_gsg/build_dpdk.rst
index 6711e07e2..d46e84e3f 100644
--- a/doc/guides/windows_gsg/build_dpdk.rst
+++ b/doc/guides/windows_gsg/build_dpdk.rst
@@ -7,15 +7,22 @@ Compiling the DPDK Target from Source
System Requirements
-------------------
-The DPDK and its applications require the Clang-LLVM C compiler
-and Microsoft MSVC linker.
+Building the DPDK and its applications requires one of the following
+environments:
+
+* The Clang-LLVM C compiler and Microsoft MSVC linker.
+* The MinGW-w64 toolchain (either native or cross).
+
The Meson Build system is used to prepare the sources for compilation
with the Ninja backend.
The installation of these tools is covered in this section.
+Option 1. Clang-LLVM C Compiler and Microsoft MSVC Linker
+---------------------------------------------------------
+
Install the Compiler
---------------------
+~~~~~~~~~~~~~~~~~~~~
Download and install the clang compiler from
`LLVM website <http://releases.llvm.org/download.html>`_.
@@ -25,7 +32,7 @@ For example, Clang-LLVM direct download link::
Install the Linker
-------------------
+~~~~~~~~~~~~~~~~~~
Download and install the Build Tools for Visual Studio to link and build the
files on windows,
@@ -34,6 +41,18 @@ When installing build tools, select the "Visual C++ build tools" option
and ensure the Windows SDK is selected.
+Option 2. MinGW-w64 Toolchain
+-----------------------------
+
+Obtain the latest version from
+`MinGW-w64 website <http://mingw-w64.org/doku.php/download>`_.
+On Windows, install to a folder without spaces in its name, like ``C:\MinGW``.
+This path is assumed for the rest of this guide.
+
+Version 4.0.4 for Ubuntu 16.04 cannot be used due to a
+`MinGW-w64 bug <https://sourceforge.net/p/mingw-w64/bugs/562/>`_.
+
+
Install the Build System
------------------------
@@ -43,6 +62,8 @@ A good option to choose is the MSI installer for both meson and ninja together::
http://mesonbuild.com/Getting-meson.html#installing-meson-and-ninja-with-the-msi-installer%22
+Recommended version is either Meson 0.47.1 (baseline) or the latest release.
+
Install the Backend
-------------------
@@ -56,23 +77,41 @@ Build the code
The build environment is setup to build the EAL and the helloworld example by
default.
-Using the ninja backend
-~~~~~~~~~~~~~~~~~~~~~~~~
+Option 1. Native Build on Windows
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Specifying the compiler might be required to complete the meson command.
+When using Clang-LLVM, specifying the compiler might be required to complete
+the meson command:
.. code-block:: console
set CC=clang
+When using MinGW-w64, it is sufficient to have toolchain executables in PATH:
+
+.. code-block:: console
+
+ set PATH=C:\MinGW\mingw64\bin;%PATH%
+
To compile the examples, the flag ``-Dexamples`` is required.
.. code-block:: console
cd C:\Users\me\dpdk
meson -Dexamples=helloworld build
- cd build
- ninja
+ ninja -C build
+
+Option 2. Cross-Compile with MinGW-w64
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The cross-file option must be specified for Meson.
+Depending on the distribution, paths in this file may need adjustments.
+
+.. code-block:: console
+
+ meson --cross-file config/x86/meson_mingw.txt -Dexamples=helloworld build
+ ninja -C build
+
Run the helloworld example
==========================
@@ -87,3 +126,8 @@ Navigate to the examples in the build directory and run `dpdk-helloworld.exe`.
hello from core 3
hello from core 0
hello from core 2
+
+Note for MinGW-w64: applications are linked to ``libwinpthread-1.dll``
+by default. To run the example, either add toolchain executables directory
+to the PATH or copy the library to the working directory.
+Alternatively, static linking may be used (mind the LGPLv2.1 license).
--
2.25.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [dpdk-dev] [PATCH v4 7/7] build: fix linker warnings with Clang on Windows
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
` (5 preceding siblings ...)
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 6/7] doc: guide for Windows build using MinGW-w64 Dmitry Kozlyuk
@ 2020-02-27 4:25 ` Dmitry Kozlyuk
2020-03-11 17:22 ` [dpdk-dev] [PATCH v4 0/7] MinGW-w64 support William Tu
2020-03-11 22:36 ` Kadam, Pallavi
8 siblings, 0 replies; 32+ messages in thread
From: Dmitry Kozlyuk @ 2020-02-27 4:25 UTC (permalink / raw)
To: dev
Cc: Dmitry Kozlyuk, Thomas Monjalon, Jeff Shaw, Pallavi Kadam,
Ranjit Menon, Harini Ramakrishnan, Anand Rawat
Clang on Windows doesn't use pthread for now, while MinGW does. Removing
`-pthread` option with MS linker fixes the following warning:
clang: warning: argument unused during compilation: '-pthread'
[-Wunused-command-line-argument]
Option `--no-as-needed` is meaningless for PE output. Disabling it on
Windows fixes the following warning:
LINK : warning LNK4044: unrecognized option '/-no-as-needed'; ignored
Fixes: 98edcbb5a ("eal/windows: introduce Windows support")
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
config/meson.build | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/config/meson.build b/config/meson.build
index 68aeb8470..abedd76f2 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -112,11 +112,15 @@ dpdk_conf.set('RTE_TOOLCHAIN_' + toolchain.to_upper(), 1)
dpdk_conf.set('RTE_ARCH_64', cc.sizeof('void *') == 8)
-add_project_link_arguments('-Wl,--no-as-needed', language: 'c')
+if not is_windows
+ add_project_link_arguments('-Wl,--no-as-needed', language: 'c')
+endif
-# use pthreads
-add_project_link_arguments('-pthread', language: 'c')
-dpdk_extra_ldflags += '-pthread'
+# use pthreads if available for the platform
+if not is_ms_linker
+ add_project_link_arguments('-pthread', language: 'c')
+ dpdk_extra_ldflags += '-pthread'
+endif
# on some OS, maths functions are in a separate library
if cc.find_library('m', required : false).found()
--
2.25.1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [dpdk-dev] [PATCH v4 0/7] MinGW-w64 support
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
` (6 preceding siblings ...)
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 7/7] build: fix linker warnings with Clang on Windows Dmitry Kozlyuk
@ 2020-03-11 17:22 ` William Tu
2020-03-18 0:24 ` Thomas Monjalon
2020-03-11 22:36 ` Kadam, Pallavi
8 siblings, 1 reply; 32+ messages in thread
From: William Tu @ 2020-03-11 17:22 UTC (permalink / raw)
To: Dmitry Kozlyuk
Cc: dev, Bruce Richardson, Thomas Monjalon, Olivier Matz,
Harini Ramakrishnan, Omar Cardona, Pallavi Kadam, Ranjit Menon,
John McNamara, Marko Kovacevic
On Wed, Feb 26, 2020 at 8:25 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> This patch series add support for building DPDK using MinGW-w64.
>
> MinGW-w64 provides GNU toolchain and independent platform SDK on
> Windows. It also supports cross-compilation to Windows from POSIX
> systems by providing cross tollchains and libraries [0]. It does NOT
> emulate a full POSIX environment, like Cygwin or MSYS do.
>
> There are advantages in using MinGW-w64 in addition to Clang:
>
> 1. Cross-compilation out-of-the-box. MinGW-w64 is provides a pthread
> implementation, GNU getopt, and Windows platform SDK.
>
> 2. Easier porting of POSIX applications using DPDK to Windows, because
> application code can use the same benefits as mentioned above.
>
> 3. Having both primary compilers enabled on Windows provides more
> diagnostics and generally prevents non-portable code.
>
> [0]: http://mingw-w64.org
>
> v4 Changes:
>
> Fix Windows headers include case.
> Recommend the latest Meson in docs (build fixed with 0.53.2).
>
> v3 Changes:
>
> Rebase onto the latest Windows EAL.
> Prevent format attribute conflict with upcoming v20.05 patches.
> Remove redundant CFLAGS from Meson.
> Remove links to Meson bugtracker from docs (PR merged into upstream).
> Fix Clang warnings using about GNU options.
>
> v2 Changes:
>
> Add patch to use lowercase system header filenames.
> Move Meson cross-file for x86 to arch directory.
> Change wording in comments.
> Add Meson version warning in documentation.
>
> Dmitry Kozlyuk (7):
> eal: introduce portable format attribute
> eal/windows: use lowercase filenames for system headers
> eal/windows: support builing with MinGW-w64
> build: MinGW-w64 support for Meson
> build: add cross-file for MinGW-w64
> doc: guide for Windows build using MinGW-w64
> build: fix linker warnings with Clang on Windows
>
> config/meson.build | 26 ++++++--
> config/x86/meson_mingw.txt | 13 ++++
> doc/guides/windows_gsg/build_dpdk.rst | 62 +++++++++++++++++---
> 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 +-
> lib/librte_eal/meson.build | 3 +
> lib/librte_eal/windows/eal/eal.c | 6 +-
> lib/librte_eal/windows/eal/include/getopt.h | 4 ++
> lib/librte_eal/windows/eal/include/pthread.h | 2 +-
> lib/librte_eal/windows/eal/include/rte_os.h | 6 +-
> lib/meson.build | 8 ++-
> 14 files changed, 133 insertions(+), 26 deletions(-)
> create mode 100644 config/x86/meson_mingw.txt
>
I reviewed and tested the v4 series and everything works ok.
Tested-by: William Tu <u9012063@gmail.com>
Thanks
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [dpdk-dev] [PATCH v4 0/7] MinGW-w64 support
2020-03-11 17:22 ` [dpdk-dev] [PATCH v4 0/7] MinGW-w64 support William Tu
@ 2020-03-18 0:24 ` Thomas Monjalon
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Monjalon @ 2020-03-18 0:24 UTC (permalink / raw)
To: Dmitry Kozlyuk
Cc: dev, Bruce Richardson, Olivier Matz, Harini Ramakrishnan,
Omar Cardona, Pallavi Kadam, Ranjit Menon, John McNamara,
Marko Kovacevic, William Tu, talshn, ophirmu
11/03/2020 18:22, William Tu:
> On Wed, Feb 26, 2020 at 8:25 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> >
> > This patch series add support for building DPDK using MinGW-w64.
> >
> > MinGW-w64 provides GNU toolchain and independent platform SDK on
> > Windows. It also supports cross-compilation to Windows from POSIX
> > systems by providing cross tollchains and libraries [0]. It does NOT
> > emulate a full POSIX environment, like Cygwin or MSYS do.
> >
> > There are advantages in using MinGW-w64 in addition to Clang:
> >
> > 1. Cross-compilation out-of-the-box. MinGW-w64 is provides a pthread
> > implementation, GNU getopt, and Windows platform SDK.
> >
> > 2. Easier porting of POSIX applications using DPDK to Windows, because
> > application code can use the same benefits as mentioned above.
> >
> > 3. Having both primary compilers enabled on Windows provides more
> > diagnostics and generally prevents non-portable code.
> >
> > [0]: http://mingw-w64.org
> >
[..]
> > Dmitry Kozlyuk (7):
> > eal: introduce portable format attribute
> > eal/windows: use lowercase filenames for system headers
> > eal/windows: support builing with MinGW-w64
> > build: MinGW-w64 support for Meson
> > build: add cross-file for MinGW-w64
> > doc: guide for Windows build using MinGW-w64
> > build: fix linker warnings with Clang on Windows
>
> I reviewed and tested the v4 series and everything works ok.
>
> Tested-by: William Tu <u9012063@gmail.com>
The doc patch was split and squashed in relevant commits.
Applied, thanks
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH v4 0/7] MinGW-w64 support
2020-02-27 4:25 ` [dpdk-dev] [PATCH v4 " Dmitry Kozlyuk
` (7 preceding siblings ...)
2020-03-11 17:22 ` [dpdk-dev] [PATCH v4 0/7] MinGW-w64 support William Tu
@ 2020-03-11 22:36 ` Kadam, Pallavi
8 siblings, 0 replies; 32+ messages in thread
From: Kadam, Pallavi @ 2020-03-11 22:36 UTC (permalink / raw)
To: Dmitry Kozlyuk, dev
Cc: Bruce Richardson, Thomas Monjalon, Olivier Matz,
Harini Ramakrishnan, Omar Cardona, Ranjit Menon, John McNamara,
Marko Kovacevic
On 2/26/2020 8:25 PM, Dmitry Kozlyuk wrote:
> This patch series add support for building DPDK using MinGW-w64.
>
> MinGW-w64 provides GNU toolchain and independent platform SDK on
> Windows. It also supports cross-compilation to Windows from POSIX
> systems by providing cross tollchains and libraries [0]. It does NOT
> emulate a full POSIX environment, like Cygwin or MSYS do.
>
> There are advantages in using MinGW-w64 in addition to Clang:
>
> 1. Cross-compilation out-of-the-box. MinGW-w64 is provides a pthread
> implementation, GNU getopt, and Windows platform SDK.
>
> 2. Easier porting of POSIX applications using DPDK to Windows, because
> application code can use the same benefits as mentioned above.
>
> 3. Having both primary compilers enabled on Windows provides more
> diagnostics and generally prevents non-portable code.
>
> [0]: http://mingw-w64.org
>
> v4 Changes:
>
> Fix Windows headers include case.
> Recommend the latest Meson in docs (build fixed with 0.53.2).
>
> v3 Changes:
>
> Rebase onto the latest Windows EAL.
> Prevent format attribute conflict with upcoming v20.05 patches.
> Remove redundant CFLAGS from Meson.
> Remove links to Meson bugtracker from docs (PR merged into upstream).
> Fix Clang warnings using about GNU options.
>
> v2 Changes:
>
> Add patch to use lowercase system header filenames.
> Move Meson cross-file for x86 to arch directory.
> Change wording in comments.
> Add Meson version warning in documentation.
>
> Dmitry Kozlyuk (7):
> eal: introduce portable format attribute
> eal/windows: use lowercase filenames for system headers
> eal/windows: support builing with MinGW-w64
> build: MinGW-w64 support for Meson
> build: add cross-file for MinGW-w64
> doc: guide for Windows build using MinGW-w64
> build: fix linker warnings with Clang on Windows
>
> config/meson.build | 26 ++++++--
> config/x86/meson_mingw.txt | 13 ++++
> doc/guides/windows_gsg/build_dpdk.rst | 62 +++++++++++++++++---
> 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 +-
> lib/librte_eal/meson.build | 3 +
> lib/librte_eal/windows/eal/eal.c | 6 +-
> lib/librte_eal/windows/eal/include/getopt.h | 4 ++
> lib/librte_eal/windows/eal/include/pthread.h | 2 +-
> lib/librte_eal/windows/eal/include/rte_os.h | 6 +-
> lib/meson.build | 8 ++-
> 14 files changed, 133 insertions(+), 26 deletions(-)
> create mode 100644 config/x86/meson_mingw.txt
>
Acked-by: Pallavi Kadam <pallavi.kadam@intel.com>
Tested-by: Pallavi Kadam <pallavi.kadam@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread