On Thu, Mar 14, 2024 at 11:30:21PM -0700, Tyler Retzlaff wrote:
> MSVC is the only compiler that can produce usable shared libraries for
> DPDK on Windows because of the use of exported TLS variables.
>
> Disable building of shared libraries with LLVM and MinGW so that
> remaining __declspec macros needed for the functional libraries built by
> MSVC can be used without triggering errors in LLVM and MinGW builds.
>
> For Windows only install the default_library type to avoid confusion.
> Windows builds cannot build both shared and static in a single pass so
> install only the functional variant.
>
> Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com>
Comments inline below.
/Bruce
> ---
> app/meson.build | 6 +++
> config/meson.build | 27 ++++++++++++
> drivers/meson.build | 72 +++++++++++++++----------------
> drivers/net/octeontx/base/meson.build | 2 +-
> examples/meson.build | 6 +++
> lib/meson.build | 79
> +++++++++++++++--------------------
> 6 files changed, 108 insertions(+), 84 deletions(-)
>
> diff --git a/app/meson.build b/app/meson.build
> index 21b6da2..f4ed0f1 100644
> --- a/app/meson.build
> +++ b/app/meson.build
> @@ -46,6 +46,8 @@ default_cflags = machine_args + ['-DALLOW_EXPERIMENTAL_API']
> default_ldflags = []
> if get_option('default_library') == 'static' and not is_windows
> default_ldflags += ['-Wl,--export-dynamic']
> +elif get_option('default_library') == 'shared' and is_ms_compiler
> + default_ldflags += ['/experimental:tlsDllInterface']
> endif
>
Is this necessary, given that you add this flag as a project-wide argument
below in config/meson.build?
> foreach app:apps
> @@ -104,6 +106,10 @@ foreach app:apps
> link_libs = dpdk_static_libraries + dpdk_drivers
> endif
>
> + if is_windows and is_shared_enabled
> + cflags += '-DRTE_BUILD_SHARED_LIB'
> + endif
> +
> exec = executable('dpdk-' + name,
> sources,
> c_args: cflags,
> diff --git a/config/meson.build b/config/meson.build
> index 8c8b019..dd7971e 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -53,6 +53,9 @@ if is_ms_compiler
>
> # enable statement expressions extension
> add_project_arguments('/experimental:statementExpressions', language:
> 'c')
> +
> + # enable export of thread_local variables
> + add_project_arguments('/experimental:tlsDllInterface', language: 'c')
> endif
>
> # set the major version, which might be used by drivers and libraries
> @@ -516,4 +519,28 @@ if get_option('default_library') == 'both'
> NOTE: DPDK always builds both shared and static libraries. Please set
> "default_library" to either "static" or "shared" to select default linkage
> for apps and any examples.''')
> +elif get_option('default_library') == 'shared' and is_windows and not
> is_ms_compiler
> + error( '''
> + Unsupported value "shared" for "default_library" option.
> +
> + NOTE: DPDK Windows shared is only supported when building with MSVC. Please
> set
> + "default_library" to either "static" or use MSVC.''')
> +endif
> +
> +if is_windows
> + if is_ms_compiler and get_option('default_library') == 'shared'
> + is_shared_enabled=true
> + else
> + is_shared_enabled=false
> + endif
> +else
> + is_shared_enabled=true
> +endif
> +
This can be shortened to:
is_shared_enabled=true
if is_windows and (not is_ms_compiler or not get_option(...) == 'shared')
is_shared_enabled=false
endif
> +if is_windows
> + install_static = get_option('default_library') == 'static'
> + install_shared = get_option('default_library') == 'shared'
> +else
> + install_static=true
> + install_shared=true
> endif
Might be better merged with the block above:
is_shared_enabled = true
install_static = true
install_shared = true
if is_windows
install_static = get_option('default_library') == 'static'
install_shared = get_option('default_library') == 'shared'
if not is_ms_compiler or not install_shared
is_shared_enabled = false
endif
endif
> diff --git a/drivers/meson.build b/drivers/meson.build
> index 66931d4..c8b2d13 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -80,7 +80,7 @@ foreach subpath:subdirs
> subdir(class)
> skip_class = false
> foreach d:std_deps
> - if not is_variable('shared_rte_' + d)
> + if not is_variable('static_rte_' + d)
> skip_class = true
> reason = 'missing internal dependency, "@0@"'.format(d)
> if dpdk_libs_deprecated.contains(d)
> @@ -173,7 +173,7 @@ foreach subpath:subdirs
> if not build
> break
> endif
> - if not is_variable('shared_rte_' + d)
> + if not is_variable('static_rte_' + d)
> build = false
> reason = 'missing internal dependency, "@0@"'.format(d)
> if dpdk_libs_deprecated.contains(d)
> @@ -188,7 +188,9 @@ foreach subpath:subdirs
> +'\tPlease enable missing dependency
> "@0@"'.format(d))
> endif
> else
> - shared_deps += [get_variable('shared_rte_' + d)]
> + if is_shared_enabled
> + shared_deps += [get_variable('shared_rte_' + d)]
> + endif
> static_deps += [get_variable('static_rte_' + d)]
> endif
> endforeach
> @@ -208,6 +210,9 @@ foreach subpath:subdirs
> enabled_drivers += name
> lib_name = '_'.join(['rte', class, name])
> cflags += '-DRTE_LOG_DEFAULT_LOGTYPE=' + '.'.join([log_prefix, name])
> + if is_windows and is_shared_enabled
> + cflags += '-DRTE_BUILD_SHARED_LIB'
> + endif
Rather than setting this in cflags in the drivers or lib meson.build files,
can we set this once as a project-level cflag and be done with it?
> if annotate_locks and cc.get_id() == 'clang' and
> cc.version().version_compare('>=3.5.0')
> cflags += '-DRTE_ANNOTATE_LOCKS'
> cflags += '-Wthread-safety'
> @@ -247,7 +252,7 @@ foreach subpath:subdirs
> include_directories: includes,
> dependencies: static_deps,
> c_args: cflags,
> - install: true)
> + install: install_static)
>
> # now build the shared driver
> version_map =
> '@0@/@1@/version.map'.format(meson.current_source_dir(), drv_path)
> @@ -271,55 +276,46 @@ foreach subpath:subdirs
> endif
> endif
>
> - if is_windows
> - if is_ms_linker
> + if is_shared_enabled
> + if is_ms_compiler
> def_file = custom_target(lib_name + '_def',
> command: [map_to_win_cmd, '@INPUT@', '@OUTPUT@'],
> input: version_map,
> output: '@0@_exports.def'.format(lib_name))
> lk_deps += [def_file]
> -
> - lk_args = ['-Wl,/def:' + def_file.full_path()]
> - if meson.version().version_compare('<0.54.0')
> - lk_args += ['-Wl,/implib:drivers\\lib' + lib_name +
> '.dll.a']
> - endif
> + lk_args = ['/experimental:tlsDllInterface', '/def:' +
> def_file.full_path()]
Again, the /experimental flag shouldn't be necessary, since it's set as
project option in config/meson.build.
> else
> - mingw_map = custom_target(lib_name + '_mingw',
> - command: [map_to_win_cmd, '@INPUT@', '@OUTPUT@'],
> - input: version_map,
> - output: '@0@_mingw.map'.format(lib_name))
> - lk_deps += [mingw_map]
> -
> - lk_args = ['-Wl,--version-script=' + mingw_map.full_path()]
> + lk_args = ['-Wl,--version-script=' + version_map]
> endif
> - else
> - lk_args = ['-Wl,--version-script=' + version_map]
> endif
>
> - shared_lib = shared_library(lib_name, sources,
> - objects: objs,
> - include_directories: includes,
> - dependencies: shared_deps,
> - c_args: cflags,
> - link_args: lk_args,
> - link_depends: lk_deps,
> - version: abi_version,
> - soversion: so_version,
> - install: true,
> - install_dir: driver_install_path)
> -
> - # create a dependency object and add it to the global dictionary so
> - # testpmd or other built-in apps can find it if necessary
> - shared_dep = declare_dependency(link_with: shared_lib,
> - include_directories: includes,
> - dependencies: shared_deps)
> + if is_shared_enabled
> + shared_lib = shared_library(lib_name, sources,
> + objects: objs,
> + include_directories: includes,
> + dependencies: shared_deps,
> + c_args: cflags,
> + link_args: lk_args,
> + link_depends: lk_deps,
> + version: abi_version,
> + soversion: so_version,
> + install: install_shared,
> + install_dir: driver_install_path)
> +
> + # create a dependency object and add it to the global dictionary
> so
> + # testpmd or other built-in apps can find it if necessary
> + shared_dep = declare_dependency(link_with: shared_lib,
> + include_directories: includes,
> + dependencies: shared_deps)
> +
> + set_variable('shared_@0@'.format(lib_name), shared_dep)
Just a thought - could you avoid some of the changes throughout this set,
if you add an else-leg to the "is_shared_enabled" check, where, if shared
is not enabled, you assign shared_dep to be an empty string, or an empty
object or something.
That would mean that you wouldn't need to move the set_variable line as
here, you also wouldn't need to put conditionals around all the
Just a thought - could you avoid some of the changes throughout this set,
if you add an else-leg to the "is_shared_enabled" check, where, if shared
is not enabled, you assign shared_dep to be an empty string, or an empty
object or something.
That would mean that you wouldn't need to move the set_variable line as
here, you also wouldn't need to put conditionals around all the
'shared_deps +=' lines when processing deps for a component, and lastly,
you may not even need to change all the searches for shared_rte_ to
static_rte, since the variables would exist for shared - they'd just be
empty!
> + endif
> static_dep = declare_dependency(
> include_directories: includes,
> dependencies: static_deps)
>
> dpdk_drivers += static_lib
<snip for brevity>
