[git commit master] nptl: mark symbols with libc forwarder hidden

[Timo Teräs]

On 09/20/2010 01:46 PM, Carmelo AMOROSO wrote:
> On 4/23/2010 4:31 PM, Timo Teräs wrote:
>> diff --git a/libpthread/nptl/init.c b/libpthread/nptl/init.c
>> index b651a3e..87c08fa 100644
>> --- a/libpthread/nptl/init.c
>> +++ b/libpthread/nptl/init.c
>> @@ -111,8 +111,8 @@ static const struct pthread_functions pthread_functions =
>>      .ptr___pthread_key_create = __pthread_key_create_internal,
>>      .ptr___pthread_getspecific = __pthread_getspecific_internal,
>>      .ptr___pthread_setspecific = __pthread_setspecific_internal,
>> -    .ptr__pthread_cleanup_push_defer = __pthread_cleanup_push_defer,
>> -    .ptr__pthread_cleanup_pop_restore = __pthread_cleanup_pop_restore,
>> +    .ptr__pthread_cleanup_push_defer = _pthread_cleanup_push_defer,
>> +    .ptr__pthread_cleanup_pop_restore = _pthread_cleanup_pop_restore,
>>      .ptr_nthreads = &__nptl_nthreads,
>>      .ptr___pthread_unwind = &__pthread_unwind,
>>      .ptr__nptl_deallocate_tsd = __nptl_deallocate_tsd,
> 
> sorry for coming to this back with a so long delay, but we have recently
> updated our uClibc branch against master, and we have found some
> problems to boot with Upstart due to this change.
> 
> I've seen that a following commit has reverted most of the changes done
> by this patch, but the init.c has not been reverted.
> 
> Indeed, it's not so clear to me the reason for the change above; could
> you explain better ?

IRC the change idea is to prohibit any library (preload, or linked
before pthread/libc) to override the _pthread_cleanup_* functions. I
believe this is the intended behavior, because if we eventually get to
libc pthread call we have already traversed through all the hooks and
should dispatch the call to the actual real implementation in
libpthread. This is also the way glibc version works.

> Anyway I can say that with this change in place, Upstart cannot work
> properly: we got a segfault and I guess it's due to a stack overflow.
> 
> Changing it back to the original version, allowed us to boot successfully.
> 
> What is your opinion ?

The commit was reverted due to some corner cases how libc and libpthread
use each other's symbols. The revert again made the original test case
fail. This was later fixed in using protected symbols in commit:
 a501a33e9761f32b3d38ab9f113892abe7cef3f1

Which hardware you are using? The patch fails to run if ld.so does not
support protected symbols properly. See commits:
 ba38f0cec27b91cc7c605417ad047c4dc77d732f (x86)
 48fb264beaac8114e5ac3e80e70dda473fbce96d (arm)

Preferably implement protected symbol on your architecture. Reverting
the init.c change will likely make my original test case fail again
which is one of:
 1) linking -lc -lpthread (libc before libpthread)
 2) linking without -lpthread, but later dlopen a library linking
against -lpthread

Hopefully this explains it a bit. I do remember that getting all the
corner cases right was extremely tricky, and I tested it later only on
x86. But most likely it's the protected symbol thingy that's missing for
you.

 - Timo