On Tue, Apr 2, 2013 at 7:46 PM, David T. Lewis
<lewis@mail.msen.com> wrote:
On Fri, Mar 29, 2013 at 11:21:00AM -0400, David T. Lewis wrote:
> On Fri, Mar 29, 2013 at 03:04:20PM +0000, Frank Shearar wrote:
> > On 29 March 2013 14:17, David T. Lewis <lewis@mail.msen.com> wrote:
> > >
> > > I moved these two new tests from Inbox to Trunk. One test fails on recent
> > > trunk images. I do not know if this is a bug or a feature, but I cannot
> > > explain the regression so I don't want to assume it's a feature.
> > >
> > > Dave
> >
> > http://build.squeak.org/job/SqueakTrunk/237/testReport/junit/Tests.Compiler/CompilerTest/testMaxLiteralsWithClassReferenceInClosure/
> >
> > shows the test failure.
>
> The actual change to the image that causes this test failure occurred between
> SqueakTrunk/212 and SqueakTrunk/213. In other words, if this test had been
> part of the test suite all along, then you would have seen the failure appear
> in 213.
>
> I suspect that this is a side effect of class lookups associated with the Environments
> changes, but I cannot say if it is a bug or an explainable variation. Either way
> it's good to note the change, and we can either fix the bug or fix the test
> as needed.
(self flag: #dubious and: [self flag: #bogus]) ifTrue: [self askForGuidance]
LiteralVariableNode>>sizeCodeForStore: is flagged thus, which I suspect
is somehow related to the apparently incorrect warning about too many literals.
I doubt it. How to reproduce the incorrect warning? The point about LiteralVariableNode>>sizeCodeForStore: (& LiteralVariableNode>>emitCodeForStore:encoder:) is that they don't do what's advertised. i.e. they don't leave the value of the expression that is stored on top of stack unless the special write binding answers the expression in response to #value:. So if one were to write
v := SomeSpecialBinding := expr
v would end up holding SomeSpecialBinding, not expr.
This could perhaps be corrected by ensuring that all classes that implement isSpecialWriteBinding also implement #value: to answer the argument, not the receiver. e.g. instead of
Binding methods for accessing
value: anObject
(AttemptToWriteReadOnlyGlobal signal: 'Cannot store into read-only bindings')
ifTrue: [value := anObject]
we use
value: anObject
(AttemptToWriteReadOnlyGlobal signal: 'Cannot store into read-only bindings')
ifTrue: [value := anObject].
^anObject
but that's tricky.
The problem for the code generator is that there's no swap operation. e.g. the following would work:
stack before:
expr
first half of code:
dup
push binding
swap
stack during:
expr
binding
expr
second half of code
send #value:
pop
stack after
expr
but that's a) a new bytecode and b) a lot of bytes to do a store.
A different approach would be to rewrite emit because it knows the stack pointer to write something like
push binding
push temp N (where N is the offset of expr on the stack)
send value:
I think this is the correct approach. It might require fixing the decompiler, but it shouldn't be hard. I'll have a go. In any case we need a test that ensures that
| v expr |
expr := Object new.
v := SomeSpecialBinding := expr.
self assert: v == expr
Reducing to a smaller test case, compiling 'Time now' yields a parse tree
with LiteralVariableNode {Time} that has code -4 which causes dual reserving
in sizeCodeForValue: and this in turn leads to a mis-counting of literals
in the encoder.
In an earlier version of trunk, the same LiteralVariableNode has a non-negative
code value that does not produce the dual reserving in sizeCodeForValue:
Dave