Order of evaluation bug with in lined to:do: in both Opal and Squeak compilers.
Hi All, just stumbled across a bytecode compiler bug that's in both the Squeak compiler and the Opal compiler. I'm told by Clément that Opal mimics the bug to avoid crashing legacy code. So there may be places that depend on this bug. It would be good to eliminate the dependencies and the bug. For illustration look at the to:do: loop in the ifNil: arm in Context>>privRefresh: Context>>privRefresh "Reinitialize the receiver so that it is in the state it was at its creation." closureOrNil ifNotNil: [pc := closureOrNil startpc. self stackp: closureOrNil numArgs + closureOrNil numCopiedValues. 1 to: closureOrNil numCopiedValues do: [:i | self tempAt: closureOrNil numArgs + i put: (closureOrNil at: i)]] ifNil: [pc := method initialPC. self stackp: method numTemps. method numArgs+1 to: method numTemps do: [:i | self tempAt: i put: nil]] This should evaluate method numArgs + 1 then method numTemps. If it were written as a non-in-lined (method numArgs+1 to: method numTemps) do: [:i| self tempAt: i put: nil] then the [self tempAt: i put: nil] block would be created next. But the bytecode for the inlined version is self stackp: method numTemps. 63 <70> self 64 <84 40 03> pushRcvr: 3 67 <D6> send: numTemps 68 <E1> send: stackp: 69 <87> pop iLimit := method numTemps 70 <84 40 03> pushRcvr: 3 73 <D6> send: numTemps 74 <69> popIntoTemp: 1 i := method numArgs + 1 75 <84 40 03> pushRcvr: 3 78 <D2> send: numArgs 79 <76> pushConstant: 1 80 <B0> send: + 81 <81 40> storeIntoTemp: 0 (squeak) <69> popIntoTemp: 1 (pharo) 83 <10> pushTemp: 0 84 <11> pushTemp: 1 85 <B4> send: <= 86 <AC 0B> jumpFalse: 99 There is a second bug in the Squeak bytecode; storeIntoTemp: is used to load i whereas it should be popIntoTemp:. It was this second bug that alerted me to the order-of-evaluation bug. _,,,^..^,,,_ best, Eliot
On Thu, Apr 20, 2017 at 4:41 PM, Eliot Miranda <eliot.miranda@gmail.com> wrote:
Hi All,
just stumbled across a bytecode compiler bug that's in both the Squeak compiler and the Opal compiler. I'm told by Clément that Opal mimics the bug to avoid crashing legacy code. So there may be places that depend on this bug. It would be good to eliminate the dependencies and the bug.
For illustration look at the to:do: loop in the ifNil: arm in Context>>privRefresh:
Context>>privRefresh "Reinitialize the receiver so that it is in the state it was at its creation." closureOrNil ifNotNil: [pc := closureOrNil startpc. self stackp: closureOrNil numArgs + closureOrNil numCopiedValues. 1 to: closureOrNil numCopiedValues do: [:i | self tempAt: closureOrNil numArgs + i put: (closureOrNil at: i)]] ifNil: [pc := method initialPC. self stackp: method numTemps. method numArgs+1 to: method numTemps do: [:i | self tempAt: i put: nil]]
This should evaluate method numArgs + 1 then method numTemps. If it were written as a non-in-lined (method numArgs+1 to: method numTemps) do: [:i| self tempAt: i put: nil] then the [self tempAt: i put: nil] block would be created next. But the bytecode for the inlined version is
self stackp: method numTemps. 63 <70> self 64 <84 40 03> pushRcvr: 3 67 <D6> send: numTemps 68 <E1> send: stackp: 69 <87> pop
iLimit := method numTemps 70 <84 40 03> pushRcvr: 3 73 <D6> send: numTemps 74 <69> popIntoTemp: 1
i := method numArgs + 1 75 <84 40 03> pushRcvr: 3 78 <D2> send: numArgs 79 <76> pushConstant: 1 80 <B0> send: + 81 <81 40> storeIntoTemp: 0 (squeak) <69> popIntoTemp: 1 (pharo) 83 <10> pushTemp: 0 84 <11> pushTemp: 1 85 <B4> send: <= 86 <AC 0B> jumpFalse: 99
There is a second bug in the Squeak bytecode; storeIntoTemp: is used to load i whereas it should be popIntoTemp:. It was this second bug that alerted me to the order-of-evaluation bug.
The second bug (Squeak's use of storeIntoTemp:) is actually only a poor implementation of the value/effect distinction through the inlined ifNil:ifNotNil:. Because ifNil:ifNotNil: has a value (albeit one that is discarded) the Squeak compiler generates a storeIntoTemp: to p[reserve the value of the to:do: lop, which is the initial index. So the bug is not within the generation of the to:do: (the only bug there being the order-of-evaluation one). The bug is actually outside; the loop should be being generated for effect but is being evaluated for value. On the order of evaluation bug, does anyone have any memory of which methods depended on this bug? _,,,^..^,,,_
best, Eliot
-- _,,,^..^,,,_ best, Eliot
John Brant discovered this bug recenlty: (see bottom of his post) http://forum.world.st/collection-flatCollect-something-VS-collection-collect... 2017-04-21 1:56 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
On Thu, Apr 20, 2017 at 4:41 PM, Eliot Miranda <eliot.miranda@gmail.com> wrote:
Hi All,
just stumbled across a bytecode compiler bug that's in both the Squeak compiler and the Opal compiler. I'm told by Clément that Opal mimics the bug to avoid crashing legacy code. So there may be places that depend on this bug. It would be good to eliminate the dependencies and the bug.
For illustration look at the to:do: loop in the ifNil: arm in Context>>privRefresh:
Context>>privRefresh "Reinitialize the receiver so that it is in the state it was at its creation." closureOrNil ifNotNil: [pc := closureOrNil startpc. self stackp: closureOrNil numArgs + closureOrNil numCopiedValues. 1 to: closureOrNil numCopiedValues do: [:i | self tempAt: closureOrNil numArgs + i put: (closureOrNil at: i)]] ifNil: [pc := method initialPC. self stackp: method numTemps. method numArgs+1 to: method numTemps do: [:i | self tempAt: i put: nil]]
This should evaluate method numArgs + 1 then method numTemps. If it were written as a non-in-lined (method numArgs+1 to: method numTemps) do: [:i| self tempAt: i put: nil] then the [self tempAt: i put: nil] block would be created next. But the bytecode for the inlined version is
self stackp: method numTemps. 63 <70> self 64 <84 40 03> pushRcvr: 3 67 <D6> send: numTemps 68 <E1> send: stackp: 69 <87> pop
iLimit := method numTemps 70 <84 40 03> pushRcvr: 3 73 <D6> send: numTemps 74 <69> popIntoTemp: 1
i := method numArgs + 1 75 <84 40 03> pushRcvr: 3 78 <D2> send: numArgs 79 <76> pushConstant: 1 80 <B0> send: + 81 <81 40> storeIntoTemp: 0 (squeak) <69> popIntoTemp: 1 (pharo) 83 <10> pushTemp: 0 84 <11> pushTemp: 1 85 <B4> send: <= 86 <AC 0B> jumpFalse: 99
There is a second bug in the Squeak bytecode; storeIntoTemp: is used to load i whereas it should be popIntoTemp:. It was this second bug that alerted me to the order-of-evaluation bug.
The second bug (Squeak's use of storeIntoTemp:) is actually only a poor implementation of the value/effect distinction through the inlined ifNil:ifNotNil:. Because ifNil:ifNotNil: has a value (albeit one that is discarded) the Squeak compiler generates a storeIntoTemp: to p[reserve the value of the to:do: lop, which is the initial index. So the bug is not within the generation of the to:do: (the only bug there being the order-of-evaluation one). The bug is actually outside; the loop should be being generated for effect but is being evaluated for value.
On the order of evaluation bug, does anyone have any memory of which methods depended on this bug?
_,,,^..^,,,_
best, Eliot
-- _,,,^..^,,,_ best, Eliot
I'm happy that we can remove bugs. We should redo a pass on Opal and enhance it because we need a better architecture for our future. Stef On Fri, Apr 21, 2017 at 9:20 AM, Nicolai Hess <nicolaihess@gmail.com> wrote:
John Brant discovered this bug recenlty:
(see bottom of his post) http://forum.world.st/collection-flatCollect-something-VS-collection- collect-something-flattened-tp4929422p4929479.html
2017-04-21 1:56 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
On Thu, Apr 20, 2017 at 4:41 PM, Eliot Miranda <eliot.miranda@gmail.com> wrote:
Hi All,
just stumbled across a bytecode compiler bug that's in both the Squeak compiler and the Opal compiler. I'm told by Clément that Opal mimics the bug to avoid crashing legacy code. So there may be places that depend on this bug. It would be good to eliminate the dependencies and the bug.
For illustration look at the to:do: loop in the ifNil: arm in Context>>privRefresh:
Context>>privRefresh "Reinitialize the receiver so that it is in the state it was at its creation." closureOrNil ifNotNil: [pc := closureOrNil startpc. self stackp: closureOrNil numArgs + closureOrNil numCopiedValues. 1 to: closureOrNil numCopiedValues do: [:i | self tempAt: closureOrNil numArgs + i put: (closureOrNil at: i)]] ifNil: [pc := method initialPC. self stackp: method numTemps. method numArgs+1 to: method numTemps do: [:i | self tempAt: i put: nil]]
This should evaluate method numArgs + 1 then method numTemps. If it were written as a non-in-lined (method numArgs+1 to: method numTemps) do: [:i| self tempAt: i put: nil] then the [self tempAt: i put: nil] block would be created next. But the bytecode for the inlined version is
self stackp: method numTemps. 63 <70> self 64 <84 40 03> pushRcvr: 3 67 <D6> send: numTemps 68 <E1> send: stackp: 69 <87> pop
iLimit := method numTemps 70 <84 40 03> pushRcvr: 3 73 <D6> send: numTemps 74 <69> popIntoTemp: 1
i := method numArgs + 1 75 <84 40 03> pushRcvr: 3 78 <D2> send: numArgs 79 <76> pushConstant: 1 80 <B0> send: + 81 <81 40> storeIntoTemp: 0 (squeak) <69> popIntoTemp: 1 (pharo) 83 <10> pushTemp: 0 84 <11> pushTemp: 1 85 <B4> send: <= 86 <AC 0B> jumpFalse: 99
There is a second bug in the Squeak bytecode; storeIntoTemp: is used to load i whereas it should be popIntoTemp:. It was this second bug that alerted me to the order-of-evaluation bug.
The second bug (Squeak's use of storeIntoTemp:) is actually only a poor implementation of the value/effect distinction through the inlined ifNil:ifNotNil:. Because ifNil:ifNotNil: has a value (albeit one that is discarded) the Squeak compiler generates a storeIntoTemp: to p[reserve the value of the to:do: lop, which is the initial index. So the bug is not within the generation of the to:do: (the only bug there being the order-of-evaluation one). The bug is actually outside; the loop should be being generated for effect but is being evaluated for value.
On the order of evaluation bug, does anyone have any memory of which methods depended on this bug?
_,,,^..^,,,_
best, Eliot
-- _,,,^..^,,,_ best, Eliot
participants (3)
-
Eliot Miranda -
Nicolai Hess -
Stephane Ducasse