[4/6] build: MinGW-w64 support for Meson
diff mbox series

Message ID 20200131030744.19596-5-dmitry.kozliuk@gmail.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • MinGW-w64 support
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk Jan. 31, 2020, 3:07 a.m. UTC
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>

Comments

Thomas Monjalon Feb. 4, 2020, 10:08 p.m. UTC | #1
31/01/2020 04:07, Dmitry Kozlyuk:
> 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>

I really like this patch.
So both GCC (with MinGW) and native clang are supported?

[...]
> +# MS linker requires special treatment.
> +# FIXME: use cc.get_linker_id() after upgrading to Meson >=0.53.

What does it mean? It won't work with meson 0.53?

> +is_ms_linker = is_windows and (cc.get_id() == 'clang')
[...]
> +if is_windows
> +	# Require platform SDK for Windows 7 and above.
> +	add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')

Please explain. Why Windows 7 is needed? What this define is doing?

[...]
> -			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

Looks good.
Dmitry Kozlyuk Feb. 4, 2020, 11:21 p.m. UTC | #2
> I really like this patch.
> So both GCC (with MinGW) and native clang are supported?

Thanks. Yes, I tested both toolchains.

> [...]
> > +# MS linker requires special treatment.
> > +# FIXME: use cc.get_linker_id() after upgrading to Meson >=0.53.  
> 
> What does it mean? It won't work with meson 0.53?

It will work for any Meson version, I meant that Meson 0.54 will introduce a
better way to determine linker ID. Can you suggest a better wording?

> 
> > +is_ms_linker = is_windows and (cc.get_id() == 'clang')  
> [...]
> > +if is_windows
> > +	# Require platform SDK for Windows 7 and above.
> > +	add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')  
> 
> Please explain. Why Windows 7 is needed? What this define is doing?

Yes, Windows 7 and above is need for already existing code in eal_lcore.c,
specifically for GetLogicalProcessorInformation() call.

When including <windows.h>, one must define minimum API version the
application is compiled against [0]. MSVC and Clang default to the version of
platform SDK (that is, maximum supported). MinGW defaults to Windows XP, so
this definition must be either in <rte_os.h> before #include <windows.h> or
here. Because other files may include <windows.h>, I'd prefer to have a
global definition via compiler command-line.

[0]:
https://docs.microsoft.com/en-us/windows/win32/WinProg/using-the-windows-headers
Thomas Monjalon Feb. 5, 2020, 12:41 a.m. UTC | #3
05/02/2020 00:21, Dmitry Kozlyuk:
> > I really like this patch.
> > So both GCC (with MinGW) and native clang are supported?
> 
> Thanks. Yes, I tested both toolchains.
> 
> > [...]
> > > +# MS linker requires special treatment.
> > > +# FIXME: use cc.get_linker_id() after upgrading to Meson >=0.53.  
> > 
> > What does it mean? It won't work with meson 0.53?
> 
> It will work for any Meson version, I meant that Meson 0.54 will introduce a
> better way to determine linker ID. Can you suggest a better wording?

FIXME: use cc.get_linker_id() with Meson >= 0.54


> > > +is_ms_linker = is_windows and (cc.get_id() == 'clang')  
> > [...]
> > > +if is_windows
> > > +	# Require platform SDK for Windows 7 and above.
> > > +	add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')  
> > 
> > Please explain. Why Windows 7 is needed? What this define is doing?
> 
> Yes, Windows 7 and above is need for already existing code in eal_lcore.c,
> specifically for GetLogicalProcessorInformation() call.
> 
> When including <windows.h>, one must define minimum API version the
> application is compiled against [0]. MSVC and Clang default to the version of
> platform SDK (that is, maximum supported). MinGW defaults to Windows XP, so
> this definition must be either in <rte_os.h> before #include <windows.h> or
> here. Because other files may include <windows.h>, I'd prefer to have a
> global definition via compiler command-line.
> 
> [0]:
> https://docs.microsoft.com/en-us/windows/win32/WinProg/using-the-windows-headers

OK, thanks.
Please reword the comment with something like
	"Minimum supported API is Windows 7."
Bruce Richardson Feb. 5, 2020, 2:30 p.m. UTC | #4
On Wed, Feb 05, 2020 at 01:41:24AM +0100, Thomas Monjalon wrote:
> 05/02/2020 00:21, Dmitry Kozlyuk:
> > > I really like this patch.
> > > So both GCC (with MinGW) and native clang are supported?
> > 
> > Thanks. Yes, I tested both toolchains.
> > 
> > > [...]
> > > > +# MS linker requires special treatment.
> > > > +# FIXME: use cc.get_linker_id() after upgrading to Meson >=0.53.  
> > > 
> > > What does it mean? It won't work with meson 0.53?
> > 
> > It will work for any Meson version, I meant that Meson 0.54 will introduce a
> > better way to determine linker ID. Can you suggest a better wording?
> 
> FIXME: use cc.get_linker_id() with Meson >= 0.54
> 
> 
> > > > +is_ms_linker = is_windows and (cc.get_id() == 'clang')  
> > > [...]
> > > > +if is_windows
> > > > +	# Require platform SDK for Windows 7 and above.
> > > > +	add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')  
> > > 
> > > Please explain. Why Windows 7 is needed? What this define is doing?
> > 
> > Yes, Windows 7 and above is need for already existing code in eal_lcore.c,
> > specifically for GetLogicalProcessorInformation() call.
> > 
> > When including <windows.h>, one must define minimum API version the
> > application is compiled against [0]. MSVC and Clang default to the version of
> > platform SDK (that is, maximum supported). MinGW defaults to Windows XP, so
> > this definition must be either in <rte_os.h> before #include <windows.h> or
> > here. Because other files may include <windows.h>, I'd prefer to have a
> > global definition via compiler command-line.
> > 
> > [0]:
> > https://docs.microsoft.com/en-us/windows/win32/WinProg/using-the-windows-headers
> 
> OK, thanks.
> Please reword the comment with something like
> 	"Minimum supported API is Windows 7."
> 
For this, as an alternative to putting it as a project argument, you can just
add it to dpdk_conf which means it will end up as a define in the global
rte_build_config.h and so be directly included in each compilation unit
ahead of any other headers. (rte_config.h includes rte_build_config.h)

/Bruce
Dmitry Kozlyuk Feb. 5, 2020, 8:41 p.m. UTC | #5
> > > > > +if is_windows
> > > > > +	# Require platform SDK for Windows 7 and above.
> > > > > +	add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')    
> > > > 
> > > > Please explain. Why Windows 7 is needed? What this define is doing?  
> > > 
> > > Yes, Windows 7 and above is need for already existing code in eal_lcore.c,
> > > specifically for GetLogicalProcessorInformation() call.
> > > 
> > > When including <windows.h>, one must define minimum API version the
> > > application is compiled against [0]. MSVC and Clang default to the version of
> > > platform SDK (that is, maximum supported). MinGW defaults to Windows XP, so
> > > this definition must be either in <rte_os.h> before #include <windows.h> or
> > > here. Because other files may include <windows.h>, I'd prefer to have a
> > > global definition via compiler command-line.
> > > 
> > > [0]:
> > > https://docs.microsoft.com/en-us/windows/win32/WinProg/using-the-windows-headers  
> > 
> > OK, thanks.
> > Please reword the comment with something like
> > 	"Minimum supported API is Windows 7."
> >   
> For this, as an alternative to putting it as a project argument, you can just
> add it to dpdk_conf which means it will end up as a define in the global
> rte_build_config.h and so be directly included in each compilation unit
> ahead of any other headers. (rte_config.h includes rte_build_config.h)

Can you please explain why using dpdk_conf is a better alternative? In
lib/meson.build I can see add_project_arguments('_D_GNU_SOURCE', ...), which
serves a similar purpose on POSIX systems. Compiler option also makes it
impossible to forget or redefine this constant in code by mistake.
Bruce Richardson Feb. 6, 2020, 10:59 a.m. UTC | #6
On Wed, Feb 05, 2020 at 11:41:05PM +0300, Dmitry Kozlyuk wrote:
> > > > > > +if is_windows
> > > > > > +	# Require platform SDK for Windows 7 and above.
> > > > > > +	add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')    
> > > > > 
> > > > > Please explain. Why Windows 7 is needed? What this define is doing?  
> > > > 
> > > > Yes, Windows 7 and above is need for already existing code in eal_lcore.c,
> > > > specifically for GetLogicalProcessorInformation() call.
> > > > 
> > > > When including <windows.h>, one must define minimum API version the
> > > > application is compiled against [0]. MSVC and Clang default to the version of
> > > > platform SDK (that is, maximum supported). MinGW defaults to Windows XP, so
> > > > this definition must be either in <rte_os.h> before #include <windows.h> or
> > > > here. Because other files may include <windows.h>, I'd prefer to have a
> > > > global definition via compiler command-line.
> > > > 
> > > > [0]:
> > > > https://docs.microsoft.com/en-us/windows/win32/WinProg/using-the-windows-headers  
> > > 
> > > OK, thanks.
> > > Please reword the comment with something like
> > > 	"Minimum supported API is Windows 7."
> > >   
> > For this, as an alternative to putting it as a project argument, you can just
> > add it to dpdk_conf which means it will end up as a define in the global
> > rte_build_config.h and so be directly included in each compilation unit
> > ahead of any other headers. (rte_config.h includes rte_build_config.h)
> 
> Can you please explain why using dpdk_conf is a better alternative? In
> lib/meson.build I can see add_project_arguments('_D_GNU_SOURCE', ...), which
> serves a similar purpose on POSIX systems. Compiler option also makes it
> impossible to forget or redefine this constant in code by mistake.
> 
I'm not necessarily saying it's better, it's just an alternative to
consider. :-) Having it in rte_config.h makes the define available to any
external apps using DPDK, which may or may not be desirable.
Dmitry Kozlyuk Feb. 7, 2020, 7:27 p.m. UTC | #7
> On Wed, Feb 05, 2020 at 11:41:05PM +0300, Dmitry Kozlyuk wrote:
> > > > > > > +if is_windows
> > > > > > > +	# Require platform SDK for Windows 7 and above.
> > > > > > > +	add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')      
> > > > > > 
> > > > > > Please explain. Why Windows 7 is needed? What this define is doing?    
> > > > > 
> > > > > Yes, Windows 7 and above is need for already existing code in eal_lcore.c,
> > > > > specifically for GetLogicalProcessorInformation() call.
> > > > > 
> > > > > When including <windows.h>, one must define minimum API version the
> > > > > application is compiled against [0]. MSVC and Clang default to the version of
> > > > > platform SDK (that is, maximum supported). MinGW defaults to Windows XP, so
> > > > > this definition must be either in <rte_os.h> before #include <windows.h> or
> > > > > here. Because other files may include <windows.h>, I'd prefer to have a
> > > > > global definition via compiler command-line.
> > > > > 
> > > > > [0]:
> > > > > https://docs.microsoft.com/en-us/windows/win32/WinProg/using-the-windows-headers    
> > > > 
> > > > OK, thanks.
> > > > Please reword the comment with something like
> > > > 	"Minimum supported API is Windows 7."
> > > >     
> > > For this, as an alternative to putting it as a project argument, you can just
> > > add it to dpdk_conf which means it will end up as a define in the global
> > > rte_build_config.h and so be directly included in each compilation unit
> > > ahead of any other headers. (rte_config.h includes rte_build_config.h)  
> > 
> > Can you please explain why using dpdk_conf is a better alternative? In
> > lib/meson.build I can see add_project_arguments('_D_GNU_SOURCE', ...), which
> > serves a similar purpose on POSIX systems. Compiler option also makes it
> > impossible to forget or redefine this constant in code by mistake.
> >   
> I'm not necessarily saying it's better, it's just an alternative to
> consider. :-) Having it in rte_config.h makes the define available to any
> external apps using DPDK, which may or may not be desirable.

It's certainly *not* desirable, apps usually have this value
consciously and explicitly defined on their own, and it may differ from the
one required to compile DPDK. Thanks for the tip, but as I hope you can see,
it should stay a project argument.

Patch
diff mbox series

diff --git a/config/meson.build b/config/meson.build
index 28a57f56f..a2f6f5ab1 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.
+# FIXME: use cc.get_linker_id() after upgrading to Meson >=0.53.
+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('.')
@@ -241,6 +245,16 @@  if is_freebsd
 	add_project_arguments('-D__BSD_VISIBLE', language: 'c')
 endif
 
+if is_windows
+	# Require platform SDK for Windows 7 and above.
+	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/librte_eal/meson.build b/lib/librte_eal/meson.build
index 4be5118ce..eae8c2ba8 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.get_id() == 'gcc'
+	cflags += '-D__USE_MINGW_ANSI_STDIO'
+endif
 sources = common_sources + env_sources
 objs = common_objs + env_objs
 headers = common_headers + env_headers
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