Re: [RFC v3 5/8] build: generate symbol maps

[David Marchand <david.marchand@redhat.com>]

On Fri, Mar 14, 2025 at 4:28 PM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> 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.marchand@redhat.com>
> > ---
> > 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.
>
> 2025?

Well, technically, I had written the first version of this script in 2024 :-).
But I'll align to the rest of the patch.

> I appreciate that Python was chosen instead of sh/bash.
>
> > +
> > +"""Generate a version map file used by GNU or MSVC linker."""
> > +
> > +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:
>
> Consider having output file samples documented, perhaps in this script itself, to make
> it easier to understand what this script it doing and highlight the differences between
> the formats supported (msvc, etc).

I am not sure I follow.

The differences between the format is not something "normal" DPDK
contributors/developers should care about.
DPDK documentation was giving (too much) details on the version.map
gnu linker stuff, and I would prefer we stop documenting this.
Instead, the focus should be on the new sets of export macros, which
serve as an abstraction.


>
> > +        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")
> > +            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.")
>
> Some people would argue that WARN instead of INFO is more appropriate because some attention
> is needed from the user. INFO many times is just ignored.

True, though the ABI check is supposed to fail with a big ERROR :-).

I would have to remember why we put INFO initially (I am just
reimplementing the .sh check that existed on static maps).
I think Thomas was the one who wanted it as INFO...


>
> > +                    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}")
>
> Perhaps use WARN instead of INFO.

On this part, I disagree.
Moving from a non stable (like experimental) ABI to stable or other
non stable ABI (like internal) is not an issue.


>
> > +            print(f"Please check it has gone though the deprecation process.")

[snip]


-- 
David Marchand