[dpdk-dev,PATCHv3,2/3] rte_compat: Add MAP_STATIC_SYMBOL macro

Message ID 1435242949-31520-2-git-send-email-nhorman@tuxdriver.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Neil Horman June 25, 2015, 2:35 p.m. UTC
It was pointed out in my examples that doing shared library symbol versioning by
partitioning symbols to version specific functions (as opposed to leaving the
latest symol version at the base symbol name), neglects to take into account
static builds.  Add a macro to handle that.  If you choose a versioning approach
that uniquely names every version of the symbol, then this macro lets you map
your symbol choice to the base name when building a static library

Also, while I'm at it, since we're documenting this in the guide, take the
abbreviated example out of the header

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: thomas.monjalon@6wind.com
---
 lib/librte_compat/rte_compat.h | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)
  

Comments

Maciej Gajdzica June 26, 2015, 10:13 a.m. UTC | #1
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> Sent: Thursday, June 25, 2015 4:36 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCHv3 2/3] rte_compat: Add MAP_STATIC_SYMBOL
> macro
> 
> It was pointed out in my examples that doing shared library symbol versioning by
> partitioning symbols to version specific functions (as opposed to leaving the
> latest symol version at the base symbol name), neglects to take into account
> static builds.  Add a macro to handle that.  If you choose a versioning approach
> that uniquely names every version of the symbol, then this macro lets you map
> your symbol choice to the base name when building a static library
> 
> Also, while I'm at it, since we're documenting this in the guide, take the
> abbreviated example out of the header
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: thomas.monjalon@6wind.com

Acked-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>

> ---
>  lib/librte_compat/rte_compat.h | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/librte_compat/rte_compat.h b/lib/librte_compat/rte_compat.h
> index 75920a1..d7768d5 100644
> --- a/lib/librte_compat/rte_compat.h
> +++ b/lib/librte_compat/rte_compat.h
> @@ -49,22 +49,8 @@
>   * Assumptions: DPDK 2.(X) contains a function int foo(char *string)
>   *              DPDK 2.(X+1) needs to change foo to be int foo(int index)
>   *
> - * To accomplish this:
> - * 1) Edit lib/<library>/library_version.map to add a DPDK_2.(X+1) node, in
> which
> - * foo is exported as a global symbol.
> - *
> - * 2) rename the existing function int foo(char *string) to
> - *	int foo_v20(char *string)
> - *
> - * 3) Add this macro immediately below the function
> - *	VERSION_SYMBOL(foo, _v20, 2.0);
> - *
> - * 4) Implement a new version of foo.
> - *	char foo(int value, int otherval) { ...}
> - *
> - * 5) Mark the newest version as the default version
> - *	BIND_DEFAULT_SYMBOL(foo, _v21, 2.1);
> - *
> + * Refer to the guidelines document in the docs subdirectory for
> + details on the
> + * use of these macros
>   */
> 
>  /*
> @@ -72,6 +58,8 @@
>   * b - function base name
>   * e - function version extension, to be concatenated with base name
>   * n - function symbol version string to be applied
> + * f - function prototype
> + * p - full function symbol name
>   */
> 
>  /*
> @@ -96,6 +84,19 @@
>  #define BIND_DEFAULT_SYMBOL(b, e, n) __asm__(".symver " RTE_STR(b)
> RTE_STR(e) ", " RTE_STR(b) "@@DPDK_" RTE_STR(n))  #define __vsym
> __attribute__((used))
> 
> +/*
> + * MAP_STATIC_SYMBOL
> + * If a function has been bifurcated into multiple versions, none of
> +which
> + * are defined as the exported symbol name in the map file, this macro
> +can be
> + * used to alias a specific version of the symbol to its exported name.
> +For
> + * example, if you have 2 versions of a function foo_v1 and foo_v2,
> +where the
> + * former is mapped to foo@DPDK_1 and the latter is mapped to
> +foo@DPDK_2 when
> + * building a shared library, this macro can be used to map either
> +foo_v1 or
> + * foo_v2 to the symbol foo when building a static library, e.g.:
> + * MAP_STATIC_SYMBOL(void foo(), foo_v2);  */ #define
> +MAP_STATIC_SYMBOL(f, p)
> +
>  #else
>  /*
>   * No symbol versioning in use
> @@ -104,7 +105,7 @@
>  #define __vsym
>  #define BASE_SYMBOL(b, n)
>  #define BIND_DEFAULT_SYMBOL(b, e, n)
> -
> +#define MAP_STATIC_SYMBOL(f, p) f  __attribute__((alias( RTE_STR(p))))
>  /*
>   * RTE_BUILD_SHARED_LIB=n
>   */
> --
> 2.1.0
  
Thomas Monjalon June 26, 2015, 12:52 p.m. UTC | #2
2015-06-25 10:35, Neil Horman:
> +/*
> + * MAP_STATIC_SYMBOL
> + * If a function has been bifurcated into multiple versions, none of which
> + * are defined as the exported symbol name in the map file, this macro can be
> + * used to alias a specific version of the symbol to its exported name.  For
> + * example, if you have 2 versions of a function foo_v1 and foo_v2, where the
> + * former is mapped to foo@DPDK_1 and the latter is mapped to foo@DPDK_2 when
> + * building a shared library, this macro can be used to map either foo_v1 or
> + * foo_v2 to the symbol foo when building a static library, e.g.:
> + * MAP_STATIC_SYMBOL(void foo(), foo_v2);
> + */
> +#define MAP_STATIC_SYMBOL(f, p)
> +
>  #else
>  /*
>   * No symbol versioning in use
> @@ -104,7 +105,7 @@
>  #define __vsym
>  #define BASE_SYMBOL(b, n)
>  #define BIND_DEFAULT_SYMBOL(b, e, n)
> -
> +#define MAP_STATIC_SYMBOL(f, p) f  __attribute__((alias( RTE_STR(p))))

Is it working with clang and icc?
Why not just define foo as foo_v2?
As this is the equivalent of BIND_DEFAULT_SYMBOL for the static case,
it would be easier to mix them in only one macro.
  
Neil Horman June 26, 2015, 2:30 p.m. UTC | #3
On Fri, Jun 26, 2015 at 02:52:50PM +0200, Thomas Monjalon wrote:
> 2015-06-25 10:35, Neil Horman:
> > +/*
> > + * MAP_STATIC_SYMBOL
> > + * If a function has been bifurcated into multiple versions, none of which
> > + * are defined as the exported symbol name in the map file, this macro can be
> > + * used to alias a specific version of the symbol to its exported name.  For
> > + * example, if you have 2 versions of a function foo_v1 and foo_v2, where the
> > + * former is mapped to foo@DPDK_1 and the latter is mapped to foo@DPDK_2 when
> > + * building a shared library, this macro can be used to map either foo_v1 or
> > + * foo_v2 to the symbol foo when building a static library, e.g.:
> > + * MAP_STATIC_SYMBOL(void foo(), foo_v2);
> > + */
> > +#define MAP_STATIC_SYMBOL(f, p)
> > +
> >  #else
> >  /*
> >   * No symbol versioning in use
> > @@ -104,7 +105,7 @@
> >  #define __vsym
> >  #define BASE_SYMBOL(b, n)
> >  #define BIND_DEFAULT_SYMBOL(b, e, n)
> > -
> > +#define MAP_STATIC_SYMBOL(f, p) f  __attribute__((alias( RTE_STR(p))))
> 
> Is it working with clang and icc?
No idea.  It should work with clang (though I don't have it installed at the
moment), as the docs say the .symver directive is supported

as for icc, thats out of my control completely, as I don't have any access to
it.

> Why not just define foo as foo_v2?
I'm not sure what you mean here.  Are you suggesting that we just change the abi
so applications have to call foo_v2 rather than foo when we change the
prototype.  I suppose we could do that, but that seems like it would be an awful
irritant to users.  They would rather have a single symbol to call if it does
the same function.

> As this is the equivalent of BIND_DEFAULT_SYMBOL for the static case,
> it would be easier to mix them in only one macro.
> 
Because of where its used.  If you use BIND_DEFAULT_SYMBOL to do the work of
MAP_STATIC_SYMBOL, every compilation unit will define its own alias and you'll
get symbol conflicts.

Neil

>
  
Thomas Monjalon June 28, 2015, 8:13 p.m. UTC | #4
2015-06-26 10:30, Neil Horman:
> On Fri, Jun 26, 2015 at 02:52:50PM +0200, Thomas Monjalon wrote:
> > 2015-06-25 10:35, Neil Horman:
> > > +/*
> > > + * MAP_STATIC_SYMBOL
> > > + * If a function has been bifurcated into multiple versions, none of which
> > > + * are defined as the exported symbol name in the map file, this macro can be
> > > + * used to alias a specific version of the symbol to its exported name.  For
> > > + * example, if you have 2 versions of a function foo_v1 and foo_v2, where the
> > > + * former is mapped to foo@DPDK_1 and the latter is mapped to foo@DPDK_2 when
> > > + * building a shared library, this macro can be used to map either foo_v1 or
> > > + * foo_v2 to the symbol foo when building a static library, e.g.:
> > > + * MAP_STATIC_SYMBOL(void foo(), foo_v2);
> > > + */
> > > +#define MAP_STATIC_SYMBOL(f, p)
> > > +
> > >  #else
> > >  /*
> > >   * No symbol versioning in use
> > > @@ -104,7 +105,7 @@
> > >  #define __vsym
> > >  #define BASE_SYMBOL(b, n)
> > >  #define BIND_DEFAULT_SYMBOL(b, e, n)
> > > -
> > > +#define MAP_STATIC_SYMBOL(f, p) f  __attribute__((alias( RTE_STR(p))))
> > 
> > Is it working with clang and icc?
> No idea.  It should work with clang (though I don't have it installed at the
> moment), as the docs say the .symver directive is supported
> 
> as for icc, thats out of my control completely, as I don't have any access to
> it.
> 
> > Why not just define foo as foo_v2?
> I'm not sure what you mean here.  Are you suggesting that we just change the abi
> so applications have to call foo_v2 rather than foo when we change the
> prototype.  I suppose we could do that, but that seems like it would be an awful
> irritant to users.  They would rather have a single symbol to call if it does
> the same function.
> 
> > As this is the equivalent of BIND_DEFAULT_SYMBOL for the static case,
> > it would be easier to mix them in only one macro.
> > 
> Because of where its used.  If you use BIND_DEFAULT_SYMBOL to do the work of
> MAP_STATIC_SYMBOL, every compilation unit will define its own alias and you'll
> get symbol conflicts.

Oh, you mean it shouldn't be used in a .h file?
If so, this limitation should be noted in the description above.

OK for that solution, that's the best we have at the moment.
  
Neil Horman June 29, 2015, 1:44 p.m. UTC | #5
On Sun, Jun 28, 2015 at 10:13:31PM +0200, Thomas Monjalon wrote:
> 2015-06-26 10:30, Neil Horman:
> > On Fri, Jun 26, 2015 at 02:52:50PM +0200, Thomas Monjalon wrote:
> > > 2015-06-25 10:35, Neil Horman:
> > > > +/*
> > > > + * MAP_STATIC_SYMBOL
> > > > + * If a function has been bifurcated into multiple versions, none of which
> > > > + * are defined as the exported symbol name in the map file, this macro can be
> > > > + * used to alias a specific version of the symbol to its exported name.  For
> > > > + * example, if you have 2 versions of a function foo_v1 and foo_v2, where the
> > > > + * former is mapped to foo@DPDK_1 and the latter is mapped to foo@DPDK_2 when
> > > > + * building a shared library, this macro can be used to map either foo_v1 or
> > > > + * foo_v2 to the symbol foo when building a static library, e.g.:
> > > > + * MAP_STATIC_SYMBOL(void foo(), foo_v2);
> > > > + */
> > > > +#define MAP_STATIC_SYMBOL(f, p)
> > > > +
> > > >  #else
> > > >  /*
> > > >   * No symbol versioning in use
> > > > @@ -104,7 +105,7 @@
> > > >  #define __vsym
> > > >  #define BASE_SYMBOL(b, n)
> > > >  #define BIND_DEFAULT_SYMBOL(b, e, n)
> > > > -
> > > > +#define MAP_STATIC_SYMBOL(f, p) f  __attribute__((alias( RTE_STR(p))))
> > > 
> > > Is it working with clang and icc?
> > No idea.  It should work with clang (though I don't have it installed at the
> > moment), as the docs say the .symver directive is supported
> > 
> > as for icc, thats out of my control completely, as I don't have any access to
> > it.
> > 
> > > Why not just define foo as foo_v2?
> > I'm not sure what you mean here.  Are you suggesting that we just change the abi
> > so applications have to call foo_v2 rather than foo when we change the
> > prototype.  I suppose we could do that, but that seems like it would be an awful
> > irritant to users.  They would rather have a single symbol to call if it does
> > the same function.
> > 
> > > As this is the equivalent of BIND_DEFAULT_SYMBOL for the static case,
> > > it would be easier to mix them in only one macro.
> > > 
> > Because of where its used.  If you use BIND_DEFAULT_SYMBOL to do the work of
> > MAP_STATIC_SYMBOL, every compilation unit will define its own alias and you'll
> > get symbol conflicts.
> 
> Oh, you mean it shouldn't be used in a .h file?
> If so, this limitation should be noted in the description above.
> 
Its already noted in the example, I'd rather not make a unilateral statement
about where to use it above because there can be cause to use it internally as
well.

> OK for that solution, that's the best we have at the moment.
>
  

Patch

diff --git a/lib/librte_compat/rte_compat.h b/lib/librte_compat/rte_compat.h
index 75920a1..d7768d5 100644
--- a/lib/librte_compat/rte_compat.h
+++ b/lib/librte_compat/rte_compat.h
@@ -49,22 +49,8 @@ 
  * Assumptions: DPDK 2.(X) contains a function int foo(char *string)
  *              DPDK 2.(X+1) needs to change foo to be int foo(int index)
  *
- * To accomplish this:
- * 1) Edit lib/<library>/library_version.map to add a DPDK_2.(X+1) node, in which
- * foo is exported as a global symbol.
- *
- * 2) rename the existing function int foo(char *string) to
- *	int foo_v20(char *string)
- *
- * 3) Add this macro immediately below the function
- *	VERSION_SYMBOL(foo, _v20, 2.0);
- *
- * 4) Implement a new version of foo.
- *	char foo(int value, int otherval) { ...}
- *
- * 5) Mark the newest version as the default version
- *	BIND_DEFAULT_SYMBOL(foo, _v21, 2.1);
- *
+ * Refer to the guidelines document in the docs subdirectory for details on the
+ * use of these macros
  */
 
 /*
@@ -72,6 +58,8 @@ 
  * b - function base name
  * e - function version extension, to be concatenated with base name
  * n - function symbol version string to be applied
+ * f - function prototype
+ * p - full function symbol name
  */
 
 /*
@@ -96,6 +84,19 @@ 
 #define BIND_DEFAULT_SYMBOL(b, e, n) __asm__(".symver " RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@@DPDK_" RTE_STR(n))
 #define __vsym __attribute__((used))
 
+/*
+ * MAP_STATIC_SYMBOL
+ * If a function has been bifurcated into multiple versions, none of which
+ * are defined as the exported symbol name in the map file, this macro can be
+ * used to alias a specific version of the symbol to its exported name.  For
+ * example, if you have 2 versions of a function foo_v1 and foo_v2, where the
+ * former is mapped to foo@DPDK_1 and the latter is mapped to foo@DPDK_2 when
+ * building a shared library, this macro can be used to map either foo_v1 or
+ * foo_v2 to the symbol foo when building a static library, e.g.:
+ * MAP_STATIC_SYMBOL(void foo(), foo_v2);
+ */
+#define MAP_STATIC_SYMBOL(f, p)
+
 #else
 /*
  * No symbol versioning in use
@@ -104,7 +105,7 @@ 
 #define __vsym
 #define BASE_SYMBOL(b, n)
 #define BIND_DEFAULT_SYMBOL(b, e, n)
-
+#define MAP_STATIC_SYMBOL(f, p) f  __attribute__((alias( RTE_STR(p))))
 /*
  * RTE_BUILD_SHARED_LIB=n
  */