On Tue, Mar 11, 2025 at 10:56:03AM +0100, David Marchand wrote: > Rather than maintain a file in parallel of the code, symbols to be > exported can be marked with a token RTE_EXPORT_*SYMBOL. > > From those marks, the build framework generates map files only for > symbols actually compiled (which means that the WINDOWS_NO_EXPORT hack > becomes unnecessary). > > The build framework directly creates a map file in the format that the > linker expects (rather than converting from GNU linker to MSVC linker). > > Empty maps are allowed again as a replacement for drivers/version.map. > > The symbol check is updated to only support the new format. > > Signed-off-by: David Marchand <david.march...@redhat.com>
Some comments inline below. /Bruce > --- > Changes since RFC v2: > - because of MSVC limitations wrt macro passed via cmdline, > used an internal header for defining RTE_EXPORT_* macros, > - updated documentation and tooling, > > --- > MAINTAINERS | 2 + > buildtools/gen-version-map.py | 111 ++++++++++ > buildtools/map-list-symbol.sh | 10 +- > buildtools/meson.build | 1 + > config/meson.build | 2 + > config/rte_export.h | 16 ++ > devtools/check-symbol-change.py | 90 +++++++++ > devtools/check-symbol-maps.sh | 14 -- > devtools/checkpatches.sh | 2 +- > doc/guides/contributing/abi_versioning.rst | 224 ++------------------- > drivers/meson.build | 94 +++++---- > drivers/version.map | 3 - > lib/meson.build | 91 ++++++--- > 13 files changed, 371 insertions(+), 289 deletions(-) > create mode 100755 buildtools/gen-version-map.py > create mode 100644 config/rte_export.h > create mode 100755 devtools/check-symbol-change.py > delete mode 100644 drivers/version.map > > diff --git a/MAINTAINERS b/MAINTAINERS > index 312e6fcee5..04772951d3 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -95,6 +95,7 @@ F: devtools/check-maintainers.sh > F: devtools/check-forbidden-tokens.awk > F: devtools/check-git-log.sh > F: devtools/check-spdx-tag.sh > +F: devtools/check-symbol-change.py > F: devtools/check-symbol-change.sh > F: devtools/check-symbol-maps.sh > F: devtools/checkpatches.sh > @@ -127,6 +128,7 @@ F: config/ > F: buildtools/check-symbols.sh > F: buildtools/chkincs/ > F: buildtools/call-sphinx-build.py > +F: buildtools/gen-version-map.py > F: buildtools/get-cpu-count.py > F: buildtools/get-numa-count.py > F: buildtools/list-dir-globs.py > diff --git a/buildtools/gen-version-map.py b/buildtools/gen-version-map.py > new file mode 100755 > index 0000000000..b160aa828b > --- /dev/null > +++ b/buildtools/gen-version-map.py > @@ -0,0 +1,111 @@ > +#!/usr/bin/env python3 > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright (c) 2024 Red Hat, Inc. > + > +"""Generate a version map file used by GNU or MSVC linker.""" > + While it's an internal build script not to be run by users directly, I believe a short one-line usage here might be useful, since the code below is directly referencing sys.argv[N] values. That makes it easier for the user to know what they are. Alternatively, assign them to proper names at the top of the script e.g.: scriptname, link_mode, abi_version_file, output, *input = sys.argv Final alternative (which may be a bit overkill) is to use argparse. > +import re > +import sys > + > +# From rte_export.h > +export_exp_sym_regexp = > re.compile(r"^RTE_EXPORT_EXPERIMENTAL_SYMBOL\(([^,]+), ([0-9]+.[0-9]+)\)") > +export_int_sym_regexp = re.compile(r"^RTE_EXPORT_INTERNAL_SYMBOL\(([^)]+)\)") > +export_sym_regexp = re.compile(r"^RTE_EXPORT_SYMBOL\(([^)]+)\)") > +# From rte_function_versioning.h > +ver_sym_regexp = re.compile(r"^RTE_VERSION_SYMBOL\(([^,]+), [^,]+, ([^,]+),") > +ver_exp_sym_regexp = re.compile(r"^RTE_VERSION_EXPERIMENTAL_SYMBOL\([^,]+, > ([^,]+),") > +default_sym_regexp = re.compile(r"^RTE_DEFAULT_SYMBOL\(([^,]+), [^,]+, > ([^,]+),") > + > +with open(sys.argv[2]) as f: > + abi = 'DPDK_{}'.format(re.match("([0-9]+).[0-9]", f.readline()).group(1)) > + > +symbols = {} > + > +for file in sys.argv[4:]: > + with open(file, encoding="utf-8") as f: > + for ln in f.readlines(): > + node = None > + symbol = None > + comment = None > + if export_exp_sym_regexp.match(ln): > + node = 'EXPERIMENTAL' > + symbol = export_exp_sym_regexp.match(ln).group(1) > + comment = ' # added in > {}'.format(export_exp_sym_regexp.match(ln).group(2)) > + elif export_int_sym_regexp.match(ln): > + node = 'INTERNAL' > + symbol = export_int_sym_regexp.match(ln).group(1) > + elif export_sym_regexp.match(ln): > + node = abi > + symbol = export_sym_regexp.match(ln).group(1) > + elif ver_sym_regexp.match(ln): > + node = 'DPDK_{}'.format(ver_sym_regexp.match(ln).group(1)) > + symbol = ver_sym_regexp.match(ln).group(2) > + elif ver_exp_sym_regexp.match(ln): > + node = 'EXPERIMENTAL' > + symbol = ver_exp_sym_regexp.match(ln).group(1) > + elif default_sym_regexp.match(ln): > + node = > 'DPDK_{}'.format(default_sym_regexp.match(ln).group(1)) > + symbol = default_sym_regexp.match(ln).group(2) > + > + if not symbol: > + continue > + > + if node not in symbols: > + symbols[node] = {} > + symbols[node][symbol] = comment > + > +if sys.argv[1] == 'msvc': > + with open(sys.argv[3], "w") as outfile: > + outfile.writelines(f"EXPORTS\n") > + for key in (abi, 'EXPERIMENTAL', 'INTERNAL'): > + if key not in symbols: > + continue > + for symbol in sorted(symbols[key].keys()): > + outfile.writelines(f"\t{symbol}\n") > + del symbols[key] > +else: > + with open(sys.argv[3], "w") as outfile: > + local_token = False > + for key in (abi, 'EXPERIMENTAL', 'INTERNAL'): > + if key not in symbols: > + continue > + outfile.writelines(f"{key} {{\n\tglobal:\n\n") > + for symbol in sorted(symbols[key].keys()): > + if sys.argv[1] == 'mingw' and symbol.startswith('per_lcore'): > + prefix = '__emutls_v.' > + else: > + prefix = '' > + outfile.writelines(f"\t{prefix}{symbol};") > + comment = symbols[key][symbol] > + if comment: > + outfile.writelines(f"{comment}") > + outfile.writelines("\n") How about using "" rather than None for the default comment so you can always just do a print of "{prefix}{symbol};{comment}\n". The fact that writelines doesn't output a "\n" is a little confusing here, so maybe use "print" instead. print("f\t{prefix}{symbol};{comment}", file=outfile) > + outfile.writelines("\n") > + if not local_token: > + outfile.writelines("\tlocal: *;\n") > + local_token = True > + outfile.writelines("};\n") > + del symbols[key] > + for key in sorted(symbols.keys()): > + outfile.writelines(f"{key} {{\n\tglobal:\n\n") > + for symbol in sorted(symbols[key].keys()): > + if sys.argv[1] == 'mingw' and symbol.startswith('per_lcore'): > + prefix = '__emutls_v.' > + else: > + prefix = '' > + outfile.writelines(f"\t{prefix}{symbol};") > + comment = symbols[key][symbol] > + if comment: > + outfile.writelines(f"{comment}") > + outfile.writelines("\n") > + outfile.writelines(f"}} {abi};\n") > + if not local_token: > + outfile.writelines("\tlocal: *;\n") > + local_token = True > + del symbols[key] > + # No exported symbol, add a catch all > + if not local_token: > + outfile.writelines(f"{abi} {{\n") > + outfile.writelines("\tlocal: *;\n") > + local_token = True > + outfile.writelines("};\n") > diff --git a/buildtools/map-list-symbol.sh b/buildtools/map-list-symbol.sh > index eb98451d8e..0829df4be5 100755 > --- a/buildtools/map-list-symbol.sh > +++ b/buildtools/map-list-symbol.sh > @@ -62,10 +62,14 @@ for file in $@; do > if (current_section == "") { > next; > } > + symbol_version = current_version > + if (/^[^}].*[^:*]; # added in /) { > + symbol_version = $5 > + } > if ("'$version'" != "") { > - if ("'$version'" == "unset" && current_version != "") { > + if ("'$version'" == "unset" && symbol_version != "") { > next; > - } else if ("'$version'" != "unset" && "'$version'" != > current_version) { > + } else if ("'$version'" != "unset" && "'$version'" != > symbol_version) { > next; > } > } > @@ -73,7 +77,7 @@ for file in $@; do > if ("'$symbol'" == "all" || $1 == "'$symbol'") { > ret = 0; > if ("'$quiet'" == "") { > - print "'$file' "current_section" "$1" > "current_version; > + print "'$file' "current_section" "$1" > "symbol_version; > } > if ("'$symbol'" != "all") { > exit 0; > diff --git a/buildtools/meson.build b/buildtools/meson.build > index 4e2c1217a2..b745e9afa4 100644 > --- a/buildtools/meson.build > +++ b/buildtools/meson.build > @@ -16,6 +16,7 @@ else > py3 = ['meson', 'runpython'] > endif > echo = py3 + ['-c', 'import sys; print(*sys.argv[1:])'] > +gen_version_map = py3 + files('gen-version-map.py') > list_dir_globs = py3 + files('list-dir-globs.py') > map_to_win_cmd = py3 + files('map_to_win.py') > sphinx_wrapper = py3 + files('call-sphinx-build.py') > diff --git a/config/meson.build b/config/meson.build > index f31fef216c..54657055fb 100644 > --- a/config/meson.build > +++ b/config/meson.build > @@ -303,8 +303,10 @@ endif > # add -include rte_config to cflags > if is_ms_compiler > add_project_arguments('/FI', 'rte_config.h', language: 'c') > + add_project_arguments('/FI', 'rte_export.h', language: 'c') > else > add_project_arguments('-include', 'rte_config.h', language: 'c') > + add_project_arguments('-include', 'rte_export.h', language: 'c') > endif > > # enable extra warnings and disable any unwanted warnings > diff --git a/config/rte_export.h b/config/rte_export.h > new file mode 100644 > index 0000000000..83d871fe11 > --- /dev/null > +++ b/config/rte_export.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright (c) 2025 Red Hat, Inc. > + */ > + > +#ifndef RTE_EXPORT_H > +#define RTE_EXPORT_H > + > +/* *Internal* macros for exporting symbols, used by the build system. > + * For RTE_EXPORT_EXPERIMENTAL_SYMBOL, ver indicates the > + * version this symbol was introduced in. > + */ > +#define RTE_EXPORT_EXPERIMENTAL_SYMBOL(a, ver) > +#define RTE_EXPORT_INTERNAL_SYMBOL(a) > +#define RTE_EXPORT_SYMBOL(a) > + > +#endif /* RTE_EXPORT_H */ > diff --git a/devtools/check-symbol-change.py b/devtools/check-symbol-change.py > new file mode 100755 > index 0000000000..09709e4f06 > --- /dev/null > +++ b/devtools/check-symbol-change.py > @@ -0,0 +1,90 @@ > +#!/usr/bin/env python3 > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright (c) 2025 Red Hat, Inc. > + > +"""Check exported symbols change in a patch.""" > + > +import re > +import sys > + > +file_header_regexp = re.compile(r"^(\-\-\-|\+\+\+) > [ab]/(lib|drivers)/([^/]+)/([^/]+)") > +# From rte_export.h > +export_exp_sym_regexp = > re.compile(r"^.RTE_EXPORT_EXPERIMENTAL_SYMBOL\(([^,]+),") > +export_int_sym_regexp = > re.compile(r"^.RTE_EXPORT_INTERNAL_SYMBOL\(([^)]+)\)") > +export_sym_regexp = re.compile(r"^.RTE_EXPORT_SYMBOL\(([^)]+)\)") > +# TODO, handle versioned symbols from rte_function_versioning.h > +# ver_sym_regexp = re.compile(r"^.RTE_VERSION_SYMBOL\(([^,]+), [^,]+, > ([^,]+),") > +# ver_exp_sym_regexp = > re.compile(r"^.RTE_VERSION_EXPERIMENTAL_SYMBOL\([^,]+, ([^,]+),") > +# default_sym_regexp = re.compile(r"^.RTE_DEFAULT_SYMBOL\(([^,]+), [^,]+, > ([^,]+),") > + > +symbols = {} > + > +for file in sys.argv[1:]: > + with open(file, encoding="utf-8") as f: > + for ln in f.readlines(): > + if file_header_regexp.match(ln): > + if file_header_regexp.match(ln).group(2) == "lib": > + lib = '/'.join(file_header_regexp.match(ln).group(2, 3)) > + elif file_header_regexp.match(ln).group(3) == "intel": > + lib = '/'.join(file_header_regexp.match(ln).group(2, 3, > 4)) > + else: > + lib = '/'.join(file_header_regexp.match(ln).group(2, 3)) > + > + if lib not in symbols: > + symbols[lib] = {} > + continue > + > + if export_exp_sym_regexp.match(ln): > + symbol = export_exp_sym_regexp.match(ln).group(1) > + node = 'EXPERIMENTAL' > + elif export_int_sym_regexp.match(ln): > + node = 'INTERNAL' > + symbol = export_int_sym_regexp.match(ln).group(1) > + elif export_sym_regexp.match(ln): > + symbol = export_sym_regexp.match(ln).group(1) > + node = 'stable' > + else: > + continue > + > + if symbol not in symbols[lib]: > + symbols[lib][symbol] = {} > + added = ln[0] == '+' > + if added and 'added' in symbols[lib][symbol] and node != > symbols[lib][symbol]['added']: > + print(f"{symbol} in {lib} was found in multiple ABI, please > check.") > + if not added and 'removed' in symbols[lib][symbol] and node != > symbols[lib][symbol]['removed']: > + print(f"{symbol} in {lib} was found in multiple ABI, please > check.") > + if added: > + symbols[lib][symbol]['added'] = node > + else: > + symbols[lib][symbol]['removed'] = node > + > + for lib in sorted(symbols.keys()): > + error = False > + for symbol in sorted(symbols[lib].keys()): > + if 'removed' not in symbols[lib][symbol]: > + # Symbol addition > + node = symbols[lib][symbol]['added'] > + if node == 'stable': > + print(f"ERROR: {symbol} in {lib} has been added directly > to stable ABI.") > + error = True > + else: > + print(f"INFO: {symbol} in {lib} has been added to {node} > ABI.") > + continue > + > + if 'added' not in symbols[lib][symbol]: > + # Symbol removal > + node = symbols[lib][symbol]['added'] > + if node == 'stable': > + print(f"INFO: {symbol} in {lib} has been removed from > stable ABI.") > + print(f"Please check it has gone though the deprecation > process.") > + continue > + > + if symbols[lib][symbol]['added'] == > symbols[lib][symbol]['removed']: > + # Symbol was moved around > + continue > + > + # Symbol modifications > + added = symbols[lib][symbol]['added'] > + removed = symbols[lib][symbol]['removed'] > + print(f"INFO: {symbol} in {lib} is moving from {removed} to > {added}") > + print(f"Please check it has gone though the deprecation > process.") > diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh > index 6121f78ec6..fcd3931e5d 100755 > --- a/devtools/check-symbol-maps.sh > +++ b/devtools/check-symbol-maps.sh > @@ -60,20 +60,6 @@ if [ -n "$local_miss_maps" ] ; then > ret=1 > fi > > -find_empty_maps () > -{ > - for map in $@ ; do > - [ $(buildtools/map-list-symbol.sh $map | wc -l) != '0' ] || echo $map > - done > -} > - > -empty_maps=$(find_empty_maps $@) > -if [ -n "$empty_maps" ] ; then > - echo "Found empty maps:" > - echo "$empty_maps" > - ret=1 > -fi > - > find_bad_format_maps () > { > abi_version=$(cut -d'.' -f 1 ABI_VERSION) > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > index 003bb49e04..7dcac7c8c9 100755 > --- a/devtools/checkpatches.sh > +++ b/devtools/checkpatches.sh > @@ -33,7 +33,7 @@ > VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,STRLCPY,\ > PREFER_KERNEL_TYPES,PREFER_FALLTHROUGH,BIT_MACRO,CONST_STRUCT,\ > SPLIT_STRING,LONG_LINE_STRING,C99_COMMENT_TOLERANCE,\ > LINE_SPACING,PARENTHESIS_ALIGNMENT,NETWORKING_BLOCK_COMMENT_STYLE,\ > -NEW_TYPEDEFS,COMPARISON_TO_NULL,AVOID_BUG" > +NEW_TYPEDEFS,COMPARISON_TO_NULL,AVOID_BUG,EXPORT_SYMBOL" > options="$options $DPDK_CHECKPATCH_OPTIONS" > > print_usage () { > diff --git a/doc/guides/contributing/abi_versioning.rst > b/doc/guides/contributing/abi_versioning.rst > index 88dd776b4c..addbb24b9e 100644 > --- a/doc/guides/contributing/abi_versioning.rst > +++ b/doc/guides/contributing/abi_versioning.rst > @@ -58,12 +58,12 @@ persists over multiple releases. > > .. code-block:: none > > - $ head ./lib/acl/version.map > + $ head ./build/lib/librte_acl_exports.map I must admit I'm not a fan of these long filenames. How about just "acl_exports.map"? > DPDK_21 { > global: > ... > > - $ head ./lib/eal/version.map > + $ head ./build/lib/librte_eal_exports.map > DPDK_21 { > global: > ... > @@ -77,7 +77,7 @@ that library. > > .. code-block:: none > > - $ head ./lib/acl/version.map > + $ head ./build/lib/librte_acl_exports.map > DPDK_21 { > global: > ... > @@ -88,7 +88,7 @@ that library. > } DPDK_21; > ... > > - $ head ./lib/eal/version.map > + $ head ./build/lib/librte_eal_exports.map > DPDK_21 { > global: > ... > @@ -100,12 +100,12 @@ how this may be done. > > .. code-block:: none > > - $ head ./lib/acl/version.map > + $ head ./build/lib/librte_acl_exports.map > DPDK_22 { > global: > ... > > - $ head ./lib/eal/version.map > + $ head ./build/lib/librte_eal_exports.map > DPDK_22 { > global: > ... > @@ -134,8 +134,7 @@ linked to the DPDK. > > To support backward compatibility the ``rte_function_versioning.h`` > header file provides macros to use when updating exported functions. These > -macros are used in conjunction with the ``version.map`` file for > -a given library to allow multiple versions of a symbol to exist in a shared > +macros allow multiple versions of a symbol to exist in a shared > library so that older binaries need not be immediately recompiled. > > The macros are: > @@ -169,6 +168,7 @@ Assume we have a function as follows > * Create an acl context object for apps to > * manipulate > */ > + RTE_EXPORT_SYMBOL(rte_acl_create) > struct rte_acl_ctx * > rte_acl_create(const struct rte_acl_param *param) > { > @@ -187,6 +187,7 @@ private, is safe), but it also requires modifying the > code as follows > * Create an acl context object for apps to > * manipulate > */ > + RTE_EXPORT_SYMBOL(rte_acl_create) > struct rte_acl_ctx * > rte_acl_create(const struct rte_acl_param *param, int debug) > { > @@ -203,78 +204,16 @@ The addition of a parameter to the function is ABI > breaking as the function is > public, and existing application may use it in its current form. However, the > compatibility macros in DPDK allow a developer to use symbol versioning so > that > multiple functions can be mapped to the same public symbol based on when an > -application was linked to it. To see how this is done, we start with the > -requisite libraries version map file. Initially the version map file for the > acl > -library looks like this > +application was linked to it. > > -.. code-block:: none > - > - DPDK_21 { > - global: > - > - rte_acl_add_rules; > - rte_acl_build; > - rte_acl_classify; > - rte_acl_classify_alg; > - rte_acl_classify_scalar; > - rte_acl_create; > - rte_acl_dump; > - rte_acl_find_existing; > - rte_acl_free; > - rte_acl_ipv4vlan_add_rules; > - rte_acl_ipv4vlan_build; > - rte_acl_list_dump; > - rte_acl_reset; > - rte_acl_reset_rules; > - rte_acl_set_ctx_classify; > - > - local: *; > - }; > - > -This file needs to be modified as follows > - > -.. code-block:: none > - > - DPDK_21 { > - global: > - > - rte_acl_add_rules; > - rte_acl_build; > - rte_acl_classify; > - rte_acl_classify_alg; > - rte_acl_classify_scalar; > - rte_acl_create; > - rte_acl_dump; > - rte_acl_find_existing; > - rte_acl_free; > - rte_acl_ipv4vlan_add_rules; > - rte_acl_ipv4vlan_build; > - rte_acl_list_dump; > - rte_acl_reset; > - rte_acl_reset_rules; > - rte_acl_set_ctx_classify; > - > - local: *; > - }; > - > - DPDK_22 { > - global: > - rte_acl_create; > - > - } DPDK_21; > - > -The addition of the new block tells the linker that a new version node > -``DPDK_22`` is available, which contains the symbol rte_acl_create, and > inherits > -the symbols from the DPDK_21 node. This list is directly translated into a > -list of exported symbols when DPDK is compiled as a shared library. > - > -Next, we need to specify in the code which function maps to the > rte_acl_create > +We need to specify in the code which function maps to the rte_acl_create > symbol at which versions. First, at the site of the initial symbol > definition, > we wrap the function with ``RTE_VERSION_SYMBOL``, passing the current ABI > version, > -the function return type, and the function name and its arguments. > +the function return type, the function name and its arguments. Good fix, though technically not relevant to this patch. > > .. code-block:: c > > + -RTE_EXPORT_SYMBOL(rte_acl_create) > -struct rte_acl_ctx * > -rte_acl_create(const struct rte_acl_param *param) > +RTE_VERSION_SYMBOL(21, struct rte_acl_ctx *, rte_acl_create, (const struct > rte_acl_param *param)) > @@ -293,6 +232,7 @@ We have now mapped the original rte_acl_create symbol to > the original function > > Please see the section :ref:`Enabling versioning macros > <enabling_versioning_macros>` to enable this macro in the meson/ninja build. > + Ditto. > Next, we need to create the new version of the symbol. We create a new > function name and implement it appropriately, then wrap it in a call to > ``RTE_DEFAULT_SYMBOL``. > > @@ -312,9 +252,9 @@ The macro instructs the linker to create the new default > symbol > ``rte_acl_create@DPDK_22``, which points to the function named > ``rte_acl_create_v22`` > (declared by the macro). > > -And that's it, on the next shared library rebuild, there will be two > versions of > -rte_acl_create, an old DPDK_21 version, used by previously built > applications, > -and a new DPDK_22 version, used by future built applications. > +And that's it. On the next shared library rebuild, there will be two > versions of rte_acl_create, > +an old DPDK_21 version, used by previously built applications, and a new > DPDK_22 version, > +used by future built applications. nit: not sure what others think but "future built" sounds strange to me? How about "later built" or "newly built"? > > .. note:: > > @@ -364,6 +304,7 @@ Assume we have an experimental function > ``rte_acl_create`` as follows: > * Create an acl context object for apps to > * manipulate > */ <snip>