<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2016-03-26 1:58 GMT+01: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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Nicolai,<div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Fri, Mar 25, 2016 at 2:51 AM, Nicolai Hess <span dir="ltr"><<a href="mailto:nicolaihess@gmail.com" target="_blank">nicolaihess@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"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div>2016-03-25 6:26 GMT+01: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-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Hi Nicolai,<br>
<div><div><br>
> On Mar 24, 2016, at 1:53 AM, Nicolai Hess <<a href="mailto:nicolaihess@gmail.com" target="_blank">nicolaihess@gmail.com</a>> wrote:<br>
><br>
> Hello,<br>
><br>
> we have a failing test (OCClosureCompilerTest>>#testDebuggerTempAccess)<br>
> (fails since spur, but not related - I think).<br>
><br>
> A simple example to reproduce the behavior:<br>
> | local1 remote1|<br>
> remote1:=4.<br>
> local1 :=3.<br>
> [:i | |c|<br>
> c := local1.<br>
> remote1 := i.<br>
> i halt.<br>
> "local1:=25. <-- evaluate during debugging"<br>
> ] value:1.<br>
> Transcript show:local1;cr.<br>
> Transcript show:remote1;cr.<br>
> (Executing this code and evaluating "local:=25" after the debugger halts for<br>
> "i halt" will modify the var "remote1" instead of "local1", as this is a vector tempvar , proceeding<br>
> the execution will crash at accessing the remote1 value).<br>
><br>
><br>
> The purpose of the testDebuggerTempAccess test case is, evaluating code<br>
> that changes the value of a copied temp var *does not* change the value of<br>
> this var in the outer context. (see comment in the test case:<br>
> "this is not 25 as the var is a local, non escaping variable that was copied into the block,<br>
> If the compiler would have known about the write, it would have made the var escaping".<br>
> )<br>
><br>
> But the implementation (OCCopyingTempVariable>>#writeFromContext: aContext scope: contextScope value: aValue)<br>
> explicitly iterates through all outer context(s) and changes the vars as well.<br>
><br>
> 1. Question: Who is right?<br>
<br>
</div></div>What do you mean? Because the closure model is as it is (for very good reason) the computer copies temporaries whose values cannot change during the execution of a block into that block if the block references the variable. If the debugger is to support updating such copied variables then it must create the illusion of "the variable being updated" because there isn't just one variable.<br></blockquote><div><br></div></div></div><div>I know that it only operates on a copy of this var (if the var is only read (in the original code)), but you *can* evaluate code<br></div><div>during debugging, that can modify the var.<br></div><div>I don't know why or if we want to support that. I just see a failing test case that tries to do exactly that.<br></div><div>And the supposed behavior is, that the outer context variable value does not change. </div></div></div></div></blockquote><div><br></div></div></div><div>But it would be much nicer if the debugger /did/ change all the copies of the variable it could find, right? What behaviour do we want to support? What behaviour can we afford to support?</div></div></div></div></blockquote><div><br></div><div>I think I would prefer if the debugger only changes the local copy. For me, it is like changing a local variable that shadows a variable from outer scope. Of course, you can not<br></div><div>easily see that this variable is a copy resp. shadows the one / the others from outer scope.<br><br></div><div>My main concern is that some tests fail, and you may crash pharo by changing the var from within debugger.<br></div><div> </div><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"><div class="gmail_quote"><div><div class="h5"><div><br></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>But this test fails.<br><br></div><div>I tried similar code in squeak, and there the outer context var does not change:<br><br>|local remote outerContext|<br>local :=1.<br>remote:=2.<br>outerContext := thisContext.<br>(1 to:1) do:[:i |<br> remote := i.<br> Transcript show:local;cr.<br> i halt.<br> "remote:=20"<br> "local:=10"<br> "outerContext tempAt:1 put:30"<br> Transcript show:local;cr.<br> ].<br>Transcript show:local;cr.<br>Transcript show:remote;cr.<br><br><br></div><div>If the debugger halts at "i halt" and you evaluate the code<br><br> "remote:=20"<br>the remote value changes for t he block context and for its outer context - OK<br> "local:=10"<br>the local value changes for the block context but not for its outer context - OK<br> "outerContext tempAt:1 put:30"<br></div><div>the local value only changes for the outer context - OK<br><br></div><div>Is this right?<br></div></div></div></div></blockquote><div><br></div></div></div><div>I don't think it's either right or wrong; I think it's what one would expect from a naive debugger and the current compilation semantics. I think you need to think about the problem at a tool level.</div><div><br></div><div>The right question to ask is "what behaviour would you like in the debugger?" In a debugger there are typically two interfaces that allow modifying the variables in a context. One is via a doit in which the variables are in scope, and one is an inspector on the variables embedded in a debugger. It would be nice if the two were consistent. But it is easier to make them consistent if only local values are changed. The issue is really how powerful you want to map DebuggerMap (or whatever the Pharo equivalent).</div><div><br></div><div>Here are some issues to think about. It's easy for a debugger map to work out what the right indices are for a varable that is copied in various scopes; it just asks the compiler for the relevant info. It is /not/ easy to find out all the potentially copied variables. For example, the following creates many block and many activations of those blocks, and a copy of "copied" is in all of them.</div><div><br></div><div>| copied |</div><div>copied := 7.</div><div>^(1 to: copied) collect:</div><div> [:i| [Semaphore new wait. copied] fork. [Semaphore new wait. copied]]</div><div><br></div><div>Just before the collect: terminates execution there are 9 contexts accessing copied; the context declaring it, the context for the block [:i| [Semaphore...copied]], and the 7 contexts waiting on a new semaphore. There are 15 blocks whose copiedValues includes copied, the block [:i| [Semaphore...copied]], the 7 copies of the first [Semaphore new wait. copied] block, which all have contexts whose closureOrNil points to them, and seven 7 copies of the second [Semaphore new wait. copied] block. So the only way to reliably change copy is to do allInstances on Context and BlockClosure and collect all whose home context is the context that declares copied and to change the 0th temp or the 0th copied value in each context or block. Is it worth it? Is it perhaps too expensive in some circumstances, and hence needs to be a preference? Is the preference too hard to understand or explain?</div><div><br></div><div>An alternative might be to warn the debugger user, saying something like "warning, not all references to temporary variable foo can be updated." and update only in activations on the stack.</div><div><br></div><div>Personally I would implement the all instances approach if I had the time. But I haven't had time for 8 years ;-)</div></div></div></div></blockquote><div><br></div><div>Now I really prefer to only change the local value :-)<br></div><div><br> </div><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"><div class="gmail_quote"><span class=""><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"><span><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"><span>> But the reason why the test actually fails is different. It is because evaluating<br>
> the code that modifies the var "local1:=25" ends up to a call<br>
> aContext tempAt: self indexFromIR put: aValue<br>
> but the index (indexFromIR) can be different for every context (method context / inner block<br>
> context(s)).<br>
<br>
</span>Right.<br>
<span><br>
><br>
> 2. Question: should the index be always the same?<br>
<br>
</span>How can it be? If, for example, a nullary block makes use of a copied variable and has no local temps then that variable will end up with index 0, no matter its indices in outer scopes. Surely you're not proposing padding the block with unused variables just so copied variables can have the same index?<br></blockquote><div><br></div></span><div>Yes, makes sense.<br></div><span><div> </div><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">
<span><br>
><br>
> thanks inadvance<br>
><br>
> Nicolai<br>
><br>
><br>
> ps: since spur (resp. compiler changes that were done for new spur images), the index of tempvars can be different.<br>
> In pre-spur, this testcase modifies an outer context var, but one that isn't checked (the argument "two"), therefore<br>
> the test succeed. In spur, this testcase modifes a different outer context var "remote1", and the test<br>
> fails.<br>
<br>
<br>
</span>Since Sour hasn't changed the compiler there looks to be a bug. Sour has changed the identityHash size a lot from IIRC 11 bits to 22. So hashed collections can end up with different enumerations. But that shouldn't affect the ordering assigned to temps in the compiler. I suggest tracking down why the code is different in Spur is a very important thing to do.<br></blockquote><div><br></div></span><div>Yes, we merged some code for spur support (opals compiler code) and lost some intermediate changes that were made for opal. Debugging this bugs was a mess.<br></div></div></div></div></blockquote><div><br></div></span><div>Ouch. Debugging the debugger. Always frustrating. Does Opal put the indirection vectors in the same place in each declaring scope? Also what rule does Opal use for ordering copied values? Here's the rule for Squeak's compiler:</div></div></div></div></blockquote><div><br></div><div>It is not about debugging the debugger, but finding the change that was responsible for this changed index, and to find out if it was changed on purpose<br></div><div> </div><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"><div class="gmail_quote"><div><br></div><div><div>ParseNode class>><b>tempSortBlock</b></div><div><span style="white-space:pre-wrap"> </span>"Answer a block that can sort a set of temporaries into a stable</div><div><span style="white-space:pre-wrap"> </span> order so that different compilations produce the same results."</div><div><span style="white-space:pre-wrap"> </span>^[:t1 :t2| | be1 be2 bs1 bs2 |</div><div><span style="white-space:pre-wrap"> </span> t1 index < t2 index "simple sort by index."</div><div><span style="white-space:pre-wrap"> </span> or: [t1 index = t2 index "complex tie break" </div><div><span style="white-space:pre-wrap"> </span> and: [t1 isRemote ~= t2 isRemote</div><div><span style="white-space:pre-wrap"> </span>ifTrue: [t2 isRemote] "put direct temps before indirect temps"</div><div><span style="white-space:pre-wrap"> </span>ifFalse: </div><div><span style="white-space:pre-wrap"> </span>[((be1 := t1 definingScope blockExtent) isNil</div><div><span style="white-space:pre-wrap"> </span> or: [(be2 := t2 definingScope blockExtent) isNil])</div><div><span style="white-space:pre-wrap"> </span>ifTrue: [t1 name < t2 name] "only have the name left to go on"</div><div><span style="white-space:pre-wrap"> </span>ifFalse: "put temps from outer scopes before those from inner scopes"</div><div><span style="white-space:pre-wrap"> </span>[(bs1 := be1 first) < (bs2 := be2 first)</div><div><span style="white-space:pre-wrap"> </span> or: [bs1 = bs2 and: [t1 name < t2 name]]]]]]] "only have the name left to go on"</div></div><div><br></div><div>Hence in the following:</div><div><br></div><div><div>| c1 r1 c2 r2 |</div><div>c1 := #(copied 1). c2 := #(copied 2).</div><div>r1 := #(remote 1). r2 := #(remote 2).</div><div>[ | c3 r3 |</div><div> c3 := #(copied 3).</div><div> r3 := #(remote 3).</div><div> [r1 := r1 copy. r2 := r2 copy. r3 := r3 copy.</div><div> { c1. c2. c3. thisContext shallowCopy}] value] value</div></div><div><br></div><div>Numbering the scopes 1, 2 & 3, then</div><div><br></div><div>Scope 1:</div><div>0: c1</div><div>1: c2</div><div>2: indirection1, r1@1, r2@2</div><div><br></div><div>Copied values in block for scope 2:</div><div>1: c1</div><div>2: c2</div><div>3: indirection1</div><div><br></div><div>Scope 2:</div><div><div>1: c1</div><div>2: c2</div><div>3: indirection1</div></div><div>4: c3</div><div>5: indirection2</div><div><div><br></div><div>Copied values in block for scope 3:</div></div><div>1: c1</div><div>2: c2</div><div>3: indirection1</div><div>4: c3</div><div>5: indirection2</div><div><br></div><div>Scope 3:</div><div>1: c1</div><div>2: c2</div><div>3: indirection1</div><div>4: c3</div><div>5: indirection2</div><div><br></div><div><br></div><div>Intuitively I expected</div><div><br></div><div><div>Scope 1:</div><div>0: c1</div><div>1: c2</div><div>2: indirection1, r1@1, r2@2</div><div><br></div><div>Copied values in block for scope 2:</div><div>1: c1</div><div>2: c2</div><div>3: indirection1</div><div><br></div><div>Scope 2:</div><div><div>1: c1</div><div>2: c2</div><div>3: indirection1</div></div><div>4: c3</div><div>5: indirection2</div><div><div><br></div><div>Copied values in block for scope 3:</div></div><div>1: c1</div><div>2: c2</div><div>3: c3</div>4: indirection1<div>5: indirection2</div><div><br></div><div>Scope 3:</div><div>1: c1</div><div>2: c2</div><div>3: c3</div>4: indirection1<div>5: indirection2</div></div><div><br></div><div><br></div><div>:-/</div></div></div></div></blockquote><div><br></div><div>I need more time to step through this.<br></div><div> </div><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"><div class="gmail_quote"><span class=""><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"><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">HTH<br>
<br>
Eliot<br></blockquote><div><br></div><div>thanks<br></div><span><div> </div><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">
_,,,^..^,,,_ (phone)<br>
<br>
PS if you want to understand why copied variables are so important in the closure model you need to understand context-to-stack mapping and the overhead that not copying temps adds to returns in a context-to-stack mapping VM. It's not obvious but I hope my blog does an ok job of explaining something alas complex but extremely important for performance.<br></blockquote><div><br></div></span><div>I read your blog </div></div></div></div></blockquote><div><br></div></span><div>Good. So then ask yourself what the right thing to do is, and what the affordable thing to do is. You understand the issue, now you have to make some decisions...</div></div></div></div></blockquote><div><br></div><div>I do not yet see use cases in which one or the other would be better or right.<br></div><div>Therefore I would go for the solution that is easier to implement resp. fix.<br></div><div><br> </div><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"><div class="gmail_quote"><div><br></div></div><div><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div>
</div></div>
<br><br>
<br></blockquote></div><br></div></div>