mustBeBooleanMagicIn can crash the vm
There is a certain risk, that the mustBeBooleanMagicIn method rewrite can crash the vm Issue 13805 <https://pharo.fogbugz.com/default.asp?13805> The receiver of the rewritten method is the "nonboolean" value receiving the ifTrue:ifFalse: message. But the rewritten method can include instvar accessor and self sends of the original method. We can include more rewrite rules in mustBeBooleanMagicIn like "rewrite instvar accessing" context receiver class instVarNamesAndOffsetsDo: [ :n :o | RBParseTreeRewriter new replace: n with: ('ThisContext receiver instVarAt: ', o asString); executeTree: methodNode. ]. "rewrite self sends" RBParseTreeRewriter new replace: 'self' with: 'ThisContext receiver'; executeTree: methodNode. But I don't know if this works for all possible situations. Or if there are better ways to do the rewriting.
Hi Nicolai, please try and remember to invlude vm-dev when you see a VM issue... On Sun, Aug 10, 2014 at 7:34 AM, Nicolai Hess <nicolaihess@web.de> wrote:
There is a certain risk, that the mustBeBooleanMagicIn method rewrite can crash the vm Issue 13805 <https://pharo.fogbugz.com/default.asp?13805>
Since you're using the Opal compiler ca you add the description of the bytecode (aCompiledMethod symbolic) to the issue? I'll try and take a look soon but want to be sure I'm debugging the relevant code. The receiver of the rewritten method is the "nonboolean" value receiving the
ifTrue:ifFalse: message. But the rewritten method can include instvar accessor and self sends of the original method.
We can include more rewrite rules in mustBeBooleanMagicIn like "rewrite instvar accessing" context receiver class instVarNamesAndOffsetsDo: [ :n :o | RBParseTreeRewriter new replace: n with: ('ThisContext receiver instVarAt: ', o asString); executeTree: methodNode. ]. "rewrite self sends" RBParseTreeRewriter new replace: 'self' with: 'ThisContext receiver'; executeTree: methodNode.
But I don't know if this works for all possible situations. Or if there are better ways to do the rewriting.
Remember the mirror methods: thisContext object: o instVarAt: which doesn't send to o, reaching directly into it. But what's the point of replacing "self" with "thisContext receiver"? The latter is potentially an extremely slow way of doing the former. The former won't create a context object in a method activation that doesn't need one (doesn't include a block), whereas the latter always will. But they will always yield the same result. -- curious, Eliot
Hi Nicolai and Eliot, This is not a bug in the VM but in mustBeBooleanMagic:. The last line is "^ self withArgs: {context} executeMethod: method" whereas it should be "^ context receiver withArgs: {context} executeMethod: method". Thank you for reporting this, I committed the fix. Slice in inbox. Cheers, Camille On 10 août 2014, at 17:34, Eliot Miranda <eliot.miranda@gmail.com> wrote:
Hi Nicolai,
please try and remember to invlude vm-dev when you see a VM issue...
On Sun, Aug 10, 2014 at 7:34 AM, Nicolai Hess <nicolaihess@web.de> wrote: There is a certain risk, that the mustBeBooleanMagicIn method rewrite can crash the vm Issue 13805
Since you're using the Opal compiler ca you add the description of the bytecode (aCompiledMethod symbolic) to the issue? I'll try and take a look soon but want to be sure I'm debugging the relevant code.
The receiver of the rewritten method is the "nonboolean" value receiving the ifTrue:ifFalse: message. But the rewritten method can include instvar accessor and self sends of the original method.
We can include more rewrite rules in mustBeBooleanMagicIn like "rewrite instvar accessing" context receiver class instVarNamesAndOffsetsDo: [ :n :o | RBParseTreeRewriter new replace: n with: ('ThisContext receiver instVarAt: ', o asString); executeTree: methodNode. ]. "rewrite self sends" RBParseTreeRewriter new replace: 'self' with: 'ThisContext receiver'; executeTree: methodNode.
But I don't know if this works for all possible situations. Or if there are better ways to do the rewriting.
Remember the mirror methods: thisContext object: o instVarAt: which doesn't send to o, reaching directly into it.
But what's the point of replacing "self" with "thisContext receiver"? The latter is potentially an extremely slow way of doing the former. The former won't create a context object in a method activation that doesn't need one (doesn't include a block), whereas the latter always will. But they will always yield the same result. -- curious, Eliot
Hi Eliot, I am pretty sure this is not a vm issue :) The mustBeBooleanInMagic: creates a method on the fly and executes it with the a context as argument. The "ThisContext" is not the "thisContext", but the argument of the generated method. This is the bytecode of the generated method: 17 <00> pushRcvr: 0 18 <8F 00 00 02> closureNumCopied: 0 numArgs: 0 bytes 22 to 23 22 <76> pushConstant: 1 23 <7D> blockReturn 24 <8F 00 00 02> closureNumCopied: 0 numArgs: 0 bytes 28 to 29 28 <75> pushConstant: 0 29 <7D> blockReturn 30 <F0> send: ifTrue:ifFalse: 31 <7C> returnTop The original method is foo ^ notABool1 ifTrue:[1] ifFalse:[0] The generated method compiles this part notABool1 ifTrue:[1] ifFalse:[0] to bytecode without the jumpFalse optimization. The problem is that the receiver for the ifTrue:ifFalse send is accessed through pushRcvr:0, whereas the thisContext is not the context of the original method anymore. Nicolai 2014-08-10 17:34 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
Hi Nicolai,
please try and remember to invlude vm-dev when you see a VM issue...
On Sun, Aug 10, 2014 at 7:34 AM, Nicolai Hess <nicolaihess@web.de> wrote:
There is a certain risk, that the mustBeBooleanMagicIn method rewrite can crash the vm Issue 13805 <https://pharo.fogbugz.com/default.asp?13805>
Since you're using the Opal compiler ca you add the description of the bytecode (aCompiledMethod symbolic) to the issue? I'll try and take a look soon but want to be sure I'm debugging the relevant code.
The receiver of the rewritten method is the "nonboolean" value receiving
the ifTrue:ifFalse: message. But the rewritten method can include instvar accessor and self sends of the original method.
We can include more rewrite rules in mustBeBooleanMagicIn like "rewrite instvar accessing" context receiver class instVarNamesAndOffsetsDo: [ :n :o | RBParseTreeRewriter new replace: n with: ('ThisContext receiver instVarAt: ', o asString); executeTree: methodNode. ]. "rewrite self sends" RBParseTreeRewriter new replace: 'self' with: 'ThisContext receiver'; executeTree: methodNode.
But I don't know if this works for all possible situations. Or if there are better ways to do the rewriting.
Remember the mirror methods: thisContext object: o instVarAt: which doesn't send to o, reaching directly into it.
But what's the point of replacing "self" with "thisContext receiver"? The latter is potentially an extremely slow way of doing the former. The former won't create a context object in a method activation that doesn't need one (doesn't include a block), whereas the latter always will. But they will always yield the same result. -- curious, Eliot
Hi Nicolai, On Sun, Aug 10, 2014 at 9:39 AM, Nicolai Hess <nicolaihess@web.de> wrote:
Hi Eliot,
I am pretty sure this is not a vm issue :)
The mustBeBooleanInMagic: creates a method on the fly and executes it with the a context as argument. The "ThisContext" is not the "thisContext", but the argument of the generated method.
This is the bytecode of the generated method:
17 <00> pushRcvr: 0 18 <8F 00 00 02> closureNumCopied: 0 numArgs: 0 bytes 22 to 23 22 <76> pushConstant: 1 23 <7D> blockReturn 24 <8F 00 00 02> closureNumCopied: 0 numArgs: 0 bytes 28 to 29 28 <75> pushConstant: 0 29 <7D> blockReturn 30 <F0> send: ifTrue:ifFalse: 31 <7C> returnTop
The original method is
foo ^ notABool1 ifTrue:[1] ifFalse:[0]
The generated method compiles this part notABool1 ifTrue:[1] ifFalse:[0]
to bytecode without the jumpFalse optimization. The problem is that the receiver for the ifTrue:ifFalse send is accessed through pushRcvr:0, whereas the thisContext is not the context of the original method anymore.
Ah, now I know what's going on. Thanks. I like this experiment very much. How do you extract the blocks from the method? Do you decompile or simply recompile from source or...? Have (any of) you looked at implementing the mustBeBooleanMagic by interpreting the bytecodes that already exist in the method via some specialized interpreter instead of compiling a method on the fly? Nicolai
2014-08-10 17:34 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
Hi Nicolai,
please try and remember to invlude vm-dev when you see a VM issue...
On Sun, Aug 10, 2014 at 7:34 AM, Nicolai Hess <nicolaihess@web.de> wrote:
There is a certain risk, that the mustBeBooleanMagicIn method rewrite can crash the vm Issue 13805 <https://pharo.fogbugz.com/default.asp?13805>
Since you're using the Opal compiler ca you add the description of the bytecode (aCompiledMethod symbolic) to the issue? I'll try and take a look soon but want to be sure I'm debugging the relevant code.
The receiver of the rewritten method is the "nonboolean" value receiving
the ifTrue:ifFalse: message. But the rewritten method can include instvar accessor and self sends of the original method.
We can include more rewrite rules in mustBeBooleanMagicIn like "rewrite instvar accessing" context receiver class instVarNamesAndOffsetsDo: [ :n :o | RBParseTreeRewriter new replace: n with: ('ThisContext receiver instVarAt: ', o asString); executeTree: methodNode. ]. "rewrite self sends" RBParseTreeRewriter new replace: 'self' with: 'ThisContext receiver'; executeTree: methodNode.
But I don't know if this works for all possible situations. Or if there are better ways to do the rewriting.
Remember the mirror methods: thisContext object: o instVarAt: which doesn't send to o, reaching directly into it.
But what's the point of replacing "self" with "thisContext receiver"? The latter is potentially an extremely slow way of doing the former. The former won't create a context object in a method activation that doesn't need one (doesn't include a block), whereas the latter always will. But they will always yield the same result. -- curious, Eliot
--
best, Eliot
On 10 août 2014, at 19:15, Eliot Miranda <eliot.miranda@gmail.com> wrote:
Hi Nicolai,
On Sun, Aug 10, 2014 at 9:39 AM, Nicolai Hess <nicolaihess@web.de> wrote:
Hi Eliot,
I am pretty sure this is not a vm issue :)
The mustBeBooleanInMagic: creates a method on the fly and executes it with the a context as argument. The "ThisContext" is not the "thisContext", but the argument of the generated method.
This is the bytecode of the generated method:
17 <00> pushRcvr: 0 18 <8F 00 00 02> closureNumCopied: 0 numArgs: 0 bytes 22 to 23 22 <76> pushConstant: 1 23 <7D> blockReturn 24 <8F 00 00 02> closureNumCopied: 0 numArgs: 0 bytes 28 to 29 28 <75> pushConstant: 0 29 <7D> blockReturn 30 <F0> send: ifTrue:ifFalse: 31 <7C> returnTop
The original method is
foo ^ notABool1 ifTrue:[1] ifFalse:[0]
The generated method compiles this part notABool1 ifTrue:[1] ifFalse:[0]
to bytecode without the jumpFalse optimization. The problem is that the receiver for the ifTrue:ifFalse send is accessed through pushRcvr:0, whereas the thisContext is not the context of the original method anymore.
Ah, now I know what's going on. Thanks. I like this experiment very much.
How do you extract the blocks from the method? Do you decompile or simply recompile from source or...?
We just recompile the ast message node that triggered the error without optimizations and some rewriting (temps and returns). Right now it works only for 'if' messages. For 'while' messages I'm thinking about sending #truthValue to the value of the receiver block.
Have (any of) you looked at implementing the mustBeBooleanMagic by interpreting the bytecodes that already exist in the method via some specialized interpreter instead of compiling a method on the fly?
No but that would be interesting indeed :) Another solution is to cache the generated methods somewhere instead of recompiling each time.
Nicolai
2014-08-10 17:34 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
Hi Nicolai,
please try and remember to invlude vm-dev when you see a VM issue...
On Sun, Aug 10, 2014 at 7:34 AM, Nicolai Hess <nicolaihess@web.de> wrote: There is a certain risk, that the mustBeBooleanMagicIn method rewrite can crash the vm Issue 13805
Since you're using the Opal compiler ca you add the description of the bytecode (aCompiledMethod symbolic) to the issue? I'll try and take a look soon but want to be sure I'm debugging the relevant code.
The receiver of the rewritten method is the "nonboolean" value receiving the ifTrue:ifFalse: message. But the rewritten method can include instvar accessor and self sends of the original method.
We can include more rewrite rules in mustBeBooleanMagicIn like "rewrite instvar accessing" context receiver class instVarNamesAndOffsetsDo: [ :n :o | RBParseTreeRewriter new replace: n with: ('ThisContext receiver instVarAt: ', o asString); executeTree: methodNode. ]. "rewrite self sends" RBParseTreeRewriter new replace: 'self' with: 'ThisContext receiver'; executeTree: methodNode.
But I don't know if this works for all possible situations. Or if there are better ways to do the rewriting.
Remember the mirror methods: thisContext object: o instVarAt: which doesn't send to o, reaching directly into it.
But what's the point of replacing "self" with "thisContext receiver"? The latter is potentially an extremely slow way of doing the former. The former won't create a context object in a method activation that doesn't need one (doesn't include a block), whereas the latter always will. But they will always yield the same result. -- curious, Eliot -- best, Eliot
On Sun, Aug 10, 2014 at 10:23 AM, Camille Teruel <camille.teruel@gmail.com> wrote:
On 10 août 2014, at 19:15, Eliot Miranda <eliot.miranda@gmail.com> wrote:
Hi Nicolai,
On Sun, Aug 10, 2014 at 9:39 AM, Nicolai Hess <nicolaihess@web.de> wrote:
Hi Eliot,
I am pretty sure this is not a vm issue :)
The mustBeBooleanInMagic: creates a method on the fly and executes it with the a context as argument. The "ThisContext" is not the "thisContext", but the argument of the generated method.
This is the bytecode of the generated method:
17 <00> pushRcvr: 0 18 <8F 00 00 02> closureNumCopied: 0 numArgs: 0 bytes 22 to 23 22 <76> pushConstant: 1 23 <7D> blockReturn 24 <8F 00 00 02> closureNumCopied: 0 numArgs: 0 bytes 28 to 29 28 <75> pushConstant: 0 29 <7D> blockReturn 30 <F0> send: ifTrue:ifFalse: 31 <7C> returnTop
The original method is
foo ^ notABool1 ifTrue:[1] ifFalse:[0]
The generated method compiles this part notABool1 ifTrue:[1] ifFalse:[0]
to bytecode without the jumpFalse optimization. The problem is that the receiver for the ifTrue:ifFalse send is accessed through pushRcvr:0, whereas the thisContext is not the context of the original method anymore.
Ah, now I know what's going on. Thanks. I like this experiment very much.
How do you extract the blocks from the method? Do you decompile or simply recompile from source or...?
We just recompile the ast message node that triggered the error without optimizations and some rewriting (temps and returns). Right now it works only for 'if' messages. For 'while' messages I'm thinking about sending #truthValue to the value of the receiver block.
Have (any of) you looked at implementing the mustBeBooleanMagic by interpreting the bytecodes that already exist in the method via some specialized interpreter instead of compiling a method on the fly?
No but that would be interesting indeed :) Another solution is to cache the generated methods somewhere instead of recompiling each time.
In the method's properties.
Nicolai
2014-08-10 17:34 GMT+02:00 Eliot Miranda <eliot.miranda@gmail.com>:
Hi Nicolai,
please try and remember to invlude vm-dev when you see a VM issue...
On Sun, Aug 10, 2014 at 7:34 AM, Nicolai Hess <nicolaihess@web.de> wrote:
There is a certain risk, that the mustBeBooleanMagicIn method rewrite can crash the vm Issue 13805 <https://pharo.fogbugz.com/default.asp?13805>
Since you're using the Opal compiler ca you add the description of the bytecode (aCompiledMethod symbolic) to the issue? I'll try and take a look soon but want to be sure I'm debugging the relevant code.
The receiver of the rewritten method is the "nonboolean" value receiving
the ifTrue:ifFalse: message. But the rewritten method can include instvar accessor and self sends of the original method.
We can include more rewrite rules in mustBeBooleanMagicIn like "rewrite instvar accessing" context receiver class instVarNamesAndOffsetsDo: [ :n :o | RBParseTreeRewriter new replace: n with: ('ThisContext receiver instVarAt: ', o asString); executeTree: methodNode. ]. "rewrite self sends" RBParseTreeRewriter new replace: 'self' with: 'ThisContext receiver'; executeTree: methodNode.
But I don't know if this works for all possible situations. Or if there are better ways to do the rewriting.
Remember the mirror methods: thisContext object: o instVarAt: which doesn't send to o, reaching directly into it.
But what's the point of replacing "self" with "thisContext receiver"? The latter is potentially an extremely slow way of doing the former. The former won't create a context object in a method activation that doesn't need one (doesn't include a block), whereas the latter always will. But they will always yield the same result. -- curious, Eliot
--
best, Eliot
-- best, Eliot
participants (3)
-
Camille Teruel -
Eliot Miranda -
Nicolai Hess