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 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 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-td5108125i20.html
> > 
> > 
> > 
> > -----
> > ^[^ Jaromir
> > --
> > Sent from: http://forum.world.st/Squeak-Dev-f45488.html
> > 
> > 
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20210822/83945c3f/attachment-0001.html>
> 
>