[Vm-dev] Clarification needed for sqCompareAndSwapRes

John McIntosh johnmci at smalltalkconsulting.com
Tue Apr 22 22:51:36 UTC 2014


On Tue, Apr 22, 2014 at 3:21 PM, Eliot Miranda <eliot.miranda at gmail.com>wrote:

>
>
>
>
> On Tue, Apr 22, 2014 at 3:18 PM, Nicolas Cellier <
> nicolas.cellier.aka.nice at gmail.com> wrote:
>
>>
>>
>> 2014-04-23 0:12 GMT+02:00 Eliot Miranda <eliot.miranda at gmail.com>:
>>
>>>
>>> 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.
>>>
>>>
>>
>> OK, thanks Eliot, then I'll trust what the comments say.
>> If John is able to confirm that'll be better, but I think I get a good
>> picture of the problem now.
>> I cannot build either, but once Esteban can test it and if it works OK,
>> then we'll propose a fix for integration in your svn branch, sounds right?
>>
>
> perfect, thanks !
>

Well this is all very interesting, since the original version I sent to
Eliot dated from July 2010, and it was

# define sqCompareAndSwapRes(var,old,new,res) res = var;
OSAtomicCompareAndSwap32(old, new, &var)


which I believe matched the spirit of:

 * 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.


Also consider this, but ensure you have the correct version which I don't
know


http://opensource.apple.com/source/Libc/Libc-594.1.4/arm/sys/OSAtomic-v4.c



As far as I know the old code works as expected on iOS 7 so I"m wondering
if the story about what this function should do matches the intel
assembler?


>
>>
>>  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
>>>
>>>
>>
>>
>
>
> --
> best,
> Eliot
>
>


-- 
===========================================================================
John M. McIntosh <johnmci at smalltalkconsulting.com>
Corporate Smalltalk Consulting Ltd. Twitter: squeaker68882
===========================================================================
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20140422/672194e4/attachment-0001.htm


More information about the Vm-dev mailing list