Smalltalk › Squeak › Squeak - Dev

#testBecomeForward fails when simulated in a rush

‹ Previous Topic Next Topic ›
 
classic Classic list List threaded Threaded
♦
♦
 5 messages Options
Christoph Thiede
Reply | Threaded
Open this post in threaded view
♦
♦
|

#testBecomeForward fails when simulated in a rush

Christoph Thiede

Hi all,


it's Sunday afternoon (at least in my timezone :D) and here's an interesting weekend issue for you, maybe even a bug in the VM:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | c isDead].

In my fresh trunk image, this expression always fails, i.e. a TestFailure is raised from the second assertion in the test, pt3 == pt2.

However, when running the test normally, i.e. executing the first block without simulation, the test passes as expected.

Also, when I try to debug the simulation, the test passes again.

But I even inserted a halt around the (pt3 == pt2) and could see that it actually evaluated to false.


And when I change the expression like this, it suddenly passes:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | Smalltalk garbageCollect. c isDead].

What is this? Does the VM require some kind of memory sweep before the object identity consistency is restored again? To me, this looks pretty suspicious at least.


In SqueakJS, I cannot reproduce the issue, i.e., since https://github.com/codefrau/SqueakJS/pull/117, #testBecomeForward never fails.


Best,

Christoph



Carpe Squeak!
Eliot Miranda-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: #testBecomeForward fails when simulated in a rush

Eliot Miranda-2
Hi Christoph,

On Sun, Mar 14, 2021 at 8:48 AM Thiede, Christoph <[hidden email]> wrote:

Hi all,


it's Sunday afternoon (at least in my timezone :D) and here's an interesting weekend issue for you, maybe even a bug in the VM:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | c isDead].

In my fresh trunk image, this expression always fails, i.e. a TestFailure is raised from the second assertion in the test, pt3 == pt2.

However, when running the test normally, i.e. executing the first block without simulation, the test passes as expected.

Also, when I try to debug the simulation, the test passes again.

But I even inserted a halt around the (pt3 == pt2) and could see that it actually evaluated to false.


And when I change the expression like this, it suddenly passes:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | Smalltalk garbageCollect. c isDead].

What is this? Does the VM require some kind of memory sweep before the object identity consistency is restored again? To me, this looks pretty suspicious at least.


Thank you.  You've found a bug in the interpreted version of primitive 110.  It read

primitiveIdentical
"is the receiver/first argument the same object as the (last) argument?.
pop argumentCount because this can be used as a mirror primitive."
| thisObject otherObject |
otherObject := self stackValue: 1.
thisObject := self stackTop.
((objectMemory isOopForwarded: otherObject)
or: [argumentCount > 1
and: [objectMemory isOopForwarded: thisObject]])
ifTrue:
[self primitiveFailFor: PrimErrBadArgument]
ifFalse:
[self pop: argumentCount + 1 thenPushBool: thisObject = otherObject]

but this is the wrong way around.  It should read

primitiveIdentical
"is the receiver/first argument the same object as the (last) argument?.
pop argumentCount because this can be used as a mirror primitive."
| thisObject otherObject |
thisObject := self stackValue: 1.
otherObject := self stackTop.
((objectMemory isOopForwarded: otherObject)
or: [argumentCount > 1
and: [objectMemory isOopForwarded: thisObject]])
ifTrue:
[self primitiveFailFor: PrimErrBadArgument]
ifFalse:
[self pop: argumentCount + 1 thenPushBool: thisObject = otherObject]

The invariant on invoking a primitive is that the receiver is unforwarded.  This is because it being the receiver of the message, the message send machinery ensures it is not forwarded (if it was, the cached message send logic would fail, and the slow lookup path checks for a forwarded receiver, unforwarding and repeating the send if so).  But primitive arguments could be forwarded.  So primitiveIdentical must check for a potentially forwarded argument.  Stupidly I had written this carelessly and had not got the variable names the right way round and hence was checking if the receiver was forwarded, not the argument.

In SqueakJS, I cannot reproduce the issue, i.e., since https://github.com/codefrau/SqueakJS/pull/117, #testBecomeForward never fails.

SqueakJS doesn't use transparent forwarding.  You'll also find that it won't fail in a pre-Spur image.

Best,

Christoph

_,,,^..^,,,_
best, Eliot
Christoph Thiede
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: #testBecomeForward fails when simulated in a rush

Christoph Thiede

Hi Eliot,


thanks for fixing it! I still don't understand why the bug only occurred during simulation, but glad you were able to find the cause. :-)


Best,
Christoph
Von: Squeak-dev <[hidden email]> im Auftrag von Eliot Miranda <[hidden email]>
Gesendet: Dienstag, 16. März 2021 20:43:41
An: The general-purpose Squeak developers list
Betreff: Re: [squeak-dev] #testBecomeForward fails when simulated in a rush
 
Hi Christoph,

On Sun, Mar 14, 2021 at 8:48 AM Thiede, Christoph <[hidden email]> wrote:

Hi all,


it's Sunday afternoon (at least in my timezone :D) and here's an interesting weekend issue for you, maybe even a bug in the VM:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | c isDead].

In my fresh trunk image, this expression always fails, i.e. a TestFailure is raised from the second assertion in the test, pt3 == pt2.

However, when running the test normally, i.e. executing the first block without simulation, the test passes as expected.

Also, when I try to debug the simulation, the test passes again.

But I even inserted a halt around the (pt3 == pt2) and could see that it actually evaluated to false.


And when I change the expression like this, it suddenly passes:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | Smalltalk garbageCollect. c isDead].

What is this? Does the VM require some kind of memory sweep before the object identity consistency is restored again? To me, this looks pretty suspicious at least.


Thank you.  You've found a bug in the interpreted version of primitive 110.  It read

primitiveIdentical
"is the receiver/first argument the same object as the (last) argument?.
pop argumentCount because this can be used as a mirror primitive."
| thisObject otherObject |
otherObject := self stackValue: 1.
thisObject := self stackTop.
((objectMemory isOopForwarded: otherObject)
or: [argumentCount > 1
and: [objectMemory isOopForwarded: thisObject]])
ifTrue:
[self primitiveFailFor: PrimErrBadArgument]
ifFalse:
[self pop: argumentCount + 1 thenPushBool: thisObject = otherObject]

but this is the wrong way around.  It should read

primitiveIdentical
"is the receiver/first argument the same object as the (last) argument?.
pop argumentCount because this can be used as a mirror primitive."
| thisObject otherObject |
thisObject := self stackValue: 1.
otherObject := self stackTop.
((objectMemory isOopForwarded: otherObject)
or: [argumentCount > 1
and: [objectMemory isOopForwarded: thisObject]])
ifTrue:
[self primitiveFailFor: PrimErrBadArgument]
ifFalse:
[self pop: argumentCount + 1 thenPushBool: thisObject = otherObject]

The invariant on invoking a primitive is that the receiver is unforwarded.  This is because it being the receiver of the message, the message send machinery ensures it is not forwarded (if it was, the cached message send logic would fail, and the slow lookup path checks for a forwarded receiver, unforwarding and repeating the send if so).  But primitive arguments could be forwarded.  So primitiveIdentical must check for a potentially forwarded argument.  Stupidly I had written this carelessly and had not got the variable names the right way round and hence was checking if the receiver was forwarded, not the argument.

In SqueakJS, I cannot reproduce the issue, i.e., since https://github.com/codefrau/SqueakJS/pull/117, #testBecomeForward never fails.

SqueakJS doesn't use transparent forwarding.  You'll also find that it won't fail in a pre-Spur image.

Best,

Christoph

_,,,^..^,,,_
best, Eliot


Carpe Squeak!
Eliot Miranda-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: #testBecomeForward fails when simulated in a rush

Eliot Miranda-2
Hi Christoph,

On Tue, Mar 16, 2021 at 3:23 PM Thiede, Christoph <[hidden email]> wrote:

Hi Eliot,


thanks for fixing it! I still don't understand why the bug only occurred during simulation, but glad you were able to find the cause. :-)


The normal VM is a JIT VM.  That VM has JIT versions of some primitives, and in particular it has JIT versions of the #== and #~~ primitives.  These primitives were correct, hiding the bugs in the interpreter versions. The first time the JIT VM sends a particular message (more accurately if it sends a message and the target method is not in the interpreter's method lookup cache) the VM interprets the method.  The next time, because the method is in the interpreter's method lookup cache, the method will be JITTED and the machine code will be executed.  So the interpreter version of #== and #~~ are only executed once each early in system startup.  From then on the JIT version is used.

In simulation primitives are evaluated via the tryPrimitive:withArguments: primitive and this primitive always runs the interpreter's version of a primitive.  Hence in simulation one is always using the interpreter's primitives and so one sees the bug.

Best,
Christoph
Von: Squeak-dev <[hidden email]> im Auftrag von Eliot Miranda <[hidden email]>
Gesendet: Dienstag, 16. März 2021 20:43:41
An: The general-purpose Squeak developers list
Betreff: Re: [squeak-dev] #testBecomeForward fails when simulated in a rush
 
Hi Christoph,

On Sun, Mar 14, 2021 at 8:48 AM Thiede, Christoph <[hidden email]> wrote:

Hi all,


it's Sunday afternoon (at least in my timezone :D) and here's an interesting weekend issue for you, maybe even a bug in the VM:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | c isDead].

In my fresh trunk image, this expression always fails, i.e. a TestFailure is raised from the second assertion in the test, pt3 == pt2.

However, when running the test normally, i.e. executing the first block without simulation, the test passes as expected.

Also, when I try to debug the simulation, the test passes again.

But I even inserted a halt around the (pt3 == pt2) and could see that it actually evaluated to false.


And when I change the expression like this, it suddenly passes:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | Smalltalk garbageCollect. c isDead].

What is this? Does the VM require some kind of memory sweep before the object identity consistency is restored again? To me, this looks pretty suspicious at least.


Thank you.  You've found a bug in the interpreted version of primitive 110.  It read

primitiveIdentical
"is the receiver/first argument the same object as the (last) argument?.
pop argumentCount because this can be used as a mirror primitive."
| thisObject otherObject |
otherObject := self stackValue: 1.
thisObject := self stackTop.
((objectMemory isOopForwarded: otherObject)
or: [argumentCount > 1
and: [objectMemory isOopForwarded: thisObject]])
ifTrue:
[self primitiveFailFor: PrimErrBadArgument]
ifFalse:
[self pop: argumentCount + 1 thenPushBool: thisObject = otherObject]

but this is the wrong way around.  It should read

primitiveIdentical
"is the receiver/first argument the same object as the (last) argument?.
pop argumentCount because this can be used as a mirror primitive."
| thisObject otherObject |
thisObject := self stackValue: 1.
otherObject := self stackTop.
((objectMemory isOopForwarded: otherObject)
or: [argumentCount > 1
and: [objectMemory isOopForwarded: thisObject]])
ifTrue:
[self primitiveFailFor: PrimErrBadArgument]
ifFalse:
[self pop: argumentCount + 1 thenPushBool: thisObject = otherObject]

The invariant on invoking a primitive is that the receiver is unforwarded.  This is because it being the receiver of the message, the message send machinery ensures it is not forwarded (if it was, the cached message send logic would fail, and the slow lookup path checks for a forwarded receiver, unforwarding and repeating the send if so).  But primitive arguments could be forwarded.  So primitiveIdentical must check for a potentially forwarded argument.  Stupidly I had written this carelessly and had not got the variable names the right way round and hence was checking if the receiver was forwarded, not the argument.

In SqueakJS, I cannot reproduce the issue, i.e., since https://github.com/codefrau/SqueakJS/pull/117, #testBecomeForward never fails.

SqueakJS doesn't use transparent forwarding.  You'll also find that it won't fail in a pre-Spur image.

Best,

Christoph

_,,,^..^,,,_
best, Eliot



--
_,,,^..^,,,_
best, Eliot
Christoph Thiede
Reply | Threaded
Open this post in threaded view
♦
♦
|

Re: #testBecomeForward fails when simulated in a rush

Christoph Thiede

Thanks for your explanation! :-)


Best,

Christoph

Von: Squeak-dev <[hidden email]> im Auftrag von Eliot Miranda <[hidden email]>
Gesendet: Donnerstag, 18. März 2021 20:20:25
An: The general-purpose Squeak developers list
Betreff: Re: [squeak-dev] #testBecomeForward fails when simulated in a rush
 
Hi Christoph,

On Tue, Mar 16, 2021 at 3:23 PM Thiede, Christoph <[hidden email]> wrote:

Hi Eliot,


thanks for fixing it! I still don't understand why the bug only occurred during simulation, but glad you were able to find the cause. :-)


The normal VM is a JIT VM.  That VM has JIT versions of some primitives, and in particular it has JIT versions of the #== and #~~ primitives.  These primitives were correct, hiding the bugs in the interpreter versions. The first time the JIT VM sends a particular message (more accurately if it sends a message and the target method is not in the interpreter's method lookup cache) the VM interprets the method.  The next time, because the method is in the interpreter's method lookup cache, the method will be JITTED and the machine code will be executed.  So the interpreter version of #== and #~~ are only executed once each early in system startup.  From then on the JIT version is used.

In simulation primitives are evaluated via the tryPrimitive:withArguments: primitive and this primitive always runs the interpreter's version of a primitive.  Hence in simulation one is always using the interpreter's primitives and so one sees the bug.

Best,
Christoph
Von: Squeak-dev <[hidden email]> im Auftrag von Eliot Miranda <[hidden email]>
Gesendet: Dienstag, 16. März 2021 20:43:41
An: The general-purpose Squeak developers list
Betreff: Re: [squeak-dev] #testBecomeForward fails when simulated in a rush
 
Hi Christoph,

On Sun, Mar 14, 2021 at 8:48 AM Thiede, Christoph <[hidden email]> wrote:

Hi all,


it's Sunday afternoon (at least in my timezone :D) and here's an interesting weekend issue for you, maybe even a bug in the VM:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | c isDead].

In my fresh trunk image, this expression always fails, i.e. a TestFailure is raised from the second assertion in the test, pt3 == pt2.

However, when running the test normally, i.e. executing the first block without simulation, the test passes as expected.

Also, when I try to debug the simulation, the test passes again.

But I even inserted a halt around the (pt3 == pt2) and could see that it actually evaluated to false.


And when I change the expression like this, it suddenly passes:


p := [ObjectTest new testBecomeForward] newProcess.
p runUntil: [:c | Smalltalk garbageCollect. c isDead].

What is this? Does the VM require some kind of memory sweep before the object identity consistency is restored again? To me, this looks pretty suspicious at least.


Thank you.  You've found a bug in the interpreted version of primitive 110.  It read

primitiveIdentical
"is the receiver/first argument the same object as the (last) argument?.
pop argumentCount because this can be used as a mirror primitive."
| thisObject otherObject |
otherObject := self stackValue: 1.
thisObject := self stackTop.
((objectMemory isOopForwarded: otherObject)
or: [argumentCount > 1
and: [objectMemory isOopForwarded: thisObject]])
ifTrue:
[self primitiveFailFor: PrimErrBadArgument]
ifFalse:
[self pop: argumentCount + 1 thenPushBool: thisObject = otherObject]

but this is the wrong way around.  It should read

primitiveIdentical
"is the receiver/first argument the same object as the (last) argument?.
pop argumentCount because this can be used as a mirror primitive."
| thisObject otherObject |
thisObject := self stackValue: 1.
otherObject := self stackTop.
((objectMemory isOopForwarded: otherObject)
or: [argumentCount > 1
and: [objectMemory isOopForwarded: thisObject]])
ifTrue:
[self primitiveFailFor: PrimErrBadArgument]
ifFalse:
[self pop: argumentCount + 1 thenPushBool: thisObject = otherObject]

The invariant on invoking a primitive is that the receiver is unforwarded.  This is because it being the receiver of the message, the message send machinery ensures it is not forwarded (if it was, the cached message send logic would fail, and the slow lookup path checks for a forwarded receiver, unforwarding and repeating the send if so).  But primitive arguments could be forwarded.  So primitiveIdentical must check for a potentially forwarded argument.  Stupidly I had written this carelessly and had not got the variable names the right way round and hence was checking if the receiver was forwarded, not the argument.

In SqueakJS, I cannot reproduce the issue, i.e., since https://github.com/codefrau/SqueakJS/pull/117, #testBecomeForward never fails.

SqueakJS doesn't use transparent forwarding.  You'll also find that it won't fail in a pre-Spur image.

Best,

Christoph

_,,,^..^,,,_
best, Eliot



--
_,,,^..^,,,_
best, Eliot


Carpe Squeak!
Free forum by Nabble Edit this page