Re: Another multithreading bug (I think)

[Richard Frith-Macdonald]

On 8 Sep 2006, at 08:57, David Ayers wrote:

> Richard Frith-Macdonald schrieb:
>
> > Can you see any problems with this?
> > I don't think -retain needs to lock the gate, and as long as we are
> > only locking in the -release method as above, I think we can avoid
> > using a recursive lock (as I don't think we call -release inside any
> > region where we already lock the gate), but I may have missed  
> > something.
>
> This is just tangential, but I just want to make everyone aware of the
> fact that stores may not be atomic.  I'm not certain how relevant that
> is for pointers/integers on IA-32 SMP platforms but for AMD64/EMT64/ 
> IA64
> SMP this may become a serious issue.
>
> This little detail is what makes the usage of the double checked  
> locking
> idiom an issue for us also:
>
> http://en.wikipedia.org/wiki/Double-checked_locking
> (The issue is described in Java but is valid for plain C just as  
> well.)
>
> Note that the article implies that some C++ compilers make volatile
> read/stores atomic operations, but I'm certain that's not the case for
> GCC (at least not for current versions).  Effectively, I believe,
> volatile only means that the memory won't be cached in registers.
>
> http://gcc.gnu.org/onlinedocs/gcc-4.1.1/gcc/Volatiles.html#Volatiles
>
> I understand that sacrificing double checked locking could have a  
> harsh
> performance impact, but I don't have any numbers.  Personally I would
> rather sacrifice performance for correctness but I could live with a
> compile time option if numbers show that the cost is very high.

While storage of a pointer may not be atomic on some architectures,  
it's not a problem on any modern architectures AFAIK (CPUs with 64bit  
pointers tend to use a 64bit memory bus and therefore do atomic reads/ 
write).  You may know different.

However, in one of the links from wikipedia I found something else  
that may be a problem ... the optimisation done by the compiler may  
re-order assignment, so even though we have coded things to create a  
shared object and then assign it to a static variable, the compiler  
could assign to the variable before the object is fully created.   
This is really an issue for C++ though, not one for Objective-C, as  
we create instances with method calls ... effectively function calls,  
so the return value from the function call is assigned to the static  
variable and there seems to be no scope for the compiler to reassign  
statements to assign to it during the object creation process.

I'm guessing that storing a single byte must be atomic on all  
architectures, so how about this as a possible safe implementation?

- (id) sharedInstance
{
  static volatile uint8_t	isCreated = 0;	// single byte declared  
volatile so we read from memory rather than a register
  static volatile id theInstance = nil;

  if (isCreated == 0)
    {
      [theLock lock];   
      if (isCreated == 0 && theInstance == nil)
        {
          theInstance = [theClass new];
        }
      [theLock unlock];
      isCreated = 1;
   }
  return theInstance;
}

Because the assignment to isCreated occurs outside the locked region,  
we can be sure the compiler won't re-order it before the assignment  
to theInstance, so this implementation is safe from the issue of  
compiler re-ordering statements (if it applies to Objective-C  
code ... it probably doesn't, but may become an issue with Objective-C 
++) as well as any worries about pointer assignments not being atomic.

To the best of my knowledge, the use of a lock should force ordering  
even in the case of a multiprocessor system where each processor has  
its own local cache ... as I think the lock implementation ought to  
ensure cache consistency.


> Since I currently don't have the time to deal with this, I can only
> offer writing up a bug report.  It would be great if someone had the
> time to investigate how others have implemented test cases for
> multi-threaded issues and put a pointer on the wiki.

I've never really seen testcases for threading issues ...