mysterious hang in pthread_create

I've got quite a strange issue with only one particular threaded program.

My system is 7-STABLE from around July 6 (rather old, I know).
I, of course, use libthr as a thread library.
I have plenty of threaded programs and all of them except one are
working perfectly (firefox, thunderbird, KDE).

The bad one is linuxdcpp (net-p2p/linuxdcpp, linuxdcpp-1.0.2).
I built it with debug enabled and also rebuilt libthr with debug.
It seems that the program hangs in the very first call to pthread_create.

Here's a stack trace of the hanging program:
#0  _thr_umtx_wait (mtx=0x838774c, id=0, timeout=0x0) at
/system/src/lib/libthr/thread/thr_umtx.c:93
#1  0x28f9168c in _thr_rtld_rlock_acquire (lock=0x8387740) at
/system/src/lib/libthr/thread/thr_rtld.c:129
#2  0x282a6a27 in dlopen () from /libexec/ld-elf.so.1
#3  0x282a491d in dladdr () from /libexec/ld-elf.so.1
#4  0x282a1469 in ?? () from /libexec/ld-elf.so.1
#5  0x289b3600 in ?? ()
#6  0x000000d8 in ?? ()
#7  0x000186d1 in ?? ()
#8  0x00000000 in ?? ()
#9  0x08303a00 in ?? ()
#10 0x00000246 in ?? ()
#11 0x289b3600 in ?? ()
#12 0x000000d8 in ?? ()
#13 0x28f94b5f in _tcb_ctor (thread=0x8303a00, initial=0) at
/system/src/lib/libthr/arch/i386/i386/pthread_md.c:46
#14 0x28f94215 in _thr_alloc (curthread=0x8302100) at
/system/src/lib/libthr/thread/thr_list.c:169
#15 0x28f8d22e in _pthread_create (thread=0x831cb90, attr=0x0,
start_routine=0x8170ce0 <Thread::starter(void*)>, arg=0x831cb8c)
     at /system/src/lib/libthr/thread/thr_create.c:68
#16 0x08170bd8 in Thread::start (this=0x831cb8c) at client/Thread.cpp:41
#17 0x080abfb4 in HashManager::startup (this=0x831cb60) at HashManager.h:97
#18 0x0809f4d6 in startup (f=0x827a2c0 <callBack(void*, std::string
const&)>, p=0x0) at client/DCPlusPlus.cpp:82
#19 0x0827a571 in main (argc=1, argv=0xbfbfe844) at linux/wulfor.cc:61

I tracked all calls to functions of _thr_rtld_*lock* family and it seems
that the lock in question gets acquired for writing before the above
access. The stack:
#0  _thr_rtld_wlock_acquire (lock=0x8387740) at
/system/src/lib/libthr/thread/thr_rtld.c:144
#1  0x282a6dcc in _rtld_thread_init () from /libexec/ld-elf.so.1
#2  0x28f91af6 in _thr_rtld_init () at
/system/src/lib/libthr/thread/thr_rtld.c:238
#3  0x28f938db in _thr_setthreaded (threaded=1) at
/system/src/lib/libthr/thread/thr_kern.c:56
#4  0x28f8d208 in _pthread_create (thread=0x831cb90, attr=0x0,
start_routine=0x8170ce0 <Thread::starter(void*)>, arg=0x831cb8c)
     at /system/src/lib/libthr/thread/thr_create.c:64
#5  0x08170bd8 in Thread::start (this=0x831cb8c) at client/Thread.cpp:41
#6  0x080abfb4 in HashManager::startup (this=0x831cb60) at HashManager.h:97
#7  0x0809f4d6 in startup (f=0x827a2c0 <callBack(void*, std::string
const&)>, p=0x0) at client/DCPlusPlus.cpp:82
#8  0x0827a571 in main (argc=1, argv=0xbfbfe844) at linux/wulfor.cc:61

It seems that for all other programs there is no such call as the above
( I mean wlock_acquire).
I didn't have debug symbols in rtld when I executed this test,
unfortunately.

The problem is 100% reproducible, so I can get any additional debugging
info.
I wonder what can be so special about this program, what can be going
wrong. I didn't quite get the logic with flags and masks in
_rtld_thread_init (especially when lockinfo is still default, but the
issue seems to be related to that.

--
Andriy Gapon
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

on 28/08/2008 23:29 Andriy Gapon said the following:

> I tracked all calls to functions of _thr_rtld_*lock* family and it seems
> that the lock in question gets acquired for writing before the above
> access. The stack:
> #0  _thr_rtld_wlock_acquire (lock=0x8387740) at
> /system/src/lib/libthr/thread/thr_rtld.c:144
> #1  0x282a6dcc in _rtld_thread_init () from /libexec/ld-elf.so.1
> #2  0x28f91af6 in _thr_rtld_init () at
> /system/src/lib/libthr/thread/thr_rtld.c:238
> #3  0x28f938db in _thr_setthreaded (threaded=1) at
> /system/src/lib/libthr/thread/thr_kern.c:56
> #4  0x28f8d208 in _pthread_create (thread=0x831cb90, attr=0x0,
> start_routine=0x8170ce0 <Thread::starter(void*)>, arg=0x831cb8c)
>     at /system/src/lib/libthr/thread/thr_create.c:64
> #5  0x08170bd8 in Thread::start (this=0x831cb8c) at client/Thread.cpp:41
> #6  0x080abfb4 in HashManager::startup (this=0x831cb60) at HashManager.h:97
> #7  0x0809f4d6 in startup (f=0x827a2c0 <callBack(void*, std::string
> const&)>, p=0x0) at client/DCPlusPlus.cpp:82
> #8  0x0827a571 in main (argc=1, argv=0xbfbfe844) at linux/wulfor.cc:61

Quick followup: I rebuilt ld-elf.so with debug symbols and it seems that
at the following place in rtld_lock.c 'flags' variable gets assigned a
value of five (5):
(gdb) fr 2
#2  0x282a86bf in _rtld_thread_init (pli=0xbfbfe66c) at
/system/src/libexec/rtld-elf/rtld_lock.c:275
275             flags = thread_mask_set(~0);
(gdb) list
270     {
271             int flags, i;
272             void *locks[RTLD_LOCK_CNT];
273
274             /* disable all locking while this function is running */
275             flags = thread_mask_set(~0);
276
277             if (pli == NULL)
278                     pli = &deflockinfo;
279
...
(gdb) p flags
$10 = 5

Wait, I think I just found something:
(gdb) b rtld_lock.c:143 if mask != 1
Breakpoint 8 at 0x282a8311: file
/system/src/libexec/rtld-elf/rtld_lock.c, line 143.
(gdb) c
...
(gdb) bt
#0  def_thread_set_flag (mask=4) at
/system/src/libexec/rtld-elf/rtld_lock.c:143
#1  0x282a83e0 in thread_mask_set (mask=4) at
/system/src/libexec/rtld-elf/rtld_lock.c:165
#2  0x282a8410 in wlock_acquire (lock=0x282cddb4) at
/system/src/libexec/rtld-elf/rtld_lock.c:198
#3  0x282a58b2 in dl_iterate_phdr (callback=0x28f84fc0
<__fixunssfdi+4352>, param=0xbfbfe200) at
/system/src/libexec/rtld-elf/rtld.c:2103
#4  0x28f8586f in _Unwind_Find_FDE () from /lib/libgcc_s.so.1
#5  0x28f8267c in _Unwind_GetIPInfo () from /lib/libgcc_s.so.1
#6  0x28f833be in _Unwind_GetIPInfo () from /lib/libgcc_s.so.1
#7  0x28f838c0 in _Unwind_RaiseException () from /lib/libgcc_s.so.1
#8  0x28ee045d in __cxa_throw () from /usr/lib/libstdc++.so.6
#9  0x080d60e2 in File (this=0xbfbfe710, aFileName=@0xbfbfe71c,
access=1, mode=1) at client/File.cpp:227
#10 0x08192d6f in Util::initialize () at client/Util.cpp:102
#11 0x0809f3cc in startup (f=0x827a2c0 <callBack(void*, std::string
const&)>, p=0x0) at client/DCPlusPlus.cpp:51
#12 0x0827a571 in main (argc=1, argv=0xbfbfe844) at linux/wulfor.cc:61


So can all this be a result of an exception thrown before threads are
initialized?
Is this something that might already be fixed in HEAD/trunk or in
RELENG_7? (I seem to vaguely remember something related).

--
Andriy Gapon
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

on 28/08/2008 23:52 Andriy Gapon said the following:
> So can all this be a result of an exception thrown before threads are
> initialized?

This seems to be it.
I can reproduce the issue with the following small C++ program:

/*********************************************/
#include <pthread.h>


static void * thrfunc(void * arg)
{
     return NULL;
}

int main(void)
{
     pthread_t thr;

     try {
         throw int(1);
     }
     catch (...) {}

     pthread_create(&thr, NULL, thrfunc, NULL);

     return 0;
}
/*********************************************/


$ uname -a
... FreeBSD 7.0-STABLE #9: Sun Jul  6 17:13:22 EEST 2008 ... i386

--
Andriy Gapon
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[John Hein]

Andriy Gapon wrote at 00:41 +0300 on Aug 29, 2008:
 > This seems to be it.
 > I can reproduce the issue with the following small C++ program:
 >
 > /*********************************************/
 > #include <pthread.h>
 >
 >
 > static void * thrfunc(void * arg)
 > {
 >      return NULL;
 > }
 >
 > int main(void)
 > {
 >      pthread_t thr;
 >
 >      try {
 >          throw int(1);
 >      }
 >      catch (...) {}
 >
 >      pthread_create(&thr, NULL, thrfunc, NULL);
 >
 >      return 0;
 > }
 > /*********************************************/
 >
 >
 > $ uname -a
 > ... FreeBSD 7.0-STABLE #9: Sun Jul  6 17:13:22 EEST 2008 ... i386

FYI...
I reproduced it on an older 7-stable (May 07), too (stuck in uwait).
But it works fine on 6-stable with libthr or libpthread.
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[Kostik Belousov]

On Fri, Aug 29, 2008 at 12:41:58AM +0300, Andriy Gapon wrote:

> on 28/08/2008 23:52 Andriy Gapon said the following:
> >So can all this be a result of an exception thrown before threads are
> >initialized?
>
> This seems to be it.
> I can reproduce the issue with the following small C++ program:
>
> /*********************************************/
> #include <pthread.h>
>
>
> static void * thrfunc(void * arg)
> {
>     return NULL;
> }
>
> int main(void)
> {
>     pthread_t thr;
>
>     try {
>         throw int(1);
>     }
>     catch (...) {}
>
>     pthread_create(&thr, NULL, thrfunc, NULL);
>
>     return 0;
> }
> /*********************************************/
>
>
> $ uname -a
> ... FreeBSD 7.0-STABLE #9: Sun Jul  6 17:13:22 EEST 2008 ... i386
>
> --
> Andriy Gapon

I am wondering why did you not fixed it youself with all this information.
Anyway, patch below seems to work for me. David may have an opinion on
the change.

diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
index f96bba9..785d610 100644
--- a/lib/libthr/thread/thr_init.c
+++ b/lib/libthr/thread/thr_init.c
@@ -355,6 +355,9 @@ _libpthread_init(struct pthread *curthread)
  if (_thread_event_mask & TD_CREATE)
  _thr_report_creation(curthread, curthread);
  }
+
+ if (_thr_isthreaded() == 0)
+ _thr_setthreaded(1);
 }
 
 /*

Kostik, thanks!

on 29/08/2008 17:10 Kostik Belousov said the following:
> I am wondering why did you not fixed it youself with all this information.

I am wondering that myself now :-)
I got bogged in rtld details and simply didn't think about the solution
of doing setthreaded earlier.

I will try your patch a couple of hours later.
BTW, a forward question - should this patch help in the case of an
exception thrown (and caught) before main(), i.e. in constructors of
static/global objects?

> Anyway, patch below seems to work for me. David may have an opinion on
> the change.
>
> diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
> index f96bba9..785d610 100644
> --- a/lib/libthr/thread/thr_init.c
> +++ b/lib/libthr/thread/thr_init.c
> @@ -355,6 +355,9 @@ _libpthread_init(struct pthread *curthread)
>   if (_thread_event_mask & TD_CREATE)
>   _thr_report_creation(curthread, curthread);
>   }
> +
> + if (_thr_isthreaded() == 0)
> + _thr_setthreaded(1);
>  }
>  
>  /*

--
Andriy Gapon
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[Kostik Belousov]

On Fri, Aug 29, 2008 at 05:18:18PM +0300, Andriy Gapon wrote:

>
> Kostik, thanks!
>
> on 29/08/2008 17:10 Kostik Belousov said the following:
> > I am wondering why did you not fixed it youself with all this information.
>
> I am wondering that myself now :-)
> I got bogged in rtld details and simply didn't think about the solution
> of doing setthreaded earlier.
>
> I will try your patch a couple of hours later.
> BTW, a forward question - should this patch help in the case of an
> exception thrown (and caught) before main(), i.e. in constructors of
> static/global objects?

If the objects are from the executable, then yes. I do not know about

present situation, but some time ago g++ and several other compilers
called ctr for global objects from the main() function. Regardeless
of this, init for main executable shall be called after init for
dependencies is finished. See initlist_add_objects().

On the other hand, ctr calls from linked dso may get fixed in regard
of exception throwing, or may not, depending on the relative order of
the dso loading against libthr.

>
> > Anyway, patch below seems to work for me. David may have an opinion on
> > the change.
> >
> > diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
> > index f96bba9..785d610 100644
> > --- a/lib/libthr/thread/thr_init.c
> > +++ b/lib/libthr/thread/thr_init.c
> > @@ -355,6 +355,9 @@ _libpthread_init(struct pthread *curthread)
> >   if (_thread_event_mask & TD_CREATE)
> >   _thr_report_creation(curthread, curthread);
> >   }
> > +
> > + if (_thr_isthreaded() == 0)
> > + _thr_setthreaded(1);
> >  }
> >  
> >  /*
>
>
> --
> Andriy Gapon

[Daniel Eischen]

On Fri, 29 Aug 2008, Kostik Belousov wrote:

> On Fri, Aug 29, 2008 at 05:18:18PM +0300, Andriy Gapon wrote:
>>
>> Kostik, thanks!
>>
>> on 29/08/2008 17:10 Kostik Belousov said the following:
>>> I am wondering why did you not fixed it youself with all this information.
>>
>> I am wondering that myself now :-)
>> I got bogged in rtld details and simply didn't think about the solution
>> of doing setthreaded earlier.
>>
>> I will try your patch a couple of hours later.
>> BTW, a forward question - should this patch help in the case of an
>> exception thrown (and caught) before main(), i.e. in constructors of
>> static/global objects?
> If the objects are from the executable, then yes. I do not know about
> present situation, but some time ago g++ and several other compilers
> called ctr for global objects from the main() function. Regardeless
> of this, init for main executable shall be called after init for
> dependencies is finished. See initlist_add_objects().
>
> On the other hand, ctr calls from linked dso may get fixed in regard
> of exception throwing, or may not, depending on the relative order of
> the dso loading against libthr.
>
>>
>>> Anyway, patch below seems to work for me. David may have an opinion on
>>> the change.
>>>
>>> diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
>>> index f96bba9..785d610 100644
>>> --- a/lib/libthr/thread/thr_init.c
>>> +++ b/lib/libthr/thread/thr_init.c
>>> @@ -355,6 +355,9 @@ _libpthread_init(struct pthread *curthread)
>>>   if (_thread_event_mask & TD_CREATE)
>>>   _thr_report_creation(curthread, curthread);
>>>   }
>>> +
>>> + if (_thr_isthreaded() == 0)
>>> + _thr_setthreaded(1);
>>>  }
>>>
>>>  /*

I think the intent of __isthreaded (and _thr_setthreaded()) was
to be set if there were more than 1 thread, not to indicate that
the thread library has been initialized.

--
DE
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[Kostik Belousov]

On Fri, Aug 29, 2008 at 12:25:09PM -0400, Daniel Eischen wrote:

> On Fri, 29 Aug 2008, Kostik Belousov wrote:
>
> >On Fri, Aug 29, 2008 at 05:18:18PM +0300, Andriy Gapon wrote:
> >>
> >>Kostik, thanks!
> >>
> >>on 29/08/2008 17:10 Kostik Belousov said the following:
> >>>I am wondering why did you not fixed it youself with all this
> >>>information.
> >>
> >>I am wondering that myself now :-)
> >>I got bogged in rtld details and simply didn't think about the solution
> >>of doing setthreaded earlier.
> >>
> >>I will try your patch a couple of hours later.
> >>BTW, a forward question - should this patch help in the case of an
> >>exception thrown (and caught) before main(), i.e. in constructors of
> >>static/global objects?
> >If the objects are from the executable, then yes. I do not know about
> >present situation, but some time ago g++ and several other compilers
> >called ctr for global objects from the main() function. Regardeless
> >of this, init for main executable shall be called after init for
> >dependencies is finished. See initlist_add_objects().
> >
> >On the other hand, ctr calls from linked dso may get fixed in regard
> >of exception throwing, or may not, depending on the relative order of
> >the dso loading against libthr.
> >
> >>
> >>>Anyway, patch below seems to work for me. David may have an opinion on
> >>>the change.
> >>>
> >>>diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
> >>>index f96bba9..785d610 100644
> >>>--- a/lib/libthr/thread/thr_init.c
> >>>+++ b/lib/libthr/thread/thr_init.c
> >>>@@ -355,6 +355,9 @@ _libpthread_init(struct pthread *curthread)
> >>> if (_thread_event_mask & TD_CREATE)
> >>> _thr_report_creation(curthread, curthread);
> >>> }
> >>>+
> >>>+ if (_thr_isthreaded() == 0)
> >>>+ _thr_setthreaded(1);
> >>> }
> >>>
> >>> /*
>
> I think the intent of __isthreaded (and _thr_setthreaded()) was
> to be set if there were more than 1 thread, not to indicate that
> the thread library has been initialized.

As demonstrated by Andriy' example, we need _thr_rtld_init() be called
before any rtld locks are given chance to be acquired. _thr_rtld_init()
shall be protected from repeated invocation, and _thr_setthreaded()
implements exactly this.

If calling _thr_setthreaded(1) has not quite right intent, could you,
please, suggest satisfying solution ?

[Daniel Eischen]

On Fri, 29 Aug 2008, Kostik Belousov wrote:

> On Fri, Aug 29, 2008 at 12:25:09PM -0400, Daniel Eischen wrote:
>> On Fri, 29 Aug 2008, Kostik Belousov wrote:
>>
>>> On Fri, Aug 29, 2008 at 05:18:18PM +0300, Andriy Gapon wrote:
>>>>
>>>> Kostik, thanks!
>>>>
>>>> on 29/08/2008 17:10 Kostik Belousov said the following:
>>>>> I am wondering why did you not fixed it youself with all this
>>>>> information.
>>>>
>>>> I am wondering that myself now :-)
>>>> I got bogged in rtld details and simply didn't think about the solution
>>>> of doing setthreaded earlier.
>>>>
>>>> I will try your patch a couple of hours later.
>>>> BTW, a forward question - should this patch help in the case of an
>>>> exception thrown (and caught) before main(), i.e. in constructors of
>>>> static/global objects?
>>> If the objects are from the executable, then yes. I do not know about
>>> present situation, but some time ago g++ and several other compilers
>>> called ctr for global objects from the main() function. Regardeless
>>> of this, init for main executable shall be called after init for
>>> dependencies is finished. See initlist_add_objects().
>>>
>>> On the other hand, ctr calls from linked dso may get fixed in regard
>>> of exception throwing, or may not, depending on the relative order of
>>> the dso loading against libthr.
>>>
>>>>
>>>>> Anyway, patch below seems to work for me. David may have an opinion on
>>>>> the change.
>>>>>
>>>>> diff --git a/lib/libthr/thread/thr_init.c b/lib/libthr/thread/thr_init.c
>>>>> index f96bba9..785d610 100644
>>>>> --- a/lib/libthr/thread/thr_init.c
>>>>> +++ b/lib/libthr/thread/thr_init.c
>>>>> @@ -355,6 +355,9 @@ _libpthread_init(struct pthread *curthread)
>>>>> if (_thread_event_mask & TD_CREATE)
>>>>> _thr_report_creation(curthread, curthread);
>>>>> }
>>>>> +
>>>>> + if (_thr_isthreaded() == 0)
>>>>> + _thr_setthreaded(1);
>>>>> }
>>>>>
>>>>> /*
>>
>> I think the intent of __isthreaded (and _thr_setthreaded()) was
>> to be set if there were more than 1 thread, not to indicate that
>> the thread library has been initialized.
>
> As demonstrated by Andriy' example, we need _thr_rtld_init() be called
> before any rtld locks are given chance to be acquired. _thr_rtld_init()
> shall be protected from repeated invocation, and _thr_setthreaded()
> implements exactly this.
>
> If calling _thr_setthreaded(1) has not quite right intent, could you,
> please, suggest satisfying solution ?

I'm not sure I _quite_ understand the problem, but why
wouldn't you have the same potential problem with some
other library (without libthread)?  I'll have to go back
and read the beginning of the thread - I just kinda came
in at the end.

--
DE
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[Kostik Belousov]

On Sat, Aug 30, 2008 at 11:32:35AM -0400, Daniel Eischen wrote:

> On Fri, 29 Aug 2008, Kostik Belousov wrote:
>
> >On Fri, Aug 29, 2008 at 12:25:09PM -0400, Daniel Eischen wrote:
> >>On Fri, 29 Aug 2008, Kostik Belousov wrote:
> >>
> >>>On Fri, Aug 29, 2008 at 05:18:18PM +0300, Andriy Gapon wrote:
> >>>>
> >>>>Kostik, thanks!
> >>>>
> >>>>on 29/08/2008 17:10 Kostik Belousov said the following:
> >>>>>I am wondering why did you not fixed it youself with all this
> >>>>>information.
> >>>>
> >>>>I am wondering that myself now :-)
> >>>>I got bogged in rtld details and simply didn't think about the solution
> >>>>of doing setthreaded earlier.
> >>>>
> >>>>I will try your patch a couple of hours later.
> >>>>BTW, a forward question - should this patch help in the case of an
> >>>>exception thrown (and caught) before main(), i.e. in constructors of
> >>>>static/global objects?
> >>>If the objects are from the executable, then yes. I do not know about
> >>>present situation, but some time ago g++ and several other compilers
> >>>called ctr for global objects from the main() function. Regardeless
> >>>of this, init for main executable shall be called after init for
> >>>dependencies is finished. See initlist_add_objects().
> >>>
> >>>On the other hand, ctr calls from linked dso may get fixed in regard
> >>>of exception throwing, or may not, depending on the relative order of
> >>>the dso loading against libthr.
> >>>
> >>>>
> >>>>>Anyway, patch below seems to work for me. David may have an opinion on
> >>>>>the change.
> >>>>>
> >>>>>diff --git a/lib/libthr/thread/thr_init.c
> >>>>>b/lib/libthr/thread/thr_init.c
> >>>>>index f96bba9..785d610 100644
> >>>>>--- a/lib/libthr/thread/thr_init.c
> >>>>>+++ b/lib/libthr/thread/thr_init.c
> >>>>>@@ -355,6 +355,9 @@ _libpthread_init(struct pthread *curthread)
> >>>>> if (_thread_event_mask & TD_CREATE)
> >>>>> _thr_report_creation(curthread, curthread);
> >>>>> }
> >>>>>+
> >>>>>+ if (_thr_isthreaded() == 0)
> >>>>>+ _thr_setthreaded(1);
> >>>>>}
> >>>>>
> >>>>>/*
> >>
> >>I think the intent of __isthreaded (and _thr_setthreaded()) was
> >>to be set if there were more than 1 thread, not to indicate that
> >>the thread library has been initialized.
> >
> >As demonstrated by Andriy' example, we need _thr_rtld_init() be called
> >before any rtld locks are given chance to be acquired. _thr_rtld_init()
> >shall be protected from repeated invocation, and _thr_setthreaded()
> >implements exactly this.
> >
> >If calling _thr_setthreaded(1) has not quite right intent, could you,
> >please, suggest satisfying solution ?
>
> I'm not sure I _quite_ understand the problem, but why
> wouldn't you have the same potential problem with some
> other library (without libthread)?  I'll have to go back
> and read the beginning of the thread - I just kinda came
> in at the end.

Sure, for appropriate value of any. If you mean whether the same problem
would arise for any threading library that supplies locking implementation
for rtld, then certainly yes. I looked over and patched only libthr
since this is the only survived library for now.

Anyway, I do not insist on the proposed solution, and definitely
prefer the change that is well aligned with libthr architecture.

[Daniel Eischen]

On Sat, 30 Aug 2008, Kostik Belousov wrote:

> On Sat, Aug 30, 2008 at 11:32:35AM -0400, Daniel Eischen wrote:
>> On Fri, 29 Aug 2008, Kostik Belousov wrote:
>>>
>>> As demonstrated by Andriy' example, we need _thr_rtld_init() be called
>>> before any rtld locks are given chance to be acquired. _thr_rtld_init()
>>> shall be protected from repeated invocation, and _thr_setthreaded()
>>> implements exactly this.
>>>
>>> If calling _thr_setthreaded(1) has not quite right intent, could you,
>>> please, suggest satisfying solution ?
>>
>> I'm not sure I _quite_ understand the problem, but why
>> wouldn't you have the same potential problem with some
>> other library (without libthread)?  I'll have to go back
>> and read the beginning of the thread - I just kinda came
>> in at the end.
>
> Sure, for appropriate value of any. If you mean whether the same problem
> would arise for any threading library that supplies locking implementation
> for rtld, then certainly yes. I looked over and patched only libthr
> since this is the only survived library for now.

What I mean is, is fixing libthr a solution that will work
for cases?  Or, is libthr doing something wrong?  I can't
really see that it is.

libthr assumes that everything is single-threaded (or
serialized, I guess) before a thread is created.  I
am looking at this thread:

   http://docs.freebsd.org/cgi/getmsg.cgi?fetch=5235+0+current/freebsd-threads

Where is the corresponding unlock for the wlock_acquire()?
I guess this is the problem.  When would this normally
be released (without libthr being linked in)?

Also, the __isthreaded flag is used in libc to avoid taking
locks unless necessary.  So if you have a single threaded
application that is also linked with libthr, you don't
pay the penalty of locking overhead.  Lots of 3rd-party
libraries link with a threads library, so an application
may not even know it is "threaded".

>
> Anyway, I do not insist on the proposed solution, and definitely
> prefer the change that is well aligned with libthr architecture.

I'm not arguing anything, I just don't know that the problem
lies within lib<insert thread library here>.  Of course, the
rtld init stuff could be pulled out and done in thread
initialization instead of thr_setthreaded().  That doesn't
leave much in thr_setthreaded, and it also adds locking
overhead into rtld for single-threaded programs that are
linked with libthr...

--
DE
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[Kostik Belousov]

On Sat, Aug 30, 2008 at 12:15:31PM -0400, Daniel Eischen wrote:

> On Sat, 30 Aug 2008, Kostik Belousov wrote:
>
> >On Sat, Aug 30, 2008 at 11:32:35AM -0400, Daniel Eischen wrote:
> >>On Fri, 29 Aug 2008, Kostik Belousov wrote:
> >>>
> >>>As demonstrated by Andriy' example, we need _thr_rtld_init() be called
> >>>before any rtld locks are given chance to be acquired. _thr_rtld_init()
> >>>shall be protected from repeated invocation, and _thr_setthreaded()
> >>>implements exactly this.
> >>>
> >>>If calling _thr_setthreaded(1) has not quite right intent, could you,
> >>>please, suggest satisfying solution ?
> >>
> >>I'm not sure I _quite_ understand the problem, but why
> >>wouldn't you have the same potential problem with some
> >>other library (without libthread)?  I'll have to go back
> >>and read the beginning of the thread - I just kinda came
> >>in at the end.
> >
> >Sure, for appropriate value of any. If you mean whether the same problem
> >would arise for any threading library that supplies locking implementation
> >for rtld, then certainly yes. I looked over and patched only libthr
> >since this is the only survived library for now.
>
> What I mean is, is fixing libthr a solution that will work
> for cases?  Or, is libthr doing something wrong?  I can't
> really see that it is.
>
> libthr assumes that everything is single-threaded (or
> serialized, I guess) before a thread is created.  I
> am looking at this thread:
>
>   http://docs.freebsd.org/cgi/getmsg.cgi?fetch=5235+0+current/freebsd-threads
>
> Where is the corresponding unlock for the wlock_acquire()?
> I guess this is the problem.  When would this normally
> be released (without libthr being linked in)?
>
> Also, the __isthreaded flag is used in libc to avoid taking
> locks unless necessary.  So if you have a single threaded
> application that is also linked with libthr, you don't
> pay the penalty of locking overhead.  Lots of 3rd-party
> libraries link with a threads library, so an application
> may not even know it is "threaded".
>
> >
> >Anyway, I do not insist on the proposed solution, and definitely
> >prefer the change that is well aligned with libthr architecture.
>
> I'm not arguing anything, I just don't know that the problem
> lies within lib<insert thread library here>.  Of course, the
> rtld init stuff could be pulled out and done in thread
> initialization instead of thr_setthreaded().  That doesn't
> leave much in thr_setthreaded, and it also adds locking
> overhead into rtld for single-threaded programs that are
> linked with libthr...

Ok, let me to tell the whole story. I am sure that in fact you know
it better then me.

Assuming libthr is the only threading library, there are two locking
implementations for the rtld: 'default' and the one supplied by libthr.
On the first call to pthread_create(), libthr calls _rtld_thread_init()
to substitute the default by the implementation from libthr.

In fact, default implementation is broken from my point of view. For
instance, thread_flag update is not atomic. Moreover, it does not
correctly handles sequential acquision of several locks, due
to thread_flag.

The dl_iterate_phdr() function, called by gcc exception handling support
code, does exactly this. It acquires rtld_phdr_lock, then rtld_bind_lock.
[I shall admit it does this after my change]. In particular, this would
leave the bit for the bind lock set in the thread_flag.

Andriy' example throw the exception and calls dl_iterate_phdr() before
first thread is created. On thread creation, _rtld_thread_init() is
called, that tries to move the locks according to thread_flag. This is
the cause for the reported wlock acquisition.

I do not want to change anything in the default rtld locking. It is
disfunctional from the time libc_r is gone, and I think it would be
better to make it nop. My change makes the image that is linked with
libthr, to consistently use libthr locks.

[Daniel Eischen]

On Sat, 30 Aug 2008, Kostik Belousov wrote:

> On Sat, Aug 30, 2008 at 12:15:31PM -0400, Daniel Eischen wrote:
>> On Sat, 30 Aug 2008, Kostik Belousov wrote:
>>
>>> On Sat, Aug 30, 2008 at 11:32:35AM -0400, Daniel Eischen wrote:
>>>> On Fri, 29 Aug 2008, Kostik Belousov wrote:
>>>>>
>>>>> As demonstrated by Andriy' example, we need _thr_rtld_init() be called
>>>>> before any rtld locks are given chance to be acquired. _thr_rtld_init()
>>>>> shall be protected from repeated invocation, and _thr_setthreaded()
>>>>> implements exactly this.
>>>>>
>>>>> If calling _thr_setthreaded(1) has not quite right intent, could you,
>>>>> please, suggest satisfying solution ?
>>>>
>>>> I'm not sure I _quite_ understand the problem, but why
>>>> wouldn't you have the same potential problem with some
>>>> other library (without libthread)?  I'll have to go back
>>>> and read the beginning of the thread - I just kinda came
>>>> in at the end.
>>>
>>> Sure, for appropriate value of any. If you mean whether the same problem
>>> would arise for any threading library that supplies locking implementation
>>> for rtld, then certainly yes. I looked over and patched only libthr
>>> since this is the only survived library for now.
>>
>> What I mean is, is fixing libthr a solution that will work
>> for cases?  Or, is libthr doing something wrong?  I can't
>> really see that it is.
>>
>> libthr assumes that everything is single-threaded (or
>> serialized, I guess) before a thread is created.  I
>> am looking at this thread:
>>
>>   http://docs.freebsd.org/cgi/getmsg.cgi?fetch=5235+0+current/freebsd-threads
>>
>> Where is the corresponding unlock for the wlock_acquire()?
>> I guess this is the problem.  When would this normally
>> be released (without libthr being linked in)?
>>
>> Also, the __isthreaded flag is used in libc to avoid taking
>> locks unless necessary.  So if you have a single threaded
>> application that is also linked with libthr, you don't
>> pay the penalty of locking overhead.  Lots of 3rd-party
>> libraries link with a threads library, so an application
>> may not even know it is "threaded".
>>
>>>
>>> Anyway, I do not insist on the proposed solution, and definitely
>>> prefer the change that is well aligned with libthr architecture.
>>
>> I'm not arguing anything, I just don't know that the problem
>> lies within lib<insert thread library here>.  Of course, the
>> rtld init stuff could be pulled out and done in thread
>> initialization instead of thr_setthreaded().  That doesn't
>> leave much in thr_setthreaded, and it also adds locking
>> overhead into rtld for single-threaded programs that are
>> linked with libthr...
>
> Ok, let me to tell the whole story. I am sure that in fact you know
> it better then me.
>
> Assuming libthr is the only threading library, there are two locking
> implementations for the rtld: 'default' and the one supplied by libthr.
> On the first call to pthread_create(), libthr calls _rtld_thread_init()
> to substitute the default by the implementation from libthr.
>
> In fact, default implementation is broken from my point of view. For
> instance, thread_flag update is not atomic. Moreover, it does not
> correctly handles sequential acquision of several locks, due
> to thread_flag.
>
> The dl_iterate_phdr() function, called by gcc exception handling support
> code, does exactly this. It acquires rtld_phdr_lock, then rtld_bind_lock.
> [I shall admit it does this after my change]. In particular, this would
> leave the bit for the bind lock set in the thread_flag.
>
> Andriy' example throw the exception and calls dl_iterate_phdr() before
> first thread is created. On thread creation, _rtld_thread_init() is
> called, that tries to move the locks according to thread_flag. This is
> the cause for the reported wlock acquisition.
>
> I do not want to change anything in the default rtld locking. It is
> disfunctional from the time libc_r is gone, and I think it would be
> better to make it nop. My change makes the image that is linked with
> libthr, to consistently use libthr locks.

What happens if you remove the thread_flag() stuff (support
for libc_r?) from rtld?  It seems that libc_r should be providing
its own rtld locking hooks - just like libthr does.

--
DE
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[Kostik Belousov]

On Sun, Aug 31, 2008 at 11:39:52AM -0400, Daniel Eischen wrote:

> On Sat, 30 Aug 2008, Kostik Belousov wrote:
>
> >On Sat, Aug 30, 2008 at 12:15:31PM -0400, Daniel Eischen wrote:
> >>On Sat, 30 Aug 2008, Kostik Belousov wrote:
> >>
> >>>On Sat, Aug 30, 2008 at 11:32:35AM -0400, Daniel Eischen wrote:
> >>>>On Fri, 29 Aug 2008, Kostik Belousov wrote:
> >>>>>
> >>>>>As demonstrated by Andriy' example, we need _thr_rtld_init() be called
> >>>>>before any rtld locks are given chance to be acquired. _thr_rtld_init()
> >>>>>shall be protected from repeated invocation, and _thr_setthreaded()
> >>>>>implements exactly this.
> >>>>>
> >>>>>If calling _thr_setthreaded(1) has not quite right intent, could you,
> >>>>>please, suggest satisfying solution ?
> >>>>
> >>>>I'm not sure I _quite_ understand the problem, but why
> >>>>wouldn't you have the same potential problem with some
> >>>>other library (without libthread)?  I'll have to go back
> >>>>and read the beginning of the thread - I just kinda came
> >>>>in at the end.
> >>>
> >>>Sure, for appropriate value of any. If you mean whether the same problem
> >>>would arise for any threading library that supplies locking
> >>>implementation
> >>>for rtld, then certainly yes. I looked over and patched only libthr
> >>>since this is the only survived library for now.
> >>
> >>What I mean is, is fixing libthr a solution that will work
> >>for cases?  Or, is libthr doing something wrong?  I can't
> >>really see that it is.
> >>
> >>libthr assumes that everything is single-threaded (or
> >>serialized, I guess) before a thread is created.  I
> >>am looking at this thread:
> >>
> >>  http://docs.freebsd.org/cgi/getmsg.cgi?fetch=5235+0+current/freebsd-threads
> >>
> >>Where is the corresponding unlock for the wlock_acquire()?
> >>I guess this is the problem.  When would this normally
> >>be released (without libthr being linked in)?
> >>
> >>Also, the __isthreaded flag is used in libc to avoid taking
> >>locks unless necessary.  So if you have a single threaded
> >>application that is also linked with libthr, you don't
> >>pay the penalty of locking overhead.  Lots of 3rd-party
> >>libraries link with a threads library, so an application
> >>may not even know it is "threaded".
> >>
> >>>
> >>>Anyway, I do not insist on the proposed solution, and definitely
> >>>prefer the change that is well aligned with libthr architecture.
> >>
> >>I'm not arguing anything, I just don't know that the problem
> >>lies within lib<insert thread library here>.  Of course, the
> >>rtld init stuff could be pulled out and done in thread
> >>initialization instead of thr_setthreaded().  That doesn't
> >>leave much in thr_setthreaded, and it also adds locking
> >>overhead into rtld for single-threaded programs that are
> >>linked with libthr...
> >
> >Ok, let me to tell the whole story. I am sure that in fact you know
> >it better then me.
> >
> >Assuming libthr is the only threading library, there are two locking
> >implementations for the rtld: 'default' and the one supplied by libthr.
> >On the first call to pthread_create(), libthr calls _rtld_thread_init()
> >to substitute the default by the implementation from libthr.
> >
> >In fact, default implementation is broken from my point of view. For
> >instance, thread_flag update is not atomic. Moreover, it does not
> >correctly handles sequential acquision of several locks, due
> >to thread_flag.
> >
> >The dl_iterate_phdr() function, called by gcc exception handling support
> >code, does exactly this. It acquires rtld_phdr_lock, then rtld_bind_lock.
> >[I shall admit it does this after my change]. In particular, this would
> >leave the bit for the bind lock set in the thread_flag.
> >
> >Andriy' example throw the exception and calls dl_iterate_phdr() before
> >first thread is created. On thread creation, _rtld_thread_init() is
> >called, that tries to move the locks according to thread_flag. This is
> >the cause for the reported wlock acquisition.
> >
> >I do not want to change anything in the default rtld locking. It is
> >disfunctional from the time libc_r is gone, and I think it would be
> >better to make it nop. My change makes the image that is linked with
> >libthr, to consistently use libthr locks.
>
> What happens if you remove the thread_flag() stuff (support
> for libc_r?) from rtld?  It seems that libc_r should be providing
> its own rtld locking hooks - just like libthr does.

No, libc_r uses default rtld locking, as absence of the references to
_rtld_thread_init in libc_r code indicates. This is one of the reasons
why I decline to change the code. We still support compat-4.x on all
supported systems, but I have no way to properly test it,

If changing the default implementation, I think we should just record
lock attempts to be able to transfer them when _rtld_thread_init is
called. But this would break libc_r.

I saw the use of the libthr locking from the start as the cleanest and
least intrusive. As I said, I would gladly accept any better idea.

[David Xu-2]

Kostik Belousov wrote:

> Ok, let me to tell the whole story. I am sure that in fact you know
> it better then me.
>
> Assuming libthr is the only threading library, there are two locking
> implementations for the rtld: 'default' and the one supplied by libthr.
> On the first call to pthread_create(), libthr calls _rtld_thread_init()
> to substitute the default by the implementation from libthr.
>
> In fact, default implementation is broken from my point of view. For
> instance, thread_flag update is not atomic. Moreover, it does not
> correctly handles sequential acquision of several locks, due
> to thread_flag.
>
> The dl_iterate_phdr() function, called by gcc exception handling support
> code, does exactly this. It acquires rtld_phdr_lock, then rtld_bind_lock.
> [I shall admit it does this after my change]. In particular, this would
> leave the bit for the bind lock set in the thread_flag.
>
> Andriy' example throw the exception and calls dl_iterate_phdr() before
> first thread is created. On thread creation, _rtld_thread_init() is
> called, that tries to move the locks according to thread_flag. This is
> the cause for the reported wlock acquisition.
>
> I do not want to change anything in the default rtld locking. It is
> disfunctional from the time libc_r is gone, and I think it would be
> better to make it nop. My change makes the image that is linked with
> libthr, to consistently use libthr locks.

The ancient bug is in rtld, rlock_acquire() and wlock_acquire() test
thread_flag as a boolean value, because pt_iterate_phdr() tries to
lock two locks at same time, this test will always fail once it
acquired first lock.

The following silly patch fixes the problem Andriy encountered:

Index: rtld_lock.c
===================================================================
--- rtld_lock.c （版本 182594）
+++ rtld_lock.c （工作副本）
@@ -184,7 +184,7 @@
  int
  rlock_acquire(rtld_lock_t lock)
  {
- if (thread_mask_set(lock->mask)) {
+ if (thread_mask_set(lock->mask) & lock->mask) {
     dbg("rlock_acquire: recursed");
     return (0);
  }
@@ -195,7 +195,7 @@
  int
  wlock_acquire(rtld_lock_t lock)
  {
- if (thread_mask_set(lock->mask)) {
+ if (thread_mask_set(lock->mask) & lock->mask) {
     dbg("wlock_acquire: recursed");
     return (0);
  }


Regards,
David Xu

_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

on 01/09/2008 11:10 David Xu said the following:
> The ancient bug is in rtld, rlock_acquire() and wlock_acquire() test
> thread_flag as a boolean value, because pt_iterate_phdr() tries to
> lock two locks at same time, this test will always fail once it
> acquired first lock.
>
> The following silly patch fixes the problem Andriy encountered:

I can confirm - this fixed the issue for me.
David, thanks!

> Index: rtld_lock.c
> ===================================================================
> --- rtld_lock.c    （版本 182594）
> +++ rtld_lock.c    （工作副本）
> @@ -184,7 +184,7 @@
>  int
>  rlock_acquire(rtld_lock_t lock)
>  {
> -    if (thread_mask_set(lock->mask)) {
> +    if (thread_mask_set(lock->mask) & lock->mask) {
>          dbg("rlock_acquire: recursed");
>          return (0);
>      }
> @@ -195,7 +195,7 @@
>  int
>  wlock_acquire(rtld_lock_t lock)
>  {
> -    if (thread_mask_set(lock->mask)) {
> +    if (thread_mask_set(lock->mask) & lock->mask) {
>          dbg("wlock_acquire: recursed");
>          return (0);
>      }
>
>
> Regards,
> David Xu
>

--
Andriy Gapon
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[Kostik Belousov]

On Mon, Sep 01, 2008 at 11:34:45AM +0300, Andriy Gapon wrote:

> on 01/09/2008 11:10 David Xu said the following:
> >The ancient bug is in rtld, rlock_acquire() and wlock_acquire() test
> >thread_flag as a boolean value, because pt_iterate_phdr() tries to
> >lock two locks at same time, this test will always fail once it
> >acquired first lock.
> >
> >The following silly patch fixes the problem Andriy encountered:
>
> I can confirm - this fixed the issue for me.
> David, thanks!

Does libc_r still work with patch applied ?

on 01/09/2008 11:45 Kostik Belousov said the following:

> On Mon, Sep 01, 2008 at 11:34:45AM +0300, Andriy Gapon wrote:
>> on 01/09/2008 11:10 David Xu said the following:
>>> The ancient bug is in rtld, rlock_acquire() and wlock_acquire() test
>>> thread_flag as a boolean value, because pt_iterate_phdr() tries to
>>> lock two locks at same time, this test will always fail once it
>>> acquired first lock.
>>>
>>> The following silly patch fixes the problem Andriy encountered:
>> I can confirm - this fixed the issue for me.
>> David, thanks!
>
> Does libc_r still work with patch applied ?

In what sense?
The test program that I posted seems to hang in both cases (patched and
unpatched rtld).

--
Andriy Gapon
_______________________________________________
freebsd-threads@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-threads
To unsubscribe, send any mail to "freebsd-threads-unsubscribe@..."

[Kostik Belousov]

On Mon, Sep 01, 2008 at 01:53:23PM +0300, Andriy Gapon wrote:

> on 01/09/2008 11:45 Kostik Belousov said the following:
> >On Mon, Sep 01, 2008 at 11:34:45AM +0300, Andriy Gapon wrote:
> >>on 01/09/2008 11:10 David Xu said the following:
> >>>The ancient bug is in rtld, rlock_acquire() and wlock_acquire() test
> >>>thread_flag as a boolean value, because pt_iterate_phdr() tries to
> >>>lock two locks at same time, this test will always fail once it
> >>>acquired first lock.
> >>>
> >>>The following silly patch fixes the problem Andriy encountered:
> >>I can confirm - this fixed the issue for me.
> >>David, thanks!
> >
> >Does libc_r still work with patch applied ?
>
> In what sense?
> The test program that I posted seems to hang in both cases (patched and
> unpatched rtld).

The David' patch changes the code used to support libc_r operations.
Even on CURRENT, if you run 4.x-compiled binary with the support of
compat-4x libraries, this code from /libexec/ld-elf.so.1 (installed
by CURRENT buildworld) provides locking for rtld.