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

[Jan Kiszka <jan.kiszka@domain.hid>]

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?

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

> 
> 
>>> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux