[2/2] bpf: remove use of weak functions

  Weak functions don't work well with static libraries and require the use of
"whole-archive" flag to ensure that the correct function is used when
linking.  Since the weak function is only used as a placeholder within this
library alone, we can replace it with a non-weak version protected using
preprocessor ifdefs.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_bpf/bpf_load.c  | 4 +++-
 lib/librte_bpf/meson.build | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)
  

Bruce Richardson <bruce.richardson@intel.com> writes:

> Weak functions don't work well with static libraries and require the use of
> "whole-archive" flag to ensure that the correct function is used when
> linking.  Since the weak function is only used as a placeholder within this
> library alone, we can replace it with a non-weak version protected using
> preprocessor ifdefs.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

I agree with dropping the weak implementations.

But, can't we adjust the order of objects when building with multiple
strong definitions and the linker will choose the first?  I know this
works for the GNU linker.

I do find this information to help support this statement:

https://stackoverflow.com/questions/51656838/attribute-weak-and-static-libraries

Unfortunately, I don't find anything in the C standard to govern entity
precedence.  This means it isn't something that works across linker
implementations (but compatible implementations like clang will maintain
that behavior).

>  lib/librte_bpf/bpf_load.c  | 4 +++-
>  lib/librte_bpf/meson.build | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_bpf/bpf_load.c b/lib/librte_bpf/bpf_load.c
> index d9d163b7d..194103ec7 100644
> --- a/lib/librte_bpf/bpf_load.c
> +++ b/lib/librte_bpf/bpf_load.c
> @@ -131,7 +131,8 @@ rte_bpf_load(const struct rte_bpf_prm *prm)
>  	return bpf;
>  }
>  
> -__rte_experimental __rte_weak struct rte_bpf *
> +#ifndef RTE_LIBRTE_BPF_ELF
> +__rte_experimental struct rte_bpf *
>  rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
>  	const char *sname)
>  {
> @@ -146,3 +147,4 @@ rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
>  	rte_errno = ENOTSUP;
>  	return NULL;
>  }
> +#endif
> diff --git a/lib/librte_bpf/meson.build b/lib/librte_bpf/meson.build
> index 8a79878ff..11c1fb558 100644
> --- a/lib/librte_bpf/meson.build
> +++ b/lib/librte_bpf/meson.build
> @@ -20,6 +20,7 @@ deps += ['mbuf', 'net', 'ethdev']
>  
>  dep = dependency('libelf', required: false)
>  if dep.found()
> +	dpdk_conf.set('RTE_LIBRTE_BPF_ELF', 1)
>  	sources += files('bpf_load_elf.c')
>  	ext_deps += dep
>  endif
  

On Wed, Apr 10, 2019 at 10:07:33AM -0400, Aaron Conole wrote:
> Bruce Richardson <bruce.richardson@intel.com> writes:
> 
> > Weak functions don't work well with static libraries and require the use of
> > "whole-archive" flag to ensure that the correct function is used when
> > linking.  Since the weak function is only used as a placeholder within this
> > library alone, we can replace it with a non-weak version protected using
> > preprocessor ifdefs.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> 
> I agree with dropping the weak implementations.
> 
> But, can't we adjust the order of objects when building with multiple
> strong definitions and the linker will choose the first?  I know this
> works for the GNU linker.
> 
> I do find this information to help support this statement:
> 
> https://stackoverflow.com/questions/51656838/attribute-weak-and-static-libraries
> 
> Unfortunately, I don't find anything in the C standard to govern entity
> precedence.  This means it isn't something that works across linker
> implementations (but compatible implementations like clang will maintain
> that behavior).
> 
It may be possible, but I'd be worried about the fragility of the support.
Is it a good idea to have multiple implementations of a function in a .a
file. Without running the resulting binary, we have no way of knowing the
correct function was picked up. At least with the weak versions, we can see
from nm what is used.

/Bruce
  

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, April 10, 2019 2:45 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; aconole@redhat.com
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [PATCH 2/2] bpf: remove use of weak functions
> 
> Weak functions don't work well with static libraries and require the use of
> "whole-archive" flag to ensure that the correct function is used when
> linking.  Since the weak function is only used as a placeholder within this
> library alone, we can replace it with a non-weak version protected using
> preprocessor ifdefs.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/librte_bpf/bpf_load.c  | 4 +++-
>  lib/librte_bpf/meson.build | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_bpf/bpf_load.c b/lib/librte_bpf/bpf_load.c
> index d9d163b7d..194103ec7 100644
> --- a/lib/librte_bpf/bpf_load.c
> +++ b/lib/librte_bpf/bpf_load.c
> @@ -131,7 +131,8 @@ rte_bpf_load(const struct rte_bpf_prm *prm)
>  	return bpf;
>  }
> 
> -__rte_experimental __rte_weak struct rte_bpf *
> +#ifndef RTE_LIBRTE_BPF_ELF
> +__rte_experimental struct rte_bpf *
>  rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
>  	const char *sname)
>  {
> @@ -146,3 +147,4 @@ rte_bpf_elf_load(const struct rte_bpf_prm *prm, const char *fname,
>  	rte_errno = ENOTSUP;
>  	return NULL;
>  }
> +#endif
> diff --git a/lib/librte_bpf/meson.build b/lib/librte_bpf/meson.build
> index 8a79878ff..11c1fb558 100644
> --- a/lib/librte_bpf/meson.build
> +++ b/lib/librte_bpf/meson.build
> @@ -20,6 +20,7 @@ deps += ['mbuf', 'net', 'ethdev']
> 
>  dep = dependency('libelf', required: false)
>  if dep.found()
> +	dpdk_conf.set('RTE_LIBRTE_BPF_ELF', 1)
>  	sources += files('bpf_load_elf.c')
>  	ext_deps += dep
>  endif
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Tested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.20.1