On Tue, May 1, 2012 at 1:12 PM, Mariano Martinez Peck <[email protected] > wrote:
> > > > On Tue, May 1, 2012 at 9:29 PM, Eliot Miranda <[email protected]>wrote: > >> >> >> On Tue, May 1, 2012 at 9:13 AM, Mariano Martinez Peck < >> [email protected]> wrote: >> >>> >>> >>> On Tue, May 1, 2012 at 6:09 PM, Mariano Martinez Peck < >>> [email protected]> wrote: >>> >>>> Hi guys. I noticed stef did this issue: >>>> http://code.google.com/p/pharo/issues/detail?id=5642 >>>> However, now I have the following test that fails in Pharo 2.0 but >>>> works fine in 1.4: >>>> >>>> | string context | >>>> string := 'test'. >>>> context := [self class. string asUppercase] asContext. >>>> self assert: (context tempNamed: 'string') = 'test' >>>> >>>> the current implementation of #tempNamed: is: >>>> >>>> tempNamed: aName >>>> "Returns the value of the temporaries, aName." >>>> "Implementation notes: temporary initialization in blocks simply >>>> uses pushNil to allocate and initialize each temp. So if one inspects >>>> [|a|a:=2] and sends it self method symbolic you get: >>>> >>>> 13 <8F 00 00 05> closureNumCopied: 0 numArgs: 0 bytes 17 to 21 >>>> 17 <73> pushConstant: nil >>>> 18 <77> pushConstant: 2 >>>> 19 <81 40> storeIntoTemp: 0 >>>> 21 <7D> blockReturn >>>> 22 <7C> returnTop >>>> >>>> And when we check self asContext pc we get 17, which is *before* >>>> the nil is pushed. Therefore we should pay attention when querying a >>>> temporary if the temporary allocation was executed." >>>> >>>> | index | >>>> index := (self tempNames indexOf: aName). >>>> ^ index >= stackp >>>> >>> >>> >>> Maybe the solution is to use #> rather than #>= ? >>> >> >> right. >> > > Ok, so I will integrate that as a first version. > > >> But tempNames is fundamentally broken for closures. >> > > Just to avoid confusing, #tempNames itself looks correct: > > tempNames > "Answer a SequenceableCollection of the names of the receiver's > temporary > variables, which are strings." > > ^ self debuggerMap tempNamesForContext: self > Yes. That's for ContextPart. But it doesn't work for CompiledMethod, since temps may differ between a method and its blocks. > > >> It does not answer temps in indirection vectors. That is the whole point >> of schematicTempNamesString; it gives the topology of temps in a method. >> e.g. (where => means printIt returns...) >> >> (Collection>>#inject:into:) methodNode schematicTempNamesString => >> 'thisValue binaryBlock (nextValue)[each binaryBlock (nextValue)]' >> >> This says that >> a) at method level there are three temps, thisValue, binaryBlock and an >> indirection vector, and in the indirection vector is one temp named >> nextValue. >> b) in the block in inject:into: there are three temps, each (the >> argument), binaryBlock and an indirection vector, and in that indirection >> vector is a temp named nextValue. >> >> This is all orchestrated by DebuggerMethodMap, so that >> >> aContext method debuggerMap tempNamesForContext: aContext >> > > So #tempNames implementation is correct? > Yes. > > >> >> answers a list of the flattened temp names in a context (flattening out >> indirection vectors) and for the above would answer either #('thisValue' >> 'binaryBlock' 'nextValue') or #('each' 'binaryBlock' 'nextValue'), and >> >> | map | >> map := aContext method debuggerMap. >> map namedTempAt: ((map tempNamesForContext: aContext) indexOf: >> aTempName) in: aContext >> >> gets the temp from the unflattened temps in a context. This is how the >> debugger accesses temp names. >> >> So you need to throw away the broken tempName: implementation and use >> DebuggerMethodMap or some parse over schematicTempNamesString, because >> *with closures temps are not simply at contiguous offsets on the stack*. >> Make sense? >> > > Actually, we do have: > > namedTempAt: index > "Answer the value of the temp at index in the receiver's sequence of > tempNames." > ^self debuggerMap namedTempAt: index in: self > > and > > namedTempAt: index put: aValue > "Set the value of the temp at index in the receiver's sequence of > tempNames. > (Note that if the value is a copied value it is also set out along > the lexical chain, > but alas not in along the lexical chain.)." > ^self debuggerMap namedTempAt: index put: aValue in: self > > so, if I understand correctly all we need to do is to fix tempNamed: and > tempNamedPut: so that the delegate to namedTempAt: and namedTempPut: rather > than to tempAt: and tempAtPut: ? > Yes. Now, one good way to understand this is through examples. inject:into: is >> the canonical simple example I've used for years. But we can find more >> complex examples that may help. So this query looks for all methods that >> contain a bytecode to create an indirection vector with 2 or more elements: >> >> SystemNavigation new browseAllSelect: >> [:m| | is | >> is := InstructionStream on: m. >> is scanFor: [:b| b = 138 and: [is followingByte between: 2 and: 127]]] >> >> and the simplest example in a trunk 4.3 image I find is: >> >> Bag>>sum >> "Faster than the superclass implementation when you hold many instances >> of the same value (which you probably do, otherwise you wouldn't be using a >> Bag)." >> | sum first | >> first := true. >> contents keysAndValuesDo: [ :value :count | >> first >> ifTrue: [ sum := value * count. first := false ] >> ifFalse: [ sum := sum + (value * count) ] ]. >> first ifTrue: [ self errorEmptyCollection ]. >> ^sum >> >> which needs a two-element indirection vector because the block [ :value >> :count |...] assigns to both sum and first. Hence >> >> (Bag>>#sum) methodNode schematicTempNamesString => '(sum >> first)[value count (sum first)]' >> >> So in an activation of Bag>>sum the value of first is (sumContext tempAt: >> 1) at: 2 (cuz tempAt: 1 is the indirection vector represented as (sum >> first) in schematic temps, and first is the second element. But in an >> activation of the block in Bag>>sum the value of first is (sumBlockContext >> tempAt: 3) at: 2. >> >> >> I can keep repeating myself until I'm blue in the face, but y'all have to >> put in the effort to understand this simple scheme. Temps that are >> modified after they are closed-over end up in indirection vectors. >> >> > > Thanks, I am trying to undersand and fix it :) > > >> >>> >>> >>>> ifTrue: [ nil] >>>> ifFalse: [self tempAt: (self tempNames indexOf: aName)] >>>> >>>> >>>> and previously it was: >>>> >>>> tempNamed: aName >>>> ^self tempAt: (self tempNames indexOf: aName) >>>> >>>> >>>> ideas? >>>> -- >>>> Mariano >>>> http://marianopeck.wordpress.com >>>> >>> -- >>> Mariano >>> http://marianopeck.wordpress.com >>> >> -- >> best, >> Eliot >> > -- > Mariano > http://marianopeck.wordpress.com -- best, Eliot
