Am 28.11.2023 01:29:39 schrieb Jaromir Matas <mail@jaromir.net>:

Hi Eliot, Marcel, all,

I've sent a fix Kernel-jar.1539 to the Inbox that solves the remaining bit of the chain of bugs described in the previous post. All tests are green now and I think the root cause has been found and fixed.

In this last bit I've created a version of stepToCallee that would identify a potential illegal return to a nil sender and avoid it.

Now this example can be debugged without any problems:

[[self halt. ^ 1] on: BlockCannotReturn do: #resume ] fork

If you're happy with the solution in Kernel-jar.1539, Kernel-jar.1538, Kernel-jar.1537 and the test in KernelTests-jar.447, could you please double-check and merge, please? (And remove Kernel-mt.1534 and Tools-jar.1240 from the Inbox)

Best,

Jaromir

On 27-Nov-23 12:09:37 AM, "Jaromir Matas" <mail@jaromir.net> wrote:

Hi Eliot, Christoph, all

It looks like there are some more skeletons in the closet :/

If you run this example

[[self halt. ^ 1] on: BlockCannotReturn do: [:ex | ex resume] ] fork

and step over halt and then step over ^1 you get a nonsensical error as a result of decoding nil as an instruction.

It turns out that the root cause is in the #return:from: method: it only checks whether aSender is dead but ignores the possibility that aSender sender may be nil or dead in which cases the VM also responds with sending #cannotReturn, hence I assume the simulator should do the same. In addition, the VM nills the pc in such scenario, so I added the same functionality here too:

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 |
    newTop := aSender sender.
    (aSender isDead or: [newTop isNil or: [newTop isDead]]) ifTrue:   "<--------- this is extended ------"
        [^self pc: nil; send: #cannotReturn: to: self with: {value}].  "<------ pc: nil is added ----"
    (self findNextUnwindContextUpTo: newTop) ifNotNil:
        "Send #aboutToReturn:through: with nil as the second argument to avoid this bug:
        Cannot #stepOver '^2' in example '[^2] ensure: []'.
        See http://lists.squeakfoundation.org/pipermail/squeak-dev/2022-June/220975.html"
        [^self send: #aboutToReturn:through: to: self with: {value. nil}].
    self releaseTo: newTop.
    newTop ifNotNil: [newTop push: value].
    ^newTop

In order for this to work #cannotReturn: has to be modified as in Kernel-jar.1537:

Context >> cannotReturn: result

    closureOrNil ifNotNil: [^ self cannotReturn: result to: self home sender].
    self error: 'Computation has been terminated!'        "<----------- this has to be an Error -----"

Then it almost works except when you keep stepping over in the example above, you get an MNU error on `self previousPc` in #cannotReturn:to:` with your solution of the VM crash. If you don't mind I've amended your solution and added the final context where the computation couldn't return along with the pc:

Context >> cannotReturn: result to: homeContext
    "The receiver tried to return result to homeContext that cannot be returned from.
    Capture the return context/pc in a BlockCannotReturn. Nil the pc to prevent repeat
    attempts and/or invalid continuation. Answer the result of raising the exception."

    | exception previousPc |
    exception := BlockCannotReturn new.
    previousPc := pc ifNotNil: [self previousPc].   "<----- here's a fix ----"
    exception
        result: result;
        deadHome: homeContext;
        finalContext: self;     "<----- here's the new state, if that's fine ----"
        pc: previousPc.
    pc := nil.
    ^exception signal

Unfortunately, this is still not the end of the story: there are situations where #runUntilErrorOrReturnFrom: places the two guard contexts below the bottom context. And that is a problem because when the method tries to remove the two guard contexts before returning at the end it uses #stepToCalee to do the job but this unforotunately was (ab)using the above bug in #return:from: - I'll explain: #return:from: didn't check whether aSender sender was nil and as a result it allowed to simulate a return to a "nil context" which was then (ab)used in the clean-up via #stepToCalee in the #runUntilErrorOrReturnFrom:.

When I fixed the #return:from: bug, the #runUntilErrorOrReturnFrom: cleanup of the guard contexts no longer works in that very special case where the guard contexts are below the bottom context. There's one case where this is being used: #terminateAggresively by Christoph.

If I'm right with this analysis, the #runUntilErrorOrReturnFrom: should get fixed too but I'll be away now for a few days and I won't be able to respond. If you or Christoph had a chance to take a look at Kernel-jar.1538 and Kernel-jar.1537 I'd be very grateful. I hope this super long message at least makes some sense :)
Best,
Jaromir

[1] Kernel-jar.1538, Kernel-jar.1537
[2] KernelTests-jar.447


PS: Christoph,

With Kernel-jar.1538 + Kernel-jar.1537 your example

process :=
    [(c := thisContext) pc: nil.
    2+3] newProcess.
process runUntil: [:ctx | ctx selector = #cannotReturn:].
self assert: process suspendedContext sender sender = c.
self assert: process suspendedContext arguments = {c}.

works fine, I've just corrected your first assert.

On 21-Nov-23 6:40:32 PM, "Eliot Miranda" <eliot.miranda@gmail.com> wrote:

Hi Jaromir,

On Nov 20, 2023, at 11:51 PM, Jaromir Matas <mail@jaromir.net> wrote:



Hi Eliot,

Very elegant! Now I finally got what you meant exactly :) Thanks.

Two questions:

1. in order for the enclosed test to work I'd need an Error instead of Processor debugWithTitle:full: call in #cannotReturn:. Otherwise I don't know how to catch a plain invocation of the Debugger:

cannotReturn: result

    closureOrNil ifNotNil: [^ self cannotReturn: result to: self home sender].

    self error: 'Computation has been terminated!'

Much nicer.

2. We are capturing a pc of self which is completely different context from homeContext indeed.

Right. The return is attempted from a specific return bytecode in a specific block. This is the coordinate of the return that cannot be made. This is the relevant point of origin of the cannot return exception.

Why the return fails is another matter:

- the home context’s sender is a dead context (cannot be resumed)

- the home context’s sender is nil (home already returned from)

- the block activation’s home is nil rather than a context (should not happen)

But in all these cases the pc of the home context is immaterial. The hike is being returned through/from, rather than from; the home’s pc is not relevant.

Maybe we could capture self in the exception too to make it more clear/explicit what is going on: what context the captured pc is actually associated with. Just a thought...

Yes, I like that.  I also like the idea of somehow passing the block activation’s pc to the debugger so that the relevant return expression is highlighted in the debugger.

Thanks again,

Jaromir

You’re welcome.  I love working in this part of the system. Thanks for dragging me there. I’m in a slump right now and appreciate the fellowship.  

------ Original Message ------

From "Eliot Miranda" <eliot.miranda@gmail.com>

To "Jaromir Matas" <mail@jaromir.net>

Cc squeak-dev@lists.squeakfoundation.org

Date 11/21/2023 2:17:21 AM

Subject Re: Re[2]: [squeak-dev] Re: Resuming on BlockCannotReturn exception

Hi Jaromir,

   see Kernel-eem.1535 for what I was suggesting.  This example now has an exception with the right pc value in it:

[[^1] on: BlockCannotReturn do: [:ex| ex pc inspect. ex resume]] fork

The fix is simply

Context>>cannotReturn: result to: homeContext
    "The receiver tried to return result to homeContext that cannot be returned from.
     Capture the return pc in a BlockCannotReturn. Nil the pc to prevent repeat
     attempts and/or invalid continuation. Answer the result of raising the exception."

    | exception |
    exception := BlockCannotReturn new.
    exception
        result: result;
        deadHome: homeContext;
        pc: self previousPc.
    pc := nil.
    ^exception signal

The VM crash is now avoided. The debugger displays the method, but does not highlight the offending pc, which is no big deal. A suitable defaultHandler for B lockCannotReturn may be able to get the debugger to highlight correctly on opening.  Try the following examples:

[[^1] on: BlockCannotReturn do: #resume] fork.

[[^1] on: BlockCannotReturn do: [:ex| ex pc inspect. ex resume]] fork

[[^1] value] fork.

They al; seem to behave perfectly acceptably to me.  Does this fix work for you?

On Fri, Nov 17, 2023 at 3:14 PM Jaromir Matas <mail@jaromir.net> wrote:

Hi Eliot,

How about to nil the pc just before making the return:

```

Context >> #cannotReturn: result

    self push: self pc.  "backup the pc for the sake of debugging"

    closureOrNil ifNotNil: [^self cannotReturn: result to: self home sender; pc: nil].

    Processor debugWithTitle: 'Computation has been terminated!' translated full: false

```

The nilled pc should not even potentially interfere with the #isDead now.

I hope this is at least a step in the right direction :)

However, there's still a problem when debugging the resumption of #cannotReturn because the encoders expect a reasonable index. I haven't figured out yet where to place a nil check - #step, #stepToSendOrReturn... ?

Thanks again,

Jaromir

------ Original Message ------

From "Eliot Miranda" <eliot.miranda@gmail.com>

To "Jaromir Matas" <mail@jaromir.net>

Date 11/17/2023 8:36:50 PM

Subject Re: [squeak-dev] Re: Resuming on BlockCannotReturn exception

Hi Jaromir,

On Nov 17, 2023, at 7:05 AM, Jaromir Matas <mail@jaromir.net> wrote:



Eliot, hi again,

Please disregard my previous comment about nilling the contexts that have returned. We are indeed talking about the context directly under the #cannotReturn context which is totally different from the home context in another thread that's gone.

I may still be confused but would nilling the pc of the context directly under the cannotReturn context help? Here's what I mean:

```

Context >> #cannotReturn: result

    closureOrNil ifNotNil: [^self pc: nil; cannotReturn: result to: self home sender].

    Processor debugWithTitle: 'Computation has been terminated!' translated full: false.

```

Instead of crashing the VM invokes the debugger with the 'Computation has been terminated!' message.

Does this make sense?

Nearly. But it loses the information on what the pc actually is, and that’s potentially vital information.  So IMO the ox should only be nilled between the BlockCannotReturn exception being created and raised.

[But if you try this don’t be surprised if it causes a few temporary problems.  It looks to me that without a little refactoring this could easily cause an infinite recursion around the sending of isDead.  I’m sure you’ll be able to fix the code to work correctly]

Thanks,

Jaromir

------ Original Message ------

From "Jaromir Matas" <mail@jaromir.net>

To "Eliot Miranda" <eliot.miranda@gmail.com>; "The general-purpose Squeak developers list" <squeak-dev@lists.squeakfoundation.org>

Date 11/17/2023 10:15:17 AM

Subject [squeak-dev] Re: Resuming on BlockCannotReturn exception

Hi Eliot,

------ Original Message ------

From "Eliot Miranda" <eliot.miranda@gmail.com>

To "Jaromir Matas" <mail@jaromir.net>

Cc "The general-purpose Squeak developers list" <squeak-dev@lists.squeakfoundation.org>

Date 11/16/2023 11:52:45 PM

Subject Re: Re[2]: [squeak-dev] Re: Resuming on BlockCannotReturn exception

Hi Jaromir,

On Thu, Nov 16, 2023 at 2:22 PM Jaromir Matas <mail@jaromir.net> wrote:

Hi Nicolas, Eliot,

here's what I understand is happening (see the enclosed screenshot):

1) we fork a new process to evaluate [^1]

2) the new process evaluates [^1] which means instruction 18 is being evaluated, hence pc points to instruction 19 now

3) however, the home context where ^1 should return to is gone by this time (the process that executed the fork has already returned - notice the two up arrows in the debugger screenshot)

4) the VM can't finish the instruction and returns control to the image via placing the #cannotReturn: context on top of the [^1] context

5) #cannotReturn: evaluation results in signalling the BCR exception which is then handled by the #resume handler

    (in our debugged case the [:ex | self halt. ex resume] handler)

6) ex resume is evaluated, however, this means requesting the VM to evaluate instruction 19 of the [^1] context - which is past the last instruction of the context and the crash ensues

I wonder whether such situations could/should be prevented inside the VM or whether such an expectation is wrong for some reason.

As Nicolas says, IMO this is best done at the image level.

It could be prevented in the VM, but at great cost, and only partially.  The performance issue is that the last bytecode in a method is not marked in any way, and that to determine the last bytecode the bytecodes must be symbolically evaluated from the start of the method.  See implementors of endPC at the image level (which defer to the method trailer) and implementors of endPCOf: in the VMMaker code. Doing this every time execution commences is prohibitively expensive.  The "only partially" issue is that following the return instruction may be other valid bytecodes, but these are not a continuation.  

Consider the following code in some block:

        [self expression ifTrue:

            [^1].

        ^2

The bytecodes for this are

    pushReceiver

    send #expression

    jumpFalse L1

    push 1

    methodReturnTop

L1

    push 2

    methodReturnTop

Clearly if expression is true these should be *no* continuation in which ^2 is executed.

Well, in that case there's a bug because the computation in the following example shouldn't continue past the [^1] block but it silently does:

`[[true ifTrue: [^ 1]] on: BlockCannotReturn do: #resume ] fork`

The bytecodes are

    push true

    jumpFalse L1

    push 1

    returnTop

L1

    push nil

    blockReturn

So even if the VM did try and detect whether the return was at the last block method, it would only work for special cases.

It seems to me the issue is simply that the context that cannot be returned from should be marked as dead (see Context>>isDead) by setting its pc to nil at some point, presumably after copying the actual return pc into the BlockCannotReturn exception, to avoid ever trying to resume the context.  

Does this mean, in other words, that every context that returns should nil its pc to avoid being "wrongly" reused/executed in the future, which concerns primarily those being referenced somewhere hence potentially executable in the future, is that right?

Hypothetical question: would nilling the pc during returns "fix" the example?

Thanks a lot for helping me understand this.

Best,

Jaromir

    

Thanks,

Jaromir

<bdxuqalu.png>

------ Original Message ------

From "Eliot Miranda" <eliot.miranda@gmail.com>

To "Jaromir Matas" <mail@jaromir.net>; "The general-purpose Squeak developers list" <squeak-dev@lists.squeakfoundation.org>

Date 11/16/2023 6:48:43 PM

Subject Re: [squeak-dev] Re: Resuming on BlockCannotReturn exception

Hi Jaromir,

On Nov 16, 2023, at 3:23 AM, Jaromir Matas <mail@jaromir.net> wrote:



Hi Nicloas,

No no, I don't have any practical scenario in mind, I'm just trying to understand why the VM is implemented like this, whether there were a reason to leave this possibility of a crash, e.g. it would slow down the VM to try to prevent such a dumb situation (who would resume from BCR in his right mind? :) ) - or perhaps I have overlooked some good reason to even keep this behavior in the VM. That's all.

Let’s first understand what’s really happening. Presumably at tone point a context is resumed those pc is already at the block return bytecode (effectively, because it crashes in JITted code, but I bet the stack vm will crash also, but not as cleanly - it will try and execute the bytes in the encoded method trailer). So which method actually sends resume, and to what, and what state is resume’s receiver when resume is sent?

Thanks for your reply. 

Regards,

Jaromir

------ Original Message ------

From "Nicolas Cellier" <nicolas.cellier.aka.nice@gmail.com>

To "Jaromir Matas" <mail@jaromir.net>; "The general-purpose Squeak developers list" <squeak-dev@lists.squeakfoundation.org>

Date 11/16/2023 7:20:20 AM

Subject Re: [squeak-dev] Resuming on BlockCannotReturn exception

Hi Jaromir,

Is there a scenario where it would make sense to resume a BlockCannotReturn?

If not, I would suggest to protect at image side and override #resume.

Le mer. 15 nov. 2023, 23:42, Jaromir Matas <mail@jaromir.net> a écrit :

Hi Eliot, Christoph, All,

It's known the following example crashes the VM. Is this an intended behavior or a "tolerated bug"? 

`[[^ 1] on: BlockCannotReturn do: #resume] fork`

I understand why it crashes: the non-local return has nowhere to return to and so resuming the computation leads to a crash. But why not raise another BCR exception to prevent the crash? Potential infinite loop? Perhaps I'm just missing the purpose of this behavior...

Thanks for an explanation.

Best,

Jaromir

--

Jaromir Matas

--

_,,,^..^,,,_

best, Eliot

<Context-cannotReturn.st>

--

_,,,^..^,,,_

best, Eliot

<ProcessTest-testResumeAfterBCR.st>