Re: [Pharo-dev] eventual crashes pharo vm
Locked
Re: [Pharo-dev] eventual crashes pharo vm
 
|
Hi Henry, Hi Marcus,
On Sun, Aug 13, 2017 at 5:08 AM, henry <[hidden email]> wrote:
I took a look at this and I think you've found a bug in the mustBeBooleanMagic: code. What's happening is a mustBeBoolean in Integer>>* due to evaluating 10 * 42 eventual in RefsTest>>testFailureArithmeticPrimitivesWithPromiseArgument Since 42 eventual is a NearERef the SmallInteger>>* primitive fails and does ^super * anInteger (where anInteger is the NearERef). So that evaluates Integer>>* Integer>>* aNumber "Refer to the comment in Number * " aNumber isInteger ifTrue: [^ self digitMultiply: aNumber neg: self negative ~~ aNumber negative]. ^ aNumber adaptToInteger: self andSend: #* aNumber, being a NearERef, answers a PromiseERef for the isInteger send, and this provokes a mustBeBoolean for the isInteger ifTrue: [... After the mustBeBooleanMagic: the stack looks wrong. The activation of Integer>>*, which is about to do ^ aNumber adaptToInteger: self andSend: #* does not have enough items on the stack. Instead of containing a NearERef (for 42) 10 #* it contains a PromiseERef (for 42 eventual isInteger) and the send of #adaptToInteger:andSend: ends up taking more form the stack than the VM can handle and it crashes. The bug appears to be with the use of sendNode irInstruction nextBytecodeOffsetAfterJump in Object>>mustBeBooleanMagic: since execution should resume at bytecode 55 below, but does so at bytecode 57 41 <10> pushTemp: 0 42 <D0> send: isInteger 43 <AC 09> jumpFalse: 54 45 <70> self 46 <10> pushTemp: 0 47 <70> self 48 <D1> send: negative 49 <10> pushTemp: 0 50 <D1> send: negative 51 <E2> send: ~~ 52 <F3> send: digitMultiply:neg: 53 <7C> returnTop 54 <10> pushTemp: 0 55 <70> self 56 <24> pushConstant: #* 57 <F5> send: adaptToInteger:andSend: 58 <7C> returnTop So the positioning of the context's pc must be before any argument marshaling for the next send, not simply the send itself. Put a breakpoint at the end of Object>>mustBeBooleanMagic: and add initlaPC and resumePC temporaries at the beginning and capture them via initialPC := context pc. at the beginning and then context pc: (resumePC := sendNode irInstruction nextBytecodeOffsetAfterJump) to see what I'm seeing. Phew. Glad it's not a VM bug :-) HTH _,,,^..^,,,_ best, Eliot |
Re: [Pharo-dev] eventual crashes pharo vm
|
|
Hi Henry,
On Wed, Aug 16, 2017 at 6:33 PM, henry <[hidden email]> wrote:
I'm not informed enough to know. One could implement mustBeBoolean in the ERef hierarchy and resolve the promise before going on. One could rely on the mustBeBooleanMagic: if one wanted a fully lazy system. I don't know the trade-offs between the two. I do know that while the miustBeBooleanMagic: solution is cool and fun it is extremely slow. So if performance is an issue use the first approach.
Yes that's an issue There is already a problem with #==. It needs to be symmetric for correctness.
Well, the issue is simply that the wrong pc is chosen for the continuation after the mustBeBoolean. I'm sure the right answer is straight-forward to obtain.
You're welcome.
_,,,^..^,,,_ best, Eliot |
|
|
Hi Eliot, I have disabled that test for the time being. It will require some deeper thought regarding immediate selectors, I think, and I am deep into another area right now. As mustBeBoolean is from inside the VM, a different approach may be the right solution. A part of me thinks autocoercion between msg sending (async) and msg calling (sync) is what is needed, but I want continuation-based VatSemaphores to prevent a liveness lock on the event loop to support. Again, I have not thought deep enough in this area. Thanks again for your help, - HH
|
|
|
To let you know what I tried, trying to learn about this system as I dig into it, I added isInteger to the #blockedSelectors of NearERef. This crashed the squeak vm when I ran the test, and as I look into how blockedSelectors is used it smashes the resolver. What seems to be needed is a list of immediateSelectors, though that may grow to be a sizeable list. This causes me to think there must be a different approach between async and sync code. - HH
|
Re: [Pharo-dev] eventual crashes pharo vm
|
|
Hi Henry,
On Wed, Aug 16, 2017 at 9:04 PM, henry <[hidden email]> wrote:
Please wait until mustBeBooleanMagic: is fixed before you write this off. In Squeak what happens if you implement mustBeBoolean to resolve the promise?
_,,,^..^,,,_ best, Eliot |
Re: [Pharo-dev] eventual crashes pharo vm
|
|
Even with the bug fixed, if every send is lazily recorded for later evaluation, how does the framework deal with non-local branched returns though?
Seems to me, branching selectors need to be blocking points; otherwise, (like in this code) if the PromiseERef records it should evaluate ifTrue: with the block argument later as the needed value is as yet missing, it will also record the following adaptToInteger: which should only occur if value was false. A possible fix *: nextBytecodeOffsetAfterJump "check if we are in ifTrue:ifFalse: / ifFalse:ifTrue: or in ifTrue: / ifFalse: and answers the next byte code offset" ^destination last isJump ifTrue: [ destination last destination first bytecodeOffset ] ifFalse: [ (destination sequence at: (destination sequence size min: 2)) bytecodeOffset ] new test, crashes with old version of nextBytecodeOffsetAfterJump , passes with new (it and the others): testIfTrueWithCallReturnAfterTrueBlock | myBooleanObject | myBooleanObject := MyFalseBooleanObject new. self should: [10 * myBooleanObject] raise: MessageNotUnderstood (With MyFalseBooleanObject subclass of MyBooleanObject, but ifTrue: implementation doing nothing) Cheers, Henry *Don't ask me how /if I'm sure it works in all cases, I just checked that tests pass... (...but the idea is we want to skip the pushTemp (if present, which seems the case other than when followed by block, in which case sequence size is 1), since the deoptimized node has been called as a method, and its return will already be on stack.?.?.). A better implementation would probably be asserting the instruction we are skipping is in fact the temp push. |
Re: [Pharo-dev] eventual crashes pharo vm
|
|
Hi Henrik, > On Aug 18, 2017, at 5:30 AM, Henrik Sperre Johansen <[hidden email]> wrote: > > > Even with the bug fixed, if every send is lazily recorded for later > evaluation, how does the framework deal with non-local branched returns > though? > > Seems to me, branching selectors need to be blocking points; otherwise, > (like in this code) if the PromiseERef records it should evaluate ifTrue: > with the block argument later as the needed value is as yet missing, it will > also record the following adaptToInteger: which should only occur if value > was false. Wouldn't it be the case that the continuation gets created but never executed? There is nothing to force the adaptToInteger path to be executed unless the wrong answer to isInteger is computed. So I think this is more of a performance issue than a correctness issue. > > A possible fix *: > nextBytecodeOffsetAfterJump > "check if we are in ifTrue:ifFalse: / ifFalse:ifTrue: or in ifTrue: / > ifFalse: and answers the next byte code offset" > ^destination last isJump > ifTrue: [ destination last destination first bytecodeOffset ] > ifFalse: [ > (destination sequence at: (destination sequence size min: 2)) > bytecodeOffset ] > > new test, crashes with old version of nextBytecodeOffsetAfterJump , passes > with new (it and the others): > testIfTrueWithCallReturnAfterTrueBlock > | myBooleanObject | > myBooleanObject := MyFalseBooleanObject new. > self should: [10 * myBooleanObject] raise: MessageNotUnderstood > > (With MyFalseBooleanObject subclass of MyBooleanObject, but ifTrue: > implementation doing nothing) Cool! > > Cheers, > Henry > > *Don't ask me how /if I'm sure it works in all cases, I just checked that > tests pass... (...but the idea is we want to skip the pushTemp (if present, > which seems the case other than when followed by block, in which case > sequence size is 1), since the deoptimized node has been called as a method, > and its return will already be on stack.?.?.). A better implementation would > probably be asserting the instruction we are skipping is in fact the temp > push. Well I would assert that the stack depth is correct. In VMMaker.oscog you can find StackDepthFinder, a subclass of InstructionStream that computes the stack depth at each bytecode. This can be used to check that the stack pointer of the sender context of mustBeBoolean agrees with the pc that mustBeBooleanMagic: chooses to resume at. The vm simulator uses StackDepthFinder to check stack depth at every send and non-local return. It has found lots of bugs. HTH > -- > View this message in context: http://forum.world.st/Re-Pharo-dev-eventual-crashes-pharo-vm-tp4961763p4962031.html > Sent from the Squeak VM mailing list archive at Nabble.com. Best, Eliot (phone) |
«
Return to Squeak VM
|
1 view|%1 views
| Free forum by Nabble | Edit this page |