From 55591557b629fd73d9e81c8e684421bfb52c6f10 Mon Sep 17 00:00:00 2001 From: Nick Briggs Date: Thu, 7 Mar 2024 09:30:59 -0800 Subject: [PATCH 1/3] Clarify the expected size/packing of bit fields in stack structures While packing is not guaranteed, and most compilers will combine adjacent bit fields regardless of the alignment requirements of the datatype that is being divided, here we can be clearer that we expect packing into 16-bit fields and we do not require (and must not have) 32-bit alignment of the collections of bitfields. --- inc/stack.h | 102 ++++++++++++++++++++++++++-------------------------- 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/inc/stack.h b/inc/stack.h index f93be91..721c15d 100644 --- a/inc/stack.h +++ b/inc/stack.h @@ -79,22 +79,22 @@ typedef struct fnhead { } FNHEAD; typedef struct frameex1 { - unsigned flags : 3; - unsigned fast : 1; - unsigned nil2 : 1; /* not used, prev: This frame treats N-func */ - unsigned incall : 1; - unsigned validnametable : 1; + unsigned short flags : 3; + unsigned short fast : 1; + unsigned short nil2 : 1; /* not used, prev: This frame treats N-func */ + unsigned short incall : 1; + unsigned short validnametable : 1; /* 0: look for FunctionHeader 1: look for NameTable on this FrameEx */ - unsigned nopush : 1; - unsigned usecount : 8; + unsigned short nopush : 1; + unsigned short usecount : 8; DLword alink; /* alink pointer (Low addr) */ #ifdef BIGVM LispPTR fnheader; /* pointer to FunctionHeader (Hi2 addr) */ #else DLword lofnheader; /* pointer to FunctionHeader (Low addr) */ - unsigned hi1fnheader : 8; /* pointer to FunctionHeader (Hi1 addr) */ - unsigned hi2fnheader : 8; /* pointer to FunctionHeader (Hi2 addr) */ + unsigned short hi1fnheader : 8; /* pointer to FunctionHeader (Hi1 addr) */ + unsigned short hi2fnheader : 8; /* pointer to FunctionHeader (Hi2 addr) */ #endif /* BIGVM */ DLword nextblock; /* pointer to FreeStackBlock */ DLword pc; /* Program counter */ @@ -102,23 +102,23 @@ typedef struct frameex1 { LispPTR nametable; /* ptr to NameTable of this FrameEx (Hi2 addr) */ #else DLword lonametable; /* ptr to NameTable of this FrameEx (Low addr) */ - unsigned hi1nametable : 8; /* ptr to NameTable of this FrameEx (Hi1 addr) */ - unsigned hi2nametable : 8; /* ptr to NameTable of this FrameEx (Hi2 addr) */ + unsigned short hi1nametable : 8; /* ptr to NameTable of this FrameEx (Hi1 addr) */ + unsigned short hi2nametable : 8; /* ptr to NameTable of this FrameEx (Hi2 addr) */ #endif /* BIGVM */ DLword blink; /* blink pointer (Low addr) */ DLword clink; /* clink pointer (Low addr) */ } FX; typedef struct frameex2 { - unsigned flags : 3; - unsigned fast : 1; - unsigned nil2 : 1; /* not used, prev: This frame treats N-func */ - unsigned incall : 1; - unsigned validnametable : 1; + unsigned short flags : 3; + unsigned short fast : 1; + unsigned short nil2 : 1; /* not used, prev: This frame treats N-func */ + unsigned short incall : 1; + unsigned short validnametable : 1; /* 0: look for FunctionHeader 1: look for NameTable on this FrameEx */ - unsigned nopush : 1; - unsigned usecount : 8; + unsigned short nopush : 1; + unsigned short usecount : 8; DLword alink; /* alink pointer (Low addr) */ LispPTR fnheader; /* pointer to FunctionHeader */ DLword nextblock; /* pointer to FreeStackBlock */ @@ -135,11 +135,11 @@ typedef struct fxblock { } FXBLOCK; typedef struct basic_frame { - unsigned flags : 3; - unsigned nil : 3; - unsigned residual : 1; - unsigned padding : 1; - unsigned usecnt : 8; + unsigned short flags : 3; + unsigned short nil : 3; + unsigned short residual : 1; + unsigned short padding : 1; + unsigned short usecnt : 8; DLword ivar; /* stk offset of IVARs for this frame ?? */ } Bframe; @@ -185,33 +185,33 @@ typedef struct fnhead { unsigned nil3 : 2; /* not used */ unsigned nil2 : 2; /* not used */ #endif /* BIGVM */ - unsigned argtype : 2; /* ?? */ - unsigned byteswapped : 1; /* code was reswapped. */ - unsigned nil4 : 1; /* not used, prev: native translated? */ - unsigned fvaroffset : 8; + unsigned short argtype : 2; /* ?? */ + unsigned short byteswapped : 1; /* code was reswapped. */ + unsigned short nil4 : 1; /* not used, prev: native translated? */ + unsigned short fvaroffset : 8; /* DLword offset from head of NameTable */ - unsigned nlocals : 8; /* ?? */ + unsigned short nlocals : 8; /* ?? */ DLword ntsize; /* size of NameTable */ /* NameTable of variable length is following with this structure. */ } FNHEAD; typedef struct frameex1 { DLword alink; /* alink pointer (Low addr) */ - unsigned usecount : 8; - unsigned nopush : 1; - unsigned validnametable : 1; + unsigned short usecount : 8; + unsigned short nopush : 1; + unsigned short validnametable : 1; /* 0: look for FunctionHeader 1: look for NameTable on this FrameEx */ - unsigned incall : 1; - unsigned nil2 : 1; /* not used, prev: This frame treats N-func */ - unsigned fast : 1; - unsigned flags : 3; /* hi word */ + unsigned short incall : 1; + unsigned short nil2 : 1; /* not used, prev: This frame treats N-func */ + unsigned short fast : 1; + unsigned short flags : 3; /* hi word */ #ifdef BIGVM LispPTR fnheader; /* pointer to FunctionHeader (Hi2 addr) */ #else - unsigned hi2fnheader : 8; /* pointer to FunctionHeader (Hi2 addr) */ - unsigned hi1fnheader : 8; /* pointer to FunctionHeader (Hi1 addr) */ + unsigned short hi2fnheader : 8; /* pointer to FunctionHeader (Hi2 addr) */ + unsigned short hi1fnheader : 8; /* pointer to FunctionHeader (Hi1 addr) */ DLword lofnheader; /* pointer to FunctionHeader (Low addr) */ #endif /* BIGVM */ @@ -221,8 +221,8 @@ typedef struct frameex1 { #ifdef BIGVM LispPTR nametable; /* pointer to NameTable of this FX (Hi2 addr) */ #else - unsigned hi2nametable : 8; /* pointer to NameTable of this FX (Hi2 addr) */ - unsigned hi1nametable : 8; /* pointer to NameTable of this FX (Hi1 addr) */ + unsigned short hi2nametable : 8; /* pointer to NameTable of this FX (Hi2 addr) */ + unsigned short hi1nametable : 8; /* pointer to NameTable of this FX (Hi1 addr) */ DLword lonametable; /* pointer to NameTable of this FX (Low addr) */ #endif /* BIGVM */ @@ -232,15 +232,15 @@ typedef struct frameex1 { typedef struct frameex2 { DLword alink; /* alink pointer (Low addr) */ - unsigned usecount : 8; - unsigned nopush : 1; - unsigned validnametable : 1; + unsigned short usecount : 8; + unsigned short nopush : 1; + unsigned short validnametable : 1; /* 0: look for FunctionHeader 1: look for NameTable on this FrameEx */ - unsigned incall : 1; - unsigned nil2 : 1; /* not used, prev: This frame treats N-func */ - unsigned fast : 1; - unsigned flags : 3; + unsigned short incall : 1; + unsigned short nil2 : 1; /* not used, prev: This frame treats N-func */ + unsigned short fast : 1; + unsigned short flags : 3; LispPTR fnheader; /* pointer to FunctionHeader (swapped) */ @@ -261,11 +261,11 @@ typedef struct fxblock { typedef struct basic_frame { DLword ivar; - unsigned usecnt : 8; - unsigned padding : 1; - unsigned residual : 1; - unsigned nil : 3; - unsigned flags : 3; + unsigned short usecnt : 8; + unsigned short padding : 1; + unsigned short residual : 1; + unsigned short nil : 3; + unsigned short flags : 3; } Bframe; From 939c23c03bb645efc71b334293fdb714a477c861 Mon Sep 17 00:00:00 2001 From: Nick Briggs Date: Thu, 7 Mar 2024 10:01:19 -0800 Subject: [PATCH 2/3] Additional check for bad stack offset calculation Pointer difference calculations on an inappropriate stack pointer could result in a negative offset, not just an offset that is too large to fit in 16 bits. Complain if either case occurs. --- inc/adr68k.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/inc/adr68k.h b/inc/adr68k.h index 9799780..42cae9c 100644 --- a/inc/adr68k.h +++ b/inc/adr68k.h @@ -63,8 +63,8 @@ static inline DLword StackOffsetFromNative(void *SAddr) { /* Stack offsets are expressed as an offset in DLwords from the stack base */ ptrdiff_t hoffset = (DLword *)SAddr - Stackspace; - if (hoffset > 0xffff) { - printf("Stack offset is too large: 0x%tx\n", hoffset); + if (hoffset > 0xffff || hoffset < 0) { + printf("Stack offset is out of range: 0x%tx\n", hoffset); } return (DLword)hoffset; } From 68baf6fb30f8c19c4dd7a15bd9beb6a28d7307c2 Mon Sep 17 00:00:00 2001 From: Nick Briggs Date: Thu, 7 Mar 2024 10:45:38 -0800 Subject: [PATCH 3/3] Use ...FromStackOffset functions where appropriate Rather than adding/or-ing the STK_OFFSET constant into a stack offset to convert it to a pointer in general Lisp memory and then converting from that to a native address... use the functions specifically present to do those conversions with embedded checks on stack offset validity. --- inc/return.h | 4 ++-- src/bbtsub.c | 2 +- src/dbgtool.c | 8 ++++---- src/findkey.c | 2 +- src/loopsops.c | 4 ++-- src/main.c | 4 ++-- src/return.c | 2 +- src/testtool.c | 14 +++++++------- src/ufn.c | 2 +- 9 files changed, 21 insertions(+), 21 deletions(-) diff --git a/inc/return.h b/inc/return.h index 90749fb..e493032 100644 --- a/inc/return.h +++ b/inc/return.h @@ -37,7 +37,7 @@ #define FastRetCALL \ do { \ /* Get IVar from Returnee's IVAR offset slot(BF) */ \ - IVar = NativeAligned2FromLAddr(STK_OFFSET | GETWORD((DLword *)CURRENTFX -1)); \ + IVar = NativeAligned2FromStackOffset(GETWORD((DLword *)CURRENTFX - 1)); \ /* Get FuncObj from Returnee's FNHEAD slot in FX */ \ FuncObj = (struct fnhead *)NativeAligned4FromLAddr(FX_FNHEADER); \ /* Get PC from Returnee's pc slot in FX */ \ @@ -47,7 +47,7 @@ #define FastRetCALL \ do { \ /* Get IVar from Returnee's IVAR offset slot(BF) */ \ - IVar = NativeAligned2FromLAddr(STK_OFFSET | GETWORD((DLword *)CURRENTFX -1)); \ + IVar = NativeAligned2FromStackOffset(GETWORD((DLword *)CURRENTFX - 1)); \ /* Get FuncObj from Returnee's FNHEAD slot in FX */ \ FuncObj = (struct fnhead *)NativeAligned4FromLAddr(FX_FNHEADER); \ /* Get PC from Returnee's pc slot in FX */ \ diff --git a/src/bbtsub.c b/src/bbtsub.c index af07f14..e824b09 100644 --- a/src/bbtsub.c +++ b/src/bbtsub.c @@ -1664,7 +1664,7 @@ void ccfuncall(unsigned int atom_index, int argnum, int bytenum) CURRENTFX->nextblock = (LAddrFromNative(CurrentStackPTR) & 0x0ffff) - (argnum << 1) + 4 /* +3 */; /* Setup IVar */ /* XXX: is it really only 2-byte aligned? */ - IVar = NativeAligned2FromLAddr((((LispPTR)(CURRENTFX->nextblock)) | STK_OFFSET)); + IVar = NativeAligned2FromStackOffset(CURRENTFX->nextblock); /* Set PC to the Next Instruction and save into FX */ CURRENTFX->pc = ((UNSIGNED)PC - (UNSIGNED)FuncObj) + bytenum; diff --git a/src/dbgtool.c b/src/dbgtool.c index 70ee8f4..3f0dad7 100644 --- a/src/dbgtool.c +++ b/src/dbgtool.c @@ -303,14 +303,14 @@ int sf(struct frameex1 *fx_addr68k) { if (((fx_addr68k)->alink & 1) == 0) { /* FAST */ bf = (Bframe *)(((DLword *)fx_addr68k) - 2); } else { /* SLOW */ - bf = (Bframe *)NativeAligned4FromLAddr(((fx_addr68k)->blink + STK_OFFSET)); + bf = (Bframe *)NativeAligned4FromStackOffset((fx_addr68k)->blink); } /* Print IVARs */ printf("IVAR -------\n"); BT_morep; - ptr = NativeAligned2FromLAddr(STK_OFFSET + bf->ivar); + ptr = NativeAligned2FromStackOffset(bf->ivar); i = 0; while (ptr != (DLword *)bf) { ptrlo = ptr + 1; @@ -475,7 +475,7 @@ int sf(struct frameex1 *fx_addr68k) { i++; } if (fx_addr68k->alink == 11) /* for contextsw */ - next68k = (DLword *)NativeAligned2FromLAddr((fx_addr68k->nextblock + STK_OFFSET)); + next68k = NativeAligned2FromStackOffset(fx_addr68k->nextblock); else next68k = CurrentStackPTR; @@ -493,7 +493,7 @@ int sf(struct frameex1 *fx_addr68k) { return (-1); } - next68k = (DLword *)NativeAligned2FromLAddr((fx_addr68k->nextblock + STK_OFFSET)); + next68k = NativeAligned2FromStackOffset(fx_addr68k->nextblock); ptr = (DLword *)(fx_addr68k + 1); i = 0; diff --git a/src/findkey.c b/src/findkey.c index 88d250a..f456f36 100644 --- a/src/findkey.c +++ b/src/findkey.c @@ -45,7 +45,7 @@ LispPTR N_OP_findkey(LispPTR tos, int byte) { #endif if (CURRENTFX->alink & 1) { /* slow case */ - find_end = (DLword *)NativeAligned2FromLAddr(STK_OFFSET | (CURRENTFX->blink - 4)); + find_end = NativeAligned2FromStackOffset(CURRENTFX->blink - 4); } else { /* Fast cae */ find_end = ((DLword *)CURRENTFX) - 2 - 4; } diff --git a/src/loopsops.c b/src/loopsops.c index 3544905..90e2c1e 100644 --- a/src/loopsops.c +++ b/src/loopsops.c @@ -26,7 +26,7 @@ #include "gcarraydefs.h" // for get_package_atom #include "gcdata.h" // for FRPLPTR #include "lispemul.h" // for LispPTR, state, CurrentStackPTR, NIL_PTR, NIL -#include "lispmap.h" // for S_POSITIVE, STK_OFFSET +#include "lispmap.h" // for S_POSITIVE #include "loopsopsdefs.h" // for lcfuncall, LCFetchMethod, LCFetchMethodOrHelp #include "lspglob.h" #include "lsptypes.h" // for GetDTD, GetTypeNumber, dtd, Listp, GETWORD @@ -358,7 +358,7 @@ LispPTR lcfuncall(unsigned int atom_index, int argnum, int bytenum) CURRENTFX->nextblock = (LAddrFromNative(CurrentStackPTR) & 0x0ffff) - (argnum << 1) + 4 /* +3 */; /* Setup IVar */ - IVar = NativeAligned2FromLAddr((((LispPTR)(CURRENTFX->nextblock)) | STK_OFFSET)); + IVar = NativeAligned2FromStackOffset(CURRENTFX->nextblock); /* Set PC to the Next Instruction and save into FX */ CURRENTFX->pc = ((UNSIGNED)PC - (UNSIGNED)FuncObj) + bytenum; diff --git a/src/main.c b/src/main.c index 4d23107..b6e05a8 100644 --- a/src/main.c +++ b/src/main.c @@ -727,9 +727,9 @@ void start_lisp(void) { TopOfStack = 0; Error_Exit = 0; - PVar = (DLword *)NativeAligned2FromLAddr(STK_OFFSET | InterfacePage->currentfxp) + FRAMESIZE; + PVar = NativeAligned2FromStackOffset(InterfacePage->currentfxp) + FRAMESIZE; - freeptr = next68k = NativeAligned2FromLAddr(STK_OFFSET | CURRENTFX->nextblock); + freeptr = next68k = NativeAligned2FromStackOffset(CURRENTFX->nextblock); if (GETWORD(next68k) != STK_FSB_WORD) error("Starting Lisp: Next stack block isn't free!"); diff --git a/src/return.c b/src/return.c index 0b8a23b..fe87cf6 100644 --- a/src/return.c +++ b/src/return.c @@ -129,7 +129,7 @@ void contextsw(DLword fxnum, DLword bytenum, DLword flags) Midpunt(fxnum); /* exchanging FX */ - next68k = (DLword *)NativeAligned2FromLAddr(STK_OFFSET | CURRENTFX->nextblock); + next68k = NativeAligned2FromStackOffset(CURRENTFX->nextblock); if (GETWORD(next68k) != STK_FSB_WORD) error("contextsw(): MP9316"); freeptr = next68k; diff --git a/src/testtool.c b/src/testtool.c index 1f49fd0..bf6b8a7 100644 --- a/src/testtool.c +++ b/src/testtool.c @@ -57,7 +57,7 @@ #include "gcarraydefs.h" // for aref1 #include "kprintdefs.h" // for print, prindatum #include "lispemul.h" // for DLword, LispPTR, DLbyte, state, T, ConsCell -#include "lispmap.h" // for STK_OFFSET, ATOMS_HI +#include "lispmap.h" // for ATOMS_HI #include "lspglob.h" // for Package_from_Index_word, Stackspace #include "lsptypes.h" // for GETWORD, dtd, GETBYTE, NEWSTRINGP, GetType... #include "mkatomdefs.h" // for compare_chars, make_atom @@ -852,7 +852,7 @@ void dump_bf(Bframe *bf) { if (BFRAMEPTR(bf)->residual) { goto printflags; } - ptr = NativeAligned2FromLAddr(STK_OFFSET + bf->ivar); + ptr = NativeAligned2FromStackOffset(bf->ivar); if ((((DLword *)bf - ptr) > 512) || (((UNSIGNED)ptr & 1) != 0)) { printf("\nInvalid basic frame"); return; @@ -910,7 +910,7 @@ void dump_fx(struct frameex1 *fx_addr68k) { /* should pay attention to the name table like RAID does */ - next68k = (DLword *)NativeAligned2FromLAddr((fx_addr68k->nextblock + STK_OFFSET)); + next68k = NativeAligned2FromStackOffset(fx_addr68k->nextblock); if (fx_addr68k == CURRENTFX) { next68k = CurrentStackPTR + 2; } if ((next68k < ptr) || (((UNSIGNED)next68k & 1) != 0)) { @@ -937,7 +937,7 @@ void dump_stackframe(struct frameex1 *fx_addr68k) { if ((fx_addr68k->alink & 1) == 0) { /* FAST */ bf = (Bframe *)(((DLword *)fx_addr68k) - 2); } else { /* SLOW */ - bf = (Bframe *)NativeAligned4FromLAddr((fx_addr68k->blink + STK_OFFSET)); + bf = (Bframe *)NativeAligned4FromStackOffset(fx_addr68k->blink); } dump_bf(bf); dump_fx((struct frameex1 *)fx_addr68k); @@ -1104,12 +1104,12 @@ void all_stack_dump(DLword start, DLword end, DLword silent) if (start == 0) start68k = Stackspace + InterfacePage->stackbase; else - start68k = NativeAligned2FromLAddr(STK_OFFSET | start); + start68k = NativeAligned2FromStackOffset(start); if (end == 0) end68k = Stackspace + InterfacePage->endofstack; else - end68k = NativeAligned2FromLAddr(STK_OFFSET | end); + end68k = NativeAligned2FromStackOffset(end); stkptr = (STKH *)start68k; @@ -1156,7 +1156,7 @@ void all_stack_dump(DLword start, DLword end, DLword silent) printf(" <-***current***"); size = EndSTKP - (DLword *)stkptr; } else { - size = NativeAligned2FromLAddr(STK_OFFSET | ((FX *)stkptr)->nextblock) - (DLword *)stkptr; + size = NativeAligned2FromStackOffset(((FX *)stkptr)->nextblock) - (DLword *)stkptr; } goto checksize; default: diff --git a/src/ufn.c b/src/ufn.c index 3f64be2..93eeff7 100644 --- a/src/ufn.c +++ b/src/ufn.c @@ -107,7 +107,7 @@ void ufn(DLword bytecode) (LAddrFromNative(CurrentStackPTR) & 0x0ffff) - (entry68k->arg_num << 1) + 2 /** +1 **/; /* Setup IVar */ - IVar = NativeAligned2FromLAddr((((LispPTR)(CURRENTFX->nextblock)) | STK_OFFSET)); + IVar = NativeAligned2FromStackOffset(CURRENTFX->nextblock); #ifdef LISPTRACE print(entry68k->atom_name);