Re: [PATCH v6 8/8] eal: rework function versioning macros

[Thomas Monjalon <thomas@monjalon.net>]

28/03/2025 11:52, David Marchand:
> --- a/lib/eal/common/eal_symbol_exports.h
> +++ b/lib/eal/common/eal_symbol_exports.h
> @@ -5,6 +5,8 @@
>  #ifndef EAL_SYMBOL_EXPORTS_H
>  #define EAL_SYMBOL_EXPORTS_H
>  
> +#include <rte_common.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.
> @@ -13,4 +15,68 @@
>  #define RTE_EXPORT_INTERNAL_SYMBOL(a)
>  #define RTE_EXPORT_SYMBOL(a)
>  
> +#if !defined(RTE_USE_FUNCTION_VERSIONING) && (defined(RTE_CC_GCC) || defined(RTE_CC_CLANG))
> +#define VERSIONING_WARN RTE_PRAGMA_WARNING(Use of function versioning disabled. \
> +       Is "use_function_versioning=true" in meson.build?)
> +#else
> +#define VERSIONING_WARN
> +#endif

Why no warning for other compilers?
Why not warn at Meson level?

> +
> +/*
> + * Provides backwards compatibility when updating exported functions.

I feel a word is missing. What "provides" it?

> + * When a symbol is exported from a library to provide an API, it also provides a
> + * calling convention (ABI) that is embodied in its name, return type,
> + * arguments, etc.  On occasion that function may need to change to accommodate
> + * new functionality, behavior, etc.  When that occurs, it is desirable to
> + * allow for backwards compatibility for a time with older binaries that are
> + * dynamically linked to the dpdk.
> + */
> +
> +#ifdef RTE_BUILD_SHARED_LIB
> +
> +/*

Why not Doxygen style?

> + * RTE_VERSION_SYMBOL

No need to repeat the macro name here.

> + * Creates a symbol version table entry binding symbol <name>@DPDK_<ver> to the internal

Imperative form is more usual.

> + * function name <name>_v<ver>.
> + */
> +#define RTE_VERSION_SYMBOL(ver, type, name, args) VERSIONING_WARN \
> +__asm__(".symver " RTE_STR(name) "_v" RTE_STR(ver) ", " RTE_STR(name) "@DPDK_" RTE_STR(ver)); \
> +__rte_used type name ## _v ## ver args; \
> +type name ## _v ## ver args

This is only GNU style. It should be highlighted both in this file and the commit log.

> +
> +/*
> + * RTE_VERSION_EXPERIMENTAL_SYMBOL
> + * Similar to RTE_VERSION_SYMBOL but for experimental API symbols.
> + * This is mainly used for keeping compatibility for symbols that get promoted to stable ABI.
> + */
> +#define RTE_VERSION_EXPERIMENTAL_SYMBOL(type, name, args) VERSIONING_WARN \
> +__asm__(".symver " RTE_STR(name) "_exp, " RTE_STR(name) "@EXPERIMENTAL") \
> +__rte_used type name ## _exp args; \
> +type name ## _exp args
> +
> +/*
> + * RTE_DEFAULT_SYMBOL
> + * Creates a symbol version entry instructing the linker to bind references to
> + * symbol <name> to the internal symbol <name>_v<ver>.
> + */
> +#define RTE_DEFAULT_SYMBOL(ver, type, name, args) VERSIONING_WARN \
> +__asm__(".symver " RTE_STR(name) "_v" RTE_STR(ver) ", " RTE_STR(name) "@@DPDK_" RTE_STR(ver)); \
> +__rte_used type name ## _v ## ver args; \
> +type name ## _v ## ver args

Only 3 macros, that's simple and clean, thank you.