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>