[Vm-dev] Re: [squeak-dev] Re: [Pharo-project] problem with
tempNamed: in Pharo 2.0
Mariano Martinez Peck
marianopeck at gmail.com
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:
>>> 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 #>= ?
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:
"Answer a SequenceableCollection of the names of the receiver's
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
> 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:
"Answer the value of the temp at index in the receiver's sequence of
^self debuggerMap namedTempAt: index in: self
namedTempAt: index put: aValue
"Set the value of the temp at index in the receiver's sequence of
(Note that if the value is a copied value it is also set out along the
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:
> "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
> | sum first |
> first := true.
> contents keysAndValuesDo: [ :value :count |
> ifTrue: [ sum := value * count. first := false ]
> ifFalse: [ sum := sum + (value * count) ] ].
> first ifTrue: [ self errorEmptyCollection ].
> 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)
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the Vm-dev