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]}
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.
Cheers, Hans-Martin
Hi Hans-Martin,
On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner hmm@heeg.de 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).
Cheers, Hans-Martin
On 19 May 2010 00:17, Eliot Miranda eliot.miranda@gmail.com wrote:
Hi Hans-Martin,
On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner hmm@heeg.de 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. So, one should use it instead of putting meaningless #(0 0 0 0) everywhere.
Cheers, Hans-Martin
On Tue, May 18, 2010 at 4:10 PM, Igor Stasenko siguctua@gmail.com wrote:
On 19 May 2010 00:17, Eliot Miranda eliot.miranda@gmail.com wrote:
Hi Hans-Martin,
On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner hmm@heeg.de
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 eliot.miranda@gmail.com wrote:
On Tue, May 18, 2010 at 4:10 PM, Igor Stasenko siguctua@gmail.com wrote:
On 19 May 2010 00:17, Eliot Miranda eliot.miranda@gmail.com wrote:
Hi Hans-Martin,
On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner hmm@heeg.de 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.
To elaborate, what i have in mind, suppose a following scheme:
1. you got a compilation request from user's domain 2. a sources manager forms a 'request object' (mainly capturing a source code to compile) 3. compiler doing its work 4. if everything ok, then sources manager commits a new source to its storage, and forms an appropriate method trailer 5. system performs a final operation, like generating a final method & installing it into a class
On 19 May 2010 03:33, Igor Stasenko siguctua@gmail.com wrote:
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 eliot.miranda@gmail.com wrote:
On Tue, May 18, 2010 at 4:10 PM, Igor Stasenko siguctua@gmail.com wrote:
On 19 May 2010 00:17, Eliot Miranda eliot.miranda@gmail.com wrote:
Hi Hans-Martin,
On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner hmm@heeg.de 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.
On Tue, May 18, 2010 at 02:17:40PM -0700, Eliot Miranda wrote:
Hi Hans-Martin,
On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner hmm@heeg.de 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.
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"
Dave
Hi David,
On Tue, May 18, 2010 at 4:33 PM, David T. Lewis lewis@mail.msen.com wrote:
On Tue, May 18, 2010 at 02:17:40PM -0700, Eliot Miranda wrote:
Hi Hans-Martin,
On Tue, May 18, 2010 at 12:15 PM, Hans-Martin Mosner hmm@heeg.de
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.
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.
cheers Eliot
Dave
On Tue, May 18, 2010 at 05:02:05PM -0700, Eliot Miranda wrote:
Hi David,
On Tue, May 18, 2010 at 4:33 PM, David T. Lewis lewis@mail.msen.com 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.
Yes, I understand, I was intentionally truncating the changes file to see what would happen. Prior to applying Decompiler.1.cs, the browser would show the decompiled (but incorrect) source. After applying Decompiler.1.cs, it showed corrupt source. But oops, that was because I had caused things to be written to the changes file, so of course it looked like garbage source.
Sorry for the noise.
Thanks, Dave
Am 18.05.2010 23:17, schrieb Eliot Miranda:
Hi Hans-Martin,
Any way of reproducing this without OMeta, e.g. in a workspace?
I haven't found a simple way, which is probably due to the fact that I was not aware of how the decompile with temp names worked...
Anyway, I've tried your change set and there are two results: - when shift is not pressed, I get a syntax error window on OMeta methods (workaround should be similar to that in CompiledMethod>>methodNode). - with shift pressed, the decompiled method seems to be correct.
Cheers, Hans-Martin
squeak-dev@lists.squeakfoundation.org
-
David T. Lewis -
Eliot Miranda -
Hans-Martin Mosner -
Igor Stasenko