| Re: [hatari-devel] Hatari's GEMDOS emulation |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/hatari-devel Archives
]
Hi,
On 11/26/2016 11:12 AM, Thorsten Otto wrote:
Yesterday, i had some weird crashes of a program that i'm currently writing.
In the end, it turned out to be a bug in the program, but it only manifested
itself in Hatari, StonX and Aranym worked fine. The crash happened at $fa00dc,
that is in the cartridge routines which are used for the GEMDOS emulation.
Cartridge part is used for GEMDOS HD Pexec() emulation, because it
requires asking emulated TOS to allocate new basepage, and it's
easier from emulated code than emulator itself.
To be more precisely, that is near the end of find_prog. That routine does a
Fgetdta(), copies the DTA structure to the stack, calls Fsfirst, then copies
the structure back. It crashed writing to address $4, when writing the last
two longs back, which indicates that Fgetdta() got a null pointer.
I think it indicates that either program basepage, or DTA value
in it was invalid.
Fsfirst() call goes to GEMDOS HD emu, which gets DTA from basepage,
and does some checks for it, and call fails if those checks don't pass.
You should see warning about that in console.
Which TOS version you're using and was the Pexec() done by TOS desktop
or something else?
While further investigating, i noticed that in gemdos.c there is only a single
DTA pointer maintained. This is definitely not quite right, every process has
it own DTA address. Assume a situation like:
- Desktop calls Fsetdta. The address is remembered in gemdos.c
- Desktop executes a program.
- Program calls Fsetdta. The address is remembered in gemdos.c
As far as I can see, the stored value is never used
(except for saving it redundantly to memory snapshot
so that it can be restored).
Only value from Fsetda() seems to be getting slightly
earlier warning about invalid DTA. Could be removed
too.
See the attached GEMDOS HD emu cleanup patch.
How others on the list feel about those changes, any
comments / objections?
The other patch adds initial "info dta" command to debugger,
with which one can investigate also non-GEMDOS HD DTAs.
- Program exits.
- Desktop calls Fsfirst, without setting the DTA pointer again. That call,
when emulated in gemdos.c, will now access the DTA that was set by the
program, which is no longer valid.
>
A similar situation will occur when the Program calls Fsfirst without calling
Fsetdta. In that case, the DTA structure of the Desktop will be used. However,
every new process has its initial DTA set to the basepage's command line (at
least with TOS, dunno for MagiC). That initial address is not set with
Fsetdta, it is only set by manipulating the p_dta member of the basepage, so
Hatari does not catch that.
If you'll read gemdos.c more closely, you'll notice that the DTA
pointer is on every Fsfirst() & Fsnext() refreshed from basepage.
Only thing that's not refreshed from TOS side, is DTAIndex index
to internal DTA array.
Conclusion: that handling of the dta access should be reworked.
It definitely has too many global variables, code would
be clearer if few of them would be passed as arguments.
- Eero
GEMDOS HD emulation cleanup:
* Move duplicated file handle & DTA initialization
code to common function(s)
* Remove redundant global variables pDTA & DTA_Gemdos
and pass them as arguments to PopulateDTA()
* Replace redundant snapshot store/restore for DTA_Gemdos
with a dummy (to avoid snapshot incompatibility)
* Remove redundant Fsetdta() catcher function
* Replace magic basepage values with defines
* Add define for masking DTA count
diff -r ffa6fd8d1065 src/gemdos.c
--- a/src/gemdos.c Wed Feb 15 23:50:11 2017 +0200
+++ b/src/gemdos.c Thu Feb 16 01:13:10 2017 +0200
@@ -65,6 +65,10 @@
/* Maximum supported length of a GEMDOS path: */
#define MAX_GEMDOS_PATH 256
+#define BASEPAGE_SIZE (0x80+0x80) /* info + command line */
+#define BASEPAGE_OFFSET_DTA 0x20
+#define BASEPAGE_OFFSET_PARENT 0x24
+
/* Have we re-directed GemDOS vector to our own routines yet? */
bool bInitGemDOS;
@@ -81,10 +85,12 @@
#define TOS_NAMELEN 14
typedef struct {
+ /* GEMDOS internals */
Uint8 index[2];
Uint8 magic[4];
- char dta_pat[TOS_NAMELEN];
- char dta_sattrib;
+ char dta_pat[TOS_NAMELEN]; /* unused */
+ char dta_sattrib; /* unused */
+ /* TOS API */
char dta_attrib;
Uint8 dta_time[2];
Uint8 dta_date[2];
@@ -94,6 +100,7 @@
#define DTA_MAGIC_NUMBER 0x12983476
#define MAX_DTAS_FILES 256 /* Must be ^2 */
+#define MAX_DTAS_MASK (MAX_DTAS_FILES-1)
#define CALL_PEXEC_ROUTINE 3 /* Call our cartridge pexec routine */
#define BASE_FILEHANDLE 64 /* Our emulation handles - MUST not be valid TOS ones, but MUST be <256 */
@@ -135,9 +142,6 @@
static FILE_HANDLE FileHandles[MAX_FILE_HANDLES];
static INTERNAL_DTA InternalDTAs[MAX_DTAS_FILES];
static int DTAIndex; /* Circular index into above */
-static Uint32 DTA_Gemdos; /* DTA address in ST memory space */
-static DTA *pDTA; /* Our GEMDOS hard drive Disk Transfer Address structure */
- /* This a direct pointer to DTA_Gemdos using STMemory_STAddrToPointer() */
static Uint16 CurrentDrive; /* Current drive (0=A,1=B,2=C etc...) */
static Uint32 act_pd; /* Used to get a pointer to the current basepage */
static Uint16 nAttrSFirst; /* File attribute for SFirst/Snext */
@@ -277,7 +281,7 @@
* Populate the DTA buffer with file info.
* @return 0 if entry is ok, 1 if entry should be skipped, < 0 for errors.
*/
-static int PopulateDTA(char *path, struct dirent *file)
+static int PopulateDTA(char *path, struct dirent *file, DTA *pDTA, Uint32 DTA_Gemdos)
{
/* TODO: host file path can be longer than MAX_GEMDOS_PATH */
char tempstr[MAX_GEMDOS_PATH];
@@ -324,22 +328,22 @@
/*-----------------------------------------------------------------------*/
/**
- * Clear a used DTA structure.
+ * Clear given DTA structure.
*/
-static void ClearInternalDTA(void)
+static void ClearInternalDTA(int idx)
{
int i;
/* clear the old DTA structure */
- if (InternalDTAs[DTAIndex].found != NULL)
+ if (InternalDTAs[idx].found != NULL)
{
- for (i=0; i < InternalDTAs[DTAIndex].nentries; i++)
- free(InternalDTAs[DTAIndex].found[i]);
- free(InternalDTAs[DTAIndex].found);
- InternalDTAs[DTAIndex].found = NULL;
+ for (i = 0; i < InternalDTAs[idx].nentries; i++)
+ free(InternalDTAs[idx].found[i]);
+ free(InternalDTAs[idx].found);
+ InternalDTAs[idx].found = NULL;
}
- InternalDTAs[DTAIndex].nentries = 0;
- InternalDTAs[DTAIndex].bUsed = false;
+ InternalDTAs[idx].nentries = 0;
+ InternalDTAs[idx].bUsed = false;
}
@@ -453,6 +457,23 @@
ForcedHandles[i].Basepage = 0;
}
+/**
+ * Clear & un-force all file handles
+ */
+static void GemDOS_ClearAllFileHandles(void)
+{
+ int i;
+
+ for(i = 0; i < ARRAY_SIZE(FileHandles); i++)
+ {
+ GemDOS_CloseFileHandle(i);
+ }
+ for(i = 0; i < ARRAY_SIZE(ForcedHandles); i++)
+ {
+ GemDOS_UnforceFileHandle(i);
+ }
+}
+
/*-----------------------------------------------------------------------*/
/**
@@ -479,18 +500,12 @@
int i;
bInitGemDOS = false;
- /* Clear handles structure */
- memset(FileHandles, 0, sizeof(FileHandles));
- for(i = 0; i < ARRAY_SIZE(ForcedHandles); i++)
- {
- GemDOS_UnforceFileHandle(i);
- }
+ GemDOS_ClearAllFileHandles();
+
/* Clear DTAs */
for(i = 0; i < ARRAY_SIZE(InternalDTAs); i++)
{
- InternalDTAs[i].bUsed = false;
- InternalDTAs[i].nentries = 0;
- InternalDTAs[i].found = NULL;
+ ClearInternalDTA(i);
}
DTAIndex = 0;
}
@@ -504,20 +519,7 @@
{
int i;
- /* Init file handles table */
- for (i = 0; i < ARRAY_SIZE(FileHandles); i++)
- {
- GemDOS_CloseFileHandle(i);
- }
- for(i = 0; i < ARRAY_SIZE(ForcedHandles); i++)
- {
- GemDOS_UnforceFileHandle(i);
- }
- for (DTAIndex = 0; DTAIndex < MAX_DTAS_FILES; DTAIndex++)
- {
- ClearInternalDTA();
- }
- DTAIndex = 0;
+ GemDOS_Init();
if (emudrives)
{
@@ -533,11 +535,8 @@
}
/* Reset */
- bInitGemDOS = false;
CurrentDrive = nBootDrive;
Symbols_RemoveCurrentProgram();
- DTA_Gemdos = 0x0;
- pDTA = NULL;
}
/*-----------------------------------------------------------------------*/
@@ -744,7 +743,7 @@
/*-----------------------------------------------------------------------*/
/**
- * Un-init the GEMDOS drive
+ * Un-init GEMDOS drives
*/
void GemDOS_UnInitDrives(void)
{
@@ -776,7 +775,7 @@
*/
void GemDOS_MemorySnapShot_Capture(bool bSave)
{
- int i;
+ int i, dummy;
bool bEmudrivesAvailable;
/* Save/Restore the emudrives structure */
@@ -826,35 +825,14 @@
MemorySnapShot_Store(&DTAIndex,sizeof(DTAIndex));
MemorySnapShot_Store(&bInitGemDOS,sizeof(bInitGemDOS));
MemorySnapShot_Store(&act_pd, sizeof(act_pd));
- if (bSave)
- {
- /* Store the value in ST memory space */
- MemorySnapShot_Store ( &DTA_Gemdos , sizeof(DTA_Gemdos) );
- }
- else
- {
- /* Restore the value in ST memory space and update pDTA */
- MemorySnapShot_Store ( &DTA_Gemdos , sizeof(DTA_Gemdos) );
- if ( DTA_Gemdos == 0x0 )
- pDTA = NULL;
- else
- pDTA = (DTA *)STMemory_STAddrToPointer( DTA_Gemdos );
- }
+ MemorySnapShot_Store(&dummy, sizeof(dummy)); /* redundant DTA_Gemdos */
MemorySnapShot_Store(&CurrentDrive,sizeof(CurrentDrive));
/* Don't save file handles as files may have changed which makes
* it impossible to get a valid handle back
*/
if (!bSave)
{
- /* Clear file handles */
- for(i = 0; i < ARRAY_SIZE(FileHandles); i++)
- {
- GemDOS_CloseFileHandle(i);
- }
- for(i = 0; i < ARRAY_SIZE(ForcedHandles); i++)
- {
- GemDOS_UnforceFileHandle(i);
- }
+ GemDOS_ClearAllFileHandles();
}
}
@@ -887,11 +865,11 @@
{
int maxparents = 12; /* prevent basepage parent loops */
Uint32 basepage = STMemory_ReadLong(act_pd);
- while (maxparents-- > 0 && STMemory_CheckAreaType ( basepage, 0x100, ABFLAG_RAM ) )
+ while (maxparents-- > 0 && STMemory_CheckAreaType(basepage, BASEPAGE_SIZE, ABFLAG_RAM))
{
if (basepage == checkbase)
return true;
- basepage = STMemory_ReadLong(basepage + 0x24); /* parent */
+ basepage = STMemory_ReadLong(basepage + BASEPAGE_OFFSET_PARENT);
}
return false;
}
@@ -1436,34 +1414,6 @@
/*-----------------------------------------------------------------------*/
/**
- * GEMDOS Set Disk Transfer Address (DTA)
- * Call 0x1A
- */
-static bool GemDOS_SetDTA(Uint32 Params)
-{
- /* Look up on stack to find where DTA is */
- DTA_Gemdos = STMemory_ReadLong(Params);
-
- LOG_TRACE(TRACE_OS_GEMDOS, "GEMDOS 0x1A Fsetdta(0x%x) at PC 0x%X\n", DTA_Gemdos,
- M68000_GetPC());
-
- if ( STMemory_CheckAreaType ( DTA_Gemdos, sizeof(DTA), ABFLAG_RAM ) )
- {
- /* Store as PC pointer */
- pDTA = (DTA *)STMemory_STAddrToPointer(DTA_Gemdos);
- }
- else
- {
- Log_Printf(LOG_WARN, "GEMDOS Fsetdta() failed due to invalid DTA address 0x%x\n", DTA_Gemdos);
- DTA_Gemdos = 0x0;
- pDTA = NULL;
- }
- /* redirect to TOS */
- return false;
-}
-
-/*-----------------------------------------------------------------------*/
-/**
* GEMDOS Dfree Free disk space.
* Call 0x36
*/
@@ -2629,17 +2579,17 @@
struct dirent **temp;
int Index;
int ret;
+ DTA *pDTA;
+ Uint32 DTA_Gemdos;
LOG_TRACE(TRACE_OS_GEMDOS, "GEMDOS 0x4F Fsnext() at PC 0x%X\n" , M68000_GetPC());
/* Refresh pDTA pointer (from the current basepage) */
- DTA_Gemdos = STMemory_ReadLong(STMemory_ReadLong(act_pd)+32);
+ DTA_Gemdos = STMemory_ReadLong(STMemory_ReadLong(act_pd) + BASEPAGE_OFFSET_DTA);
if ( !STMemory_CheckAreaType ( DTA_Gemdos, sizeof(DTA), ABFLAG_RAM ) )
{
Log_Printf(LOG_WARN, "GEMDOS Fsnext() failed due to invalid DTA address 0x%x\n", DTA_Gemdos);
- DTA_Gemdos = 0x0;
- pDTA = NULL;
Regs[REG_D0] = GEMDOS_EINTRN; /* "internal error */
return true;
}
@@ -2653,7 +2603,7 @@
}
/* Find index into our list of structures */
- Index = do_get_mem_word(pDTA->index) & (MAX_DTAS_FILES-1);
+ Index = do_get_mem_word(pDTA->index) & MAX_DTAS_MASK;
if (nAttrSFirst == GEMDOS_FILE_ATTRIB_VOLUME_LABEL)
{
@@ -2672,7 +2622,8 @@
}
ret = PopulateDTA(InternalDTAs[Index].path,
- temp[InternalDTAs[Index].centry++]);
+ temp[InternalDTAs[Index].centry++],
+ pDTA, DTA_Gemdos);
} while (ret == 1);
if (ret < 0)
@@ -2702,6 +2653,8 @@
int Drive;
DIR *fsdir;
int i,j,count;
+ DTA *pDTA;
+ Uint32 DTA_Gemdos;
/* Find filename to search for */
pszFileName = (char *)STMemory_STAddrToPointer(STMemory_ReadLong(Params));
@@ -2722,13 +2675,11 @@
szActualFileName, sizeof(szActualFileName));
/* Refresh pDTA pointer (from the current basepage) */
- DTA_Gemdos = STMemory_ReadLong(STMemory_ReadLong(act_pd)+32);
+ DTA_Gemdos = STMemory_ReadLong(STMemory_ReadLong(act_pd) + BASEPAGE_OFFSET_DTA);
if ( !STMemory_CheckAreaType ( DTA_Gemdos, sizeof(DTA), ABFLAG_RAM ) )
{
Log_Printf(LOG_WARN, "GEMDOS Fsfirst() failed due to invalid DTA address 0x%x\n", DTA_Gemdos);
- DTA_Gemdos = 0x0;
- pDTA = NULL;
Regs[REG_D0] = GEMDOS_EINTRN; /* "internal error */
return true;
}
@@ -2744,7 +2695,7 @@
do_put_mem_long(pDTA->magic, DTA_MAGIC_NUMBER);
if (InternalDTAs[DTAIndex].bUsed == true)
- ClearInternalDTA();
+ ClearInternalDTA(DTAIndex);
InternalDTAs[DTAIndex].bUsed = true;
/* Were we looking for the volume label? */
@@ -2814,7 +2765,7 @@
GemDOS_SNext();
/* increment DTA index */
DTAIndex++;
- DTAIndex &= (MAX_DTAS_FILES-1);
+ DTAIndex &= MAX_DTAS_MASK;
return true;
}
@@ -3264,9 +3256,6 @@
case 0x0e:
Finished = GemDOS_SetDrv(Params);
break;
- case 0x1a:
- Finished = GemDOS_SetDTA(Params);
- break;
case 0x31:
Finished = GemDOS_Ptermres(Params);
break;
@@ -3370,6 +3359,7 @@
case 0x09: /* Cconws */
case 0x0A: /* Cconrs */
+ case 0x1A: /* Fsetdta */
case 0x20: /* Super */
case 0x48: /* Malloc */
case 0x49: /* Mfree */
diff -r ffa6fd8d1065 src/debug/debugInfo.c
--- a/src/debug/debugInfo.c Wed Feb 15 23:50:11 2017 +0200
+++ b/src/debug/debugInfo.c Thu Feb 16 01:13:10 2017 +0200
@@ -152,6 +152,25 @@
}
/**
+ * DebugInfo_DTA: ask GEMDOS to show DTA info after validating
+ * basepage address, if given.
+ */
+static void DebugInfo_DTA(FILE *fp, Uint32 basepage)
+{
+ basepage = DebugInfo_CurrentBasepage(basepage, false);
+ if (!basepage) {
+ fprintf(fp, "ERROR: no basepage!\n");
+ return;
+ }
+ if (!STMemory_CheckAreaType(basepage, BASEPAGE_SIZE, ABFLAG_RAM) ||
+ STMemory_ReadLong(basepage) != basepage) {
+ fprintf(fp, "Pointer 0x%06x to basepage address is invalid!\n", basepage);
+ return;
+ }
+ GemDOS_InfoDTA(fp, basepage);
+}
+
+/**
* DebugInfo_GetTEXT: return current program TEXT segment address
* or zero if basepage missing/invalid. For virtual debugger variable.
*/
@@ -652,6 +671,7 @@
{ true, "dspmemdump",DebugInfo_DspMemDump, DebugInfo_DspMemArgs, "Dump DSP memory from given <space> <address>" },
{ true, "dspregs", DebugInfo_DspRegister,NULL, "Show DSP register contents" },
#endif
+ { false, "dta", DebugInfo_DTA, NULL, "Show DTA information from current [or given] basepage" },
{ true, "file", DebugInfo_FileParse, DebugInfo_FileArgs, "Parse commands from given debugger input <file>" },
{ false,"gemdos", GemDOS_Info, NULL, "Show GEMDOS HDD emu information (with <value>, show opcodes)" },
{ true, "history", History_Show, NULL, "Show history of last <count> instructions" },
diff -r ffa6fd8d1065 src/includes/gemdos.h
--- a/src/includes/gemdos.h Wed Feb 15 23:50:11 2017 +0200
+++ b/src/includes/gemdos.h Thu Feb 16 01:13:10 2017 +0200
@@ -27,6 +27,7 @@
extern void GemDOS_CreateHardDriveFileName(int Drive, const char *pszFileName, char *pszDestName, int nDestNameLen);
extern bool GemDOS_IsDriveEmulated(int drive);
extern void GemDOS_Info(FILE *fp, Uint32 bShowOpcodes);
+extern void GemDOS_InfoDTA(FILE *fp, Uint32 basepage);
extern void GemDOS_OpCode(void);
extern void GemDOS_Boot(void);
@@ -3209,11 +3160,52 @@
fputs("- None.\n", fp);
}
+/**
+ * Show given basepage current DTA info
+ * (works also without GEMDOS HD emu)
+ */
+void GemDOS_InfoDTA(FILE *fp, Uint32 basepage)
+{
+ Uint32 dta_atari, magic;
+ char name[TOS_NAMELEN+1];
+ DTA *dta;
+
+ Uint32 gemdos_basepage = STMemory_ReadLong(act_pd);
+ if (gemdos_basepage != basepage)
+ fprintf(fp, "WARNING: given basepage (0x%x) doesn't match GEMDOS emu one (0x%x)\n",
+ basepage, gemdos_basepage);
+
+ /* Refresh pDTA pointer (from the current basepage) */
+ dta_atari = STMemory_ReadLong(basepage + BASEPAGE_OFFSET_DTA);
+ if (dta_atari >= basepage && dta_atari + sizeof(DTA) < basepage + BASEPAGE_SIZE)
+ fprintf(fp, "WARNING: basepage (0x%x) DTA (0x%x) is within basepage!\n",
+ basepage, dta_atari);
+
+ dta = (DTA *)STMemory_STAddrToPointer(dta_atari);
+
+ fprintf(fp, "Basepage (0x%x) DTA (0x%x):\n", basepage, dta_atari);
+ memcpy(name, dta->dta_name, TOS_NAMELEN);
+ name[TOS_NAMELEN] = '\0';
+ magic = do_get_mem_long(dta->magic);
+ fprintf(fp, "- magic: 0x%08x (GEMDOS HD = 0x%08x)\n", magic, DTA_MAGIC_NUMBER);
+ if (magic == DTA_MAGIC_NUMBER)
+ fprintf(fp, "- index: 0x%04x\n", do_get_mem_word(dta->index));
+ fprintf(fp, "- attr: 0x%x\n", dta->dta_attrib);
+ fprintf(fp, "- time: 0x%04x\n", do_get_mem_word(dta->dta_time));
+ fprintf(fp, "- date: 0x%04x\n", do_get_mem_word(dta->dta_date));
+ fprintf(fp, "- size: %d\n", do_get_mem_long(dta->dta_size));
+ fprintf(fp, "- name: '%s'\n", name);
+}
+
#else /* !ENABLE_TRACING */
void GemDOS_Info(FILE *fp, Uint32 bShowOpcodes)
{
fputs("Hatari isn't configured with ENABLE_TRACING\n", fp);
}
+void GemDOS_InfoDTA(FILE *fp, Uint32 basepage)
+{
+ fputs("Hatari isn't configured with ENABLE_TRACING\n", fp);
+}
#endif /* !ENABLE_TRACING */