[Vm-dev] Clarification needed for sqCompareAndSwapRes
Eliot Miranda
eliot.miranda at gmail.com
Tue Apr 22 23:10:03 UTC 2014
On Tue, Apr 22, 2014 at 3:51 PM, John McIntosh <
johnmci at smalltalkconsulting.com> wrote:
>
>
>
>
> 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 changed to read
# 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)
because e.g.
if (expr) sqCompareAndSwap(foo,bar,baz)
will /not/ work as intended if written as
# define sqCompareAndSwapRes(var,old,new,res) res = var; if
(OSAtomicCompareAndSwap32(old, new, &var))
But this does seem broken. How is res atomically set to var's previous
value? There's a suspension point between "res = var" and
OSAtomicCompareAndSwap32, so res could end up being assigned a stale
version of var's previous value.
> 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
> ===========================================================================
>
>
>
--
best,
Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20140422/a025e3db/attachment-0001.htm
More information about the Vm-dev
mailing list