Skip to content

Conversation

@masinter
Copy link
Member

@masinter masinter commented Jul 27, 2022

The code in \RPLPTR (which is only run when running interpreted, e.g., with LOOPS low-level code) hadn't been updated for the large VMEM changes. The address space was increased from 24-bit pointers (8 bits padding, 8 bits HILOC, 16 bits LOLOC) to 28 bits. RPLPTR opcode in maiko saves the existing high bits (used, e.g., in CDR-coding and tightly packed datatypes. Oddly, PUTBASEPTR just clears the high bits.)

We should be on the lookout for other instances of this bug.

@nbriggs
Copy link
Contributor

nbriggs commented Jul 27, 2022

Does the \RPLPTR.UFN suffer from the same problem?

@nbriggs
Copy link
Contributor

nbriggs commented Jul 27, 2022

The opcode RPLPTR does preserve the high-order 4 bits of the target. Was that the "oops, misread..." ?

@masinter
Copy link
Member Author

masinter commented Jul 28, 2022

I went through the functions / UFNs in LLNEW and compared against the Maiko definitions.
It looks like \PUTBASEPTR resets the high 4 bits, but \RPLPTR doesn't.
\GETBASEPTR clears those bits. The functions on LLNEW are mainly used "renamed" to I.\name during the makeinit process which produces I-NEW and I-NEW.LCOM. (also RDSYS and RDSYS.LCOM which are used only minimally during MAKEINIT).

@masinter masinter requested a review from nbriggs July 28, 2022 14:33
@nbriggs
Copy link
Contributor

nbriggs commented Jul 28, 2022

I can't get to this for a few hours, but will review today.

@nbriggs
Copy link
Contributor

nbriggs commented Jul 29, 2022

OK, the only difference I see now is that the LLNEW implementation of \GETBASEPTR will return the full 32 bits of the result while the maiko GETBASEPTR_N opcode returns only the lower 28 bits of the result.

    TOPOFSTACK = (POINTERMASK & *((LispPTR *)Addr68k_from_LADDR((POINTERMASK & TOPOFSTACK) + (N)))); \

There's a comment in the VAG2 opcode saying


/*********************************************/
/* Note: No matter how smart it seems, don't */
/* AND in POINTERMASK in VAG2, because there */
/* is code that depends on VAG2 building     */
/* full, 32-bit pointers from 16-bit ints.   */
/*********************************************/
#define N_OP_VAG2                                             ...

@masinter
Copy link
Member Author

image
\GETBASEPTR returns \VAG2 (LOGAND #xFFF ... masking the high bits.
I saw that comment -- there are apparently some unboxed arithmetic opcodes?

it's odd about \PUTBASEPTR vs \RPLPTR wrt preserving high bits.
These are used by 'replace' (or 'freplace') for DATATYPE and BLOCKRECORD fields.
those do fcfs bin-packing and I think there may some declarations that assumed there was a spare BYTE before a (24-bit?) XPOINTER....

@nbriggs
Copy link
Contributor

nbriggs commented Jul 29, 2022

\GETBASEPTR returns \VAG2 (LOGAND #xFFF ... masking the high bits.
I saw that comment -- there are apparently some unboxed arithmetic opcodes?

Yes, there are unboxed ops operating on floating point numbers, I believe.

But -- \GETBASEPTR masks off the high nibble in the input to \VAG2's first argument, but the result of \VAG2 is going to have the high-order nibble of the result intact because of the \VAG2 opcode implementation. Compare with GETBASEPTR.N, (copied from above, shortened so it's more obvious):

    TOS = (POINTERMASK & *((LispPTR *)LADDR((POINTERMASK & TOS) + (N))));

@masinter
Copy link
Member Author

I don't understand. This PR attempts to make the Lisp and C implementations agree.
did I miss?

@nbriggs
Copy link
Contributor

nbriggs commented Jul 30, 2022

Yes, you missed. The VAG2 opcode does not strip off the pointer flags, so in the \GETBASEPTR implementation where it just returns the result of \VAG2(...) it is not doing what the GETBASEPTR.N where it masks off the top nibble of the result.

@masinter
Copy link
Member Author

In the LLNEW lisp implementation, \GETBASEPTR is defined by performing two \GETBASE calls which return SMALLPs, and then, after masking the "hiloc", using VAG2 to put together the result.
It does it this way rather than risking allocating memory.

@nbriggs
Copy link
Contributor

nbriggs commented Jul 30, 2022

Yes... but it doesn't mask off the top 4 bits of the result of the VAG2, it just returns it.

@nbriggs
Copy link
Contributor

nbriggs commented Jul 30, 2022

If returning a full 32-bit value is OK then there should be a comment in the Lisp code noting that this is different from the GETBASEPTR.N opcode and why it's OK and/or why it's not possible to do it "correctly".

@masinter
Copy link
Member Author

image
The LOGAND 4095 with \GETBASE of the 'hiloc' already masks the high bits already.

@nbriggs
Copy link
Contributor

nbriggs commented Jul 30, 2022

I finally see what was confusing me -- the \VAG2 is just combining the two halves without an additional indirect. The difference between this code and the maiko code is that maiko does (X & POINTERMASK) before using it while the lisp code assumes that the X argument is already only 28 bits.

@masinter masinter merged commit 27a6063 into master Jul 31, 2022
@masinter masinter deleted the rplptr-interp-fix branch July 31, 2022 23:14
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.

3 participants