Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Make xnarch_init_timeconv an uninlined weak function

[Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org>]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>>>> GIT version control wrote:
>>>>>>>>>>>>>>> Module: xenomai-jki
>>>>>>>>>>>>>>> Branch: for-upstream
>>>>>>>>>>>>>>> Commit: 6b40653e9c3c4a2433bb4e91344fc378eb860f75
>>>>>>>>>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=6b40653e9c3c4a2433bb4e91344fc378eb860f75
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>>>>>>>> Date:   Wed Feb 10 13:24:29 2010 +0100
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Make xnarch_init_timeconv an uninlined weak function
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Otherwise the wrong set of time conversion variables might get
>>>>>>>>>>>>>>> initialized when using > 1 skin libraries.
>>>>>>>>>>>>>> If that would be possible, then it is the conversion variables which
>>>>>>>>>>>>>> should made be weak, not the function.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The way I see it, the posix and native skins currently get a different
>>>>>>>>>>>>>> set of variables and functions, which works, but with your change, since
>>>>>>>>>>>>>> there is only one function, only one set of variable gets initialized by
>>>>>>>>>>>>>> the two function calls. And one skin just broke.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Or am I missing something? Does the patch fix a problem you really had?
>>>>>>>>>>>>> Frankly, I wasn't able to test in the field yet as replacing the libs
>>>>>>>>>>>>> there is non-trivial. But I was able to observe that only one set of
>>>>>>>>>>>>> functions is used - which is logical considering the weak marks. And
>>>>>>>>>>>>> this breaks due to the static inline initialization.
>>>>>>>>>>>>>
>>>>>>>>>>>>> However, let's mark both functions and variables weak to fix the issue
>>>>>>>>>>>>> and avoid leaving unused variables around. Will update my patch in a minute.
>>>>>>>>>>>> Ok. I am reverting this patch until you provide me with another
>>>>>>>>>>>> solution. It causes latency to segfault purely and simply at startup on
>>>>>>>>>>>> my dual PIII.
>>>>>>>>>>>>
>>>>>>>>>>> Cannot reproduce yet. Do you have a backtrace?
>>>>>>>>>> No. But the problem is probably the same as the one signaled by Henri,
>>>>>>>>>> a misplaced weak directive ending up in a symbol with no address at all.
>>>>>>>>>> Since the current situation works, I am going to wait for the "clean"
>>>>>>>>>> fix which puts some code/data in the src/skins/common directory.
>>>>>>>>>>
>>>>>>>>> Find it in my tree. But it's not yet well tested.
>>>>>>>> I do not like it either. Functions which are in src/skins/common should
>>>>>>>> still be weak, since this lib is included in all the skins libraries.
>>>>>>> Those functions are now in libxeno_common only, so I see no point in
>>>>>>> allowing them to be overloaded.
>>>>>> Yes, but libxeno_common is included in libpthread_rt.so and
>>>>>> libnative.so. So, if you link with both libraries, you get
>>>>>> libxeno_common twice.
>>>>>>
>>>>> Do we link libxeno_common statically? Otherwise, this conflict is not
>>>>> logical to me. Also, there are other symbols in bind.c that are non-weak.
>>>> libxeno_common is a "convenience library", which means that when
>>>> libpthread_rt.so is assembled, the object files from libxeno_common are
>>>> included.
>>> BTW, what speaks against making it a dynamic library?
>> Complications. Currently the way of linking a native application is to do:
>>
>> `xeno-config --ldflags` -lnative
>>
>> Since when linking, the order matter, we can not hide this new library
>> in xeno-config --ldflags, so we would have to turn this into:
>>
>> `xeno-config --ldflags` -lnative -lxeno-common
>>
>> i.e. breaking user way of building applications. I do not think the gain
>> is worth the trouble: linking against only one xenomai library looks to
>> me like the most common case.
> 
> What would break when swapping -lnative and -xeno-common arbitrarily?

If you are linking with static libraries (which is the case that
matters), the linker would skip all objects from xeno-common completely
since none of their symbols are required, then fail after libnative
because of missing symbols (defined in libxeno_common).

> 
> There is a _lot_ of cleanup potential. E.g. all that redundant __real
> wrappers are good candidates for libxeno-common.

Thanks to the wrap-link.sh script, the __real wrappers are no longer
required. Besides, they are only needed for the posix skin, so there is
no need to put them in libxeno-common. This is something which needs to
be fixed too.

> 
>>
>>>> I guess the loader eliminates the duplicates.
>>>>
>>> It has to. For the same reason, we should be able to clean up
>>> skins/common/current.c now.
>> I am not that confident about that change. By using the weak directive,
>> we make sure that the same __thread variable is used for instance. If we
>> do not do that, what prevents the code from using the local symbols in
>> each library ? IOW, removing the duplicate could work for libxeno_common
>> symbols when invoked externally (which probably almost never happens),
>> but not inside the skin libraries. I do not know about that, which is
>> why I kept the weak declarations.
> 
> Mixing weak functions with non-weak variables can cause troubles as
> well. Sticking our head into the sand is no good approach, understanding
> the requirements for weak or not is better - and eliminating weak would
> be best.

Yes, all non static variables must be made weak. But this should already
be the case in libxeno_common.

Using the weak attribute is the best approach. Having symbols multiply
defined is a bad idea and not "standard compliant". The problem is not
to understand what happens with one instance of a toolchain, the problem
is to know whether the behaviour is the same on all the toolchains of
all the seven architectures supported by Xenomai. The --wrap directive
proved that not all versions of all toolchains on all platforms are equal.

The only alternative I would accept is a guaranteed portable ld
directive that would make all the symbols of the library automagically weak.


-- 
					    Gilles.