Re: weak functions in some drivers

[Zoltan Kiss <zoltan.kiss@linaro.org>]

Hi,

On 01/07/16 11:19, Sergio Gonzalez Monroy wrote:
> On 01/07/2016 11:16, Elo, Matias (Nokia - FI/Espoo) wrote:
>>> -----Original Message-----
>>> From: Sergio Gonzalez Monroy [mailto:sergio.gonzalez.monroy@intel.com]
>>> Sent: Friday, July 01, 2016 1:05 PM
>>> To: Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia-bell-labs.com>;
>>> dev@dpdk.org
>>> Cc: ferruh.yigit@intel.com; damarion@cisco.com
>>> Subject: Re: [dpdk-dev] weak functions in some drivers
>>>
>>> On 01/07/2016 10:42, Elo, Matias (Nokia - FI/Espoo) wrote:
>>>>>>>>> What is not clear to me is motivation to use weak here instead
>>>>>>>>> of simply
>>>>>> using >CONFIG_RTE_I40E_INC_VECTOR
>>>>>>>>> macro to exclude stubs in i40e_rxtx.c. It will make library
>>>>>>>>> smaller and
>>> avoid
>>>>>> issues like this one
>>>>>>>>> which are quite hard to troubleshoot.
>>>>>>>> Since this issue seen in fd.io, I didn't investigated more, but
>>>>>>>> I don't
>>>>>>>> want to clock your valid question, this is an attempt to
>>>>>>>> resurrect the
>>>>>>>> question ...
>>>>>>> Hi,
>>>>>>>
>>>>>>> We are having exactly the same problem. For us the aforementioned
>>>>>> workaround doesn't seem to work and vector mode is always disabled
>>>>>> with
>>> the
>>>>>> i40e drivers. If I modify i40e_rxtx.c and exclude the stub
>>>>>> functions using
>>>>>> CONFIG_RTE_I40E_INC_VECTOR everything works as expected.
>>>>>>> We are building DPDK with the CONFIG_RTE_BUILD_COMBINE_LIBS option
>>>>>> enabled and link DPDK library to our application.
>>>>>>> Any other ideas how this could be fixed?
>>>>>>>
>>>>>>> Regards,
>>>>>>> Matias
>>>>>>>
>>>>>> So you have tried to link a combined static lib with --whole-archive
>>>>>> -ldpdk --no-whole-archive and still get the wrong/weak function
>>>>>> definition?
>>>>>>
>>>>>> Sergio
>>>>> I actually just managed to fix the problem. In our case I had to add
>>>>> '-Wl,--whole-archive,-ldpdk,--no-whole-archive'  to the end of
>>>>> AM_LDFLAGS.
>>>>>
>>>> It turned out that the problem actually wasn't fixed.
>>>>
>>>> DPDK is built with CONFIG_RTE_BUILD_COMBINE_LIBS=y and
>>> EXTRA_CFLAGS="-fPIC"
>>>> What we are linking originally:
>>>>     -l:libdpdk.a
>>>>
>>>> This works otherwise but vector mode i40e is not enabled.
>>>>
>>>> When trying:
>>>>     -Wl,--whole-archive,-l:libdpdk.a,--no-whole-archive
>>>>
>>>> Linking fails with ' undefined reference' errors to several dpdk
>>>> functions
>>> (rte_eal_init, rte_cpu_get_flag_enabled, rte_eth_stats_get...).
>>>> Btw. there seems to be a Stack Overflow question related to this:
>>> http://stackoverflow.com/questions/38064021/dpdk-include-libraries-in-dpdk-
>>>
>>> application-compiled-as-shared-library
>>>> -Matias
>>> What DPDK version are you using?
>> v16.04
>
> Ok. I was asking because there is no CONFIG_RTE_BUILD_COMBINE_LIBS in
> 16.04.
>
>
> Could you provide full link/compile command line? I'm not able to
> reproduce the issue so far

I've dug into this issue a bit, let me explain it with some example code:

main.c:
void bar(void);
void foo(void);

int main() {
	bar();
	//foo();
}
======================================
weak.c:
#include <stdio.h>

void __attribute__((weak)) foo(void) {
	printf("Weak\n");
}

void bar(void) {
	foo();
	printf("No call\n");
}
======================================
strong.c:
#include <stdio.h>

void foo(void) {
	printf("Strong\n");
}

Compile the second two into a library and link it with main:

gcc -o strong.o -c strong.c
ar rcs libbar.a strong.o weak.o
gcc main.c -L. -lbar -o main

Then look which foo were used:

objdump -x libbar.a | grep foo
0000000000000000 g     F .text	0000000000000010 foo
0000000000000000  w    F .text	0000000000000010 foo
0000000000000015 R_X86_64_PC32     foo-0x0000000000000004

objdump -x main | grep foo
0000000000400538 w F .text 0000000000000010 foo

So it got the weak version, simply because bar() was referred, and that 
already pulled the other symbols from that .o file, so ld stop looking 
for the another ones. If you uncomment the foo() call in main(), it will 
do a proper search, and pulls in the strong symbol.

I think there are only workarounds here:
- use --whole-archive, but that pulls in a lot of stuff you don't 
necessarily need. And if you feed that stuff to libtool, it seems to 
misbehave when it sees the -Wl parameter. The Ubuntu 14.04 libtool 
definitely has a bug around that. Matias ran into this with ODP-DPDK: it 
fixed the weak symbol problem in the .so, but then libtool forgot to add 
-ldpdk when doing statical linking
- use only dynamic linking: I guess that's not a viable restriction

The best would be to get rid of these weak functions, as they don't work 
the same with dynamic and static linking, especially the latter is 
problematic. I think Damjan suggested something like that in his 
original message.

I propose something like this:

@@ -3268,10 +3268,40 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
  }

  /* Stubs needed for linkage when CONFIG_RTE_I40E_INC_VECTOR is set to 
'n' */
-int __attribute__((weak))
+int __attribute__((cold))
  i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev __rte_unused *dev)
  {
+#ifndef RTE_LIBRTE_I40E_INC_VECTOR
         return -1;
+#else
[stuff from the vector version]
+#endif /* (RTE_I40E_INC_VECTOR == n) */
  }


And then get rid of the vector version of this function. How about that?
And of course this applies to all weak functions.

Regards,

Zoltan