Re: [PR19826] fix non-LE TLS in static programs

[Torvald Riegel <triegel at redhat dot com>]

On Thu, 2016-09-29 at 18:51 -0300, Alexandre Oliva wrote:
> On Sep 29, 2016, Torvald Riegel <triegel@redhat.com> wrote:
> 
> > Consider an exaggerated but similar example:  I want to add a
> > non-trivial piece of code, and like to reason about it using notes on
> > paper.  Yet glibc has decided that it wants comments in the code.  What
> > should I do?  Should I just give up, or try to find a way to do what
> > glibc wants and how it decided to maintain the code base (eg, finding a
> > back-and-forth conversion between glibc model of taking notes and mine)?
> 
> IMHO the important question is not what *you* should do in this
> scenario, but what should the GNU Libc community do, when an occasional
> contributor comes up with a fix for an existing bug.

First of all, you are not a hobbyist contributor AFAIK.

> Should you boss
> around playing language police

This is part of patch review.  Being consistent re how we document and
reason about concurrency, and ensuring that such comments are
technically correct and match the code, is important.

I've offered my help plenty of times, and the standard for new
concurrent code we set is not unreasonable (eg, glibc doesn't use a
custom memory model or atomics, but something that all modern C and C++
code will use).  So asking patch contributors to adhere to that is fine
I think.  We're also expecting code to be proper C in all other aspects,
so why should we make an exception for concurrency aspects?

> and demand further unrelated changes, so
> as to push the contributor away,

I asked whether you could look at related parts of the code, because you
mentioned that you know this larger part of the code base.

> > Right.  But we also need to consider that more than one person will
> > eventually want to maintain any piece of glibc code.
> 
> We also have to consider that by the time some of these people do, the
> standards will have changed again, and the language in the comments will
> again no longer be in line with the newer consensus of the community.

When do you think will we get a new ISO C/C++ memory model?  You seem to
think that this will happen in just a few years...

> >> but IIRC there are
> >> self-concurrency issues if DTV resizing is interrupted by a signal that
> >> uses GD and starts further resizing) and IIRC there is plenty of
> >> suspicious stuff in slotinfo manipulation vs use (the data structure is
> >> only modified while holding the rtld lock, but it's read while updating
> >> a DTV without any explicit synchronization; I never looked into it
> >> enough to tell what the assumptions were that make it safe at some
> >> point, to tell whether there's any chance they still are);
> 
> > That sounds like a incorrect attempt at double-checked locking, which
> > I've found several cases of so far, so it would be "within the pattern".
> 
> I don't see how you could possibly have drawn that conclusion, since the
> readers do NOT take locks, before or after any test.

Your description sounded as if it could match, in the sense that readers
try to, for example, detect whether the initialization has been done
(which uses a critical section); in that case, readers take no lock.
Even if it's not initialization-only, the similar point is that you
can't publish a result consiting of more than one memory location
without memory barriers, and you can't read from it without barriers.

> > Also note that even if assuming a strong memory model, some of the nscd
> > synchronization would be broken.
> 
> > Furthermore, you reviewed nscd-related code and marked it as
> > thread-safe.
> 
> The review was limited to functions provided by glibc and documented in
> the glibc manual.  nscd provides none.  So your claim that I reviewed
> them may very well be the result of a misunderstanding leading to an
> incorrect assumption.

The manual contains annotations of several functions that start with
"nscd_".  These are in the nscd client code, and are called from
functions provided by glibc.  Was it somebody else that put these
annotations into the manual?

nscd client-server synchronization is broken.  That there is something
wrong is easy to see because there clearly is synchronization and
comments that hint at low-level synchronization -- but there are no
atomic operations (nor sufficient locking), and thus no memory barriers
for example; also, volatile-qualified variables are used even though
there's no low-level IO, which is an indicator for synchronization done
in a way before proper compiler support for it.

> Regardless, if you found any comments about nscd code in my
> thread-safety annotations (and presumably corrected them), it would have
> been polite (and appreciated) for you to copy me, so that I could have
> been informed and educated about the changes.  I don't have recollection
> of having been copied on any such messages.

I have already sent the draft patch to a list you have access to.  I
haven't checked all the annotations again yet, but I believe that my
patch now makes the annotations correct (considering the thread-safety
aspect here).