Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: make variable types in simulate_unbind match OP_unbind #381

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

masinter
Copy link
Member

Just posting looking for ssome help. When I stepped through the code in values / simulate_unbind, there were some cases where "value" was a negative int instead of a LispPTR*. So I tried (cargo cult programming) making simulate_unbind look like OP_unbind,
renaming variables and declarations to match. I left the MAKEFREEBLOCK commented out.

@nbriggs
Copy link
Collaborator

nbriggs commented May 15, 2021

Did you look at where you are changing the signedness of the variables that are being right-shifted? When you go from signed to unsigned you will lose the sign-extension -- are you sure that that is what is needed?

@nbriggs
Copy link
Collaborator

nbriggs commented May 15, 2021

The code for the unbind implementation isn't N_OP_unbind, it's the code in inlineC.h --

#define UNBIND                                                  \
  do {                                                          \
    register int num;                                           \
    register LispPTR *ppvar;                                    \
    register int i;                                             \
    register LispPTR value;                                     \
    for (; (((int)*--CSTKPTRL) >= 0);)                          \
      ;                                                         \
    value = *CSTKPTR;                                           \
    num = (~value) >> 16;                                       \
    ppvar = (LispPTR *)((DLword *)PVAR + 2 + GetLoWord(value)); \
    for (i = num; --i >= 0;) { *--ppvar = 0xffffffff; }         \
    nextop1;                                                    \
  } while (0)

@masinter
Copy link
Member Author

I'm doing this since i'm not sure what's going on.
The unbind count should be reset to 0 each RETURN scanned and stackframe. Each frame popped off by values OR valueslist should be freed?

@nbriggs
Copy link
Collaborator

nbriggs commented May 16, 2021

Yeah, I've seen way too much cargo-cult programming, mostly on the VLC dev list, where the practitioners are often roundly excoriated for it.

I'm not sure that the problem here is actually in the simulate_unbind() or at least not in the difference from the UNBIND opcode.

The BIND/UNBIND in the regular opcode stream aren't making new stack frames, are they? They should be stuffing a value into, or marking as unused, slots that are in the current frame.

My understanding from the code is that the MISCN/values code, where it does

caller->pc = (UNSIGNED)pc + FN_OPCODE_SIZE - (UNSIGNED)fnhead;

is simulating the return of the lower procedure and skipping back to where the \MVLIST would have returned to, if it were actually being called, and if there were any UNBIND operations between here and there, the PVAR entries in those frames need to be unbound (i.e., have the binding smashed to the unbound mark).

So, if the stack isn't being properly set up it's not a problem in the simulated unbind, but more likely in the way the return is being faked when the PC of the caller is adjusted, and the lower procedure's stack frame isn't being freed.

@nbriggs
Copy link
Collaborator

nbriggs commented May 16, 2021

Rereading, a question: shouldn't it only need to simulate unbinds in frames that will still exist after we fake the return? Stack frames that (should) have been released don't need to have adjustments made.

@nbriggs
Copy link
Collaborator

nbriggs commented May 16, 2021

I've annotated what I think is going on here -- the "outer" code is stackbug, the indented is stkbug1.
Does this make more sense to you now? I think it clears it up quite a bit for me:

  0:    2C stkmin
  2:     0 na
  4:     0 pv
  6:    30 startpc
  8:    60 byteswapped: NIL argtype: 0
  A:  E340 frame name: STACKBUG
  C:     8 ntsize
  E:   200 nlocals: 2 fvaroffset: 0
name table: 
 10:   0 60 E2 FA   20:  PVAR 1: A0023
 14:   0  0  3 F9   24:  PVAR 0: CHARCODE
 18:   0  0  0  0   28: 
 1C:   0  0  0  0   2C: 

----
 30:         11 10  0         BIND      ; CHARCODE
 33:         67  0 60 E3 36   ACONST    STKBUG1
 38:         11  1  1         BIND       A0023;                 binding PVAR 1
 3B:         69               'T
 3C:         6B               '1
 3D:         49               PVAR      A0023
 3E:          E               APPLYFN                           calls STKBUG1, inline here

        ----
         18:         6A               '0
         19:         6C  A            SIC       10
         1B:          A  0  0  A  F   FN2       RAND
         20:         6A               '0
         21:         F0               EQ
         22:         6C 2A            SIC       42
         24:         24  1  2         MISCN                     values skips this opcode
         27:         10               RETURN                    notes return, gets caller from CLINK
         28:          0               -X-                       positions scan pc to #3F
        ----

 3F:         12               UNBIND                            #3F: and finds UNBIND opcode
 40:          9  0  0  3 5D   FN1       \MVLIST                 then finds FN1 \MVLIST, notices that if we returned to #45
                                                                we would have skipped an UNBIND, so we must simulate it,
                                                                create the result, and fake a return to #45.  The stack frame for
                                                                STKBUG1 should be released, but that has nothing to do with unbind?
 45:          1               CAR
 46:         58               PVAR_     CHARCODE
 47:         B3 EC            TJUMPX-> 33
 49:         69               'T
 4A:         10               RETURN
 4B:          0               -X-

@nbriggs
Copy link
Collaborator

nbriggs commented May 16, 2021

The next thing to investigate is exactly how it implements the return from STKBUG1 to the point following the \MVLIST... does it actually execute a RETURN opcode to clean up the stack frames? Is it faked and thus (bugily) missing a necessary stack adjustment?

@masinter
Copy link
Member Author

the problem only happens when returning multiple values to an UNBIND followed by a FN1 \MVLIST. No UNBIND, no problem.
Would it help to try it on a 32-bit or Big-endian arch machine?

@nbriggs
Copy link
Collaborator

nbriggs commented May 16, 2021

I did that long ago. It makes no difference whether it's 32- or 64- bit, or little- or big-endian.

@nbriggs
Copy link
Collaborator

nbriggs commented May 16, 2021

Even if simulate_unbind() does nothing but print the stack pointer and return... the stack pointer passed is increasing by 4 bytes each time, and I don't think that that is correct. I surmise that the problem is not in simulate_unbind().

@nbriggs
Copy link
Collaborator

nbriggs commented May 16, 2021

In the case we are having trouble with, the way it returns from STKBUG1 is to just continue execution after the MISCN opcode, executing the RETURN opcode, however at that point the saved PC in the frame belonging to STACKBUG has been adjusted so that it skips over the UNBIND and FN1 \MVLIST and continues normal execution at the CAR opcode. It doesn't matter whether you execute simulate_unbind() or not, you get the same results, and the stack grows by 4 bytes at each call.
Here's an execution trace (slightly modified from the default, opcodes in hex), and where STKBUG1 has just SIC/SIC/MISCN/

PC= 0x10a5bc0 (fn(10a5b90)+0x30) op= 11 TOS= 0x8c3      START OF STACKBUG
PC= 0x10a5bc3 (fn(10a5b90)+0x33) op= 67 TOS= 0xfffe0000
PC= 0x10a5bc8 (fn(10a5b90)+0x38) op= 11 TOS= 0x60e336
PC= 0x10a5bcb (fn(10a5b90)+0x3b) op= 69 TOS= 0xfffe0002
PC= 0x10a5bcc (fn(10a5b90)+0x3c) op= 6b TOS= 0x4c
PC= 0x10a5bcd (fn(10a5b90)+0x3d) op= 49 TOS= 0xe0001
PC= 0x10a5bce (fn(10a5b90)+0x3e) op= 0e TOS= 0x60e336     APPLY
PC= 0x10f781c (fn(10f7800)+0x1c) op= 6c TOS= 0x4c              SIC 33
PC= 0x10f781e (fn(10f7800)+0x1e) op= 6c TOS= 0xe0021        SIC 42
PC= 0x10f7820 (fn(10f7800)+0x20) op= 24 TOS= 0xe002a       MISCN
OP_miscn: enter PC 0x10f7820 CurrentStackPTR 0x4aab50 arg_count 2
OP_miscn: exit PC 0x10f7823 CurrentStackPTR 0x4aab4c stk 0x4aab50
PC= 0x10f7823 (fn(10f7800)+0x23) op= 10 TOS= 0x6257b6     RETURN
PC= 0x10a5bd5 (fn(10a5b90)+0x45) op= 01 TOS= 0x6257b6    CAR
PC= 0x10a5bd6 (fn(10a5b90)+0x46) op= 58 TOS= 0xe0021     PVAR_0
PC= 0x10a5bd7 (fn(10a5b90)+0x47) op= b3 TOS= 0xe0021      TJUMPX->33
PC= 0x10a5bc3 (fn(10a5b90)+0x33) op= 67 TOS= 0xfffe0002   ACONST 'STKBUG1
PC= 0x10a5bc8 (fn(10a5b90)+0x38) op= 11 TOS= 0x60e336      BIND
PC= 0x10a5bcb (fn(10a5b90)+0x3b) op= 69 TOS= 0xfffe0002   T
PC= 0x10a5bcc (fn(10a5b90)+0x3c) op= 6b TOS= 0x4c              '1
PC= 0x10a5bcd (fn(10a5b90)+0x3d) op= 49 TOS= 0xe0001       PVAR 
PC= 0x10a5bce (fn(10a5b90)+0x3e) op= 0e TOS= 0x60e336     APPLY
PC= 0x10f781c (fn(10f7800)+0x1c) op= 6c TOS= 0x4c              SIC 33
PC= 0x10f781e (fn(10f7800)+0x1e) op= 6c TOS= 0xe0021        SIC 42
PC= 0x10f7820 (fn(10f7800)+0x20) op= 24 TOS= 0xe002a       MISCN
OP_miscn: enter PC 0x10f7820 CurrentStackPTR 0x4aab54 arg_count 2      but CurrentStackPTR has moved
OP_miscn: exit PC 0x10f7823 CurrentStackPTR 0x4aab50 stk 0x4aab54
PC= 0x10f7823 (fn(10f7800)+0x23) op= 10 TOS= 0x6257b4
PC= 0x10a5bd5 (fn(10a5b90)+0x45) op= 01 TOS= 0x6257b4
PC= 0x10a5bd6 (fn(10a5b90)+0x46) op= 58 TOS= 0xe0021
PC= 0x10a5bd7 (fn(10a5b90)+0x47) op= b3 TOS= 0xe0021
PC= 0x10a5bc3 (fn(10a5b90)+0x33) op= 67 TOS= 0xfffe0002
PC= 0x10a5bc8 (fn(10a5b90)+0x38) op= 11 TOS= 0x60e336
PC= 0x10a5bcb (fn(10a5b90)+0x3b) op= 69 TOS= 0xfffe0002
PC= 0x10a5bcc (fn(10a5b90)+0x3c) op= 6b TOS= 0x4c
PC= 0x10a5bcd (fn(10a5b90)+0x3d) op= 49 TOS= 0xe0001
PC= 0x10a5bce (fn(10a5b90)+0x3e) op= 0e TOS= 0x60e336
PC= 0x10f781c (fn(10f7800)+0x1c) op= 6c TOS= 0x4c
PC= 0x10f781e (fn(10f7800)+0x1e) op= 6c TOS= 0xe0021
PC= 0x10f7820 (fn(10f7800)+0x20) op= 24 TOS= 0xe002a
OP_miscn: enter PC 0x10f7820 CurrentStackPTR 0x4aab58 arg_count 2
OP_miscn: exit PC 0x10f7823 CurrentStackPTR 0x4aab54 stk 0x4aab58
PC= 0x10f7823 (fn(10f7800)+0x23) op= 10 TOS= 0x6257b2
PC= 0x10a5bd5 (fn(10a5b90)+0x45) op= 01 TOS= 0x6257b2
PC= 0x10a5bd6 (fn(10a5b90)+0x46) op= 58 TOS= 0xe0021
PC= 0x10a5bd7 (fn(10a5b90)+0x47) op= b3 TOS= 0xe0021
PC= 0x10a5bc3 (fn(10a5b90)+0x33) op= 67 TOS= 0xfffe0002
PC= 0x10a5bc8 (fn(10a5b90)+0x38) op= 11 TOS= 0x60e336
PC= 0x10a5bcb (fn(10a5b90)+0x3b) op= 69 TOS= 0xfffe0002
PC= 0x10a5bcc (fn(10a5b90)+0x3c) op= 6b TOS= 0x4c
PC= 0x10a5bcd (fn(10a5b90)+0x3d) op= 49 TOS= 0xe0001
PC= 0x10a5bce (fn(10a5b90)+0x3e) op= 0e TOS= 0x60e336
PC= 0x10f781c (fn(10f7800)+0x1c) op= 6c TOS= 0x4c
PC= 0x10f781e (fn(10f7800)+0x1e) op= 6c TOS= 0xe0021
PC= 0x10f7820 (fn(10f7800)+0x20) op= 24 TOS= 0xe002a
OP_miscn: enter PC 0x10f7820 CurrentStackPTR 0x4aab5c arg_count 2
OP_miscn: exit PC 0x10f7823 CurrentStackPTR 0x4aab58 stk 0x4aab5c
PC= 0x10f7823 (fn(10f7800)+0x23) op= 10 TOS= 0x6257ae
PC= 0x10a5bd5 (fn(10a5b90)+0x45) op= 01 TOS= 0x6257ae
PC= 0x10a5bd6 (fn(10a5b90)+0x46) op= 58 TOS= 0xe0021
PC= 0x10a5bd7 (fn(10a5b90)+0x47) op= b3 TOS= 0xe0021
PC= 0x10a5bc3 (fn(10a5b90)+0x33) op= 67 TOS= 0xfffe0002

@nbriggs
Copy link
Collaborator

nbriggs commented May 17, 2021

and again, with the stack pointer included in the trace -- so what opcode is not having the expected effect on the stack (this notes stack pointer at the start of the opcode)

PC= 0x1085bc0 (fn(1085b90)+0x30) op= 11 TOS= 0x8c3 STACK 0x48ab1c               BIND    ; CHARCODE

PC= 0x1085bc3 (fn(1085b90)+0x33) op= 67 TOS= 0xfffe0000 STACK 0x48ab20          ACONST  'STKBUG1 (push)
PC= 0x1085bc8 (fn(1085b90)+0x38) op= 11 TOS= 0x60e336 STACK 0x48ab24            BIND    A0023
PC= 0x1085bcb (fn(1085b90)+0x3b) op= 69 TOS= 0xfffe0002 STACK 0x48ab24          'T      (push)
PC= 0x1085bcc (fn(1085b90)+0x3c) op= 6b TOS= 0x4c STACK 0x48ab28                '1     (push)
PC= 0x1085bcd (fn(1085b90)+0x3d) op= 49 TOS= 0xe0001 STACK 0x48ab2c             PVAR    A0023    (push)
PC= 0x1085bce (fn(1085b90)+0x3e) op= 0e TOS= 0x60e336 STACK 0x48ab30            APPLYFN

PC= 0x10d781c (fn(10d7800)+0x1c) op= 6c TOS= 0x4c STACK 0x48ab4c                SIC 33  <<<<      (push)
PC= 0x10d781e (fn(10d7800)+0x1e) op= 6c TOS= 0xe0021 STACK 0x48ab50             SIC 42      (push)
PC= 0x10d7820 (fn(10d7800)+0x20) op= 24 TOS= 0xe002a STACK 0x48ab54             MISCN    (pop)
OP_miscn: enter PC 0x10d7820 CurrentStackPTR 0x48ab50 arg_count 2
OP_miscn: exit PC 0x10d7823 CurrentStackPTR 0x48ab4c stk 0x48ab50
PC= 0x10d7823 (fn(10d7800)+0x23) op= 10 TOS= 0x6257a4 STACK 0x48ab50            RETURN

PC= 0x1085bd5 (fn(1085b90)+0x45) op= 01 TOS= 0x6257a4 STACK 0x48ab28            CAR
PC= 0x1085bd6 (fn(1085b90)+0x46) op= 58 TOS= 0xe0021 STACK 0x48ab28             PVAR_ CHARCODE
PC= 0x1085bd7 (fn(1085b90)+0x47) op= b3 TOS= 0xe0021 STACK 0x48ab28             TJUMPX -> 33

PC= 0x1085bc3 (fn(1085b90)+0x33) op= 67 TOS= 0xfffe0002 STACK 0x48ab24          ACONST  'STKBUG1 (push)
PC= 0x1085bc8 (fn(1085b90)+0x38) op= 11 TOS= 0x60e336 STACK 0x48ab28
PC= 0x1085bcb (fn(1085b90)+0x3b) op= 69 TOS= 0xfffe0002 STACK 0x48ab28
PC= 0x1085bcc (fn(1085b90)+0x3c) op= 6b TOS= 0x4c STACK 0x48ab2c

So when we arrive back at STACKBUG+0x33, we've got 4 more bytes on the stack than when we started.

@masinter
Copy link
Member Author

it's the missing FN1 \MVLIST, which SHOULD pop the stack with its parameter and PUSH the return value.
Because it incrememts the PC so the FN1 \MVLIST doesn't happen, the stack is 1 too big. The only puzzle is why it doesn't stack overflow more often.

@nbriggs
Copy link
Collaborator

nbriggs commented May 17, 2021

Wouldn't the combination of POP the parameter and PUSH the return value be stack-neutral, so no adjustment should be necessary? What am I missing? I don't think an argument got pushed for \MVLIST, and the result of the MISCN should (?) have been on the stack as the argument for CAR...?

@masinter
Copy link
Member Author

The call decrements the stack and the return pushes the return value. Yes, FN1 is neutral.

@masinter
Copy link
Member Author

masinter commented Jun 1, 2021

I've been thinking of ways of doing multiple-value-return in a way that is straightforward but doesn't require mass recompiling / macro changes. I have an idea but it requires changing one Lisp function, \MVLIST as well as maiko -- a coordinated change.

The way it's set up now, there are to opcodes, Values and ValuesList. Both return one value (the first for Values and CAR of the list for ValuesList) UNLESS the next opcode is (a) a call to \MVLIST or a RETURN, JUMP, or UNBIND (being the only cases that pass along multiple values). In those cases it keeps on looking for \MVLIST, following up the control stack or the jumps.

There are 4 cases: the first is ordinary return to ordinary returnee, MV return to MVLIST returnee , MV return to 1-value returnee, ordinary return to MVLIST returnee.

in case 2 it finds the returnees MVLIST call and forces a return with the PC set to skip the call to \MVLIST. In case 3 it doesn't find a MVLIST call and just returns the single value as if cl:values was PROG1. In case 4, you wind up calling \MVLIST which is just (lambda (x) (list x)).

The solution I was going to propose was not to modify the PC or frame pointer or simulate any unbinds, but instead pass on a return value (cons '\mvlist values-list) in the case where a fn1 \mvlist was found. at the same change \mvlist (lambda (x) (cond ((and (listp x) (eq (car x) '\mvlist)) (cdr x)) (t (list x)))))
no simulate, no slow return, no changing the PC on the stack.

But now that I've written this, i want to go back and check to see if simulate_unbind properly pops the bind mark off the stack (if I can remember what BIND does).

@nbriggs
Copy link
Collaborator

nbriggs commented Jun 1, 2021

simulate_unbind (and the unbind opcode itself) don't pop the bind mark off the stack, or manipulate the stack pointer at all, they just smash the binding slot with a value that indicates unbound.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants