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

Eliot Miranda eliot.miranda at gmail.com
Tue May 1 20:34:03 UTC 2012


On Tue, May 1, 2012 at 1:12 PM, Mariano Martinez Peck <marianopeck at gmail.com
> wrote:

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

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


More information about the Vm-dev mailing list