[squeak-dev] Bug in BytecodeGenerator>>#jump:if:

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Bug in BytecodeGenerator>>#jump:if:

André Wendt-3
Hi all,

apologies if this doesn't belong here. Mantis is a usability-nightmare
and I didn't know where else to report a bug in the NewCompiler. The
corresponding ML on squeakfoundation.org hasn't been posted to in over
six months.

I've noticed that BytecodeGenerator>>#jump:if: has a bug when given a
distance >= 1024.

It states

hi := distance // 256.

and then goes on checking if hi < 8. If that's false, the jump is
assumed to be too big. However, the check should be hi < 4.

Assume the method gets an argument of 1024, and hi is 4. The following

self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.

generates bytecode 176, which is an arithmetic message send instead of a
conditional jump.

I've attached a changeset that checks both jump conditions for hi < 4.

Regards,
André

'From Squeak3.10.1 of ''26 May 2008'' [latest update: #7175] on 22 June 2008 at 12:36:17 pm'!

!BytecodeGenerator methodsFor: 'private' stamp: 'awe 6/22/2008 12:35'!
jump: distance if: condition

        | hi |
        distance = 0 ifTrue: [
                "jumps to fall through, no-op"
                ^ self nextPut: (Bytecodes at: #popStackBytecode)].
        condition ifTrue: [
                hi := distance // 256.
                hi < 4 ifFalse: [self error: 'true jump too big'].
                self nextPut: (Bytecodes at: #longJumpIfTrue) first + hi.
                self nextPut: distance \\ 256.
        ] ifFalse: [
                distance <= 8 ifTrue: [
                        self nextPut: (Bytecodes at: #shortConditionalJump) first + distance - 1.
                ] ifFalse: [
                        hi := distance // 256.
                        hi < 4 ifFalse: [self error: 'false jump too big'].
                        self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.
                        self nextPut: distance \\ 256.
                ].
        ]
! !



Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
Hello André,

Good find!
I confirm only 4 bytes codes are reserved for longJumpIfFalse and 4
bytes code for longJumpIfTrue.
So this seems like a MAJOR bug. The kind of bug able to crash squeak if
you generate and execute a method with a jump > 1024 and < 2048...

I recommend you post to NewCompiler mailing list, even if not very
active, some members don't listen at squeak-dev.

I recommend you also check latest squeak source Monticello updates for
NewCompiler.
(MCHttpRepository
        location: 'http://www.squeaksource.com/NewCompiler'
        user: ''
        password: '')

Last, I recommend you persist a little bit with Mantis. Creating an
account and posting a bugform should not be that difficult. What is the
problem you encountered with Mantis?

Nicolas

André Wendt wrote:

> Hi all,
>
> apologies if this doesn't belong here. Mantis is a usability-nightmare
> and I didn't know where else to report a bug in the NewCompiler. The
> corresponding ML on squeakfoundation.org hasn't been posted to in over
> six months.
>
> I've noticed that BytecodeGenerator>>#jump:if: has a bug when given a
> distance >= 1024.
>
> It states
>
> hi := distance // 256.
>
> and then goes on checking if hi < 8. If that's false, the jump is
> assumed to be too big. However, the check should be hi < 4.
>
> Assume the method gets an argument of 1024, and hi is 4. The following
>
> self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.
>
> generates bytecode 176, which is an arithmetic message send instead of a
> conditional jump.
>
> I've attached a changeset that checks both jump conditions for hi < 4.
>
> Regards,
> André
>
>
> ------------------------------------------------------------------------
>
>


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
nice wrote:
> Hello André,
>
> Good find!
> I confirm only 4 bytes codes are reserved for longJumpIfFalse and 4
> bytes code for longJumpIfTrue.
> So this seems like a MAJOR bug. The kind of bug able to crash squeak if
> you generate and execute a method with a jump > 1024 and < 2048...
>

Just for the fun of it: how do you write non regression tests for such ones?

self shouldnt: [SmalltalkImage crash] ???


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Klaus D. Witzel
On Sun, 22 Jun 2008 16:05:57 +0200, Nicolas wrote:

> nice wrote:
>> Hello André,
>>  Good find!
>> I confirm only 4 bytes codes are reserved for longJumpIfFalse and 4  
>> bytes code for longJumpIfTrue.
>> So this seems like a MAJOR bug. The kind of bug able to crash squeak if  
>> you generate and execute a method with a jump > 1024 and < 2048...
>>
>
> Just for the fun of it: how do you write non regression tests for such  
> ones?
>
> self shouldnt: [SmalltalkImage crash] ???

You might want to check the CompiledMethod of the following method (the  
bytecode which in my .image has offset 34):

!UndefinedObject methodsFor: 'as yet unclassified' stamp: 'kwl 6/22/2008  
16:37'!
longJumpIfFalseTest
        "(nil class>>#longJumpIfFalseTest) inspect"

        true ifTrue:[
                {42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
                42. 42. 42. 42. 42. 42
                }
        ]! !

/Klaus

>



Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Hans-Martin Mosner
In reply to this post by Nicolas Cellier-3
nice schrieb:

> nice wrote:
>> Hello André,
>>
>> Good find!
>> I confirm only 4 bytes codes are reserved for longJumpIfFalse and 4
>> bytes code for longJumpIfTrue.
>> So this seems like a MAJOR bug. The kind of bug able to crash squeak
>> if you generate and execute a method with a jump > 1024 and < 2048...
>>
>
> Just for the fun of it: how do you write non regression tests for such
> ones?
>
> self shouldnt: [SmalltalkImage crash] ???
>
>
>
You'd probably compile a method which would trigger the bug and run it
through the simulator. The simulator would most likely generate some
kind of array out of bounds exception, but it can't crash the image.

Cheers,
Hans-Martin

Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
In reply to this post by Klaus D. Witzel
Klaus D. Witzel wrote:

> On Sun, 22 Jun 2008 16:05:57 +0200, Nicolas wrote:
>
>> nice wrote:
>>> Hello André,
>>>  Good find!
>>> I confirm only 4 bytes codes are reserved for longJumpIfFalse and 4
>>> bytes code for longJumpIfTrue.
>>> So this seems like a MAJOR bug. The kind of bug able to crash squeak
>>> if you generate and execute a method with a jump > 1024 and < 2048...
>>>
>>
>> Just for the fun of it: how do you write non regression tests for such
>> ones?
>>
>> self shouldnt: [SmalltalkImage crash] ???
>
> You might want to check the CompiledMethod of the following method (the
> bytecode which in my .image has offset 34):
>
> !UndefinedObject methodsFor: 'as yet unclassified' stamp: 'kwl 6/22/2008
> 16:37'!
> longJumpIfFalseTest
>     "(nil class>>#longJumpIfFalseTest) inspect"
>
>     true ifTrue:[
>         {42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42. 42.
>         42. 42. 42. 42. 42. 42
>         }
>     ]! !
>
> /Klaus
>
>>

Thanks Klaus, i know how to generate the bug - believe me, I'm really
good at generating bugs :)

My problem is with the quality dogma: "provide a test that fails before
the patch and pass after".

So what would you assert about longJumpIfFalseTest?

We could assert that trying to compile such a method should raise an
Exception (let's say a ByteCodeLimitException).
But if we install a handler that overcome the limitation (like compiling
above example using a BlockClosure, true ifTrue: [aBlock value]) the
test would not raise the ByteCodeLimitException and thus would fail!
This would not be a very good test...

A better test should be to try and compile, and if it compiles to assert
the result. And if the result is a crash then...

        self shouldnt: [...] raise: SmalltalkImageCrash.

Ah Ah, would i be the maintainer of an image containing this bug, that i
would not wait for a non regression test. I would correct it as quickly
as possible.

If a good non regression tests comes, that's a plus, I take it. But
should never be a dogma. Just take a look at
http://bugs.squeak.org/view.php?id=6584 i bet the bug still is in the
latests images.

Nicolas


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
In reply to this post by Hans-Martin Mosner
Hans-Martin Mosner wrote:
> You'd probably compile a method which would trigger the bug and run it
> through the simulator. The simulator would most likely generate some
> kind of array out of bounds exception, but it can't crash the image.
>
> Cheers,
> Hans-Martin
>
>

That's a clever way to follow.
Eventually, if the Simulator doesn't raise an Exception we can still
assert the result of execution.

But my joke was about the dogma "provide a failure test before your
change is accepted in Squeak".

Correcting such bug is ten times easier than writing a TestCase, and is
an emergency. So in some exceptional cases like this the dogma should be
"Correct immediately then provide a non regression test case".

Nicolas


Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Hans-Martin Mosner
nice schrieb:
>
>
> But my joke was about the dogma "provide a failure test before your
> change is accepted in Squeak".
>
> Correcting such bug is ten times easier than writing a TestCase, and
> is an emergency. So in some exceptional cases like this the dogma
> should be "Correct immediately then provide a non regression test case".
<offtopic>As a christian I'm pretty cautious about dogmata - they are
always formulated by people who have only partial insight and don't know
the whole truth - as Paul wrote in 1 Cor. 13,12... So I take them with a
grain of salt.</offtopic>

If you only accept SUnit tests, you're bound to encounter cases such as
this where it is not possible to write a meaningful failure test.
But when you accept other kinds of tests, it's pretty simple. The test
would be a method which when compiled and executed crashes the image.
With the fix in place, the image does not crash.
The most important feature of tests is that they are repeatable, which
this test would obviously be.

Cheers,
Hans-Martin

Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
Hans-Martin Mosner wrote:

> nice schrieb:
>>
>> But my joke was about the dogma "provide a failure test before your
>> change is accepted in Squeak".
>>
>> Correcting such bug is ten times easier than writing a TestCase, and
>> is an emergency. So in some exceptional cases like this the dogma
>> should be "Correct immediately then provide a non regression test case".
> <offtopic>As a christian I'm pretty cautious about dogmata - they are
> always formulated by people who have only partial insight and don't know
> the whole truth - as Paul wrote in 1 Cor. 13,12... So I take them with a
> grain of salt.</offtopic>
>
> If you only accept SUnit tests, you're bound to encounter cases such as
> this where it is not possible to write a meaningful failure test.
> But when you accept other kinds of tests, it's pretty simple. The test
> would be a method which when compiled and executed crashes the image.
> With the fix in place, the image does not crash.
> The most important feature of tests is that they are repeatable, which
> this test would obviously be.
>
> Cheers,
> Hans-Martin
>
>

You get it, thus the joke self shouldnt: [SmalltalkImage crash].


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Klaus D. Witzel
In reply to this post by Nicolas Cellier-3
On Sun, 22 Jun 2008 17:16:37 +0200, Nicolas wrote:

> So what would you assert about longJumpIfFalseTest?

;)

> We could assert that trying to compile such a method should raise an  
> Exception (let's say a ByteCodeLimitException).

No, I wouldn't do that. I'd wrap the instance of CompiledMethod into an  
InstructionStream using temps | is msg | and check it on the meta level,  
without executing the method and without the possibility that this crashes  
the VM, like

  (msg := is nextInstruction) selector == #pushConstant: and: [msg  
arguments = #(true)].
  (msg := is nextInstruction) selector == #jump:if: and:  [msg arguments =  
#(offset false)]

where offset is the one expected.

/Klaus


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

André Wendt-3
In reply to this post by Nicolas Cellier-3
Hello Nicolas,

nice wrote:
> Hello André,
>
> Good find!
> I confirm only 4 bytes codes are reserved for longJumpIfFalse and 4
> bytes code for longJumpIfTrue.
> So this seems like a MAJOR bug. The kind of bug able to crash squeak if
> you generate and execute a method with a jump > 1024 and < 2048...

Well, not in my case, I was wondering why I got a
True>>doesNotUnderstand: #adaptToNumberAndSend:

The reason was that a send:#+ was generated after a Boolean got pushed.
So no crash, but weird behavior. YMMV.

> I recommend you post to NewCompiler mailing list, even if not very
> active, some members don't listen at squeak-dev.

OK, I'll do that.

> I recommend you also check latest squeak source Monticello updates for
> NewCompiler.
> (MCHttpRepository
>     location: 'http://www.squeaksource.com/NewCompiler'
>     user: ''
>     password: '')

Well, I have confirmed this with NewCompiler-md.281.mcz, there doesn't
semm to be a more recent version on [1].

> Last, I recommend you persist a little bit with Mantis. Creating an
> account and posting a bugform should not be that difficult. What is the
> problem you encountered with Mantis?

Well, for starters the font is so small it makes my eyes hurt. I know I
can increase it with my browser, but why isn't there a usable default
value? :(

For obvious reasons, I always want to make sure the bug isn't reported
yet before filing it -- but there's no search on the homepage. :(

What kind of "project" does this bug belong to? Why do I have to care?

Oh wait, there it is: "View Issues". I can apply "filters", but why not
a general fuzzy search? A big fat "SEARCH: [__________] [GO]" that
doesn't give me a zillion options would be nice.

Let's put it the marketing way: "There's room for improvement." ;-)

Anyway, I'll forward this to the NewCompiler list and will file a bug in
Mantis.

Regards,
André

[1] http://www.squeaksource.com/NewCompiler/

>
> Nicolas
>
> André Wendt wrote:
>> Hi all,
>>
>> apologies if this doesn't belong here. Mantis is a usability-nightmare
>> and I didn't know where else to report a bug in the NewCompiler. The
>> corresponding ML on squeakfoundation.org hasn't been posted to in over
>> six months.
>>
>> I've noticed that BytecodeGenerator>>#jump:if: has a bug when given a
>> distance >= 1024.
>>
>> It states
>>
>> hi := distance // 256.
>>
>> and then goes on checking if hi < 8. If that's false, the jump is
>> assumed to be too big. However, the check should be hi < 4.
>>
>> Assume the method gets an argument of 1024, and hi is 4. The following
>>
>> self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.
>>
>> generates bytecode 176, which is an arithmetic message send instead of a
>> conditional jump.
>>
>> I've attached a changeset that checks both jump conditions for hi < 4.
>>
>> Regards,
>> André
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Bug in BytecodeGenerator>>#jump:if:

stephane ducasse
In reply to this post by André Wendt-3
Normally there is a mailing-list for the newcompiler but indeed we  
should have a bugtracker.

Stef

On Jun 22, 2008, at 12:54 PM, André Wendt wrote:

> Hi all,
>
> apologies if this doesn't belong here. Mantis is a usability-nightmare
> and I didn't know where else to report a bug in the NewCompiler. The
> corresponding ML on squeakfoundation.org hasn't been posted to in over
> six months.
>
> I've noticed that BytecodeGenerator>>#jump:if: has a bug when given a
> distance >= 1024.
>
> It states
>
> hi := distance // 256.
>
> and then goes on checking if hi < 8. If that's false, the jump is
> assumed to be too big. However, the check should be hi < 4.
>
> Assume the method gets an argument of 1024, and hi is 4. The following
>
> self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.
>
> generates bytecode 176, which is an arithmetic message send instead  
> of a
> conditional jump.
>
> I've attached a changeset that checks both jump conditions for hi < 4.
>
> Regards,
> André
> 'From Squeak3.10.1 of ''26 May 2008'' [latest update: #7175] on 22  
> June 2008 at 12:36:17 pm'!
>
> !BytecodeGenerator methodsFor: 'private' stamp: 'awe 6/22/2008 12:35'!
> jump: distance if: condition
>
> | hi |
> distance = 0 ifTrue: [
> "jumps to fall through, no-op"
> ^ self nextPut: (Bytecodes at: #popStackBytecode)].
> condition ifTrue: [
> hi := distance // 256.
> hi < 4 ifFalse: [self error: 'true jump too big'].
> self nextPut: (Bytecodes at: #longJumpIfTrue) first + hi.
> self nextPut: distance \\ 256.
> ] ifFalse: [
> distance <= 8 ifTrue: [
> self nextPut: (Bytecodes at: #shortConditionalJump) first +  
> distance - 1.
> ] ifFalse: [
> hi := distance // 256.
> hi < 4 ifFalse: [self error: 'false jump too big'].
> self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.
> self nextPut: distance \\ 256.
> ].
> ]
> ! !
>
>


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
In reply to this post by Klaus D. Witzel
Klaus D. Witzel wrote:

> On Sun, 22 Jun 2008 17:16:37 +0200, Nicolas wrote:
>
>> So what would you assert about longJumpIfFalseTest?
>
> ;)
>
>> We could assert that trying to compile such a method should raise an
>> Exception (let's say a ByteCodeLimitException).
>
> No, I wouldn't do that. I'd wrap the instance of CompiledMethod into an
> InstructionStream using temps | is msg | and check it on the meta level,
> without executing the method and without the possibility that this
> crashes the VM, like
>
>  (msg := is nextInstruction) selector == #pushConstant: and: [msg
> arguments = #(true)].
>  (msg := is nextInstruction) selector == #jump:if: and:  [msg arguments
> = #(offset false)]
>
> where offset is the one expected.
>
> /Klaus
>
>
>

Thanks, that's a nice alternative which is future proof toward Bytecode
definition change.

However, i have two objections:
1) the test would still not pass if i install a handler that can
overcome ByteCodeLimitException
2) the test won't avoid para-phrasing ByteCodeGenerator job. Because you
need to calculate the offset...

In other words, I would always prefer higher level tests if possible
(like asserting the result of execution of ifTrue:ifFalse:).

Nicolas


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

André Wendt-3
In reply to this post by Nicolas Cellier-3
nice wrote:

> Hello André,
>
> Good find!
> I confirm only 4 bytes codes are reserved for longJumpIfFalse and 4
> bytes code for longJumpIfTrue.
> So this seems like a MAJOR bug. The kind of bug able to crash squeak if
> you generate and execute a method with a jump > 1024 and < 2048...
>
> I recommend you post to NewCompiler mailing list, even if not very
> active, some members don't listen at squeak-dev.
>
> I recommend you also check latest squeak source Monticello updates for
> NewCompiler.
> (MCHttpRepository
>     location: 'http://www.squeaksource.com/NewCompiler'
>     user: ''
>     password: '')
>
> Last, I recommend you persist a little bit with Mantis. Creating an
> account and posting a bugform should not be that difficult. What is the
> problem you encountered with Mantis?

I've reported a bug on this, see http://bugs.squeak.org/view.php?id=7100

André

> Nicolas
>
> André Wendt wrote:
>> Hi all,
>>
>> apologies if this doesn't belong here. Mantis is a usability-nightmare
>> and I didn't know where else to report a bug in the NewCompiler. The
>> corresponding ML on squeakfoundation.org hasn't been posted to in over
>> six months.
>>
>> I've noticed that BytecodeGenerator>>#jump:if: has a bug when given a
>> distance >= 1024.
>>
>> It states
>>
>> hi := distance // 256.
>>
>> and then goes on checking if hi < 8. If that's false, the jump is
>> assumed to be too big. However, the check should be hi < 4.
>>
>> Assume the method gets an argument of 1024, and hi is 4. The following
>>
>> self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.
>>
>> generates bytecode 176, which is an arithmetic message send instead of a
>> conditional jump.
>>
>> I've attached a changeset that checks both jump conditions for hi < 4.
>>
>> Regards,
>> André
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
In reply to this post by André Wendt-3
André Wendt wrote:

> Hello Nicolas,
>
> nice wrote:
>> Hello André,
>>
>> So this seems like a MAJOR bug. The kind of bug able to crash squeak if
>> you generate and execute a method with a jump > 1024 and < 2048...
>
> Well, not in my case, I was wondering why I got a
> True>>doesNotUnderstand: #adaptToNumberAndSend:
>
> The reason was that a send:#+ was generated after a Boolean got pushed.
> So no crash, but weird behavior. YMMV.
>

You're lucky, i crashed a st-80 image 20 years ago for the very same
long jump reasons (with automatically generated code).

Nicolas


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
In reply to this post by André Wendt-3
André Wendt wrote:
>
> I've reported a bug on this, see http://bugs.squeak.org/view.php?id=7100
>

Thanks!


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Klaus D. Witzel
In reply to this post by Nicolas Cellier-3
On Sun, 22 Jun 2008 18:17:45 +0200, Nicolas wrote:

> Klaus D. Witzel wrote:
>> On Sun, 22 Jun 2008 17:16:37 +0200, Nicolas wrote:
>>
>>> So what would you assert about longJumpIfFalseTest?
>>  ;)
>>
>>> We could assert that trying to compile such a method should raise an  
>>> Exception (let's say a ByteCodeLimitException).
>>  No, I wouldn't do that. I'd wrap the instance of CompiledMethod into  
>> an InstructionStream using temps | is msg | and check it on the meta  
>> level, without executing the method and without the possibility that  
>> this crashes the VM, like
>>   (msg := is nextInstruction) selector == #pushConstant: and: [msg  
>> arguments = #(true)].
>>  (msg := is nextInstruction) selector == #jump:if: and:  [msg arguments  
>> = #(offset false)]
>>  where offset is the one expected.
>>  /Klaus
>>
>
> Thanks, that's a nice alternative which is future proof toward Bytecode  
> definition change.
>
> However, i have two objections:
> 1) the test would still not pass if i install a handler that can  
> overcome ByteCodeLimitException

This your ififthenthenelseelse did not parse here :(

> 2) the test won't avoid para-phrasing ByteCodeGenerator job. Because you  
> need to calculate the offset...

You could use a computer, perhaps programmed in Smalltalk, for a loop  
which creates a CompiledMethod which is guaranteed to exhibit the desired  
offset? ;)

> In other words, I would always prefer higher level tests if possible  
> (like asserting the result of execution of ifTrue:ifFalse:).
>
> Nicolas
>


Reply | Threaded
Open this post in threaded view
|

[squeak-dev] Re: Bug in BytecodeGenerator>>#jump:if:

Nicolas Cellier-3
In reply to this post by stephane ducasse
stephane ducasse wrote:
> Normally there is a mailing-list for the newcompiler but indeed we
> should have a bugtracker.
>
> Stef
>

Yes, at least a NewCompiler category in Mantis.


Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Bug in BytecodeGenerator>>#jump:if:

Marcus Denker
In reply to this post by stephane ducasse


On Sun, Jun 22, 2008 at 6:11 PM, stephane ducasse <[hidden email]> wrote:
Normally there is a mailing-list for the newcompiler but indeed we should have a bugtracker.

I have now set up a google code bugtracker for the NewCompiler:


   Marcus

 

Stef

On Jun 22, 2008, at 12:54 PM, André Wendt wrote:

Hi all,

apologies if this doesn't belong here. Mantis is a usability-nightmare
and I didn't know where else to report a bug in the NewCompiler. The
corresponding ML on squeakfoundation.org hasn't been posted to in over
six months.

I've noticed that BytecodeGenerator>>#jump:if: has a bug when given a
distance >= 1024.

It states

hi := distance // 256.

and then goes on checking if hi < 8. If that's false, the jump is
assumed to be too big. However, the check should be hi < 4.

Assume the method gets an argument of 1024, and hi is 4. The following

self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.

generates bytecode 176, which is an arithmetic message send instead of a
conditional jump.

I've attached a changeset that checks both jump conditions for hi < 4.

Regards,
André
'From Squeak3.10.1 of ''26 May 2008'' [latest update: #7175] on 22 June 2008 at 12:36:17 pm'!

!BytecodeGenerator methodsFor: 'private' stamp: 'awe 6/22/2008 12:35'!
jump: distance if: condition

       | hi |
       distance = 0 ifTrue: [
               "jumps to fall through, no-op"
               ^ self nextPut: (Bytecodes at: #popStackBytecode)].
       condition ifTrue: [
               hi := distance // 256.
               hi < 4 ifFalse: [self error: 'true jump too big'].
               self nextPut: (Bytecodes at: #longJumpIfTrue) first + hi.
               self nextPut: distance \\ 256.
       ] ifFalse: [
               distance <= 8 ifTrue: [
                       self nextPut: (Bytecodes at: #shortConditionalJump) first + distance - 1.
               ] ifFalse: [
                       hi := distance // 256.
                       hi < 4 ifFalse: [self error: 'false jump too big'].
                       self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.
                       self nextPut: distance \\ 256.
               ].
       ]      
! !






Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] Bug in BytecodeGenerator>>#jump:if:

Mathieu SUEN
Cool
On Jun 24, 2008, at 10:51 AM, Marcus Denker wrote:

>
>
> On Sun, Jun 22, 2008 at 6:11 PM, stephane ducasse <[hidden email]
> > wrote:
> Normally there is a mailing-list for the newcompiler but indeed we  
> should have a bugtracker.
>
> I have now set up a google code bugtracker for the NewCompiler:
>
>    http://code.google.com/p/squeaknewcompiler/issues/
>
>    Marcus
>
>
>
> Stef
>
> On Jun 22, 2008, at 12:54 PM, André Wendt wrote:
>
> Hi all,
>
> apologies if this doesn't belong here. Mantis is a usability-nightmare
> and I didn't know where else to report a bug in the NewCompiler. The
> corresponding ML on squeakfoundation.org hasn't been posted to in over
> six months.
>
> I've noticed that BytecodeGenerator>>#jump:if: has a bug when given a
> distance >= 1024.
>
> It states
>
> hi := distance // 256.
>
> and then goes on checking if hi < 8. If that's false, the jump is
> assumed to be too big. However, the check should be hi < 4.
>
> Assume the method gets an argument of 1024, and hi is 4. The following
>
> self nextPut: (Bytecodes at: #longJumpIfFalse) first + hi.
>
> generates bytecode 176, which is an arithmetic message send instead  
> of a
> conditional jump.
>
> I've attached a changeset that checks both jump conditions for hi < 4.
>
> Regards,
> André
> 'From Squeak3.10.1 of ''26 May 2008'' [latest update: #7175] on 22  
> June 2008 at 12:36:17 pm'!
>
> !BytecodeGenerator methodsFor: 'private' stamp: 'awe 6/22/2008 12:35'!
> jump: distance if: condition
>
>        | hi |
>        distance = 0 ifTrue: [
>                "jumps to fall through, no-op"
>                ^ self nextPut: (Bytecodes at: #popStackBytecode)].
>        condition ifTrue: [
>                hi := distance // 256.
>                hi < 4 ifFalse: [self error: 'true jump too big'].
>                self nextPut: (Bytecodes at: #longJumpIfTrue) first +  
> hi.
>                self nextPut: distance \\ 256.
>        ] ifFalse: [
>                distance <= 8 ifTrue: [
>                        self nextPut: (Bytecodes at:  
> #shortConditionalJump) first + distance - 1.
>                ] ifFalse: [
>                        hi := distance // 256.
>                        hi < 4 ifFalse: [self error: 'false jump too  
> big'].
>                        self nextPut: (Bytecodes at:  
> #longJumpIfFalse) first + hi.
>                        self nextPut: distance \\ 256.
>                ].
>        ]
> ! !
>
>
>
>
>

        Mth