This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add STATIC_GETENV macro.


I don't think a macro is wise here.  You seem to want to make the macro
safe for use in multi-threaded programs, so this is similar to a
pthread_once except that there can be more than one call to get_env (ie,
the equivalent to the initialization function), but exactly on
assignment.
Maybe a pthread_once_store(pointer, producer) would be better (*if* we
actually want to add such a feature): pointer is like p in your code,
producer is a function that is called when we actually need to
initialize but can be safely called more than once; we exactly-once set
pointer to the value returned by one of the producer calls.

Implementing this properly requires more than just a simple CAS (see
pthread_once and my comments below); thus, you'll need to use proper
atomics or whatever the compiler has available, which requires you to
take care of all of that in the context of the client's compilation
environment.

On Sat, 2013-11-09 at 11:38 +0100, OndÅej BÃlka wrote:
> +#define STATIC_GETENV(c) \
> +({									      \
> +  static char *__p = (char *) &__p;					      \
> +  char *__new = __p;							      \
> +  if (__p == (char *) &__p)						      \

This is a data race (at least in C11, which has a concept of threads).
You need to use an atomic access, and it must have acquire memory order.
Consume memory order could also be sufficient, I believe.

> +    {									      \
> +      __new = getenv (c);						      \
> +      char *__val = __sync_val_compare_and_swap (&__p, (char *) &__p, __new); \

This needs acquire-release memory order.  Seq-cst would be too much,
anything less than acquire-release won't work on archs with weak memory
models.

You really should also document synchronization code thoroughly.  That
we need a CAS for exactly-once might be obvious to many; but I guess
it's not so obvious which memory orders we need (otherwise, I guess you
wouldn't have missed them :).  Therefore, do document which accesses are
supposed to synchronize with each other, and which acquire/release pairs
you need to establish the happens-before relations required for
correctness.

> +      __new = (__val ==  (char *) &__p) ? __new : __val;		      \
> +    }									      \
> +  __new;								      \
> +})
> +



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]