Re: [PR18457] Don't require rtld lock to compute DTV addr for static TLS

[Alexandre Oliva <aoliva at redhat dot com>]

On Jun  5, 2015, "Carlos O'Donell" <carlos@redhat.com> wrote:

> The solution does not meet my standards for fixing atomicity issues.
> It lacks proper documentation on the synchronization changes being made.

Could it be because there aren't any synchronization changes being made?
Seriously, the changes, if any, were made in the previous patch, that
didn't meet such a storm of stonewalling and nitpicking.  This one just
fixes a tiny part of the change in the previous patch.

Why is it drawing so much attention?  Why are so different standards
being applied to this one?  It's not like our consensus has changed
since then.  It's not like the problem fixed by this patch is less
serious than the one fixed before.

>> But if this larger fix does not contain the very change I'm proposing,
>> tls_get_addr on variables assigned static TLS will remain much slower
>> than needed, because there's absolutely no need to take a lock when it
>> is already decided whether the module should use static or dynamic TLS.

> I agree there is no need to take the lock, but your patch needs to be
> expanded in more detail to make it crystal clear exactly what
> synchronization is being done and why.

  /* If the TLS block for the map is already assigned to dynamic, or
     to some static TLS offset, the decision is final, and no lock is
     required.  Now, if the decision hasn't been made, take the rtld
     lock, so that an ongoing dlopen gets a chance to complete,
     possibly assigning the module to static TLS and initializing the
     corresponding TLS area for all threads, and then retest; if the
     decision is still pending, force the module to dynamic TLS.

     The risk that the thread accesses an earlier value in that memory
     location, from before it was recycled into a link map in another
     thread, is removed by the need for some happens before
     relationship between the loader that set that up and the TLS
     access that referenced the module id.  In the presence of such a
     relationship, the value will be at least as recent as the
     initialization, and in its absence, calling tls_get_addr with its
     module id invokes undefined behavior.  */

Seriously, what's missing from this?  What's not crystal clear about it?

>> So what is this simple regression fix waiting for to get installed,
>> again?

> As of today only Fedora is using your previous patches which reveal these
> older bugs, and until we branch for 2.22, this patch can wait while we
> look into a more holistic fix.

And then you're going to have a single patch fixing two or more
different issues?  Isn't that against our conventions, that prefer
separate patches to address separate issues?

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer