Re: [hatari-devel] Debugger GUI enquiry

[ Thread Index | Date Index | More lists.tuxfamily.org/hatari-devel Archives ]

Hello

Attached is an early patch to try to hook remote debug command processing into the existing core loop. Some notes:

- the idea is to jump in when DebugUI() is called and service commands from the network socket. The code in control.c registers a callback into the debugui system when it connects/disconnects.
- it doesn't actually do anything useful yet -- the DebugUI_ProcessRemoteDebug call is basically a stub, i.e. it doesn't implement any useful commands yet. I have a local set of changes which implements a system similar to the existing console debug commands, but stripped down to handle the simpler cases of remote-debug, plus some rudimentary implementations of disasm/memory/register commands.
- there are no changes to the Python UI in this. I have something running (see a screenshot at https://pasteboard.co/J7VKUw5f.png) at a good interactive speed when single-steeping, but it's strictly a prototype with no UI refinement or design, and the code is a mess.

Anyway, see what you think. I've had a bad day at work, so be gentle :)

cheers
Steve



On Mon, 11 May 2020 at 11:27, Eero Tamminen <oak@xxxxxxxxxxxxxx> wrote:
Hi,

On 5/10/20 11:13 PM, Steven Tattersall wrote:
> On Sun, 10 May 2020 at 10:08, Eero Tamminen <oak@xxxxxxxxxxxxxx> wrote:
> I'm slowly getting there with an initial patch. To make it manageable it
> will probably only contain the core bit where the hooks have to go into the
> main loop and route DebugUI via the control socket, not the implementation
> of test debugging commands or the python UI code. Should I send it to you
> direct, or to the mailing list?

Just send it to the mailing list.  It affects
Hatari main loop so others may have comments on
it too.


>> What do you think of the idea of doing the disasm
>> etc output on the Hatari side in SDL2 window, and
>> just doing the control in the remote GUI process?
>
> It would allow you some quite responsive update/refresh I think.

Using intermediate files for Hatari internal views
could be awkward.  So, all the related functions
would need to be changed to support writing to a
string buffer in addition to just to a given FILE.

That can be quite a lot of changes.


> However,
> even views like disassembly would need to be interactive (for instance,
> setting breakpoints) so I wonder if it would get quite messy trying to
> arbitrate between external and internal GUI control?

Possibly. I think we need to flesh out a bit more
how it could or should work, both to make the
interaction most sensible to the user and for
code managing them.

How does following sound?


Register & HW info views (updated on every
debugger invocation) don't need any interaction,
assuming they're large enough to show all relevant
content.  So they can be controlled completely from the remote GUI.

In the debugger CLI, commands for controlling
them could be something like:
------------------------------------------
        view registers -> opens new view
        view videl     -> opens new view
        view list      -> lists supported views
registers (shown)
blitter
videl (shown)
....
        view rm registers -> removes view
        view rm all -> removes all views
------------------------------------------


As to interactive SDL2 views...

User will want to scroll disasm & memdump views,
and I think that's best done with direct view
interaction (using arrows keys and page up /
down).

Setting what memory area should be shown whenever
debugger is invoked, should IMHO be done when
view is configured, but I think it should be
possible to re-configure interactive views without
needing to recreate them.

If view is already open, it will just jump to
a different address:
        view memdump <start address> [lines]
        view disasm  <start address> [lines]
        view dspdisasm  <start address> [lines]

(If number of lines changes, I guess view SDL2
texture needs to be recreated.)


List of debugger views and their options could
be one configuration dialog in the remote
debugger.


As to breakpoints, I think all the direct user
interaction can also be done on the Hatari side
and remote debugger is just informed of what was
done on its behalf (breakpoint set/unset at
pc=xxxx).

Remote debugger would then just have a window
with a list of current address breakpoints and
their options:
- checkbox: remove after triggering
- counter:  trigger every Nth hit

And highlighting for last triggered and/or set
breakpoint.

It should be possible to delete breakpoints from
the remote debugger too, and remote GUI needs to
be informed of breakpoint removals done through
other means than the interactive disasm view.


Some users will want multiple (CPU & DSP) memdump
and disasm views, but I'm not sure whether they
should be supported.  If yes, view command needs
indexes for them:
        view disasm <idx> <address> [lines]
        view rm disasm <idx>



        - Eero


diff --git a/src/control.c b/src/control.c
index 783fd78c..6d0e9190 100644
--- a/src/control.c
+++ b/src/control.c
@@ -333,6 +333,10 @@ void Control_ProcessBuffer(const char *orig)
 				ok = Change_ApplyCommandline(arg);
 			} else if (strcmp(cmd, "hatari-debug") == 0) {
 				ok = DebugUI_ParseLine(arg);
+			} else if (strcmp(cmd, "hatari-rdb") == 0) {
+				// Handle remote debugger commands
+				DebugUI_ProcessRemoteDebug(arg);
+				ok = true;	// rdb always carry on processing until nothing exists.
 			} else if (strcmp(cmd, "hatari-shortcut") == 0) {
 				ok = Shortcut_Invoke(arg);
 			} else if (strcmp(cmd, "hatari-event") == 0) {
@@ -468,6 +472,7 @@ bool Control_CheckUpdates(void)
 			fprintf(stderr, "ready control socket with 0 bytes available -> close socket\n");
 			close(ControlSocket);
 			ControlSocket = 0;
+			DebugUI_RegisterRemoteDebug(NULL);
 			return false;
 		}
 		buffer[bytes] = '\0';
@@ -488,6 +493,7 @@ void Control_RemoveFifo(void)
 	if (ControlFifo) {
 		close(ControlFifo);
 		ControlFifo = 0;
+		DebugUI_RegisterRemoteDebug(NULL);
 	}
 	if (FifoPath) {
 		Log_Printf(LOG_DEBUG, "removing command FIFO: %s\n", FifoPath);
@@ -529,6 +535,8 @@ const char *Control_SetFifo(const char *path)
 		return "opening non-blocking read-only FIFO failed";
 	}
 	ControlFifo = fifo;
+
+	DebugUI_RegisterRemoteDebug(Control_CheckUpdates);
 	return NULL;
 }
 
@@ -565,8 +573,10 @@ const char *Control_SetSocket(const char *socketpath)
 				
 	if (ControlSocket) {
 		close(ControlSocket);
+		DebugUI_RegisterRemoteDebug(NULL);
 	}
 	ControlSocket = newsock;
+	DebugUI_RegisterRemoteDebug(Control_CheckUpdates);
 	Log_Printf(LOG_INFO, "new control socket is '%s'\n", socketpath);
 	return NULL;
 }
diff --git a/src/debug/debugui.c b/src/debug/debugui.c
index 2d7e9589..84defce6 100644
--- a/src/debug/debugui.c
+++ b/src/debug/debugui.c
@@ -61,6 +61,14 @@ static const char *parseFileName;
 /* to which directory to change after (potentially recursed) scripts parsing finishes */
 static char *finalDir;
 
+/* Remote debugging break command was sent from debugger */
+static bool bRemoteBreakRequest = false;
+
+/* Processing is stopped and the remote debug loop is active */
+static bool bRemoteBreakIsActive = false;
+
+/* Function to read incoming remote debugger commands (set when a socket is attached) */
+static DebugUI_ProcessRemoteCommands remoteDebugcmdCallback = NULL;
 
 /**
  * Save/Restore snapshot of debugging session variables
@@ -1141,39 +1149,67 @@ void DebugUI(debug_reason_t reason)
 	DebugCpu_InitSession();
 	DebugDsp_InitSession();
 	Symbols_LoadCurrentProgram();
-	DebugInfo_ShowSessionInfo();
-
-	/* override paused message so that user knows to look into console
-	 * on how to continue in case he invoked the debugger by accident.
-	 */
-	Statusbar_AddMessage("Console Debugger", 100);
-	Statusbar_Update(sdlscrn, true);
 
 	/* disable normal GUI alerts while on console */
 	alertLevel = Log_SetAlertLevel(LOG_FATAL);
 
-	cmdret = DEBUGGER_CMDDONE;
-	do
+	if (remoteDebugcmdCallback)
 	{
-		/* Read command from the keyboard and give previous
-		 * command for freeing / adding to history
+		/* Replacement loop for the console debugger,
+		 * for when single-stepping or breakpointing has occurred.
 		 */
-		psCmd = DebugUI_GetCommand(psCmd);
-		if (!psCmd)
-			break;
 
-		/* returns new expression expanded string */
-		if (!(expCmd = DebugUI_EvaluateExpressions(psCmd)))
-			continue;
-
-		/* Parse and execute the command string */
-		cmdret = DebugUI_ParseCommand(expCmd);
-		free(expCmd);
+		/* override paused message so that user knows to look at debugger
+		 * on how to continue in case he invoked the debugger by accident.
+		 */
+		Statusbar_AddMessage("Remote Debugging", 100);
+		Statusbar_Update(sdlscrn, true);
+
+		/* output response to flag successful break request */
+		fprintf(debugOutput, "#break PC:%x\n", M68000_GetPC());
+		fflush(debugOutput);
+		bRemoteBreakRequest = false;
+		bRemoteBreakIsActive = true;
+		while (bRemoteBreakIsActive)
+		{
+			(void) remoteDebugcmdCallback();
+			/* allow wakeup on a thread here? Run any other SDL update requirements? */
+			usleep(100);
+		}
 	}
-	while (cmdret != DEBUGGER_END);
+	else
+	{
+		DebugInfo_ShowSessionInfo();
+
+		/* override paused message so that user knows to look into console
+		* on how to continue in case he invoked the debugger by accident.
+		*/
+		Statusbar_AddMessage("Console Debugger", 100);
+		Statusbar_Update(sdlscrn, true);
 
-	/* free exit command */
-	DebugUI_FreeCommand(psCmd);
+		cmdret = DEBUGGER_CMDDONE;
+		do
+		{
+			/* Read command from the keyboard and give previous
+			* command for freeing / adding to history
+			*/
+			psCmd = DebugUI_GetCommand(psCmd);
+			if (!psCmd)
+				break;
+
+			/* returns new expression expanded string */
+			if (!(expCmd = DebugUI_EvaluateExpressions(psCmd)))
+				continue;
+
+			/* Parse and execute the command string */
+			cmdret = DebugUI_ParseCommand(expCmd);
+			free(expCmd);
+		}
+		while (cmdret != DEBUGGER_END);
+
+		/* free exit command */
+		DebugUI_FreeCommand(psCmd);
+	}
 
 	Log_SetAlertLevel(alertLevel);
 
@@ -1348,3 +1384,54 @@ void DebugUI_Exceptions(int nr, long pc)
 	fprintf(stderr,"%s exception at 0x%lx!\n", ex[nr].name, pc);
 	DebugUI(REASON_CPU_EXCEPTION);
 }
+
+/**
+ * Debugger invocation if requested by remote debugger
+ */
+void DebugUI_CheckRemoteBreak(void)
+{
+	if (bRemoteBreakRequest)
+	{
+		bRemoteBreakRequest = false;
+		// Stop and wait for inputs from the control socket
+		DebugUI(REASON_USER);
+	}
+}
+
+/* Register the callback to process remote command input */
+void DebugUI_RegisterRemoteDebug(DebugUI_ProcessRemoteCommands cmdCallback)
+{
+	remoteDebugcmdCallback = cmdCallback;
+}
+
+/* Process a single Remote Debug command.
+   Command data is null-terminated.
+   Commands write output to debugOutput stream.
+*/
+int DebugUI_ProcessRemoteDebug(const char *input)
+{
+	int ret;
+	/* NOTE: all this functionality is stubbed. It needs
+	 *filling out to actually provide responses to the debugger requests.
+	 */
+	ret = DEBUGGER_CMDDONE;
+
+	char *cmd = strdup(input);
+
+	// Split off starting command and generate argument start
+	char* arg = strchr(cmd, ' ');
+	if (arg) {
+		*arg = '\0';
+		arg = Str_Trim(arg+1);
+	}
+	if (strcmp(cmd, "cmd") == 0) {
+		/* This is a wrapper for typing on the commmand line, with a response for confirmation.
+		   Mainly for debug/testing purposes */
+		ret = DebugUI_ParseCommand(arg);
+		fprintf(debugOutput, "#cmd ok\n");
+	}
+
+	fflush(debugOutput);
+	free(cmd);
+	return ret;
+}
diff --git a/src/debug/debugui.h b/src/debug/debugui.h
index 9a4d6ab7..e9445bf8 100644
--- a/src/debug/debugui.h
+++ b/src/debug/debugui.h
@@ -29,6 +29,9 @@ typedef enum {
 	REASON_USER        // e.g. keyboard shortcut
 } debug_reason_t;
 
+/* Callback type to register if remote debugging is enabled */
+typedef bool (*DebugUI_ProcessRemoteCommands)(void);
+
 extern void DebugUI_Init(void);
 extern void DebugUI(debug_reason_t reason);
 extern void DebugUI_Exceptions(int nr, long pc);
@@ -36,4 +39,14 @@ extern bool DebugUI_ParseLine(const char *input);
 extern bool DebugUI_SetParseFile(const char *input);
 extern void DebugUI_MemorySnapShot_Capture(const char *path, bool bSave);
 
+// Read the flag to see if remote break was requested
+extern void DebugUI_CheckRemoteBreak(void);
+
+// Register the callback to process remote command input
+extern void DebugUI_RegisterRemoteDebug(DebugUI_ProcessRemoteCommands cmdCallback);
+
+// Process a single input debug command including args, null-terminated.
+// Returns DEBUGGER_xx code.
+extern int DebugUI_ProcessRemoteDebug(const char *cmd);
+
 #endif /* HATARI_DEBUGUI_H */
diff --git a/src/video.c b/src/video.c
index a57fea44..deabd9b2 100644
--- a/src/video.c
+++ b/src/video.c
@@ -452,7 +452,7 @@ const char Video_fileid[] = "Hatari video.c : " __DATE__ " " __TIME__;
 #include "ikbd.h"
 #include "floppy_ipf.h"
 #include "statusbar.h"
-
+#include "debugui.h"
 
 /* The border's mask allows to keep track of all the border tricks		*/
 /* applied to one video line. The masks for all lines are stored in the array	*/
@@ -4438,6 +4438,13 @@ void Video_InterruptHandler_VBL ( void )
 	/* Process shortcut keys */
 	ShortCut_ActKey();
 
+	/* Check if remote debug requested a break.
+	 * Ideally it would be good to move this check somewhere else. Living here means
+	 * that single-stepping after break immediately jumps into the VBL routine
+	 * which can be very confusing, but it needs to be somewhere near here in
+	 * the emulation loop. But for the moment it mimics the keyboard shortcut. */
+	DebugUI_CheckRemoteBreak();
+
 	/* Update the IKBD's internal clock */
 	IKBD_UpdateClockOnVBL ();

Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/