Re: [PATCH] Add STATIC_GETENV macro.

[OndÅej BÃlka <neleai at seznam dot cz>]

On Wed, Nov 13, 2013 at 04:34:18PM +0100, Torvald Riegel wrote:
> 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.
> 
Yes but this does not need forcing users to link with pthread.


> 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)						      \
>
There should be
if (__new == (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.

You do not need atomic access for correctness, worst that can happen is
performance degradation by repeately recalculating value.

Unless compiler decides to inline functions so that it accesses variable
in loop which when called first time would cause repeated
recomputations. A solution would be mark variable volatile.
> 
> > +    {									      \
> > +      __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.
> 
No need to complicate matters with custom order as this path will likely be 
visited at most once per program lifetime.

> 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;								      \
> > +})
> > +
>