<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 22, 2014 at 4:43 PM, John McIntosh <span dir="ltr"><<a href="mailto:johnmci@smalltalkconsulting.com" target="_blank">johnmci@smalltalkconsulting.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
On Tue, Apr 22, 2014 at 4:10 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br>
<div class="gmail_quote">
On Tue, Apr 22, 2014 at 3:51 PM, John McIntosh <span dir="ltr"><<a href="mailto:johnmci@smalltalkconsulting.com" target="_blank">johnmci@smalltalkconsulting.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br>
<div class="gmail_quote">
On Tue, Apr 22, 2014 at 3:21 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><div dir="ltr"><br><div class="gmail_extra"><br><br>
<div class="gmail_quote">
On Tue, Apr 22, 2014 at 3:18 PM, Nicolas Cellier <span dir="ltr"><<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">
2014-04-23 0:12 GMT+02:00 Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><div dir="ltr">Hi Esteban,<br><div class="gmail_extra">
<br><br><div class="gmail_quote">On Tue, Apr 22, 2014 at 2:40 PM, Esteban Lorenzano <span dir="ltr"><<a href="mailto:estebanlm@gmail.com" target="_blank">estebanlm@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br><div style="word-wrap:break-word"><br><div><div>On 22 Apr 2014, at 23:34, Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">2014-04-22 12:47 GMT+02:00 Esteban Lorenzano <span dir="ltr"><<a href="mailto:estebanlm@gmail.com" target="_blank">estebanlm@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"> <br><div style="word-wrap:break-word">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></blockquote><div><br></div><div>Nah, concerning the best vm I feel disqualified, I let Eliot care of it ;)<br></div></div></div></div></blockquote><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>We just want a better VM with a few hacks, corrections and support for some specific Pharo libraries/extensions.<br>
</div><div>Eliot, do you agree that the<span> sqCompareAndSwapRes provided #ifdef</span><span> TARGET_OS_IS_IPHONE is buggish (see below)?<br></span></div></div></div></div></blockquote></div></div></blockquote><div><br>
</div><div>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.</div>
<div> </div></div></div></div></blockquote><div><br></div><div>OK, thanks Eliot, then I'll trust what the comments say.<br>If John is able to confirm that'll be better, but I think I get a good picture of the problem now.<br>
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?<br></div></div></div></div></blockquote><div><br></div><div>perfect, thanks !</div>
</div></div></div></blockquote><div><br></div><div>Well this is all very interesting, since the original version I sent to Eliot dated from July 2010, and it was</div><div><br></div><div><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(120,73,42)">
# define sqCompareAndSwapRes(var,old,new,res) res = var; OSAtomicCompareAndSwap32(old, new, &var) </p></div></div></div></div></blockquote><div><br></div><div>which I changed to read</div><div><br></div><div># define sqCompareAndSwap(var,old,new) OSAtomicCompareAndSwap32(old, new, &var)</div>
<div>/* N.B. This is not atomic in fetching var's old value :( */</div><div># define sqCompareAndSwapRes(var,old,new,res) do { res = var; if (OSAtomicCompareAndSwap32(old, new, &var)) res = new; } while (0)</div>
<div><br></div><div>because e.g.</div><div><br></div><div> if (expr) sqCompareAndSwap(foo,bar,baz)</div><div><br></div><div>will /not/ work as intended if written as</div><div><br></div><div># define sqCompareAndSwapRes(var,old,new,res) res = var; if (OSAtomicCompareAndSwap32(old, new, &var))<br>
</div><div><br></div><div>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.</div>
<div><br></div></div></div></div></blockquote><div><br></div><div>? <span style="color:rgb(0,132,0);font-family:Menlo;font-size:11px">in any case, res</span></div><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(120,73,42)">
</p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)"> * is set to the previous value of var.</p><div><br></div><div>Er so yes res would get the previous value of var. Assuming it's not stale, do you mean it could be stale where two processes hit the semaphore and res is not atomically set correctly? </div>
</div></div></div></blockquote><div><br></div><div>Yes. Here's the timeline:</div><div><br></div><div>Var: 0</div><div><span class="" style="white-space:pre"> </span>P1<span class="" style="white-space:pre"> </span><span style="white-space:pre"> </span><span style="white-space:pre"> </span><span style="white-space:pre"> P2</span></div>
<div>res := var (res = 0)</div><div><span style="white-space:pre"> (suspended)</span></div><div><span style="white-space:pre"> </span><span style="white-space:pre"> </span><span style="white-space:pre"> </span><span style="white-space:pre"> </span><span style="white-space:pre"> res := var (now res = 0)</span></div>
<div><span style="white-space:pre"> </span><span style="white-space:pre"> </span><span style="white-space:pre"> </span><span style="white-space:pre"> </span><span style="white-space:pre"> </span>OSAtomicCompareAndSwap32(0,2,&var) (now var = 2)</div>
<div>OSAtomicCompareAndSwap32(0,2,&var)<br></div><div>(still var = 2)</div><div>res = 0, but should = 2</div><div><br></div><div>And of course there are lots of other possible interleavings.</div><div><br></div><div><br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>Or does the intel assembler match the story?</div></div></div></div></blockquote><div><br></div><div><blush>no</blush>. It suffers exactly the same flaw.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(120,73,42)"><br></p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(120,73,42)">
which I believe matched the spirit of:</p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)"> * sqCompareAndSwapRes(var,old,new,res) arranges atomically that if var's value</p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)">
* is equal to old, then var's value is set to new, and that in any case, res</p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(120,73,42)">
</p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)"> * is set to the previous value of var.</p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)"><br></p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)">
Also consider this, but ensure you have the correct version which I don't know</p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)"><br></p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)">
<a href="http://opensource.apple.com/source/Libc/Libc-594.1.4/arm/sys/OSAtomic-v4.c" target="_blank">http://opensource.apple.com/source/Libc/Libc-594.1.4/arm/sys/OSAtomic-v4.c</a><br></p><p style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(0,132,0)">
<br></p></div><div> </div><div>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? </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><span></span></div><div><span>Do you agree with my proposed fix, or have a better idea?</span></div></div></div></div></blockquote><br><div>I will test it on iPad as soon as I can (busy days)… if that helps :)</div>
<div>
can you propose a pull request?</div><div><br></div><div>cheers, </div><div>Esteban</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span>
<br></span></div><div><span>Thanks<br></span></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word">
<div><div><div>On 22 Apr 2014, at 11:14, Nicolas Cellier <<a href="mailto:nicolas.cellier.aka.nice@gmail.com" target="_blank">nicolas.cellier.aka.nice@gmail.com</a>> wrote:</div><br><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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;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></div><br></blockquote></div><br></div></div>
</blockquote></div><br></div><br></blockquote></div><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div>
</div></div>
<br></blockquote></div><br></div></div>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div>
</div></div>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>===========================================================================<br>John M. McIntosh <<a href="mailto:johnmci@smalltalkconsulting.com" target="_blank">johnmci@smalltalkconsulting.com</a>><br>
Corporate Smalltalk Consulting Ltd. Twitter: squeaker68882<br>===========================================================================<br><br>
</div></div>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div>
</div></div>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>===========================================================================<br>John M. McIntosh <<a href="mailto:johnmci@smalltalkconsulting.com" target="_blank">johnmci@smalltalkconsulting.com</a>><br>
Corporate Smalltalk Consulting Ltd. Twitter: squeaker68882<br>===========================================================================<br><br>
</div></div>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>best,<div>Eliot</div>
</div></div>