On Fri, Apr 12, 2024 at 04:04:12PM -0700, Tyler Retzlaff wrote:
> On Fri, Apr 12, 2024 at 02:00:43PM +0100, Bruce Richardson wrote:
> > 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?
>
> yes, i need to provide tlsDllInterface as an argument to both the
> compiler and the linker commands. project-wide argument from
> config/meson.build only adds the argument to the compiler command line
> but not the linker. there doesn't appear to be a global variable that
> can be used as the same for the linker so it has been added to the
> various ldflags/lkflags variables in individual meson.build files.
>
The add_project_link_arguments() function should be what you are looking
for, I think.
https://mesonbuild.com/Reference-manual_functions.html#add_project_link_arguments
> > > 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
>
> as per above, this only causes the argument to be added to the compiler
> not the linker.
>
> > >
> > > # 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
>
> done in v3, much nicer thanks.
>
> >
> > > 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?
>
> i don't think so, there are drivers (notably mellanox) that appear to
> assume it is not always set by default even with a shared build. i think
> this one might best be cleaned up and clarified in a separate series.
>
Oh, how does this assumption show itself or what impact has it? Using git
grep in the source code for the macro only shows the expected usage in
function_versioning header, and in lib/meson.build.
> >
> > > 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.
>
> as per above, let me know if what i said seems crazy.
>
> >
> > > 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!
>
> did this in v3, still requires a diff -b to read what actually changed
> but appears to have worked out as you anticipated. so i think probably
> it was a good improvement.
>
Great, thanks for trying it!
> >
> > > + endif
> > > static_dep = declare_dependency(
> > > include_directories: includes,
> > > dependencies: static_deps)
> > >
> > > dpdk_drivers += static_lib
> >
> > <snip for brevity>
