Hi all

Picking up an old discussion, I'd like to include both of Eliot fixes
regarding contextOn:do: and ensure into trunk before release.
Any vetoes?

Best regards
	-Tobias


On 31.03.2015, at 23:10, Eliot Miranda <eliot.miranda at gmail.com> wrote:

> Hi Tobias,
> 
>     first let me sympathize.  This is such horrible code :-(.  Ovr the years, getting this code to work with Cog's context-to-stack-mapping machinery has given me headaches :-).
> 
> 
> On Tue, Mar 31, 2015 at 9:01 AM, Tobias Pape <Das.Linux at gmx.de> wrote:
> Hey fellow Squeakers
> 
> [Attention: low level lengthy stuff]
> 
> I today encountered a strange behavior.
> When running the Startup code, somewhen ContextPart>>#complete is called,
> which, in the process issues #contextOn:do:, which subsequently somewhere interanlly
> does a jump:
> 
> contextOn: exceptionClass do: block
>         "Create an #on:do: context that is ready to return from executing its receiver"
> 
>         | ctxt chain |
>         ctxt := thisContext.
>         [chain := thisContext sender cut: ctxt. ctxt jump] on: exceptionClass do: block.
>         "jump above will resume here without unwinding chain"
>         ^ chain
> 
> The idea is that we end up right before '^ chain' as the comment indicates.
> so what happens is that ctxt is the context of #contextOn:do: itself and it gets
> send #jump in the closure.
> 
> Jump now does the following:
> 
> jump
>         "Abandon thisContext and resume self instead (using the same current process).  You may want to save thisContext's sender before calling this so you can jump back to it.
>         Self MUST BE a top context (ie. a suspended context or a abandoned context that was jumped out of).  A top context already has its return value on its stack (see Interpreter>>primitiveSuspend and other suspending primitives).
>         thisContext's sender is converted to a top context (by pushing a nil return value on its stack) so it can be jump back to."
> 
>         | top |
>         "Make abandoned context a top context (has return value (nil)) so it can be jumped back to"
>         thisContext sender push: nil.
> 
>         "Pop self return value then return it to self (since we jump to self by returning to it)"
>         stackp = 0 ifTrue: [self stepToSendOrReturn].
>         stackp = 0 ifTrue: [self push: nil].  "must be quick return self/constant"
>         top := self pop.
>         thisContext privSender: self.
>         ^ top
> 
> So. bytecode for #contextOn:do: is:
> 
> 29 <8A 01> push: (Array new: 1)
> 31 <6B> popIntoTemp: 3
> 32 <89> pushThisContext:
> 33 <6A> popIntoTemp: 2
> 34 <12> pushTemp: 2
> 35 <13> pushTemp: 3
> 36 <8F 20 00 0A> closureNumCopied: 2 numArgs: 0 bytes 40 to 49
> 40      <89> pushThisContext:
> 41      <D2> send: sender
> 42      <10> pushTemp: 0
> 43      <E1> send: cut:
> 44      <8E 00 01> popIntoTemp: 0 inVectorAt: 1
> 47      <10> pushTemp: 0
> 48      <D3> send: jump
> 49      <7D> blockReturn
> 50 <10> pushTemp: 0
> 51 <11> pushTemp: 1
> 52 <F0> send: on:do:
> 53 <87> pop
> 54 <8C 00 03> pushTemp: 0 inVectorAt: 3
> 57 <7C> returnTop
> 
> 
> The jump lands right at 53 and does a pop.
> HOWEVER, at this point the stack of this context is empty and the pop actually pops the 3rd temp
> from the temps that 'just happens' to be right under the stack. This should be fatal.
> HOWEVER again, Squeak actually does not pop but only decrement the SP so the temp access still
> works(this _could_ be fine but some  implementations (Eg, RSqueak) tried to separate temps and
> stack; which is not possible currently).
> 
> What could be the problem here?
> - are the 'stackp = 0'-checks in #jump wrong and they actually should check for the actual stack depth _after_ temps?
> 
> It does look like it.  I would have expected this to be more correct:
> 
> jump
> 	"Abandon thisContext and resume self instead (using the same current process).
> 	 You may want to save thisContext's sender before calling this so you can jump back to it.
> 	Self MUST BE a top context (ie. a suspended context or a abandoned context that was jumped
> 	 out of).  A top context already has its return value on its stack (see Interpreter>>primitiveSuspend
> 	 and other suspending primitives). thisContext's sender is converted to a top context (by pushing a
> 	 nil return value on its stack) so it can be jump back to."
> 
> 	| top |
> 	"Make abandoned context a top context (has return value (nil)) so it can be jumped back to"
> 	thisContext sender push: nil.
> 
> 	"Pop self return value then return it to self (since we jump to self by returning to it)"
> 	stackp <= self numTemps ifTrue: [self stepToSendOrReturn].
> 	(stackp <= self numTemps
> 	 and: [self willJustPop]) ifTrue: [self push: nil].  "must be quick return self/constant"
> 
> 	top := self pop.
> 	thisContext privSender: self.
> 	^top
>  
> - should we put in a "sacrificial anode" in #contextOn:do: so that the pop does not pop the empty stack? (like this:
> 
> contextOn: exceptionClass do: block
>         "Create an #on:do: context that is ready to return from executing its receiver"
> 
>         | ctxt chain |
>         ctxt := thisContext.
>         [chain := thisContext sender cut: ctxt.
>          ctxt push: nil. "sacrifical anode"
>          ctxt jump
>         ] on: exceptionClass do: block.
>         "jump above will resume here without unwinding chain"
>         ^ chain
> 
> That looks right.  Once the on:do: is sent the thisContext of contextOn:do:'s stack contains only exceptionClass, block, context and the indirection vector containing chain.  So it is in the stack = self numTemps case.
>  
> 
> - Or is there an even better way?
> 
> I'm not sure the other ways are any better.  The way to transfer to a context without disturbing its stack is to do a process switch.  So you do something equivalent to
> 
> jump
> 	"Abandon thisContext and resume self instead (using the same current process)."
> 
> 	| process semaphore |
> 	process := Processor activeProcess.
> 	semaphore := Semaphore new.
> 
> 	[process suspendedContext unwindTo: self.
> 	 process suspendedContext: self.
> 	 semaphore signal] fork.
> 
> 	semaphore wait
> 
> This way no bizarre stack manipulations are going on, and no return value is pushed, because there isn't a return.  One may get away with:
> 
> jump
> 	"Abandon thisContext and resume self instead (using the same current process)."
> 
> 	[| process |
> 	 process := Processor activeProcess.
> 	 process suspendedContext unwindTo: self.
> 	 process suspendedContext: self] fork.
> 
> 	Processor yield
> 
> I'd be interested in your experience using either of these.  One of the advantages the process switch versions have is in not updating the receiving context sp there's a chance the context-to-stack mapping machinery won't flush the context to the heap.  In the end it might actually be faster.
> -- 
> best,
> Eliot