<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-04-22 0:40 GMT+02:00 Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div>In platforms/Cross/vm/sqAtomicOps.h a comment tells<br><br><span><span>* sqCompareAndSwapRes(var,old,new,res) arranges atomically that if var's value</span></span><span><span><br>
* is equal to old, then var's value is set to new, and that in any case, res</span></span><span><span></span></span><span><span><br> * is set to the previous value of var.</span></span><span></span><span></span><br>
<br></div>But for some implementation, if var==old, res is first set to var (old) then to new which is the new value of var:<br><br><pre><li><span><span>#if defined(__GNUC__) && (defined(i386) || defined(__i386) || defined(__i386__) || defined(_X86_))</span></span></li>
<li><span> </span></li><li><span><span>#ifdef TARGET_OS_IS_IPHONE</span></span></li><li><span><span># define sqCompareAndSwap(var,old,new) OSAtomicCompareAndSwap32(old, new, &var) </span></span></li>
<li><span><span>/* N.B. This is not atomic in fetching var's old value :( */</span></span></li><li><span><span># define sqCompareAndSwapRes(var,old,new,res) do { res = var; if (OSAtomicCompareAndSwap32(old, new, &var)) res = new; } while (0)</span></span></li>
</pre><br>Indeed, comparedAndSwap32 will answer true if var==old as explained here <a href="https://www.mikeash.com/pyblog/friday-qa-2011-03-04-a-tour-of-osatomic.html" target="_blank">https://www.mikeash.com/pyblog/friday-qa-2011-03-04-a-tour-of-osatomic.html</a><br>
<br>That is going to be a problem in lockSignalQueue in ./platforms/Cross/vm/sqExternalSemaphores.c<br>because variable old will then be 1 in all cases and the loop won't exit once the lock acquired:<br><br>static inline void lockSignalQueue()<br>
{<br> volatile int old;<br> /* spin to obtain a lock */<br><br> do {<br> sqLowLevelMFence();<br> sqCompareAndSwapRes(sigLock, 0, 1, old );<br> } while (old != 0);<br><br>}<br><br>Knowing we are on iphone, I would just write it:<br>
<br>static inline void lockSignalQueue()<br>{<br> /* spin to obtain a lock */<br> do {<br> sqLowLevelMFence();<br> } while (! OSAtomicCompareInt32(sigLock, 0, 1));<br>}<br></div></div></blockquote><div>ahem, of course, the arguments are not in the same order:<br>
while (! OSAtomicCompareInt32( 0, 1, &sigLock))<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br>Esteban "fixed" that by swapping the arguments in the macro<span><span> sqCompareAndSwapRes<br>
</span></span></div><div><span><span>But IMO, this is wrong, as I understand it, it completely desactivates the lock...<br><br></span></span></div><div><span><span>I did not know about the problem, but found the diff with Eliot's branch very suspicious so I opened<br>
<a href="https://pharo.fogbugz.com/f/cases/13199/Stinky-argument-inversion-of-atomic-swap-MACRO-for-iphone-gcc-branch" target="_blank">https://pharo.fogbugz.com/f/cases/13199/Stinky-argument-inversion-of-atomic-swap-MACRO-for-iphone-gcc-branch</a><br>
</span></span></div><div><span><span><br>and pulled a request </span></span><span><span><a rel="nofollow" href="https://github.com/pharo-project/pharo-vm/pull/38" target="_blank">https://github.com/pharo-project/pharo-vm/pull/38</a> for restoring (the supposedly wrong) Eliot's version.<br>
<br></span></span></div><div><span><span>Now I need some guru advice, because this kind of code is brainfucking (especially when old in sender is not old in sendee).<br><br></span></span></div><div><span><span>Do </span></span><span><span>you agree that TARGET_OS_IS_IPHONE branch is wrong?<br>
</span></span><span><span></span></span></div><div><div>Do you agree that Esteban's fix is wrong?<br></div><div>What do you suggest for a fix?<br><br></div></div></div></blockquote><div><br></div><div>maybe<br><br><pre>
<li><span><span># define sqCompareAndSwapRes(var,old,new,res) do { res = var; if (OSAtomicCompareAndSwap32(old, new, &var)) res = old; } while (0)</span></span></li></pre> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div><div></div><div>Thanks<span class=""><font color="#888888"><br><br></font></span></div><span class=""><font color="#888888"><div>Nicolas<br></div>
<div><br></div></font></span></div></div>
</blockquote></div><br></div></div>