[dpdk-dev,v2,6/6] mk: reduce scope of whole-archive static linking

Message ID 1465564749-1405-7-git-send-email-thomas.monjalon@6wind.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Thomas Monjalon June 10, 2016, 1:19 p.m. UTC
  From: Ferruh Yigit <ferruh.yigit@intel.com>

The --whole-archive argument is only required for plugins (drivers)
and libraries used by these plugins.
Currently it covers all libraries.
Reducing the scope of this argument slightly reduce final application size
when statically linked.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
v2: keep some basic libs in the whole-archive scope
---
 mk/rte.app.mk | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

Comments

Ferruh Yigit June 10, 2016, 3:03 p.m. UTC | #1
On 6/10/2016 2:19 PM, Thomas Monjalon wrote:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> The --whole-archive argument is only required for plugins (drivers)
> and libraries used by these plugins.
> Currently it covers all libraries.
> Reducing the scope of this argument slightly reduce final application size
> when statically linked.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---

Technically the scope of --whole-archive should be PMDs, wider scope
required because of unresolved references, but I think this is not good
workaround.

If libraries ordered as suggested before: pmd->librte->external, won't
need to extend scope of parameter

Still some applications will have problem for now, because of cyclic
dependency of librte_eal and librte_mempool, which seems targeted to fix.

So, instead of adding --whole-archive as workaround that efffects whole
binaries, I am for adding workaround for effected application.
The be effected this problem, an application should have direct calls to
librte_eal, but shouldn't have any calls to librte_mempool, I think this
rare case, that is why only a unit test application effected. And when
dependency issue removed we can fix this.

To be able to apply workaround per application, LDFLAGS should be set in
a way to commented previous patch.

Thanks,
ferruh
  
Thomas Monjalon June 10, 2016, 3:18 p.m. UTC | #2
2016-06-10 16:03, Ferruh Yigit:
> On 6/10/2016 2:19 PM, Thomas Monjalon wrote:
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > 
> > The --whole-archive argument is only required for plugins (drivers)
> > and libraries used by these plugins.
> > Currently it covers all libraries.
> > Reducing the scope of this argument slightly reduce final application size
> > when statically linked.
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > ---
> 
> Technically the scope of --whole-archive should be PMDs, wider scope
> required because of unresolved references, but I think this is not good
> workaround.
> 
> If libraries ordered as suggested before: pmd->librte->external, won't
> need to extend scope of parameter
> 
> Still some applications will have problem for now, because of cyclic
> dependency of librte_eal and librte_mempool, which seems targeted to fix.
> 
> So, instead of adding --whole-archive as workaround that efffects whole
> binaries, I am for adding workaround for effected application.

I would prefer keeping a small workaround in rte.app.mk instead of in the
application makefile. We must be careful with applications which may break
because of our changes. This is the kind of issue that they won't fix
quickly.

> The be effected this problem, an application should have direct calls to
> librte_eal, but shouldn't have any calls to librte_mempool, I think this
> rare case, that is why only a unit test application effected. And when
> dependency issue removed we can fix this.
> 
> To be able to apply workaround per application, LDFLAGS should be set in
> a way to commented previous patch.
  

Patch

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 99a7047..e9969fc 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -60,8 +60,6 @@  _LDLIBS-y += -L$(RTE_SDK_BIN)/lib
 # Order is important: from higher level to lower level
 #
 
-_LDLIBS-y += --whole-archive
-
 ifeq ($(CONFIG_RTE_EXEC_ENV_LINUXAPP),y)
 _LDLIBS-$(CONFIG_RTE_LIBRTE_KNI)            += -lrte_kni
 _LDLIBS-$(CONFIG_RTE_LIBRTE_IVSHMEM)        += -lrte_ivshmem
@@ -81,6 +79,8 @@  _LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
 _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
 _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
 
+_LDLIBS-y += --whole-archive
+
 _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
 _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
 _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
@@ -138,6 +138,8 @@  endif # CONFIG_RTE_LIBRTE_CRYPTODEV
 
 endif # !CONFIG_RTE_BUILD_SHARED_LIBS
 
+_LDLIBS-y += --no-whole-archive
+
 ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),n)
 # The static libraries do not know their dependencies.
 # So linking with static library requires explicit dependencies.
@@ -155,8 +157,6 @@  endif # !CONFIG_RTE_BUILD_SHARED_LIBS
 
 _LDLIBS-y += $(EXECENV_LDLIBS)
 
-_LDLIBS-y += --no-whole-archive
-
 LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
 
 # Eliminate duplicates without sorting