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

Message ID 20190410134517.63896-3-bruce.richardson@intel.com
State New
Delegated to: Thomas Monjalon
Headers show
Series
  • remove use of weak functions from libraries
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Bruce Richardson April 10, 2019, 1:45 p.m.
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(-)

Comments

Aaron Conole April 10, 2019, 2:07 p.m. | #1
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
Bruce Richardson April 10, 2019, 2:27 p.m. | #2
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
Ananyev, Konstantin April 10, 2019, 2:57 p.m. | #3
> -----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

Patch

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