Re: [dpdk-dev] [PATCH 1/4 v2] compat: Add infrastructure to support symbol versioning

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Sep 26, 2014 at 12:22:56PM -0400, Neil Horman wrote:
> On Fri, Sep 26, 2014 at 04:33:04PM +0100, Sergio Gonzalez Monroy wrote:
> > On Fri, Sep 26, 2014 at 11:16:30AM -0400, Neil Horman wrote:
> > > On Fri, Sep 26, 2014 at 03:16:08PM +0100, Sergio Gonzalez Monroy wrote:
> > > > On Thu, Sep 25, 2014 at 02:52:32PM -0400, Neil Horman wrote:
> > > > > Add initial pass header files to support symbol versioning.
> > > > > 
> > > > > ---
> > > > > Change notes
> > > > > v2)
> > > > > * Fixed ifdef in rte_compat.h to test for RTE_BUILD_SHARED_LIB instead of the
> > > > > non-existant RTE_SYMBOL_VERSIONING
> > > > > 
> > > > > * Fixed VERSION_SYMBOL macro to add the needed extra @ to make versioning work
> > > > > properly
> > > > > 
> > > > > * Improved/Clarified documentation
> > > > > 
> > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > > > CC: Thomas Monjalon <thomas.monjalon@6wind.com>
> > > > > CC: "Richardson, Bruce" <bruce.richardson@intel.com>
> > > > > CC: "Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy@intel.com>
> > > > > ---
> > > > >  lib/Makefile                   |  1 +
> > > > >  lib/librte_compat/Makefile     | 38 ++++++++++++++++++
> > > > >  lib/librte_compat/rte_compat.h | 87 ++++++++++++++++++++++++++++++++++++++++++
> > > > >  mk/rte.lib.mk                  |  6 +++
> > > > >  4 files changed, 132 insertions(+)
> > > > >  create mode 100644 lib/librte_compat/Makefile
> > > > >  create mode 100644 lib/librte_compat/rte_compat.h
> > > > > 
> > > > > diff --git a/lib/Makefile b/lib/Makefile
> > > > > index 10c5bb3..a85b55b 100644
> > > > > --- a/lib/Makefile
> > > > > +++ b/lib/Makefile
> > > > > @@ -32,6 +32,7 @@
> > > > >  include $(RTE_SDK)/mk/rte.vars.mk
> > > > >  
> > > > >  DIRS-$(CONFIG_RTE_LIBC) += libc
> > > > > +DIRS-y += librte_compat
> > > > >  DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
> > > > >  DIRS-$(CONFIG_RTE_LIBRTE_MALLOC) += librte_malloc
> > > > >  DIRS-$(CONFIG_RTE_LIBRTE_RING) += librte_ring
> > > > > diff --git a/lib/librte_compat/Makefile b/lib/librte_compat/Makefile
> > > > > new file mode 100644
> > > > > index 0000000..3415c7b
> > > > > --- /dev/null
> > > > > +++ b/lib/librte_compat/Makefile
> > > > > @@ -0,0 +1,38 @@
> > > > > +#   BSD LICENSE
> > > > > +#
> > > > > +#   Copyright(c) 2010-2014 Neil Horman <nhorman@tuxdriver.com>
> > > > > +#   All rights reserved.
> > > > > +#
> > > > > +#   Redistribution and use in source and binary forms, with or without
> > > > > +#   modification, are permitted provided that the following conditions
> > > > > +#   are met:
> > > > > +#
> > > > > +#     * Redistributions of source code must retain the above copyright
> > > > > +#       notice, this list of conditions and the following disclaimer.
> > > > > +#     * Redistributions in binary form must reproduce the above copyright
> > > > > +#       notice, this list of conditions and the following disclaimer in
> > > > > +#       the documentation and/or other materials provided with the
> > > > > +#       distribution.
> > > > > +#     * Neither the name of Intel Corporation nor the names of its
> > > > > +#       contributors may be used to endorse or promote products derived
> > > > > +#       from this software without specific prior written permission.
> > > > > +#
> > > > > +#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > > > > +#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > > > > +#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > > > > +#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > > > > +#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > > > > +#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > > > > +#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > > > > +#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > > > > +#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > > > > +#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > > > > +#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > > > > +
> > > > > +include $(RTE_SDK)/mk/rte.vars.mk
> > > > > +
> > > > > +
> > > > > +# install includes
> > > > > +SYMLINK-y-include := rte_compat.h
> > > > > +
> > > > > +include $(RTE_SDK)/mk/rte.lib.mk
> > > > > diff --git a/lib/librte_compat/rte_compat.h b/lib/librte_compat/rte_compat.h
> > > > > new file mode 100644
> > > > > index 0000000..cff9aea
> > > > > --- /dev/null
> > > > > +++ b/lib/librte_compat/rte_compat.h
> > > > > @@ -0,0 +1,87 @@
> > > > > +/*-
> > > > > + *   BSD LICENSE
> > > > > + *
> > > > > + *   Copyright(c) 2010-2014 Neil Horman <nhorman@tuxdriver.com>.
> > > > > + *   All rights reserved.
> > > > > + *
> > > > > + *   Redistribution and use in source and binary forms, with or without
> > > > > + *   modification, are permitted provided that the following conditions
> > > > > + *   are met:
> > > > > + *
> > > > > + *     * Redistributions of source code must retain the above copyright
> > > > > + *       notice, this list of conditions and the following disclaimer.
> > > > > + *     * Redistributions in binary form must reproduce the above copyright
> > > > > + *       notice, this list of conditions and the following disclaimer in
> > > > > + *       the documentation and/or other materials provided with the
> > > > > + *       distribution.
> > > > > + *     * Neither the name of Intel Corporation nor the names of its
> > > > > + *       contributors may be used to endorse or promote products derived
> > > > > + *       from this software without specific prior written permission.
> > > > > + *
> > > > > + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > > > > + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > > > > + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> > > > > + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > > > > + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > > > > + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > > > > + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > > > > + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > > > > + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > > > > + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> > > > > + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > > > > + */
> > > > > +
> > > > > +#ifndef _RTE_COMPAT_H_
> > > > > +#define _RTE_COMPAT_H_
> > > > > +
> > > > > +/*
> > > > > + * This is just a stringification macro for use below.
> > > > > + */
> > > > > +#define SA(x) #x
> > > > > +
> > > > > +#ifdef RTE_BUILD_SHARED_LIB
> > > > > +
> > > > > +/*
> > > > > + * Provides backwards compatibility when updating exported functions.
> > > > > + * When a symol 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 accomodate
> > > > > + * new functionality, behavior, etc.  When that occurs, it is desireable to
> > > > > + * allow for backwards compatibility for a time with older binaries that are
> > > > > + * dynamically linked to the dpdk.  to support that the __vsym and
> > > > > + * VERSION_SYMBOL macros are created.  They, in conjunction with the
> > > > > + * <library>_version.map file for a given library allow for multiple versions of
> > > > > + * a symbol to exist in a shared library so that older binaries need not be
> > > > > + * immediately recompiled. Their use is outlined in the following example:
> > > > > + * Assumptions: DPDK 1.(X) contains a function int foo(char *string)
> > > > > + *              DPDK 1.(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_1.(X+1) node, in which
> > > > > + * foo is exported as a global symbol.  Note that foo must be removed from the
> > > > > + * DPDK.(X) node, or you will see multiple symbol definitions
> > > > > + *
> > > > 
> > > > By removing the symbol from the previous node in the version map, you make
> > > > it local instead of global and applications linked against DPDK 1.8 will fail
> > > > with the new library.
> > > > 
> > > It sounds like you just did the remove part, and not the add part.  What does
> > > your new version map file look like?
> > > 
> > > Neil
> > > 
> > 
> > $ cat lib/librte_acl/rte_acl_version.map
> > DPDK_1.8 {
> >     global:
> >     rte_acl_find_existing;
> >     rte_acl_free;
> >     rte_acl_add_rules;
> >     rte_acl_reset_rules;
> >     rte_acl_build;
> >     rte_acl_reset;
> >     rte_acl_classify;
> >     rte_acl_dump;
> >     rte_acl_list_dump;
> >     rte_acl_ipv4vlan_add_rules;
> >     rte_acl_ipv4vlan_build;
> >     rte_acl_classify_scalar;
> >     rte_acl_classify_alg;
> >     rte_acl_set_ctx_classify;
> > 
> >     local: *;
> > };
> > 
> > DPDK_1.9 {
> >     global:
> >     rte_acl_create;
> > } DPDK_1.8;
> > 
> > 
> > Anyway, if the DPDK_1.9 node was not defined, the dso would not have the symbol:
> > rte_acl_create@@DPDK_1.9
> > 
> > Sergio
> > 
> > > > Following the steps you describe, if we create a new version of the function
> > > > rte_acl_create we would end up with the following dso:
> > > > 
> > > > $ readelf -s x86_64-native-linuxapp-gcc/lib/librte_acl.so | grep "create\|\.symtab\|\.dynsym"
> > > > Symbol table '.dynsym' contains 42 entries:
> > > >     28: 0000000000001990   627 FUNC    GLOBAL DEFAULT   12 rte_acl_create@@DPDK_1.9
> > > > Symbol table '.symtab' contains 147 entries:
> > > >     94: 0000000000001960    36 FUNC    LOCAL  DEFAULT   12 rte_acl_create_v18
> > > >    105: 0000000000001960    36 FUNC    LOCAL  DEFAULT   12 rte_acl_create@@DPDK_1.8
> > > >    138: 0000000000001990   627 FUNC    GLOBAL DEFAULT   12 rte_acl_create
> > > > 
> > > > You can check that applications linked with the old lib will fail to run.
> > > > Note that to easily check this you should define the environment variable
> > > > LD_BIND_NOW to resolve all symbols at program startup (man ld.so).
> > > > 
> > > > Sergio
> > > > 
> Hm, thats odd, Using the same changes in my build here, I get both an exported
> global rte_acl_create@@DPDK_1.8 and @@DPDK_1.9 symbol.  Let me take a closer
> look at it here once I get through the rest of my email
> Neil
> 
> > > > > + * 2) rename the existing function int foo(char *string) to 
> > > > > + * 	int __vsym foo_v18(char *string)
> > > > > + *
> > > > > + * 3) Add this macro immediately below the function
> > > > > + * 	VERSION_SYMBOL(foo, _v18, 1.8);
> > > > > + *
> > > > > + */
> > > > > +#define VERSION_SYMBOL(b, e, v) __asm__(".symver " SA(b) SA(e) ", "SA(b)"@@DPDK_"SA(v))
> > > > > +#define __vsym __attribute__((used))
> > > > > +
> > > > > +#else
> > > > > +/*
> > > > > + * No symbol versioning in use
> > > > > + */
> > > > > +#define VERSION_SYMBOL(b, e, v)
> > > > > +#define __vsym
> > > > > +
> > > > > +/*
> > > > > + * RTE_BUILD_SHARED_LIB
> > > > > + */
> > > > > +#endif
> > > > > +
> > > > > +
> > > > > +#endif /* _RTE_COMPAT_H_ */
> > > > > diff --git a/mk/rte.lib.mk b/mk/rte.lib.mk
> > > > > index f458258..82ac309 100644
> > > > > --- a/mk/rte.lib.mk
> > > > > +++ b/mk/rte.lib.mk
> > > > > @@ -40,8 +40,12 @@ VPATH += $(SRCDIR)
> > > > >  
> > > > >  ifeq ($(RTE_BUILD_SHARED_LIB),y)
> > > > >  LIB := $(patsubst %.a,%.so,$(LIB))
> > > > > +
> > > > > +CPU_LDFLAGS += --version-script=$(EXPORT_MAP)
> > > > > +
> > > > >  endif
> > > > >  
> > > > > +
> > > > >  _BUILD = $(LIB)
> > > > >  _INSTALL = $(INSTALL-FILES-y) $(SYMLINK-FILES-y) $(RTE_OUTPUT)/lib/$(LIB)
> > > > >  _CLEAN = doclean
> > > > > @@ -160,7 +164,9 @@ endif
> > > > >  $(RTE_OUTPUT)/lib/$(LIB): $(LIB)
> > > > >  	@echo "  INSTALL-LIB $(LIB)"
> > > > >  	@[ -d $(RTE_OUTPUT)/lib ] || mkdir -p $(RTE_OUTPUT)/lib
> > > > > +ifneq ($(LIB),)
> > > > >  	$(Q)cp -f $(LIB) $(RTE_OUTPUT)/lib
> > > > > +endif
> > > > >  
> > > > >  #
> > > > >  # Clean all generated files
> > > > > -- 
> > > > > 1.9.3
> > > > > 
> > > > 
> > 
> 


Well, I have to apologize Sergio.  Apparently I misread something in the guide
for symbol versioning and this isn't in fact working.  It appeared to be working
for me because something was messed up in my tree and I wasn't relinking when I
updated the map file.  So self-NAK on this series for now, I'll repost it
shortly.  The good news is I have a bit of smaple code working properly, which
I've verified, and I should have a new version of this series (which should by
an large look the same) ready early next week

Best
Neil