[v2,2/2] build: support building ABI versioned files twice

  Any file with ABI versioned functions needs different macros for shared and
static builds, so we need to accomodate that. Rather than building
everything twice, we just flag to the build system which libraries need
that handling, by setting use_function_versioning in the meson.build files.

To ensure we don't get silent errors at build time due to this meson flag
being missed, we add an explicit error to the function versioning header
file if a known C macro is not defined. Since "make" builds always only
build one of shared or static libraries, this define can be always set, and
so is added to the common_base file. For meson, the build flag - and
therefore the C define - is set for the three libraries that need the
function versioning: "distributor", "lpm" and "timer".

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 config/common_base                               |  1 +
 config/rte_config.h                              |  3 ---
 doc/guides/contributing/coding_style.rst         |  7 +++++++
 lib/librte_distributor/meson.build               |  1 +
 .../common/include/rte_function_versioning.h     |  4 ++++
 lib/librte_lpm/meson.build                       |  1 +
 lib/librte_timer/meson.build                     |  1 +
 lib/meson.build                                  | 16 +++++++++++++---
 8 files changed, 28 insertions(+), 6 deletions(-)
  

Thanks Bruce for the patch.  I like the idea of splitting versioning out
of rte_compat.h, but I have some comments.

On 9/27/19 10:59 PM, Bruce Richardson wrote:
[...]
> --- a/config/common_base
> +++ b/config/common_base
> @@ -111,6 +111,7 @@ CONFIG_RTE_MAX_VFIO_CONTAINERS=64
>  CONFIG_RTE_MALLOC_DEBUG=n
>  CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n
>  CONFIG_RTE_USE_LIBBSD=n
> +CONFIG_RTE_USE_FUNCTION_VERSIONING=y

I'm not fond of this config option - it is not really an option to be
changed by the user.  I would prefer to just add flag to CFLAGS in
mk/target/generic/rte.vars.mk.

>  #
>  # Recognize/ignore the AVX/AVX512 CPU flags for performance/power testing.
> diff --git a/config/rte_config.h b/config/rte_config.h
> index 0bbbe274f..b63a2fdea 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -31,9 +31,6 @@
>  
>  /****** library defines ********/
>  
> -/* compat defines */
> -#define RTE_BUILD_SHARED_LIB
> -

So now everything builds "as static lib" (but with "-fPIC") apart from
those libraries that use symbol versioning.  I'm OK with that however
I'd like to note that code might be using RTE_BUILD_SHARED_LIB and do
different things e.g. app/test-bbdev/test_bbdev_perf.c.  I know that was
already the case - just wanted to say that aloud to make sure we are all
aware of this :).

> diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
> index 449b33494..e95a1a2be 100644
> --- a/doc/guides/contributing/coding_style.rst
> +++ b/doc/guides/contributing/coding_style.rst
> @@ -948,6 +948,13 @@ reason
>  	built. For missing dependencies this should be of the form
>  	``'missing dependency, "libname"'``.
>  
> +use_function_versioning
> +	**Default Value = false**.
> +	Specifies if the library in question has ABI versioned functions. If it
> +	has, this value should be set to ensure that the C files are compiled
> +	twice with suitable parameters for each of shared or static library
> +	builds.
> +

Maybe a different name for this option?  In general an "ideal
theoretical" solution would be for build system to figure out on its own
that separate build is necessary automatically - but that might incur
some performance penalty (additional grep'ing of sources or so).  So I'm
fine with this option however I'd like to rename it to actually indicate
what it's effect is.  Like 'separate_build' or 'split_build' or
'rebuild_objects' or ...

The intention of using of versioned symbols is already indicated by
inclusion of the relevant header.

> diff --git a/lib/librte_eal/common/include/rte_function_versioning.h b/lib/librte_eal/common/include/rte_function_versioning.h
> index ce963d4b1..55e88ffae 100644
> --- a/lib/librte_eal/common/include/rte_function_versioning.h
> +++ b/lib/librte_eal/common/include/rte_function_versioning.h
> @@ -7,6 +7,10 @@
>  #define _RTE_FUNCTION_VERSIONING_H_
>  #include <rte_common.h>
>  
> +#ifndef RTE_USE_FUNCTION_VERSIONING
> +#error Use of function versioning disabled, is "use_function_versioning=true" in meson.build?
> +#endif

If you accept above suggestion then change this message to something
like: "Function versioning requires 'separate_build=true' in meson.build"

BTW it turned out that this need for separate build for versioned
symbols is a result of long standing gcc bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48200

I'll test this with clang and if this will work then maybe we could
guard this #if with another check for 'gcc'.

Best regards
Andrzej

Tested-by: Andrzej Ostruszka <amo@semihalf.com>
  

On Tue, Oct 01, 2019 at 03:23:47PM +0200, Andrzej Ostruszka wrote:
> Thanks Bruce for the patch.  I like the idea of splitting versioning out
> of rte_compat.h, but I have some comments.
> 
> On 9/27/19 10:59 PM, Bruce Richardson wrote:
> [...]
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -111,6 +111,7 @@ CONFIG_RTE_MAX_VFIO_CONTAINERS=64
> >  CONFIG_RTE_MALLOC_DEBUG=n
> >  CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n
> >  CONFIG_RTE_USE_LIBBSD=n
> > +CONFIG_RTE_USE_FUNCTION_VERSIONING=y
> 
> I'm not fond of this config option - it is not really an option to be
> changed by the user.  I would prefer to just add flag to CFLAGS in
> mk/target/generic/rte.vars.mk.
> 

Ok, that sounds reasonable enough.

> >  #
> >  # Recognize/ignore the AVX/AVX512 CPU flags for performance/power testing.
> > diff --git a/config/rte_config.h b/config/rte_config.h
> > index 0bbbe274f..b63a2fdea 100644
> > --- a/config/rte_config.h
> > +++ b/config/rte_config.h
> > @@ -31,9 +31,6 @@
> >  
> >  /****** library defines ********/
> >  
> > -/* compat defines */
> > -#define RTE_BUILD_SHARED_LIB
> > -
> 
> So now everything builds "as static lib" (but with "-fPIC") apart from
> those libraries that use symbol versioning.  I'm OK with that however
> I'd like to note that code might be using RTE_BUILD_SHARED_LIB and do
> different things e.g. app/test-bbdev/test_bbdev_perf.c.  I know that was
> already the case - just wanted to say that aloud to make sure we are all
> aware of this :).

Thanks for pointing this out, I wasn't aware of it! Doing a git grep this
seems to be the only place in a C file (other than rte_config.h and
rte_compat.h) where the SHARED_LIB flag is being checked. I'll need to
follow up on that to see what the logic is there, because it seems strange
to require such a check.

> 
> > diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
> > index 449b33494..e95a1a2be 100644
> > --- a/doc/guides/contributing/coding_style.rst
> > +++ b/doc/guides/contributing/coding_style.rst
> > @@ -948,6 +948,13 @@ reason
> >  	built. For missing dependencies this should be of the form
> >  	``'missing dependency, "libname"'``.
> >  
> > +use_function_versioning
> > +	**Default Value = false**.
> > +	Specifies if the library in question has ABI versioned functions. If it
> > +	has, this value should be set to ensure that the C files are compiled
> > +	twice with suitable parameters for each of shared or static library
> > +	builds.
> > +
> 
> Maybe a different name for this option?  In general an "ideal
> theoretical" solution would be for build system to figure out on its own
> that separate build is necessary automatically - but that might incur
> some performance penalty (additional grep'ing of sources or so). 

I was thinking about that, and how we can do it automatically. The trouble
is that for correctness we would need to recheck every file after it had
changed, and since the result of the check means that we have different
build steps it would basically mean doing a full reconfiguration for every
file change. That's not really practical, hence this proposed solution.

> So I'm
> fine with this option however I'd like to rename it to actually indicate
> what it's effect is.  Like 'separate_build' or 'split_build' or
> 'rebuild_objects' or ...
> 
> The intention of using of versioned symbols is already indicated by
> inclusion of the relevant header.

I actually feel the opposite. I'd rather have the name tied in to the fact
that it's related to using function versioning - subject to what is found
on investigating the #ifdef in the bbdev perf test. However, if you feel
strongly about the name something else, I can probably compromise on it :-)

> 
> > diff --git a/lib/librte_eal/common/include/rte_function_versioning.h b/lib/librte_eal/common/include/rte_function_versioning.h
> > index ce963d4b1..55e88ffae 100644
> > --- a/lib/librte_eal/common/include/rte_function_versioning.h
> > +++ b/lib/librte_eal/common/include/rte_function_versioning.h
> > @@ -7,6 +7,10 @@
> >  #define _RTE_FUNCTION_VERSIONING_H_
> >  #include <rte_common.h>
> >  
> > +#ifndef RTE_USE_FUNCTION_VERSIONING
> > +#error Use of function versioning disabled, is "use_function_versioning=true" in meson.build?
> > +#endif
> 
> If you accept above suggestion then change this message to something
> like: "Function versioning requires 'separate_build=true' in meson.build"
> 

Agreed. This obviously needs to be kept in sync with whatever the build
flag is.

> BTW it turned out that this need for separate build for versioned
> symbols is a result of long standing gcc bug:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48200
> 
> I'll test this with clang and if this will work then maybe we could
> guard this #if with another check for 'gcc'.
> 
> Best regards
> Andrzej
> 
> Tested-by: Andrzej Ostruszka <amo@semihalf.com>

Let me know what your testing throws up, thanks.

/Bruce
  

On Tue, Oct 01, 2019 at 05:53:05PM +0100, Bruce Richardson wrote:
> On Tue, Oct 01, 2019 at 03:23:47PM +0200, Andrzej Ostruszka wrote:
> > Thanks Bruce for the patch.  I like the idea of splitting versioning out
> > of rte_compat.h, but I have some comments.
> > 
> > On 9/27/19 10:59 PM, Bruce Richardson wrote:
> > [...]
> > > --- a/config/common_base
> > > +++ b/config/common_base
> > > @@ -111,6 +111,7 @@ CONFIG_RTE_MAX_VFIO_CONTAINERS=64
> > >  CONFIG_RTE_MALLOC_DEBUG=n
> > >  CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES=n
> > >  CONFIG_RTE_USE_LIBBSD=n
> > > +CONFIG_RTE_USE_FUNCTION_VERSIONING=y
> > 
> > I'm not fond of this config option - it is not really an option to be
> > changed by the user.  I would prefer to just add flag to CFLAGS in
> > mk/target/generic/rte.vars.mk.
> > 
> 
> Ok, that sounds reasonable enough.

Done in V3.

> 
> > >  #
> > >  # Recognize/ignore the AVX/AVX512 CPU flags for performance/power testing.
> > > diff --git a/config/rte_config.h b/config/rte_config.h
> > > index 0bbbe274f..b63a2fdea 100644
> > > --- a/config/rte_config.h
> > > +++ b/config/rte_config.h
> > > @@ -31,9 +31,6 @@
> > >  
> > >  /****** library defines ********/
> > >  
> > > -/* compat defines */
> > > -#define RTE_BUILD_SHARED_LIB
> > > -
> > 
> > So now everything builds "as static lib" (but with "-fPIC") apart from
> > those libraries that use symbol versioning.  I'm OK with that however
> > I'd like to note that code might be using RTE_BUILD_SHARED_LIB and do
> > different things e.g. app/test-bbdev/test_bbdev_perf.c.  I know that was
> > already the case - just wanted to say that aloud to make sure we are all
> > aware of this :).
> 
> Thanks for pointing this out, I wasn't aware of it! Doing a git grep this
> seems to be the only place in a C file (other than rte_config.h and
> rte_compat.h) where the SHARED_LIB flag is being checked. I'll need to
> follow up on that to see what the logic is there, because it seems strange
> to require such a check.
> 

This #ifdef can be removed, see patchset: 
http://patches.dpdk.org/project/dpdk/list/?series=6699

This then leaves function versioning as the only use-case where we need
different code paths for static vs shared builds.

> > 
> > > diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
> > > index 449b33494..e95a1a2be 100644
> > > --- a/doc/guides/contributing/coding_style.rst
> > > +++ b/doc/guides/contributing/coding_style.rst
> > > @@ -948,6 +948,13 @@ reason
> > >  	built. For missing dependencies this should be of the form
> > >  	``'missing dependency, "libname"'``.
> > >  
> > > +use_function_versioning
> > > +	**Default Value = false**.
> > > +	Specifies if the library in question has ABI versioned functions. If it
> > > +	has, this value should be set to ensure that the C files are compiled
> > > +	twice with suitable parameters for each of shared or static library
> > > +	builds.
> > > +
> > 
> > Maybe a different name for this option?  In general an "ideal
> > theoretical" solution would be for build system to figure out on its own
> > that separate build is necessary automatically - but that might incur
> > some performance penalty (additional grep'ing of sources or so). 
> 
> I was thinking about that, and how we can do it automatically. The trouble
> is that for correctness we would need to recheck every file after it had
> changed, and since the result of the check means that we have different
> build steps it would basically mean doing a full reconfiguration for every
> file change. That's not really practical, hence this proposed solution.
> 

I've not made any changes here for the V3. However, if we want to reduce
the number of changes required, we could always switch to having the
rte_function_versioning.h header file included on the basis of the flag in
the meson.build file. Having the C flag compile vary based on the meson one
is normal, having the inverse is problematic because of what I explained
above - you'd basically need to reconfigure to check after each file
change.

Personally, I don't think changing things to auto-include the header is
worth it, hence the fact of no-change in v3.

Regards,
/Bruce