<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">thanks Nicolas for your words. <div>now… let’s go back to our common passion: to help this community on having the best vm possible :)</div><div><br></div><div>Esteban</div><div><br><div><div>On 22 Apr 2014, at 11:14, Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com">nicolas.cellier.aka.nice@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">I want to publicly apologize for sounding harsh toward Esteban.<br></div><div class="gmail_extra">I think he found a great bug and should be thanked and rewarded for that, rather than bashed for a maybe incomplete fix.<br>
</div><div class="gmail_extra">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.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">
Nicolas<br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_quote">2014-04-22 1:01 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">2014-04-22 1:00 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>:<div>
<div class="h5"><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 class="gmail_extra"><div class="gmail_quote">2014-04-22 0:48 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>:<div>
<div><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"><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>:<div>
<div><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>In platforms/Cross/vm/sqAtomicOps.h a comment tells<br><br><span>* sqCompareAndSwapRes(var,old,new,res) arranges atomically that if var's value</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><br> * is set to the previous value of var.</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>#if defined(__GNUC__) && (defined(i386) || defined(__i386) || defined(__i386__) || defined(_X86_))</span></li>
<li><span> </span></li><li><span>#ifdef TARGET_OS_IS_IPHONE</span></li><li><span># define sqCompareAndSwap(var,old,new) OSAtomicCompareAndSwap32(old, new, &var) </span></li>
<li><span>/* N.B. This is not atomic in fetching var's old value :( */</span></li><li><span># define sqCompareAndSwapRes(var,old,new,res) do { res = var; if (OSAtomicCompareAndSwap32(old, new, &var)) res = new; } while (0)</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></blockquote></div></div>
<div>ahem, of course, the arguments are not in the same order:<br>
while (! OSAtomicCompareInt32( 0, 1, &sigLock))<br><br></div><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> sqCompareAndSwapRes<br></span></div></div></blockquote></div></div></div></div></blockquote><div><br></div></div></div><div>
Here is a ref to the "fix":<br>
<br><a href="https://github.com/nicolas-cellier-aka-nice/pharo-vm/commit/7f3f991283344d4cf0aa6a50b8615416062ec381" target="_blank">https://github.com/nicolas-cellier-aka-nice/pharo-vm/commit/7f3f991283344d4cf0aa6a50b8615416062ec381</a><br>
<br></div></div></div></div></blockquote><div><br></div></div></div><div>argh, <a href="https://github.com/pharo-project/pharo-vm/commit/7f3f991283344d4cf0aa6a50b8615416062ec381" target="_blank">https://github.com/pharo-project/pharo-vm/commit/7f3f991283344d4cf0aa6a50b8615416062ec381</a><br>
</div><div>those forks are driving me nuts<br> <br></div><div class=""><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 class="gmail_extra">
<div class="gmail_quote">
<div></div><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 class="gmail_extra"><div class="gmail_quote"><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><span>
</span></div><div><span>But IMO, this is wrong, as I understand it, it completely desactivates the lock...<br><br></span></div><div><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></div><div><span><br>and pulled a request </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></div><div><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></div><div><span>Do </span><span>you agree that TARGET_OS_IS_IPHONE branch is wrong?<br>
</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><div>maybe<br>
<br><pre><li><span># define sqCompareAndSwapRes(var,old,new,res) do { res = var; if (OSAtomicCompareAndSwap32(old, new, &var)) res = old; } while (0)</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>Thanks<span><font color="#888888"><br><br></font></span></div><span><font color="#888888"><div>Nicolas<br></div>
<div><br></div></font></span></div>
</blockquote></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div><div class="gmail_extra"><br></div></div>
</blockquote></div><br></div></body></html>