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

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

> Waiting with absolute or relative timeouts is split into separate
> functions.  This allows for removing a few cases of code duplication in
> pthreads code, which uses absolute timeouts; also, it allows us to put
> platform-specific code to go from an absolute to a relative timeout into
> the platform-specific futex abstractions.  The latter is done by adding
> lll_futex_abstimed_wait.  I expect that we will refactor this later on,
> depending on how we do the lll_ parts.

I don't understand the motivation for adding lll_futex_abstimed_wait now
at all.  Its only users are in futex-internal.h implementations, which
are already OS-specific.  So why make the new files have identical
copies of the wrappers around the implementations living in the moribund
files?  What is the downside to simply having each futex-internal.h's
futex_abstimed_wait{,_cancelable} be the real implementation?

> There are separate versions for both Linux and NaCl; while they
> currently differ only slightly, my expectation is that the separate
> versions of lowlevellock-futex.h will eventually be merged into
> futex-internal.h when we get to move the lll_ functions over to the new
> futex API.

This is not just an expectation, it's the core plan and the whole effort
would be pointless if we failed to actually do this in the future.

> The sanity checks regarding whether shared futexes are supported abort
> instead of returning errors because POSIX error specs don't really
> consider that there could be no support for shared futexes.  Aborting is
> better than returning an unspecified error or an error specified for a
> different condition; only NaCl has no support for shared futexes.

This is not the right behavior.  It is indeed improper to use errno code
E in function F for condition X when POSIX specifies that F returns E
for condition Y.  It is also improper to use errno code E in function F
for condition X when POSIX specifies errno code E2 for condition X in
function F.  But it is entirely proper to return an errno code that
POSIX does not specify for a given function to diagnose a condition that
POSIX does not specify shall or may be diagnosed (see 2.3 Error Numbers).

For pthread_*attr_setpshared, the sensible thing to do for an
unsupported (but valid) value is to return ENOTSUP.  That's what these
functions should do for PTHREAD_PROCESS_SHARED on NaCl.  Conversely,
it's entirely reasonable not to account for any possibility of
PTHREAD_PROCESS_PRIVATE not being supported, because to request that
is to request the default behavior.

> Interacting with futex words requires atomic accesses, which isn't done
> by most of glibc's current futex callers. [...]

I certainly concur with leaving this until later.  What I think will be
reasonable to do eventually is to use the <stdatomic.h> type names in
all our internal interfaces (probably only atomic_int or atomic_uint
should be used for futex stuff).  When building with older compilers
that don't have those, our own internal headers can typedef the ones
we use to the simple types (or perhaps volatile-qualified ones?).

> Roland, okay for NaCl?  I decided to not try to "optimize" the
> shared/private setting at data structure initialization time because I
> didn't see a good way to specify the error conditions for the futex_*
> functions then: We do want those to sanity check shared/private and not
> just rely on the shared/private initialization to do the right thing --
> but if we do that, we can as well transform private into shared if
> that's actually necessary.

I don't follow your logic here.  Why do you think we want any
argument-validity checks applied inside internal interfaces?  It's
reasonable enough to have asserts inside the futex-internal.h
functions if you feel like it.  Those can only fire when other libc
code has a bug or there is memory clobberation.  Those are both cases
where it's nice to fail catastrophically rather than mysteriously, but
also both cases where we don't spend extra effort to diagnose the
(supposedly) impossible situations and certainly where we never
propagate such an error back to the user as a return or errno value.

What I have in mind is (names and signatures are straw men):

* In pthread_*attr_setpshared:
	error = futex_pshared (pshared, &iattr->pshared);
	if (__glibc_unlikely (error))
	  return error;
  This transforms the public ABI values PTHREAD_PROCESS_{PRIVATE,SHARED}
  in PSHARED into the internal form in IATTR->pshared.  On Linux the
  internal form would be FUTEX_PRIVATE_FLAG or 0, to be simply OR'd
  into the operation in the syscall.  On NaCl the internal form would
  just be some constant value, only stored at all to avoid confusing
  valgrind et al.

* In *_init:
	object->private = iattr->pshared;
  There's really no need for checks here, since it's undefined
  behavior to pass a bogus attributes object.  

  It can't be literally that alone since there has to be a default for
  a null attributes pointer.  If it seems worthwhile, we could have it
  sanity check the value in the attributes struct so as to detect
  clobbered or uninitialized attributes; that would be done by a
  futex-internal.h function to transfer attributes-field format into
  object-field format (it would be up to the particular implementation
  to decide if those are the same or different).  But I don't see a
  real need for that.

* In the actual uses:
	futex_wake (&obj->futex, nw, obj->private);
  That is, nothing special.  All the checking was done before.
  If the futex-internal.h implementation cares to do redundant checks
  for clobberation, it can assert inside there.  There should be no
  provision for returning errors to indicate the private field has an
  invalid value.

What don't you like about that (rough) picture?


Thanks,
Roland