[Vm-dev] Re: [squeak-dev] Re: [Pharo-project] problem with tempNamed: in Pharo 2.0

Tue May 1 20:12:57 UTC 2012

On Tue, May 1, 2012 at 9:29 PM, Eliot Miranda <eliot.miranda at gmail.com>wrote:

>
>
> On Tue, May 1, 2012 at 9:13 AM, Mariano Martinez Peck <
> marianopeck at gmail.com> wrote:
>
>>
>>
>> On Tue, May 1, 2012 at 6:09 PM, Mariano Martinez Peck <
>> marianopeck at gmail.com> 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

> 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?

>
> 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:   ?

>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20120501/29196330/attachment-0001.htm