Squeak4.2-10892-beta image

On Thu, 13 Jan 2011, Nicolas Cellier wrote:

Follow up on unused temps :

Wanna play with temps ? Try this :

testUnusedVariable
       self
               compiling: 'griffle | goo | ^nil'
               shouldRaise: UnusedVariable;
               compiling: 'griffle | | ^[ | goo | ]'
               shouldRaise: UnusedVariable;
               compiling: 'griffle | | [ | goo | goo := nil. goo] yourself. ^[ | goo | ]'
               shouldRaise: UnusedVariable;
               compiling: 'griffle | | [ | goo | ] yourself. ^[ | goo | goo := nil. goo]'
               shouldRaise: UnusedVariable

What happens is that Parser.scopeTable has a single entry for 'goo'.
Consequently, the last registered goo wins.
In the 3rd case, the last goo is unused, so hasRef = false, so
UnusedVariable is raised.
In the 4th case, the last goo nowHasRef, and we don't get any UnusedVariable.

Until we get a scopeTable mapping each temp of each block, we gonna
live in trouble.

Right. I'd probably create a separate class for the Encoder's scopeTable, because it's accessed in various ways from several places and it's hard to see what change will have side effects in the current code. But I'm a total newbie in this area, so I may be wrong.

Levente

Nicolas

2011/1/12 Nicolas Cellier <[hidden email]>:

2011/1/12 Levente Uzonyi <[hidden email]>:

On Wed, 12 Jan 2011, Chris Muller wrote:

I've just posted a new beta image at:

 ftp://ftp.squeak.org/4.2/

This one does not have wrecked PackageInfo's, and has most (but not
all) of the aesthetic changes for 4.2.

The test suite is looking better:

'CompilerExceptionsTest>>#testUnusedVariable

This one is new, I'd be happy if someone could have a look at it.

This is due to latest improvements from Eliot.
Now the BlockNode have their own tempsMark (set by
Parser>>#temporaryBlockVariablesFor:).
The Parser only records top level tempsMark in Parser>>#temporaries.

If you look at the code in Parser>>#removeUnusedTemps, you see some
defensive programming
       ((tempsMark between: 1 and: str size)
               and: [(str at: tempsMark) = $|]) ifFalse: [^ self].

But hey, the top level does not have any tempsMark, so the method
prematurely returns without checking for some unused in the blocks.
self
       justTriedCompiling: 'griffle | | ^[ | goo | ]'
       andItCorrectlyRaised: UnusedVariable.

Shall we remove the tempsMark protection ?

Nicolas

Here's another decompiler problem that I actually ran into in production code. When decompiling this method, either the block-local temp should remain block-local, or one of the vars needs to be renamed:

blockLocalsAndArgs
       | product |
       product := 1.
       1 to: 2 do: [:i |
               i = 1
                       ifTrue: [ | factor | factor := 6.
                               product := product * factor ]
                       ifFalse: [ #(9) do: [:factor |
                               product := product * factor ] ] ].
       ^ product = 13r42

This works fine if decompiled without temps but the standard in Squeak is to use the actual temp names - just accept this as a method and switch to decompiled view.

The best fix might be if the decompiler always declared the variables in the inner-most block possible. Maybe moving the declarations is not even that hard to do as a post-process on the AST?

- Bert -

On 13.01.2011, at 00:51, Nicolas Cellier wrote:

> Follow up on unused temps :
>
> Wanna play with temps ? Try this :
>
> testUnusedVariable
>       self
>               compiling: 'griffle | goo | ^nil'
>               shouldRaise: UnusedVariable;
>               compiling: 'griffle | | ^[ | goo | ]'
>               shouldRaise: UnusedVariable;
>               compiling: 'griffle | | [ | goo | goo := nil. goo] yourself. ^[ | goo | ]'
>               shouldRaise: UnusedVariable;
>               compiling: 'griffle | | [ | goo | ] yourself. ^[ | goo | goo := nil. goo]'
>               shouldRaise: UnusedVariable
>
> What happens is that Parser.scopeTable has a single entry for 'goo'.
> Consequently, the last registered goo wins.
> In the 3rd case, the last goo is unused, so hasRef = false, so
> UnusedVariable is raised.
> In the 4th case, the last goo nowHasRef, and we don't get any UnusedVariable.
>
> Until we get a scopeTable mapping each temp of each block, we gonna
> live in trouble.
>
> Nicolas

On Thu, Jan 13, 2011 at 2:29 AM, Bert Freudenberg <[hidden email]> wrote:

Here's another decompiler problem that I actually ran into in production code. When decompiling this method, either the block-local temp should remain block-local, or one of the vars needs to be renamed:

blockLocalsAndArgs
       | product |
       product := 1.
       1 to: 2 do: [:i |
               i = 1
                       ifTrue: [ | factor | factor := 6.
                               product := product * factor ]
                       ifFalse: [ #(9) do: [:factor |
                               product := product * factor ] ] ].
       ^ product = 13r42

This works fine if decompiled without temps but the standard in Squeak is to use the actual temp names - just accept this as a method and switch to decompiled view.

The best fix might be if the decompiler always declared the variables in the inner-most block possible. Maybe moving the declarations is not even that hard to do as a post-process on the AST?

Right.  And there's now a visitor method for determining that scope, TempScopeEditor>>#blockNode:isEnclosingScopeFor:.  But I've been too lazy^H^H^H^Hbusy to apply it in the context of the decompiler.  Feel free to feel motivated...

best

Eliot

 

- Bert -

On 13.01.2011, at 00:51, Nicolas Cellier wrote:

> Follow up on unused temps :
>
> Wanna play with temps ? Try this :
>
> testUnusedVariable
>       self
>               compiling: 'griffle | goo | ^nil'
>               shouldRaise: UnusedVariable;
>               compiling: 'griffle | | ^[ | goo | ]'
>               shouldRaise: UnusedVariable;
>               compiling: 'griffle | | [ | goo | goo := nil. goo] yourself. ^[ | goo | ]'
>               shouldRaise: UnusedVariable;
>               compiling: 'griffle | | [ | goo | ] yourself. ^[ | goo | goo := nil. goo]'
>               shouldRaise: UnusedVariable
>
> What happens is that Parser.scopeTable has a single entry for 'goo'.
> Consequently, the last registered goo wins.
> In the 3rd case, the last goo is unused, so hasRef = false, so
> UnusedVariable is raised.
> In the 4th case, the last goo nowHasRef, and we don't get any UnusedVariable.
>
> Until we get a scopeTable mapping each temp of each block, we gonna
> live in trouble.
>
> Nicolas

On Thu, Jan 13, 2011 at 6:52 PM, Eliot Miranda <[hidden email]> wrote:

On Thu, Jan 13, 2011 at 9:33 AM, Eliot Miranda <[hidden email]> wrote:

On Thu, Jan 13, 2011 at 2:29 AM, Bert Freudenberg <[hidden email]> wrote:

Here's another decompiler problem that I actually ran into in production code. When decompiling this method, either the block-local temp should remain block-local, or one of the vars needs to be renamed:

blockLocalsAndArgs
       | product |
       product := 1.
       1 to: 2 do: [:i |
               i = 1
                       ifTrue: [ | factor | factor := 6.
                               product := product * factor ]
                       ifFalse: [ #(9) do: [:factor |
                               product := product * factor ] ] ].
       ^ product = 13r42

This works fine if decompiled without temps but the standard in Squeak is to use the actual temp names - just accept this as a method and switch to decompiled view.

The best fix might be if the decompiler always declared the variables in the inner-most block possible. Maybe moving the declarations is not even that hard to do as a post-process on the AST

 

Right.  And there's now a visitor method for determining that scope, TempScopeEditor>>#blockNode:isEnclosingScopeFor:.  But I've been too lazy^H^H^H^Hbusy to apply it in the context of the decompiler.  Feel free to feel motivated...

No, no, no, no, no, no, no.  The visitor is VariableScopeFinder and the method is ofVariable: so aParseNode accept: (VariableScopeFinder new ofVariable: var) answers nil or the smallest enclosing scope for var in aParseNode.  See Parser>>declareUndeclaredTemps.

Hi,

 

We fiddled with this a bit.

On Thu, Jan 13, 2011 at 6:52 PM, Eliot Miranda <[hidden email]> wrote:

On Thu, Jan 13, 2011 at 9:33 AM, Eliot Miranda <[hidden email]> wrote:

On Thu, Jan 13, 2011 at 2:29 AM, Bert Freudenberg <[hidden email]> wrote:

Here's another decompiler problem that I actually ran into in production code. When decompiling this method, either the block-local temp should remain block-local, or one of the vars needs to be renamed:

blockLocalsAndArgs
       | product |
       product := 1.
       1 to: 2 do: [:i |
               i = 1
                       ifTrue: [ | factor | factor := 6.
                               product := product * factor ]
                       ifFalse: [ #(9) do: [:factor |
                               product := product * factor ] ] ].
       ^ product = 13r42

This works fine if decompiled without temps but the standard in Squeak is to use the actual temp names - just accept this as a method and switch to decompiled view.

The best fix might be if the decompiler always declared the variables in the inner-most block possible. Maybe moving the declarations is not even that hard to do as a post-process on the AST

 The Decompiler modified by the attached changeset tries to do this. It gives the following output for your example:

  blockLocalsAndArgs

| product |

product := 1.

1

to: 2

do: [:i | i = 1

ifTrue: [| factor |

factor := 6.

product := product * factor]

ifFalse: [#(9 )

do: [:factor | product := product * factor]]].

^ product = 54

 

But it brakes half of the DecompilerTests. Most of them because the Decompiler inserts nil assignments to the beginnings of some blocks. eg. [ | val | ... ] becomes [ | val | val := nil. .... ].

 

And there is this snippet of code:

 

| aBlock |

aBlock := Compiler evaluate: '| x y |  [:a :b | x := a. y := b. x+y]'.

aBlock decompile

 

which breaks an assertion in the Decompiler itself.

 

So this is far from complete.

 

Right.  And there's now a visitor method for determining that scope, TempScopeEditor>>#blockNode:isEnclosingScopeFor:.  But I've been too lazy^H^H^H^Hbusy to apply it in the context of the decompiler.  Feel free to feel motivated...

No, no, no, no, no, no, no.  The visitor is VariableScopeFinder and the method is ofVariable: so aParseNode accept: (VariableScopeFinder new ofVariable: var) answers nil or the smallest enclosing scope for var in aParseNode.  See Parser>>declareUndeclaredTemps.

 

The changeset contains the following changes:

 - A new subclass of VariableScopeFinder: TempVariableScopeFinder. It finds scopes for tempvar nodes not just for undeclared nodes.

 - In DecompilerConstructorForClosures>>#codeMethod:block:tempVars:primitive:class: instead of 

     [ block temporaries: temporaries ], we execute

     [ temporaries := self pushDownTemps: temporaries toTheirInnermostPossibleScopesIn: block ]

 - The method pushDownTemps:toTheirInnermostPossibleScopesIn: which tries to do what it's name suggests and returns with the temps remaining in the top level scope.

 

Is this the right direction?

 

Cheers, Balázs