decompiler breaks some methods

> Hi Hans-Martin,
>
> On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner <[hidden email]> wrote:
>>
>> Hello,
>> while looking at the decompiled code of some OMeta methods I noticed
>> that the temp name handling in the decompiler seems to be broken.
>> When you compile a method with temps that are referenced from blocks,
>> the decompiler tends to rearrange the temp names, so this method
>>
>> test
>>    | one two |
>>    two := 2.
>>    ^{[one := 1].
>>    [ [one + two] value]}
>>
>> gets decompiled to this:
>>
>> test
>>    | one two |
>>    one := 2.
>>    ^ {[two := 1]. [[two + one] value]}
>
> OK, the issue in Squeak 4.1 is that the
> CodeHolder>>decompiledSourceIntoContents method is out of date.  Find
> attached.
>
>> But that's only a nuisance. The bug really shows up when you decompile a
>> method compiled using OMeta.
>> To see what I mean, load the OMeta2 package and have a look at method
>> OMeta2AndOrOpt>>and.
>
> Any way of reproducing this without OMeta, e.g. in a workspace?
> e.g. to test the above I used things like
> | mn m |
> mn := Compiler new compile: 'test
>    | one two |
>    two := 2.
>    ^{[one := 1].
>    [ [one + two] value]}' in: nil class classified: nil notifying: nil
> ifFail: nil.
> m := (mn generate: #(0 0 0 0)) copyWithTempsFromMethodNode: mn.
> {m decompile. m tempNamesString}
> where in 4.1 "generate: #(0 0 0 0)" needs to be replaced with "generate:
> CompiledMethodTrailer defaultMethodTrailer" (although asMethodTrailer on
> ArrayedCollection would be nice for backward-compatibility).

> Hi Hans-Martin,
>
> On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner <[hidden email]> wrote:
>
> > Hello,
> > while looking at the decompiled code of some OMeta methods I noticed
> > that the temp name handling in the decompiler seems to be broken.
> > When you compile a method with temps that are referenced from blocks,
> > the decompiler tends to rearrange the temp names, so this method
> >
> > test
> >    | one two |
> >    two := 2.
> >    ^{[one := 1].
> >    [ [one + two] value]}
> >
> > gets decompiled to this:
> >
> > test
> >    | one two |
> >    one := 2.
> >    ^ {[two := 1]. [[two + one] value]}
> >
>
> OK, the issue in Squeak 4.1 is that the
> CodeHolder>>decompiledSourceIntoContents method is out of date.  Find
> attached.

> Hi David,
>
> On Tue, May 18, 2010 at 4:33 PM, David T. Lewis <[hidden email]> wrote:
> >
> > I tried the Decompiler.1.cs patch in Squeak trunk, and it produces some
> > undesirable
> > side effects. Here is what I did:
> >
> > - saved a copy of changes file
> > - created a new method in a browser
> > - save and exit
> > - copy saved changes file back to changes file (to eliminate the source for
> > new method)
> > - restart image
> > - decompiled code in browser shows bug reported by Hans-Martin Mosner
> > - loaded Decompiler.1.cs
> > - decompiled code in browser produces a "Note:" dialog, and the following
> >  (garbage) in the codeholder window:
> >
> >    "ueak/Squeak3.10-dev/squeak.10.image"
> >
>
> that's a different issue caused by your effectively truncating the changes
> file. What you've done is arrange that the method's source pointer points
> into the SNAPSHOT marker in the original changes file.  What you need to do
> is select decompile from the source button dropdown menu in the browser, not
> switch the changes file underneath the method and expect the system to
> automatically figure out that the source is corrupt and it should decompile.

>
>
> On Tue, May 18, 2010 at 4:10 PM, Igor Stasenko <[hidden email]> wrote:
>>
>> On 19 May 2010 00:17, Eliot Miranda <[hidden email]> wrote:
>> > Hi Hans-Martin,
>> >
>> > On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner <[hidden email]>
>> > wrote:
>> >>
>> >> Hello,
>> >> while looking at the decompiled code of some OMeta methods I noticed
>> >> that the temp name handling in the decompiler seems to be broken.
>> >> When you compile a method with temps that are referenced from blocks,
>> >> the decompiler tends to rearrange the temp names, so this method
>> >>
>> >> test
>> >>    | one two |
>> >>    two := 2.
>> >>    ^{[one := 1].
>> >>    [ [one + two] value]}
>> >>
>> >> gets decompiled to this:
>> >>
>> >> test
>> >>    | one two |
>> >>    one := 2.
>> >>    ^ {[two := 1]. [[two + one] value]}
>> >
>> > OK, the issue in Squeak 4.1 is that the
>> > CodeHolder>>decompiledSourceIntoContents method is out of date.  Find
>> > attached.
>> >
>> >> But that's only a nuisance. The bug really shows up when you decompile
>> >> a
>> >> method compiled using OMeta.
>> >> To see what I mean, load the OMeta2 package and have a look at method
>> >> OMeta2AndOrOpt>>and.
>> >
>> > Any way of reproducing this without OMeta, e.g. in a workspace?
>> > e.g. to test the above I used things like
>> > | mn m |
>> > mn := Compiler new compile: 'test
>> >    | one two |
>> >    two := 2.
>> >    ^{[one := 1].
>> >    [ [one + two] value]}' in: nil class classified: nil notifying: nil
>> > ifFail: nil.
>> > m := (mn generate: #(0 0 0 0)) copyWithTempsFromMethodNode: mn.
>> > {m decompile. m tempNamesString}
>> > where in 4.1 "generate: #(0 0 0 0)" needs to be replaced with "generate:
>> > CompiledMethodTrailer defaultMethodTrailer" (although asMethodTrailer on
>> > ArrayedCollection would be nice for backward-compatibility).
>>
>> There is a Behavior>>#defaultMethodTrailer , which is backwards
>> compatible.
>
> CompiledMethod
> class>>newBytes:trailerBytes:nArgs:nTemps:nStack:nLits:primitive: (and hence
> BytecodeAgnosticMethodNode>>generate:) is no longer backward-compatible.
>  Call it with the old meaningless #(0 0 0 0) and you get an error.  One
> might be able to provide backward-compatibility if there was an
> implementation of asMethodTrailer in Arrayed Collection that deferred to
> CompiledMethodTrailer and the relevant part
> of newBytes:trailerBytes:nArgs:nTemps:nStack:nLits:primitive: read:
> ^ trailer asMethodTrailer createMethod: numberOfBytes
> header: (nArgs bitShift: 24) +
> (nTemps bitShift: 18) +
> (largeBit bitShift: 17) +
> (nLits bitShift: 9) +
> primBits.
> In any case the selector keyword is "trailerBytes:" not "methodTrailer:" or
> "trailer".  IMO you should add a new method that takes a trailer and revert
> the old one to read
> newBytes: numberOfBytes trailerBytes: trailer nArgs: nArgs nTemps: nTemps
> nStack: stackSize nLits: nLits primitive: primitiveIndex
> "Answer an instance of me. The header is specified by the message
> arguments. The remaining parts are not as yet determined."
> ^self newBytes: numberOfBytes
> trailer: trailer asMethodTrailer
> nArgs: nArgs
> nTemps: nTemps
> nStack: stackSize
> nLits: nLits
> primitive: primitiveIndex
>
>>
>> So, one should use it instead of putting meaningless #(0 0 0 0)
>> everywhere.
>
> Its not an issue of #(0 0 0 0) being meaningless.  Its an issue of not
> breaking the old way where #(0 0 0 0) was used to mean empty.
> best
> Eliot
>>
>>
>> >>
>> >> Cheers,
>> >> Hans-Martin
>> >>
>> >
>> >
>> >
>> >
>> >
>>
>>
>>
>> --
>> Best regards,
>> Igor Stasenko AKA sig.
>>
>
>
>
>
>

> My vision about trailers, that
> new trailers can be formed at different stages of compilation where
> you could have
> a separate layer, which controlling explicitly, what kind of trailers to use.
>
> While by using a combination of #(0 0 0 0) and then #asMethodTrailer
> you make it hard for
> such potential layer to control explicitly, what trailers to generate.
> Its too late to send #asMethodTrailer at method generation stage,
> since you don't know
> to whom delegate the responsibility for generating the right trailers.
>
> Meanwhile, since there is no such layer on horizon, feel free to add a
> backwards compatibility patch. :)
>
> On 19 May 2010 02:57, Eliot Miranda <[hidden email]> wrote:
>>
>>
>> On Tue, May 18, 2010 at 4:10 PM, Igor Stasenko <[hidden email]> wrote:
>>>
>>> On 19 May 2010 00:17, Eliot Miranda <[hidden email]> wrote:
>>> > Hi Hans-Martin,
>>> >
>>> > On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner <[hidden email]>
>>> > wrote:
>>> >>
>>> >> Hello,
>>> >> while looking at the decompiled code of some OMeta methods I noticed
>>> >> that the temp name handling in the decompiler seems to be broken.
>>> >> When you compile a method with temps that are referenced from blocks,
>>> >> the decompiler tends to rearrange the temp names, so this method
>>> >>
>>> >> test
>>> >>    | one two |
>>> >>    two := 2.
>>> >>    ^{[one := 1].
>>> >>    [ [one + two] value]}
>>> >>
>>> >> gets decompiled to this:
>>> >>
>>> >> test
>>> >>    | one two |
>>> >>    one := 2.
>>> >>    ^ {[two := 1]. [[two + one] value]}
>>> >
>>> > OK, the issue in Squeak 4.1 is that the
>>> > CodeHolder>>decompiledSourceIntoContents method is out of date.  Find
>>> > attached.
>>> >
>>> >> But that's only a nuisance. The bug really shows up when you decompile
>>> >> a
>>> >> method compiled using OMeta.
>>> >> To see what I mean, load the OMeta2 package and have a look at method
>>> >> OMeta2AndOrOpt>>and.
>>> >
>>> > Any way of reproducing this without OMeta, e.g. in a workspace?
>>> > e.g. to test the above I used things like
>>> > | mn m |
>>> > mn := Compiler new compile: 'test
>>> >    | one two |
>>> >    two := 2.
>>> >    ^{[one := 1].
>>> >    [ [one + two] value]}' in: nil class classified: nil notifying: nil
>>> > ifFail: nil.
>>> > m := (mn generate: #(0 0 0 0)) copyWithTempsFromMethodNode: mn.
>>> > {m decompile. m tempNamesString}
>>> > where in 4.1 "generate: #(0 0 0 0)" needs to be replaced with "generate:
>>> > CompiledMethodTrailer defaultMethodTrailer" (although asMethodTrailer on
>>> > ArrayedCollection would be nice for backward-compatibility).
>>>
>>> There is a Behavior>>#defaultMethodTrailer , which is backwards
>>> compatible.
>>
>> CompiledMethod
>> class>>newBytes:trailerBytes:nArgs:nTemps:nStack:nLits:primitive: (and hence
>> BytecodeAgnosticMethodNode>>generate:) is no longer backward-compatible.
>>  Call it with the old meaningless #(0 0 0 0) and you get an error.  One
>> might be able to provide backward-compatibility if there was an
>> implementation of asMethodTrailer in Arrayed Collection that deferred to
>> CompiledMethodTrailer and the relevant part
>> of newBytes:trailerBytes:nArgs:nTemps:nStack:nLits:primitive: read:
>> ^ trailer asMethodTrailer createMethod: numberOfBytes
>> header: (nArgs bitShift: 24) +
>> (nTemps bitShift: 18) +
>> (largeBit bitShift: 17) +
>> (nLits bitShift: 9) +
>> primBits.
>> In any case the selector keyword is "trailerBytes:" not "methodTrailer:" or
>> "trailer".  IMO you should add a new method that takes a trailer and revert
>> the old one to read
>> newBytes: numberOfBytes trailerBytes: trailer nArgs: nArgs nTemps: nTemps
>> nStack: stackSize nLits: nLits primitive: primitiveIndex
>> "Answer an instance of me. The header is specified by the message
>> arguments. The remaining parts are not as yet determined."
>> ^self newBytes: numberOfBytes
>> trailer: trailer asMethodTrailer
>> nArgs: nArgs
>> nTemps: nTemps
>> nStack: stackSize
>> nLits: nLits
>> primitive: primitiveIndex
>>
>>>
>>> So, one should use it instead of putting meaningless #(0 0 0 0)
>>> everywhere.
>>
>> Its not an issue of #(0 0 0 0) being meaningless.  Its an issue of not
>> breaking the old way where #(0 0 0 0) was used to mean empty.
>> best
>> Eliot
>>>
>>>
>>> >>
>>> >> Cheers,
>>> >> Hans-Martin
>>> >>
>>> >
>>> >
>>> >
>>> >
>>> >
>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Igor Stasenko AKA sig.
>>>
>>
>>
>>
>>
>>
>
>
>
> --
> Best regards,
> Igor Stasenko AKA sig.
>