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 <[email protected]> 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 <[email protected]>: > >> >> >> >> On Thu, Apr 20, 2017 at 4:41 PM, Eliot Miranda <[email protected]> >> 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 >> >> >
