On Tue, May 1, 2012 at 9:13 AM, Mariano Martinez Peck <marianopeck@gmail.com
wrote:
On Tue, May 1, 2012 at 6:09 PM, Mariano Martinez Peck < marianopeck@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* thenil is pushed. Therefore we should pay attention when querying a temporary if the temporary allocation was executed."
| index | index := (self tempNames indexOf: aName). ^ index >= stackpMaybe the solution is to use #> rather than #>= ?
right. But tempNames is fundamentally broken for closures. 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
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?
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.
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
On Tue, May 1, 2012 at 9:29 PM, Eliot Miranda eliot.miranda@gmail.comwrote:
On Tue, May 1, 2012 at 9:13 AM, Mariano Martinez Peck < marianopeck@gmail.com> wrote:
On Tue, May 1, 2012 at 6:09 PM, Mariano Martinez Peck < marianopeck@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* thenil is pushed. Therefore we should pay attention when querying a temporary if the temporary allocation was executed."
| index | index := (self tempNames indexOf: aName). ^ index >= stackpMaybe 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:
- 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
On Tue, May 1, 2012 at 1:12 PM, Mariano Martinez Peck <marianopeck@gmail.com
wrote:
On Tue, May 1, 2012 at 9:29 PM, Eliot Miranda eliot.miranda@gmail.comwrote:
On Tue, May 1, 2012 at 9:13 AM, Mariano Martinez Peck < marianopeck@gmail.com> wrote:
On Tue, May 1, 2012 at 6:09 PM, Mariano Martinez Peck < marianopeck@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 >= stackpMaybe 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: aContextSo #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:
- 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
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: selfYes. That's for ContextPart. But it doesn't work for CompiledMethod, since temps may differ between a method and its blocks.
Right, but weren't we always talking about Contexts? Because #tempNames of CompiledMethod is different than the thing of contexts, isn't it? It is related to the method trailer. In fact, I have just checked Squeak 4.3 and CompiledMethod does not even implement #tempNames. The implementation in Pharo is:
CompiledMethod >> #tempNames self holdsTempNames ifFalse: [^#()]. ^self tempNamesString subStrings: ' '
how would it be the correct implementation then?
Thanks
On Tue, May 1, 2012 at 1:53 PM, Mariano Martinez Peck <marianopeck@gmail.com
wrote:
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: selfYes. That's for ContextPart. But it doesn't work for CompiledMethod, since temps may differ between a method and its blocks.
Right, but weren't we always talking about Contexts? Because #tempNames of CompiledMethod is different than the thing of contexts, isn't it? It is related to the method trailer. In fact, I have just checked Squeak 4.3 and CompiledMethod does not even implement #tempNames. The implementation in Pharo is:
CompiledMethod >> #tempNames self holdsTempNames ifFalse: [^#()]. ^self tempNamesString subStrings: ' '
how would it be the correct implementation then?
I don't think there is one. Perhaps answering a set of all temp names, but that's not very useful.
Thanks
-- Mariano http://marianopeck.wordpress.com
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 I was thinking, what happens with the rest of the senders of tempAt: and tempAtPut: ? do I need to do something with them?
Thanks
On Wed, May 2, 2012 at 12:31 AM, Mariano Martinez Peck < marianopeck@gmail.com> wrote:
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 I was thinking, what happens with the rest of the senders of tempAt: and tempAtPut: ? do I need to do something with them?
Yes for those clients that assume temps are linear...
Thanks
-- Mariano http://marianopeck.wordpress.com
On Wed, May 2, 2012 at 12:31 AM, Mariano Martinez Peck < marianopeck@gmail.com> wrote:
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 I was thinking, what happens with the rest of the senders of tempAt: and tempAtPut: ? do I need to do something with them?
Let me answer more carefully. It depends. For example, uses of tempAt: in ContextPart used to simulate the bytecode set are correct. Uses in the Debugger are likely correct. When I look in Squeal trunk 4.3 I an see all but one uses are correct and one, DiskProxy>comeUpFullyOnReload:, is just scary:
(DataStream compiledMethodAt: #readArray)]) ifTrue: [ arrayIndex := (thisContext sender sender sender sender) tempAt: 4.
:)
Thanks
-- Mariano http://marianopeck.wordpress.com
vm-dev@lists.squeakfoundation.org
-
Eliot Miranda -
Mariano Martinez Peck