Hmm... at least you get a debugger. In Squeak 5.3, you are not able to put a "self halt" into that event handler in test03EventHandler. ^__^

Yet, I noticed that when stepping over that "^ self dispatchEvent:..." in #dispatchMouseDown:with:, a second debugger appears. That's a bug. The expected behavior would be that the same debugger shows the "halt." I think.

Best, Marcel Am 17.04.2021 10:58:06 schrieb Jaromir Matas m@jaromir.net: Hi Nicolas,

Nicolas Cellier wrote

When debugging things like this:

[^2] ensure: [Transcript cr; show: 'done'].

if we step into the protected block [^2], then step over ^2, we incorrectly get a BlockCannotReturn.

Hmm, that's the bug that plagued #terminate and caused unwind errors... I didn't fully realize then; I simply avoided it by eliminating the simulation code from #terminate :) This behavior happens when the VM calls #aboutToReturn:through: and the Debugger executes #stepOver it (or over #return:through: or #resume:through:) - sorry if I'm stating the obvious; unfortunately I have negligible knowledge of the Debugger. I'm really looking forward to seeing a solution! Thanks, best,

----- ^[^ Jaromir -- Sent from: http://forum.world.st/Squeak-Dev-f45488.html

Hi Christoph,

Christoph Thiede wrote

But the fact that the guard contexts are not removed again should not lead to a cannot return situation. Instead, I would rather have expected that the UI process would be terminated. But somehow, the termination in BlockClosure >> #newProcess fails. I observed that primitive 88 in Process

...

...

#suspend fails so the execution continues right from the bottom context

of the process. I'm not sure why primitiveSuspend fails here. Might this be related to the changes in termination logic? Jaromir? :-)

In my opinion the problem is that when the Debugger invokes #stepOver over a non-local return, it invokes #evaluate:onBehalfOf: down the chain of sends to execute the non-local return and that's where the problem lies: #evaluate:onBehalfOf: incorrectly executes the non-local return on the wrong stack - the debugger process uses its own stack to have #evaluate:onBehalfOf: executed the non-local return ([^2]) which fails to find its home context which is indeed on a different stack (original process's stack). The cannot return is just a consequence of the failure to find the home context...

At least this is my working hypothesis :)

Compared to #evaluate:onBehalfOf:, the #runUntilErrorOrReturnFrom: method itself jumps right into the debugged process's stack - which results in flawless execution of the non-local return! It finds it's home context and jumps into it and continues execution.

I, however, haven't explored the behavior of the combination of #evaluate:onBehalfOf: and #runUntilErrorOrReturnFrom: :o As I said my Debugger knowledge is still negligible, unfortunately.

Prior to your change from active to genuine process the `[^2] ensure: []` example caused the infinite debugger loop... after that it only causes the cannot return error. So I guess it's unlikely the change in #terminate somehow affected the bug's behavior; the bug was there before and behaves similarly.

The change in #teminate only eliminated the use #evaluate:onBehalfOf: in the termination procedure...

I'm looking at it from a different angle now, I'll keep you posted... Thanks, best,

----- ^[^ Jaromir -- Sent from: http://forum.world.st/Squeak-Dev-f45488.html

Hi Nicolas,

thanks for your quick response; stepping over #aboutToReturn:through: now works perfectly, however the same problem remains on lower levels, i.e. for stepping over #return:through: and #resume:through: - same example, same incorrect behavior:

[^2] ensure: [Transcript showln: 'done']

step into ^2 then repeat step over until you step over the return:through: and cannot return is back. Same on the level below: step into ^2 then into return:through: and then step over #resume:through: - and again, you get the cannot return error. Once you're inside #resume:through: it seems safe.

So I'm wondering whether it's possible to invoke a simulated version of #resume:through: somehow instead of #aboutToReturn:through: - I guess you'd know the answer :)

BTW: I tried your fix in Cuis and it works perfectly there as well :)

During testing I realized I'd missed one more detail in my #terminate fix so here's the latest version in the Inbox: Kernel-jar.1408 (including your fix Kernel-nice.1407)

The point is the Debugger actually resumes after finding an error and only updates its title ( which I never fully noticed :o ) but during termination each nested error opens a new debugger - this means I can't override #runUntilErrorOrReturnFrom: as I suggested previously so I created its copy named #runUnwindUntilErrorOrReturnFrom: with the modified functionality required by #terminate. (It's a code duplication, I know, but I'd like to get it working now)

On top of that I extracted a repeating part of #terminate's code to a new method #complete:to: to improve readability and avoid further code duplication.

So you can now debug safely (and correctly) even examples like:

[self error] ensure: [^2]

Thanks again,

----- ^[^ Jaromir -- Sent from: http://forum.world.st/Squeak-Dev-f45488.html

Hi Christoph,

I have just updated runUntilErrorOrReturnFrom.cs as proposed in [1], which notifies the debugger about risky context manipulations such as #jump, #swapSender:, and now also #resume:through:. I think that this is a more holistic approach than #simulatedAboutToReturn:through:.

Debugger implementation is still out of my league so at least a few observations:

\1. You inserted `here push: nil` line into #runUntilErrorOrReturnFrom: I guess it's because you wanted to make it a top context but I thought #jump already takes care of that with its first line:

`thisContext sender push: nil`

Please correct me if I'm wrong, I'm very interested :)

\2 Your #nextHandlerContext predates and thus removes Nicolas's changes made recently - is it just accidental or intentional?

...

however the same problem remains on lower levels, i.e. for stepping over #return:through: and #resume:through: - same example, same incorrect behavior:

I fully agree with this. While Nicolas's fix makes this particular situation easier to debug, IMHO it is only fighting the symptoms. In my opinion, we should instead fix the underlying problem of dangling guard contexts in #runUntilErrorOrReturnFrom:.

I've tested your changeset against the famous example:

`[self error] ensure: [^2]`

If you step through until you get to ^2 and then step over the ^2 - you get the cannot return error again (without Nicolas's fix).

My opinion is let's get this working using whichever approach (I was wondering if it would help to create a simulated version of #return:through: instead of #aboutToReturn:through:) and build on that. At least we'll have a better understanding of the mechanism :)

Thanks and regards,

----- ^[^ Jaromir -- Sent from: http://forum.world.st/Squeak-Dev-f45488.html

Hi Christoph,

Christoph Thiede wrote

Hi Jaromir,

thanks for the feedback. \2 and \3 were both minor slips which I have corrected in version 7 of the changeset, see: http://forum.world.st/BUG-REGRESSION-while-debugging-Generator-nextPut-tp510...

\1: Fair question. I think I stumbled upon some situation where stepping *over* #runUntilErrorOrReturnFrom: has not worked for me without these lines. I just had inserted the "push: nil" without thinking about it in detail, just because it had also worked for me in Context class >> #contextEnsure: and #contextOn:do: in the last year (this was also a very interesting bug, you can read the full story in the mailing archives if you are interested). But yes, that is suspicious, I need to recheck this because I cannot reproduce the need for this patch.

I've updated http://forum.world.st/BUG-s-in-Context-control-jump-runUntilErrorOrReturnFro... with my explanation why I /think/ #runUntilErrorOrReturnFrom: works correctly.

Christoph Thiede wrote

\2: This was indeed a slip because I forgot to update the image. I have moved my patch to #findNextHandlerContext - it makes the method robust against bottom-contexts that do not have a sender (i.e., sender is nil).

The changeset still seems to have the old version of #runUntilErrorOrReturnFrom: and #nextHandlerContext nixing Nicolas's changes made in the meantime...

Christoph Thiede wrote

\3: Ah, in this case, the unwind context was already marked as complete. :-) Since we appear to need the debugger information anyway, I have moved the #informDebuggerAboutContextSwitchTo: in #resume:through: before the check so your example now should work, too.

Yes!, my non-local examples work OK now, your scenarios as well; I'll keep it in my images to give it a ride :) As for the general approach - I look forward to learning from experts ;)

later,

----- ^[^ Jaromir -- Sent from: http://forum.world.st/Squeak-Dev-f45488.html

Hi Jaromir,

...

\2: This was indeed a slip because I forgot to update the image. I have moved my patch to #findNextHandlerContext - it makes the method robust against bottom-contexts that do not have a sender (i.e., sender is nil).

The changeset still seems to have the old version of #runUntilErrorOrReturnFrom: and #nextHandlerContext nixing Nicolas's changes made in the meantime...

Version 8 removes roerf finally. :-) But I could not find any trace of #nextHandlerContext in the current changeset, did you maybe forget to revert the previous version before loading v7?

What would you think about this approach: because #return:from: supplies the first unwind context for #aboutToReturn:through: prematurely, how about to supply nil instead of the first unwind context and let #resume:through: find the first unwind context at precisely the right time?

Correct me if I'm wrong, but this only would move the problem again, wouldn't it? If you press over too late, we would have the same problem again? I'd still prefer a holistic approach such as my #informDebuggerAboutContextSwitchTo: proposal. Or did miss anything different with your proposal? :-)

Best, Christoph

Hi Jaromir,

I have just worked through your proposal (Kernel-jar.1413) again. So as I understand it, you prevent the errors while stepping over a non-local return by deferring the pivotal search for the first unwind context. By sending #findNextUnwindContextUpTo: again in #resume:through:, you can rule out any other guard context from #runUntilErrorOrReturnFrom: that has already been uninstalled again when the unwindBlock is finally invoked. I am unable to debunk the correctness of your patch neither by theory nor by experiment. :-)

So it's a very genius patch, but at the same time, still a tiny hack. You are changing the semantics of the firstUnwindCtxt param of #aboutToReturn:through: - as long as the VM guarantees that it will never ever invoke this method with firstUnwindContext = nil, this will be fine, otherwise, this could lead to hypothetical confusion in the future. But this hack is really tiny only, so I would not bother ...

Nevertheless, the central argument of my previous message was a different one: Your patch is only required to make the code compatible with the simulator - the controlling logic does not contain any bug itself. That means that if we merge your proposal, we are coupling the execution logic and the simulator even closer together, and coupling of (theoretically) independent components is bad, as you know for sure ... As opposed to this, my proposed solution (runUntilErrorOrReturnFrom.cs with #informDebuggerAboutContextSwitchTo:, see [1]) defines an explicit hook for this communication, so the coupling is reduced.

And given the results from [1], I think (or hope) that we would need this hook anyway, so this would make your patch redundant, wouldn't it? :-) I also see your argument about false positives, which are indeed tedious, but your idea to recognize harmless jumps is ingenious again - I think this should be possible (I will elaborate further on this in [1]), but such a detection mechanism would be significantly more complex to implement ...

So in the end it all boils down to the question of striving an abstract form of elegancy - by keeping executor and simulator decoupled from each other - on the one hand versus reducing implementation complexity for the simulator (#informDebuggerAboutContextSwitchTo:). As we still do not have a complete overview of the entire Context implementation, the elegant solution would have to benefit of protecting ourselves from future similar incidents that would be preventable via informDebuggerAboutContextSwitchTo:. Elegancy by a single hook, or multilpe hooks with a simpler implementation? I really don't know which one is better ... Maybe you have some other arguments? If not, I guess merging your proposal would be fine - but in this case, I would request you to add a small flagged comment in both changed methods there (something like "self flag: #simulator" followed by a short explanation of the reasoning behind this nil argument, maybe also add a link to this mail conversation).

Woah - sorry for the long wall of text! This is a delightful topic, and it's a joy to discuss it with you. :-)

Best, Christoph

[1] http://lists.squeakfoundation.org/pipermail/squeak-dev/2021-May/215492.html

--- Sent from Squeak Inbox Talk

On 2021-05-29T04:38:40-05:00, m@jaromir.net wrote:

Christoph Thiede wrote

...

Hi Jaromir,

...

...

\2: This was indeed a slip because I forgot to update the image. I have

moved my patch to #findNextHandlerContext - it makes the method robust against bottom-contexts that do not have a sender (i.e., sender is nil).

The changeset still seems to have the old version of #runUntilErrorOrReturnFrom: and #nextHandlerContext nixing Nicolas's changes made in the meantime...

Version 8 removes roerf finally. :-) But I could not find any trace of #nextHandlerContext in the current changeset, did you maybe forget to revert the previous version before loading v7?

yes indeed, I forgot to remove the previous version, sorry for confusion, everything's fine :)

Christoph Thiede wrote

...

...

What would you think about this approach: because #return:from: supplies the first unwind context for #aboutToReturn:through: prematurely, how about to supply nil instead of the first unwind context and let #resume:through: find the first unwind context at precisely the right time?

Correct me if I'm wrong, but this only would move the problem again, wouldn't it? If you press over too late, we would have the same problem again? I'd still prefer a holistic approach such as my #informDebuggerAboutContextSwitchTo: proposal. Or did miss anything different with your proposal? :-)

Well, I thought the fix really solved the bug, not just pushed it further away :) I couldn't reproduce the incorrect stepOver behavior any longer but I may have missed some example - do you have something in mind? I'll comment further in [1]; I hope I'm not wrong here - please send a counterexample if you find one :)

best, Jaromir

Christoph Thiede wrote

...

Best, Christoph

[1] http://forum.world.st/BUG-REGRESSION-while-debugging-Generator-gt-gt-nextPut...

---

^[^ Jaromir

Sent from: http://forum.world.st/Squeak-Dev-f45488.html

Hi Christoph, all,

Christoph's right; using nil as a value of firstUnwindContext as a means of communication between #return:from: and #resume:through: is a 'hack' no matter how tiny.

I'm proposing a "cleaner" alternative (Kernel-jar.1415) of the solution: instead of sending nil, #return:from: would send a block that would get evaluated at the right time in #resume:through:, i.e. after #runUntilErrorOrReturnFrom: inserts it's own ensure block (which is the culprit of the bug).

Highlighted below are the changes from the original version of the solution (Kernel-jar.1413):

Context >> return: value from: aSender "For simulation. Roll back self to aSender and return value from it. Execute any unwind blocks on the way. ASSUMES aSender is a sender of self"

| newTop | aSender isDead ifTrue: [^self send: #cannotReturn: to: self with: {value}]. newTop := aSender sender. (self findNextUnwindContextUpTo: newTop) ifNotNil: - [^self send: #aboutToReturn:through: to: self with: {value. nil}]. + [^self send: #aboutToReturn:through: to: self with: {value. [thisContext findNextUnwindContextUpTo: newTop]}]. self releaseTo: newTop. newTop ifNotNil: [newTop push: value]. ^newTop

Context >> resume: value through: firstUnwindCtxt "Unwind thisContext to self and resume with value as result of last send. Execute any unwind blocks while unwinding. ASSUMES self is a sender of thisContext."

| ctxt unwindBlock | self isDead ifTrue: [self cannotReturn: value to: self]. - ctxt := firstUnwindCtxt ifNil: [thisContext findNextUnwindContextUpTo: self]. + ctxt := firstUnwindCtxt value. "evaluate in case firstUnwindCtxt is a block (used in simulation)" [ctxt isNil] whileFalse: [(ctxt tempAt: 2) ifNil: [ctxt tempAt: 2 put: true. unwindBlock := ctxt tempAt: 1. thisContext terminateTo: ctxt. unwindBlock value]. ctxt := ctxt findNextUnwindContextUpTo: self]. thisContext terminateTo: self. ^value

Christoph, all, please let me know if you prefer this approach; I've sent an updated changeset Kernel-jar.1415 to the Inbox.

Thanks,

^[^ Jaromir --

Sent from Squeak Inbox Talk

On 2021-11-14T22:42:02+01:00, mail@jaromir.net wrote:

Hi Christoph,

Sorry for the huge delay in my responding :)

It took me a while to refresh my memory but here are my comments at last:

...

You are changing the semantics of the firstUnwindCtxt param of #aboutToReturn:through: -

Well, in a subtle way; I thought allowing firstUnwindContext = nil here should be harmless :)

...

...as long as the VM guarantees that it will never ever invoke this method with firstUnwindContext = nil, this will be fine, otherwise, this could lead to hypothetical confusion in the future.

Wait - why would that matter? I understand sending #aboutToReturn:through: along with the firstUnwindContext is just a fine optimization; the VM could very well send always nil and everything would work the same, just a tiny bit slower, or not? Have I missed something?? I'm all ears :)

I tried to modify #aboutToReturn to send nil instead of firstUnwindContext and it seemed to work ok:

Context >> #aboutToReturn: result through: firstUnwindContext 
   "Called from VM when an unwindBlock is found between self and its home.
    Return to home's sender, executing unwind blocks on the way."

   self methodReturnContext return: result through: nil	"<---------"

...

Your patch is only required to make the code compatible with the simulator - the controlling logic does not contain any bug itself. That means [...] we are coupling the execution logic and the simulator even closer together, and coupling of (theoretically) independent components is bad...

I haven't realized the coupling aspect; thanks for pointing that out! Without the Debugger one might wonder why on earth firstUnwindContext should ever be nil? :) That's confusing and requires an explicit comment at least. Or we might declare a new semantics of firstUnwindContext allowing nil as a legitimate value should VM for any reason decide not to provide the first unwind context to #aboutToReturn:through: :) But even that wouldn't remove the coupling aspect. So an explanatory comment at any rate.

...

So in the end it all boils down to the question of striving an abstract form of elegancy - by keeping executor and simulator decoupled from each other - on the one hand versus reducing implementation complexity for the simulator (#informDebuggerAboutContextSwitchTo:).

Very nicely put! I guess knowing the root cause of the problem now, one can easily replace the tiny 'hack' I'm proposing with a more robust solution with less coupling effect in the future - like your #informDebuggerAboutContextSwitchTo: mechanism.

...

I guess merging your proposal would be fine - but in this case, I would request you to add a small flagged comment in both changed methods there

Thanks again for pointing this out - I should have commented this interplay in the first place ;) I'll send the commented changeset to the Inbox.

Thanks very much for this discussion,

best,

On 2021-08-22T18:28:54+02:00, christoph.thiede at student.hpi.uni-potsdam.de wrote:

...

Hi Jaromir,

I have just worked through your proposal (Kernel-jar.1413) again. So as I understand it, you prevent the errors while stepping over a non-local return by deferring the pivotal search for the first unwind context. By sending #findNextUnwindContextUpTo: again in #resume:through:, you can rule out any other guard context from #runUntilErrorOrReturnFrom: that has already been uninstalled again when the unwindBlock is finally invoked. I am unable to debunk the correctness of your patch neither by theory nor by experiment. :-)

So it's a very genius patch, but at the same time, still a tiny hack. You are changing the semantics of the firstUnwindCtxt param of #aboutToReturn:through: - as long as the VM guarantees that it will never ever invoke this method with firstUnwindContext = nil, this will be fine, otherwise, this could lead to hypothetical confusion in the future. But this hack is really tiny only, so I would not bother ...

Nevertheless, the central argument of my previous message was a different one: Your patch is only required to make the code compatible with the simulator - the controlling logic does not contain any bug itself. That means that if we merge your proposal, we are coupling the execution logic and the simulator even closer together, and coupling of (theoretically) independent components is bad, as you know for sure ... As opposed to this, my proposed solution (runUntilErrorOrReturnFrom.cs with #informDebuggerAboutContextSwitchTo:, see [1]) defines an explicit hook for this communication, so the coupling is reduced.

And given the results from [1], I think (or hope) that we would need this hook anyway, so this would make your patch redundant, wouldn't it? :-) I also see your argument about false positives, which are indeed tedious, but your idea to recognize harmless jumps is ingenious again - I think this should be possible (I will elaborate further on this in [1]), but such a detection mechanism would be significantly more complex to implement ...

So in the end it all boils down to the question of striving an abstract form of elegancy - by keeping executor and simulator decoupled from each other - on the one hand versus reducing implementation complexity for the simulator (#informDebuggerAboutContextSwitchTo:). As we still do not have a complete overview of the entire Context implementation, the elegant solution would have to benefit of protecting ourselves from future similar incidents that would be preventable via informDebuggerAboutContextSwitchTo:. Elegancy by a single hook, or multilpe hooks with a simpler implementation? I really don't know which one is better ... Maybe you have some other arguments? If not, I guess merging your proposal would be fine - but in this case, I would request you to add a small flagged comment in both changed methods there (something like "self flag: #simulator" followed by a short explanation of the reasoning behind this nil argument, maybe also add a link to this mail conversati!

on!

).

...

Woah - sorry for the long wall of text! This is a delightful topic, and it's a joy to discuss it with you. :-)

Best, Christoph

[1] http://lists.squeakfoundation.org/pipermail/squeak-dev/2021-May/215492.html

---

Sent from Squeak Inbox Talk

On 2021-05-29T04:38:40-05:00, m at jaromir.net wrote:

...

Christoph Thiede wrote

...

Hi Jaromir,

...

...

\2: This was indeed a slip because I forgot to update the image. I have

moved my patch to #findNextHandlerContext - it makes the method robust against bottom-contexts that do not have a sender (i.e., sender is nil).

The changeset still seems to have the old version of #runUntilErrorOrReturnFrom: and #nextHandlerContext nixing Nicolas's changes made in the meantime...

Version 8 removes roerf finally. :-) But I could not find any trace of #nextHandlerContext in the current changeset, did you maybe forget to revert the previous version before loading v7?

yes indeed, I forgot to remove the previous version, sorry for confusion, everything's fine :)

Christoph Thiede wrote

...

...

What would you think about this approach: because #return:from: supplies the first unwind context for #aboutToReturn:through: prematurely, how about to supply nil instead of the first unwind context and let #resume:through: find the first unwind context at precisely the right time?

Correct me if I'm wrong, but this only would move the problem again, wouldn't it? If you press over too late, we would have the same problem again? I'd still prefer a holistic approach such as my #informDebuggerAboutContextSwitchTo: proposal. Or did miss anything different with your proposal? :-)

Well, I thought the fix really solved the bug, not just pushed it further away :) I couldn't reproduce the incorrect stepOver behavior any longer but I may have missed some example - do you have something in mind? I'll comment further in [1]; I hope I'm not wrong here - please send a counterexample if you find one :)

best, Jaromir

Christoph Thiede wrote

...

Best, Christoph

[1] http://forum.world.st/BUG-REGRESSION-while-debugging-Generator-gt-gt-nextPut...

---

^[^ Jaromir

Sent from: http://forum.world.st/Squeak-Dev-f45488.html