On Wed, Apr 2, 2025 at 1:53 PM Thomas Monjalon <tho...@monjalon.net> wrote:
>
> 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?
Symbol versioning is not functional with Windows compiler / linker atm.
When it is ready, the right warning mechanism can be implemented here.
>
> > +
> > +/*
> > + * Provides backwards compatibility when updating exported functions.
>
> I feel a word is missing. What "provides" it?
I'll reword.
>
> > + * 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?
Because this header is internal, and not parsed by Doxygen?
>
> > + * 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.
Ack.
>
> > + * 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.
Yes, I'll insist on this point in the commitlog.
--
David Marchand
