Re: [PATCH v4] Add and use new glibc-internal futex API.

[Roland McGrath <roland at hack dot frob dot com>]

> I've removed the casts.  The clean-up to enums should be done with the
> patch that uses atomics to access setxid_futex.
> 
> (Note that the lack of atomic accesses to futex words is a recurring
> problem, and not one I intended to solve with this patch.)

Agreed.

> ISTM the cancelhandling change to unsigned int requires more pervasive
> code changes than in the patch you posted (see my reply to that).
> I've kept the casts for now, with the expectation that we'll change to
> unsigned int in the patch that uses atomics for all accesses to
> cancelhandling.

OK.  Anything is fine with me as long as by the end we clean things up to
remove all the casts.

> I'm now including sysdeps/nptl/futex-internal.h from the
> platform-specific futex-internal.h files.  futex_wait_simple goes there
> as well as futex_fatal_error.

That should be fine.  It wouldn't hurt to pass the errno value and perhaps
a string naming the operation to futex_fatal_error.  Then we can later
change it to call __libc_message (1, ...) for a more informative error.
(__libc_fatal takes a plain string, but __libc_message takes a minimal
printf-style format.)

> Can't do that right now because it still needs to cast away the volatile
> that's still present after your recent change that makes it volatile
> unsigned int.  I don't want to just remove the volatile either, because
> this way make the synchronization work (it really should use atomics
> though).  This can be cleaned up when we transform this to using
> atomics.

Agreed.  Perhaps every cast you add should have a formulaic (greppable)
comment so we can be sure we find them all in the later cleanup passes.

> I had posted this last year, and I remember that this is considered the
> "publication" of the code.

OK.

> Updated patch is attached.  Once this is OK, I'll send an updated patch
> that includes the (updated) sparc changes so that Dave can test (per
> Joseph's request, to allow for bisect-ability, we can't have a separate
> patch for the sparc bits).

I didn't check it closely, nor test it on NaCl.  I won't get to that until
next week.  (I'm actually on vacation this whole week, and just checking in
today to make sure I wasn't blocking you.)  Since we've resolved the issues
I raised, I'm sure it will be fine now.  I would appreciate if you could
amend it with greppable comments for everything we agreed we should clean
up later.  But beyond that if it has no regressions and nobody else
objects, it's fine with me to commit now.  If it turns out NaCl is actually
broken, I'll fix it up next week.

It wouldn't hurt to write up somewhere, probably on the wiki (though in
code comments in this patch would also be fine by me) the known/intended
further steps of transition and cleanup in this area.  I imagine that this
patch and maybe a bit more trivia is about all we'll get done before the
coming release freeze.  In practice that tends to mean we stop thinking
about the details for a month (and then that bleeds into August travel and
vacations and it always winds up longer than we intend before we get back
to it), so it seems wise to make the short-term future plans clear now
while the current discussion is fresh (and record them someplace easier to
follow than scattered in these review threads).


Thanks,
Roland