[squeak-dev] [Vm-dev] Order of evaluation bug with in lined to:do: in both Opal and Squeak compilers.

Nicolai Hess nicolaihess at gmail.com
Fri Apr 21 07:20:39 UTC 2017


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 at gmail.com>:

>
>
>
> On Thu, Apr 20, 2017 at 4:41 PM, Eliot Miranda <eliot.miranda at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20170421/d3605ee4/attachment.html>


More information about the Squeak-dev mailing list