[Vm-dev] Re: Clarification needed for sqCompareAndSwapRes

Nicolas Cellier nicolas.cellier.aka.nice at gmail.com
Tue Apr 22 09:14:12 UTC 2014


I want to publicly apologize for sounding harsh toward Esteban.
I think he found a great bug and should be thanked and rewarded for that,
rather than bashed for a maybe incomplete fix.
It was miss-communcation from my part, and I must tell that I wish we had
more Esteban for doing the hard work on pharo-vm branch.

Nicolas

2014-04-22 1:01 GMT+02:00 Nicolas Cellier <
nicolas.cellier.aka.nice at gmail.com>:

>
> 2014-04-22 1:00 GMT+02:00 Nicolas Cellier <
> nicolas.cellier.aka.nice at gmail.com>:
>
> 2014-04-22 0:48 GMT+02:00 Nicolas Cellier <
>> nicolas.cellier.aka.nice at gmail.com>:
>>
>>
>>>
>>>
>>> 2014-04-22 0:40 GMT+02:00 Nicolas Cellier <
>>> nicolas.cellier.aka.nice at 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/7f3f991283344d4cf0aa6a50b8615416062ec381
>>
>>
>
> argh,
> https://github.com/pharo-project/pharo-vm/commit/7f3f991283344d4cf0aa6a50b8615416062ec381
> those forks are driving me nuts
>
>
>>  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-swap-MACRO-for-iphone-gcc-branch
>>>>
>>>> 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
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20140422/9dc99ae9/attachment-0001.htm


More information about the Vm-dev mailing list