[Vm-dev] Clarification needed for sqCompareAndSwapRes

Tue Apr 22 22:12:56 UTC 2014

Hi Esteban,

On Tue, Apr 22, 2014 at 2:40 PM, Esteban Lorenzano <estebanlm at gmail.com>wrote:

>
>
> On 22 Apr 2014, at 23:34, Nicolas Cellier <
> nicolas.cellier.aka.nice at gmail.com> wrote:
>
>
> 2014-04-22 12:47 GMT+02:00 Esteban Lorenzano <estebanlm at gmail.com>:
>
>>
>> thanks Nicolas for your words.
>> now… let’s go back to our common passion: to help this community on
>> having the best vm possible :)
>>
>> Esteban
>>
>>
> Nah, concerning the best vm I feel disqualified, I let Eliot care of it ;)
>
> We just want a better VM with a few hacks, corrections and support for
> some specific Pharo libraries/extensions.
> Eliot, do you agree that the sqCompareAndSwapRes provided #ifdefTARGET_OS_IS_IPHONE is buggish (see below)?
>
>
To be honest I don't know.  I don't have an iPhone development library
handy.  John McIntosh write the code and he's more than competent.  I'd
rather leave this to others with experience of the platform to check.  I
/think/ the desired semantics are clear enough.  If they're not I can fix
that.  But I want to leave writing the correct code for the iPhone to those
that are actually using the platform.  Forgive me.

> Do you agree with my proposed fix, or have a better idea?
>
>
> I will test it on iPad as soon as I can (busy days)… if that helps :)
> can you propose a pull request?
>
> cheers,
> Esteban
>
>
> Thanks
>
>
>> On 22 Apr 2014, at 11:14, Nicolas Cellier <
>> nicolas.cellier.aka.nice at gmail.com> wrote:
>>
>> 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/38 for 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>>
>>
>
>
>

-- 
best,
Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20140422/9da39168/attachment-0001.htm