ifNil:ifNotNil: Bug report

Hi...

just encountered a bug:
My code looks like this and fails with stats beeing nil in the ifNotNil:
block. This is by definition impossible I think:

refreshContents
    self model
        ifNil: [#('pixelCountText' 'minText' 'maxText' 'medianText'
'meanText' 'varianceText' 'stddevText' 'histogramPixelCountText'
'histogramPixelValueText' 'histogram')
            do: [:each | (self presenterNamed: each) model: nil]]
        ifNotNil:
            [:stats |
            (self presenterNamed: 'pixelCountText') model: stats count. "<--
fails here"
            (self presenterNamed: 'minText') model: stats min.
            (self presenterNamed: 'maxText') model: stats max.
            (self presenterNamed: 'medianText') model: stats median.
            (self presenterNamed: 'meanText') model: stats mean.
            (self presenterNamed: 'varianceText') model: stats variance.
            (self presenterNamed: 'stddevText') model: stats stddev.
            (self presenterNamed: 'histogram') model: stats histogram].

I put a halt in Object>>ifNil:ifNotNil: but it is never executed!!
In debugger, inspecting "self model" is definitely NOT nil, however.
Browsing definitions of ifNil:ifNotNil: reveals only two implementations: in
Object and UndefinedObject (as expected).

I fixed this for now with:
            ...[:m || stats | stats := self model. ...

And, no, I am not using Seaside-modified VM.

Any idea?

Ciao

...Jochen

Jochen Riekhof wrote:
Look at the bytecode.  I expect you'll find that the Dolphin  compiler
optimizes ifNil: et al analogously to optimizing ifTrue: et al...


--
_______________,,,^..^,,,____________________________
Eliot Miranda              Smalltalk - Scene not herd

> Look at the bytecode.  I expect you'll find that the Dolphin  compiler
> optimizes ifNil: et al analogously to optimizing ifTrue: et al...

Hum, not shure if one can spot th error here, but here is the bytecode for
the more experienced ;-)

Ciao

...Jochen

---------
Normal, args 0, locals 2, literals 25, has block
1 Push self
2 Send[1]: #model with 0 args
3 Dup
4 Near Jump If Not Nil +15 to 21
6 Pop
7 Push Const[1]: #('pixelCountText' 'minText' 'maxText' 'medianText'
'meanText' 'varianceText' 'stddevText' 'histogramPixelCountText'
'histogramPixelValueText' 'histogram')
8 Block Copy, 1 args, skip +7 to 19
12 Pop Store Temp[0]
13 Push self and Temp[0]
15 Send[3]: #presenterNamed: with 1 args
16 Push nil
17 Send[4]: #model: with 1 args
18 Return From Block
19 Send[5]: #do: with 1 args
20 Return self
21 Pop Store Temp[0]
22 Push self
23 Push Const[5]: 'pixelCountText'
24 Send[3]: #presenterNamed: with 1 args
25 Push Temp[1]
26 Send[7]: #count with 0 args
27 Send[4]: #model: with 1 args
28 Pop
29 Push self
30 Push Const[7]: 'minText'
31 Send[3]: #presenterNamed: with 1 args
32 Push Temp[1]
33 Send[9]: #min with 0 args
34 Send[4]: #model: with 1 args
35 Pop
36 Push self
37 Push Const[9]: 'maxText'
38 Send[3]: #presenterNamed: with 1 args
39 Push Temp[1]
40 Send[11]: #max with 0 args
41 Send[4]: #model: with 1 args
42 Pop
43 Push self
44 Push Const[11]: 'medianText'
45 Send[3]: #presenterNamed: with 1 args
46 Push Temp[1]
47 Send[13]: #median with 0 args
48 Send[4]: #model: with 1 args
49 Pop
50 Push self
51 Push Const[13]: 'meanText'
52 Send[3]: #presenterNamed: with 1 args
53 Push Temp[1]
54 Send[15]: #mean with 0 args
55 Push Const[15]: 1.0e-003
56 Send[17]: #roundTo: with 1 args
58 Send[4]: #model: with 1 args
59 Pop
60 Push self
61 Push Const[17]: 'varianceText'
63 Send[3]: #presenterNamed: with 1 args
64 Push Temp[1]
65 Send[19]: #variance with 0 args
67 Push Const[19]: 1.0e-003
69 Send[17]: #roundTo: with 1 args
71 Send[4]: #model: with 1 args
72 Pop
73 Push self
74 Push Const[20]: 'stddevText'
76 Send[3]: #presenterNamed: with 1 args
77 Push Temp[1]
78 Send[22]: #stddev with 0 args
80 Push Const[22]: 1.0e-003
82 Send[17]: #roundTo: with 1 args
84 Send[4]: #model: with 1 args
85 Pop
86 Push self
87 Push Const[23]: 'histogram'
89 Send[3]: #presenterNamed: with 1 args
90 Push Temp[1]
91 Send[25]: #histogram with 0 args
93 Send[4]: #model: with 1 args
94 Return self

Jochen,

Eliot is correct about the optimisation, IIRC Blair added it just before D5
was released.  Instruction 4 in your list

4 Near Jump If Not Nil +15 to 21

would normally be a message send but has been optimised down to a test and
jump. That explains why adding a #halt to Object>>ifNil:ifNotNil: did not
have the desired effect, the method is never actually used.

I can't offhand see why the argument is nil but it could be just a side
effect of the walkback.  By the time the debugger has opened the object has
disappeared and you are left thinking it's nil.  One thing you could try is
making the start of your notNil branch to print the argument ...

.......
        ifNotNil:
            [:stats |
            Transcript print: stats; cr.
            (self presenterNamed: 'pixelCountText') model: stats count. "<--
fails here"

... and see what that shows.

--
Ian Bartholomew

I wrote:
> I can't offhand see why the argument is nil

but on reflection I don't like the look of

21 Pop Store Temp[0]
22 Push self
23 Push Const[5]: 'pixelCountText'
24 Send[3]: #presenterNamed: with 1 args
25 Push Temp[1]
26 Send[7]: #count with 0 args

I would have thought 21 should be storing into Temp[1] as that is being
pushed as the receiver iin 25 (and others) for the #count message.

One thing that appears to correct the above behaviour is reversing the order
of the selector

self model
    ifNotNil: [...]
    ifNil: [...]

Very strange though.

--
Ian Bartholomew

Jochen Riekhof wrote:
>
> > Look at the bytecode.  I expect you'll find that the Dolphin  compiler
> > optimizes ifNil: et al analogously to optimizing ifTrue: et al...
>
> Hum, not shure if one can spot th error here, but here is the bytecode for
> the more experienced ;-)

Well, if foo ifNil: nilBlock ifNotNil: notNilBlock were not optimized in
the compiler you'd expect to see bytecode like

        push foo
        push nilBlock
        push notNilBlock
        send #ifNil:ifNotNil:

or concretely from VisualWorks:

1 <10> push local 0
2 <1C> push BlockClosure [] in UndefinedObject>>unboundMethod
3 <1D> push BlockClosure [] in UndefinedObject>>unboundMethod
4 <92> send ifNil:ifNotNil:
5 <66> pop

but if it were optimized (as it is) you'd expect to see bytecode like

        push foo
        push nil
        send #==
        jump false L1
        ... code for ifNil: block placed inline...
        jump L2
L1: ...code for ifNotNil: block placed inline...
L2:

or concretely from Dolphin what you see below, with the
        push nil
        send #==
        jump false L1
replaced by the more-efficient-to-interpret (and more compact) jump
notNil bytecode.

See

http://users.ipa.net/~dwighth/smalltalk/bluebook/bluebook_chapter26.html#TheBytecodes26

and in particular read the section on Jump Bytecodes and compare with
the description of blockCopy:.

Note that blockCopy: is typically implemented rather differently in
contemporary Smalltalk implementations, i.e. as a bytecode rather than
as a message.

--
_______________,,,^..^,,,____________________________
Eliot Miranda              Smalltalk - Scene not herd

"Ian Bartholomew" <[hidden email]> wrote in message
news:av4qgf$bkfkg$[hidden email]...
Yes, there does appear to be a compiler bug there. The compiler is wrongly
re-using the method temp[0] to store the ifNotNil: block argument, stats,
into, when it should be using temp[1]. Subsequently to that it uses the
correct temp for all the references to stats, but of course that temp has
not been assigned. Since D5 implements Smalltalk-80 block semantics all the
temps in the method, including those in the blocks, are allocated in a
single shared MethodContext.  temp[0] has been allocated as the slot for the
argument to the do: block in the ifNil: branch, and should not be used for
any other temps.

For those interested in these sort of things I've posted the bytecodes from
the forthcoming D6, which implements full closure semantics. Ignoring the
changes caused by the macro instructions, one can see that temp[0] is used
for both the #do: block argument, each, and apparently the ifNotNil: block
argument, stats. This is OK because in D6 the block arguments and block
local temps (where possible) live on the stack as part of the activation
record created when a method or block is invoked, and so temp[0] in a block
is not the same slot as temp[0] in its home method.

Regards

Blair
-----------------------------------------
Normal, 0 args, 1 stack temps, 21 literals

 1 Send Self [1]: #model with 0 args
 2 Dup
 3 Near Jump If Not Nil +18 to 21
 5 Pop
 6 Push Const[1]: #('pixelCountText' 'minText' 'maxText' 'medianText'
'meanText' 'varianceText' 'stddevText' 'histogramPixelCountText'
'histogramPixelValueText' 'histogram')
 7 Block Copy, 1 args, needs self, skip +12 to 19
 14 Push self and Temp[0]
 15 Send[3]: #presenterNamed: with 1 args
 16 Push nil
 17 Send[4]: #model: with 1 args
 18 Return From Block
 19 Send[5]: #do: with 1 args
 20 Pop & Return self
 21 Pop Store Temp[0]
 22 Push self
 23 Push Const[5]: 'pixelCountText'
 24 Send[3]: #presenterNamed: with 1 args
 25 Push Temp[0]
 26 Send[7]: #count with 0 args
 27 Send[4]: #model: with 1 args
 28 Pop & Push Self
 29 Push Const[7]: 'minText'
 30 Send[3]: #presenterNamed: with 1 args
 31 Push Temp[0]
 32 Send[9]: #min with 0 args
 33 Send[4]: #model: with 1 args
 34 Pop & Push Self
 35 Push Const[9]: 'maxText'
 36 Send[3]: #presenterNamed: with 1 args
 37 Push Temp[0]
 38 Send[11]: #max with 0 args
 39 Send[4]: #model: with 1 args
 40 Pop & Push Self
 41 Push Const[11]: 'medianText'
 42 Send[3]: #presenterNamed: with 1 args
 43 Push Temp[0]
 44 Send[13]: #median with 0 args
 45 Send[4]: #model: with 1 args
 46 Pop & Push Self
 47 Push Const[13]: 'meanText'
 48 Send[3]: #presenterNamed: with 1 args
 49 Push Temp[0]
 50 Send[15]: #mean with 0 args
 52 Send[4]: #model: with 1 args
 53 Pop & Push Self
 54 Push Const[15]: 'varianceText'
 55 Send[3]: #presenterNamed: with 1 args
 56 Push Temp[0]
 57 Send[17]: #variance with 0 args
 59 Send[4]: #model: with 1 args
 60 Pop & Push Self
 61 Push Const[17]: 'stddevText'
 63 Send[3]: #presenterNamed: with 1 args
 64 Push Temp[0]
 65 Send[19]: #stddev with 0 args
 67 Send[4]: #model: with 1 args
 68 Pop & Push Self
 69 Push Const[19]: 'histogram'
 71 Send[3]: #presenterNamed: with 1 args
 72 Push Temp[0]
 73 Send[21]: #histogram with 0 args
 75 Send[4]: #model: with 1 args
 76 Pop & Return self

Thanks for the link!

Ciao

...Jochen

You might also want to have a look (if you haven't come across it already)
at the Dolphin method ByteCodeInterpreter class>>initialize which has a list
of all the byte codes implemented by Dolphin.

--
Ian Bartholomew