From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7CC3143E55; Sat, 13 Apr 2024 01:04:15 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 05A4740263; Sat, 13 Apr 2024 01:04:15 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 757B84014F for ; Sat, 13 Apr 2024 01:04:13 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id A692720EBE36; Fri, 12 Apr 2024 16:04:12 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com A692720EBE36 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1712963052; bh=N60JcU6ab7Wd5og32EUHSM40OK3lbRREox40Jmk8dz8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XcCxYq7JA7G+Z5+UTDpewDPY+/s5waB1TCtpvvPU2bKOQS0gi28uUr8/4IvUjL+VQ 5IJDTbxjM8D+olEk8n/Wi11m2fYZkUziKzMBaI1uKM+WeDDNDoUCi8r5NiVsCHEiJv v6DaAlqBCVeGHrLW8dBqfpol7yJB3y1iSyU7eDQ8= Date: Fri, 12 Apr 2024 16:04:12 -0700 From: Tyler Retzlaff To: Bruce Richardson Cc: dev@dpdk.org, Harman Kalra Subject: Re: [PATCH v2 1/2] build: build only one library type on Windows Message-ID: <20240412230412.GB23425@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1710445477-23848-1-git-send-email-roretzla@linux.microsoft.com> <1710484222-17310-1-git-send-email-roretzla@linux.microsoft.com> <1710484222-17310-2-git-send-email-roretzla@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > > 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. > > 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. > > > 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. > > > + endif > > static_dep = declare_dependency( > > include_directories: includes, > > dependencies: static_deps) > > > > dpdk_drivers += static_lib > >