Hi Jaromir,

sorry to bring this up again with so much delay, but honestly I don't think this is a good patch and we should revert it in the trunk.

One core assumption about the simulation engine is that it mimics the behavior of the VM interpreter in every utmost detail. This includes the arguments sent through special messages sent by the VM such as #aboutToReturn:through:. Just because the current implementation of Context>>aboutToReturn:through: does not require the value of firstUnwindContext, this does not mean that we can just break the existing contract of the VM and provide nil for that argument from the simulator.

Besides that theoretical perspective, your change also broke a couple of tests in SimulationStudio as I discovered today, because SimulationStudio relies on many of these assumptions about the code execution. If you would like to, you can also explore the failing test yourself: SimulationContextTest>>#testNonLocalReturn. SimulationContext>>#runSimulated:contextAtEachStep: manually checks the arguments passed to #aboutToReturn:through: (or more precisely, of #resume:through:) to detect when the simulator is unwinding the code provided to a custom simulator. As you niled out that value, I can no longer check that.

I think that your patch is just a hack that fixes a symptom rather than resolving the original problem, which is still the guard contexts inserted by Context>>#runUntilErrorOrReturnFrom: that are not fully transparent to the executed process. See https://lists.squeakfoundation.org/archives/list/squeak-dev@lists.squeakfoundation.org/message/JXLLHUWWSL4ICZPHMNZVVXPTC2WUHITB/. The changeset that I proposed there (runUntilErrorOrReturnFrom.8.cs), which basically ensures before every irregular context switch that the guard contexts are removed and the debugger is halted again, fixes the original issue in the debugger as well. Try reverting your patch from Kernel-jar.1503 and loading my change set instead and you will end up in Context>>#resume:through: after stepping over the non-local return, which I would consider even favorable to the behavior of your patch which makes the logic from the unwind blocks unaccessible in that situation.

So, tl;dr, I see three issues with this patch: (i) it violates the VM contract of #aboutToReturn:through:, which (ii) breaks SimulationStudio due to a missing piece of information, and (iii) stepping over non-local returns no longer allows programmers to observe or step into unwind blocks. Given that, I would ask you for your agreement to reverting that patch and working on fixing the underlying issue with non-transparent guard contexts in general instead.

Despite that, please feel not discouraged to continue contributing to this important part of Squeak: It's just that you're sharing so many interesting ideas and Eliot and me and others lack the time to read all of them but certain changes nevertheless should require a proper review ... Have a nice end of Christmas! :-)

Best,
Christoph

---
Sent from Squeak Inbox Talk

On 2023-02-14T22:08:39+00:00, mail@jaromir.net wrote:

> Hi,
>
> I'm enclosing a changeset that entirely removes unwind code duplicity in Context AND at the same time fixes the "stepOver bug" still persisting in the system.
>
> To refresh what the bug is: You cannot #stepOver '^2' when you debug;
> [^2] ensure: [] "step through to ^2 and try step over --> you get BCR"
>
> The root cause is #return:from: supplies 'firstUnwindContext' to #aboutToReturn:through: but before this gets evaluated the real first unwind context changes during simulation and that causes the bug.
>
> Originally (prior 2012), #aboutToReturn:through: just called 'self methodReturnContext return: result' and didn't use the 'firstUnwindContext' but in 2012 the call has been replaced with 'self methodReturnContext return: result through: firstUnwindContext' and the bug was born.
>
> In the changeset I suggest to use one common method replacing #resume:through: and #resumeEvaluating: which solves the bug as a side-effect with only a tiny change in #return:from: (simply supply nil instead of 'firstUnwindContext').
>
> I will post the changeset to the Inbox soon.
>
> Best,
> Jaromir
>
>
> --
>
> Jaromír Matas
>
> mail at jaromir.net
>
>
> From: Jaromir Matas<mailto:mail at jaromir.net>
> Sent: Thursday, February 2, 2023 15:07
> To: Squeak Dev<mailto:squeak-dev at lists.squeakfoundation.org>
> Subject: [squeak-dev] Code duplication around unwinding
>
> Hi all,
>
> When I started learning Squeak I wondered why so much code duplication around unwinding: Context>>#restart, #resume:through:, #resumeEvaluating:, #terminate and #unwindTo: all implementing the exact same algorithm. Kernel-jar.1499 suggests removing all this duplicity while keeping the current unwind semantics. All Process and Exceptions tests are green.
>
> This changeset is the first step in my effort to reduce the "noise" around the "collection" of resume/return methods in Context and Exception. One idea is to return calling the unwind mechanism back to the Exception class, as originally implemented in Squeak 2.x up to 3.5; please let me know if you find something fundamentally wrong with this idea.
>
> Example: I find this call sequence begging for simplification:
> Exception>>resume: --> resumeUnchecked: --> resumeEvaluating: --> Context>>returnEvaluating: --> resumeEvaluating:
>
> Best,
>
> Jaromir
>
>
> --
>
> Jaromír Matas
>
> mail at jaromir.net