Re: [PATCH] Add STATIC_GETENV macro.

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

On Thu, Nov 14, 2013 at 04:49:57PM +0100, Torvald Riegel wrote:
> On Thu, 2013-11-14 at 13:52 +0100, OndÅej BÃlka wrote:
> > On Thu, Nov 14, 2013 at 11:13:58AM +0100, Torvald Riegel wrote:
> > > On Wed, 2013-11-13 at 22:33 +0100, OndÅej BÃlka wrote:
> > > > 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)
> > > 
> > > Even with that change, the load of p needs to be atomic with
> > > acquire/consume memory order.
> > >
> > We should support older gcc that do not support C11 atomics. 
> 
> Sure, but you still need something equivalent to make it work correctly.
> In effect, you're just arguing that it shouldn't be a macro but a
> function call, where glibc can deal with how to support older
> compilers / language standards.
>
Also as current getenv code contains nonatomic reads of pointers
following code would work on all architectures where current getenv
works. To support other architectures a patch that convert getenv is
needed.
 
> > Also when you are concerned about machines that do issue nonatomic reads
> > then relaxed model is appropriate, acquire/consume could yield unacceptable
> > performance overhead.
> 
> Which relaxed model?  Do you mean relaxed memory order?  Atomicity is
> orthogonal to ordering constraints enforced by particular memory orders.
> Also, acquire/consume don't need any HW instructions on x86, sparc, and
> the like (they still constrain compiler optzns, of course).  Consume
> should be very little overhead on Power, and even acquire shouldn't be
> too much on Power and ARM.
> 
All that is needed is that value returned has is from set initial/value
returned by getenv.

It does not matter which one is read as long as this is from this set,
so you do not need synchronization.

In fact if one changed that variable to thread local without changing
semantics.

As this could be made genericaly will post a separate patch.