[Vm-dev] Clarification needed for sqCompareAndSwapRes

[Esteban Lorenzano]

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

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 macro sqCompareAndSwapRes
> 
> 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
> 
> 
> 
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20140422/4c900f6d/attachment.htm