Re: [PATCH 1/5][v2][BZ #15022] Avoid repeated calls to DL_STATIC_INIT

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Thu, 20 Jun 2013, Roland McGrath wrote:

> > The difference between calling from dl_open_worker
> > and _dl_open is nested locks, which may be a problem.
> 
> That's a good point to raise.  But it looks like it's not actually a
> problem.  Both _dl_static_init implementations have their own private
> lock (though it's not clear to me why they actually need one at all).
> They call _dl_lookup_symbol_x directly, which does not do any locking
> of its own (it expects its callers to have serialized access to the
> data structures it uses).

 Thank you both for your input.  Having looked at the pieces of code 
involved I think what H.J. refers to is that _dl_static_init is called 
with GL(dl_load_lock) held and then the function itself requests 
_dl_static_lock.  This would cause a trouble if a piece of code elsewhere 
requested _dl_static_lock first and then with that lock held requested 
GL(dl_load_lock) => deadlock.

 However AFAICT this is not going to matter in practice with the code as 
it is, because _dl_static_lock is never requested outside _dl_static_init 
and with my change applied that function is never going to be called with 
GL(dl_load_lock) released.  And then we have full control over 
_dl_static_init and can make sure that with any future changes the 
deadlock scenario does not happen.

 Moreover, AFAICS it is GL(dl_load_lock) that serialises accesses to the 
data structures used here, so given what you wrote about locking in the 
context of _dl_lookup_symbol_x I conclude first that the move of 
_dl_static_init under the guard of GL(dl_load_lock) is actually needed and 
a bug fix by itself and second that, as you say, with GL(dl_load_lock) 
held _dl_static_lock seems not needed anymore.  So it looks to me like the 
lock can be safely removed along this change.  Do you agree?

 What could bother me here is calling _dl_init (that is also called from 
dl_open_worker and with my change applied right after _dl_static_init).  
That also happens with GL(dl_load_lock) held, however unlike 
_dl_static_init _dl_init calls into user code.  We don't have control over 
what users put into static constructors and I suspect that part could be 
prone to a deadlock if a piece of user code called back into the wrong 
part of libdl/libc/ld.so.  But perhaps I'm overly cautious here -- do you 
or anyone else happen to know if what can or cannot be put into static 
constructors has been standardised anyhow anywhere?

> So I think we can go ahead with macro's change unless anybody else
> sees a concrete problem.

 Thanks for your review.  If we agree on removing _dl_static_lock at this 
time, then I'll post an updated change, or otherwise I'll wait a couple of 
days and will commit this part unchanged unless someone objects.

  Maciej