2014-04-22 0:48 GMT+02:00 Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com>:
2014-04-22 0:40 GMT+02:00 Nicolas Cellier < nicolas.cellier.aka.nice@gmail.com>:
In platforms/Cross/vm/sqAtomicOps.h a comment tells
- sqCompareAndSwapRes(var,old,new,res) arranges atomically that if var's
value
- is equal to old, then var's value is set to new, and that in any case,
res
- is set to the previous value of var.
But for some implementation, if var==old, res is first set to var (old) then to new which is the new value of var:
#if defined(__GNUC__) && (defined(i386) || defined(__i386) || defined(__i386__) || defined(_X86_)) #ifdef TARGET_OS_IS_IPHONE # define sqCompareAndSwap(var,old,new) OSAtomicCompareAndSwap32(old, new, &var) /* N.B. This is not atomic in fetching var's old value :( */ # define sqCompareAndSwapRes(var,old,new,res) do { res = var; if (OSAtomicCompareAndSwap32(old, new, &var)) res = new; } while (0)
Indeed, comparedAndSwap32 will answer true if var==old as explained here https://www.mikeash.com/pyblog/friday-qa-2011-03-04-a-tour-of-osatomic.html
That is going to be a problem in lockSignalQueue in ./platforms/Cross/vm/sqExternalSemaphores.c because variable old will then be 1 in all cases and the loop won't exit once the lock acquired:
static inline void lockSignalQueue() { volatile int old; /* spin to obtain a lock */
do { sqLowLevelMFence(); sqCompareAndSwapRes(sigLock, 0, 1, old ); } while (old != 0);}
Knowing we are on iphone, I would just write it:
static inline void lockSignalQueue() { /* spin to obtain a lock */ do { sqLowLevelMFence(); } while (! OSAtomicCompareInt32(sigLock, 0, 1)); }
ahem, of course, the arguments are not in the same order: while (! OSAtomicCompareInt32( 0, 1, &sigLock))
Esteban "fixed" that by swapping the arguments in the macrosqCompareAndSwapRes
Here is a ref to the "fix":
https://github.com/nicolas-cellier-aka-nice/pharo-vm/commit/7f3f991283344d4c...
But IMO, this is wrong, as I understand it, it completely desactivates the
lock...
I did not know about the problem, but found the diff with Eliot's branch very suspicious so I opened
https://pharo.fogbugz.com/f/cases/13199/Stinky-argument-inversion-of-atomic-...
and pulled a request https://github.com/pharo-project/pharo-vm/pull/38for restoring (the supposedly wrong) Eliot's version.
Now I need some guru advice, because this kind of code is brainfucking (especially when old in sender is not old in sendee).
Do you agree that TARGET_OS_IS_IPHONE branch is wrong? Do you agree that Esteban's fix is wrong? What do you suggest for a fix?
maybe
# define sqCompareAndSwapRes(var,old,new,res) do { res = var; if (OSAtomicCompareAndSwap32(old, new, &var)) res = old; } while (0)
Thanks
Nicolas