GP-834: Fixes for GDB model tests (first round)

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/GdbCause.java

@@ -20,6 +20,7 @@ import agent.gdb.manager.impl.GdbPendingCommand;
 /**
  * Identifies the cause of an event emitted by GDB
  * 
+ * <p>
  * This is not a concept native to GDB. Rather, it is a means to distinguish events that result from
  * commands issued by the {@link GdbManager} from those issued by the user or some other means. For
  * example, a call to {@link GdbManager#addInferior()} will emit a

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/GdbEvent.java

@@ -23,7 +23,7 @@ import agent.gdb.manager.GdbState;
  *
  * @param <T> the type of parsed information detailing the event
  */
-public interface GdbEvent<T> {
+public interface GdbEvent<T> extends GdbCause {
 
 	/**
 	 * Get the information detailing the event

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/GdbManagerImpl.java

@@ -70,7 +70,7 @@ public class GdbManagerImpl implements GdbManager {
 		CLI, MI2;
 	}
 
-	private static final boolean LOG_IO =
+	private static final boolean LOG_IO = true |
 		Boolean.parseBoolean(System.getProperty("agent.gdb.manager.log"));
 	private static final PrintWriter DBG_LOG;
 	static {
@@ -748,7 +748,12 @@ public class GdbManagerImpl implements GdbManager {
 		Integer tid = curCmd.impliesCurrentThreadId();
 		GdbThreadImpl thread = null;
 		if (tid != null) {
-			thread = getThread(tid);
+			thread = threads.get(tid);
+			if (thread == null) {
+				Msg.info(this, "Thread " + tid + " no longer exists");
+				return;
+				// Presumably, some event will have announced the new current thread
+			}
 		}
 		Integer level = curCmd.impliesCurrentFrameId();
 		GdbStackFrameImpl frame = null;
@@ -1345,7 +1350,7 @@ public class GdbManagerImpl implements GdbManager {
 		}
 		if (evtThread != null) {
 			GdbStackFrameImpl frame = evt.getFrame(evtThread);
-			event(() -> listenersEvent.fire.threadSelected(evtThread, frame, evt.getCause()),
+			event(() -> listenersEvent.fire.threadSelected(evtThread, frame, evt),
 				"inferiorState-stopped");
 		}
 	}

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/GdbModuleImpl.java

@@ -97,7 +97,10 @@ public class GdbModuleImpl implements GdbModule {
 			}
 			return AsyncUtils.NIL;
 		}).thenAccept(__ -> {
-			assert loadSections.isDone();
+			if (!loadSections.isDone()) {
+				Msg.warn(this,
+					"Module's sections still not known: " + name + ". Probably got unloaded.");
+			}
 		});
 	}
 

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/GdbStackFrameImpl.java

@@ -52,8 +52,9 @@ public class GdbStackFrameImpl implements GdbStackFrame {
 
 	@Override
 	public String toString() {
-		return "<GdbStackFrame: level=" + level + ",addr=0x" + addr.toString(16) + ",func='" +
-			func + "'>";
+		String strAddr = addr == null ? "<null>" : addr.toString(16);
+		String strFunc = func == null ? "<null>" : func.toString();
+		return "<GdbStackFrame: level=" + level + ",addr=0x" + strAddr + ",func='" + strFunc + "'>";
 	}
 
 	@Override

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/manager/impl/cmd/GdbConsoleExecCommand.java

@@ -74,4 +74,8 @@ public class GdbConsoleExecCommand extends AbstractGdbCommandWithThreadAndFrameI
 		}
 		return builder.toString();
 	}
+
+	public Output getOutputTo() {
+		return to;
+	}
 }

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/model/impl/GdbModelTargetInferior.java

@@ -31,11 +31,8 @@ import ghidra.dbg.target.schema.*;
 import ghidra.dbg.util.PathUtils;
 import ghidra.lifecycle.Internal;
 
-@TargetObjectSchemaInfo(
-	name = "Inferior",
-	elements = {
-		@TargetElementType(type = Void.class) },
-	attributes = {
+@TargetObjectSchemaInfo(name = "Inferior", elements = {
+	@TargetElementType(type = Void.class) }, attributes = {
 		@TargetAttributeType(type = Void.class) })
 public class GdbModelTargetInferior
 		extends DefaultTargetObject<TargetObject, GdbModelTargetInferiorContainer>
@@ -138,10 +135,7 @@ public class GdbModelTargetInferior
 		return threads;
 	}
 
-	@TargetAttributeType(
-		name = GdbModelTargetBreakpointLocationContainer.NAME,
-		required = true,
-		fixed = true)
+	@TargetAttributeType(name = GdbModelTargetBreakpointLocationContainer.NAME, required = true, fixed = true)
 	public GdbModelTargetBreakpointLocationContainer getBreakpoints() {
 		return breakpoints;
 	}
@@ -220,9 +214,8 @@ public class GdbModelTargetInferior
 	}
 
 	protected CompletableFuture<Void> inferiorStarted(Long pid) {
-		parent.getListeners().fire.event(
-			parent, null, TargetEventType.PROCESS_CREATED, "Inferior " + inferior.getId() +
-				" started " + inferior.getExecutable() + " pid=" + pid,
+		parent.getListeners().fire.event(parent, null, TargetEventType.PROCESS_CREATED,
+			"Inferior " + inferior.getId() + " started " + inferior.getExecutable() + " pid=" + pid,
 			List.of(this));
 		AsyncFence fence = new AsyncFence();
 		fence.include(modules.refreshInternal());
@@ -264,7 +257,7 @@ public class GdbModelTargetInferior
 		}
 		else {
 			changeAttributes(List.of(), Map.of( //
-				STATE_ATTRIBUTE_NAME, TargetExecutionState.TERMINATED, //
+				STATE_ATTRIBUTE_NAME, state = TargetExecutionState.TERMINATED, //
 				DISPLAY_ATTRIBUTE_NAME, updateDisplay() //
 			), "Exited");
 		}
@@ -292,15 +285,13 @@ public class GdbModelTargetInferior
 			}
 			gatherThreads(params, sco.getAffectedThreads());
 			impl.session.getListeners().fire.event(impl.session, targetEventThread,
-				TargetEventType.BREAKPOINT_HIT,
-				bpHit.desc(), params);
+				TargetEventType.BREAKPOINT_HIT, bpHit.desc(), params);
 		}
 		else if (reason instanceof GdbEndSteppingRangeReason) {
 			List<Object> params = new ArrayList<>();
 			gatherThreads(params, sco.getAffectedThreads());
 			impl.session.getListeners().fire.event(impl.session, targetEventThread,
-				TargetEventType.STEP_COMPLETED,
-				reason.desc(), params);
+				TargetEventType.STEP_COMPLETED, reason.desc(), params);
 		}
 		else if (reason instanceof GdbSignalReceivedReason) {
 			GdbSignalReceivedReason signal = (GdbSignalReceivedReason) reason;
@@ -308,15 +299,13 @@ public class GdbModelTargetInferior
 			params.add(signal.getSignalName());
 			gatherThreads(params, sco.getAffectedThreads());
 			impl.session.getListeners().fire.event(impl.session, targetEventThread,
-				TargetEventType.SIGNAL,
-				reason.desc(), params);
+				TargetEventType.SIGNAL, reason.desc(), params);
 		}
 		else {
 			List<Object> params = new ArrayList<>();
 			gatherThreads(params, sco.getAffectedThreads());
 			impl.session.getListeners().fire.event(impl.session, targetEventThread,
-				TargetEventType.STOPPED,
-				reason.desc(), params);
+				TargetEventType.STOPPED, reason.desc(), params);
 		}
 	}
 

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/model/impl/GdbModelTargetModuleContainer.java

@@ -94,8 +94,16 @@ public class GdbModelTargetModuleContainer
 		for (GdbModelTargetModule mod : modules) {
 			fence.include(mod.init());
 		}
+
+		/**
+		 * NB. Modules may have changed by the time the fence completes. We'll just remove invalid
+		 * modules, since any additions should cause a follow-on update.
+		 */
 		return fence.ready().thenAccept(__ -> {
-			changeElements(List.of(), modules, "Refreshed");
+			List<GdbModelTargetModule> validOnly = modules.stream()
+					.filter(m -> m.isValid())
+					.collect(Collectors.toList());
+			changeElements(List.of(), validOnly, "Refreshed");
 		});
 	}
 

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/model/impl/GdbModelTargetSession.java

@@ -18,9 +18,11 @@ package agent.gdb.model.impl;
 import java.io.IOException;
 import java.util.*;
 import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.atomic.AtomicInteger;
 
 import agent.gdb.manager.*;
+import agent.gdb.manager.impl.*;
+import agent.gdb.manager.impl.cmd.GdbConsoleExecCommand;
+import agent.gdb.manager.impl.cmd.GdbConsoleExecCommand.Output;
 import agent.gdb.manager.impl.cmd.GdbStateChangeRecord;
 import agent.gdb.manager.reason.GdbReason;
 import ghidra.async.AsyncUtils;
@@ -52,7 +54,6 @@ public class GdbModelTargetSession extends DefaultTargetModelRoot
 	protected final GdbModelTargetBreakpointContainer breakpoints;
 
 	private boolean accessible = true;
-	protected AtomicInteger focusFreezeCount = new AtomicInteger();
 	protected GdbModelSelectableObject focus;
 
 	protected String debugger = "gdb"; // Used by GdbModelTargetEnvironment
@@ -148,8 +149,32 @@ public class GdbModelTargetSession extends DefaultTargetModelRoot
 		// Otherwise, we'll presumably get the =thread-selected event 
 	}
 
+	protected boolean isFocusInternallyDriven(GdbCause cause) {
+		if (cause == null || cause == GdbCause.Causes.UNCLAIMED) {
+			return false;
+		}
+		if (cause instanceof GdbEvent<?>) {
+			return false;
+		}
+		if (cause instanceof GdbPendingCommand<?>) {
+			GdbPendingCommand<?> pcmd = (GdbPendingCommand<?>) cause;
+			GdbCommand<?> cmd = pcmd.getCommand();
+			if (cmd instanceof GdbConsoleExecCommand) {
+				GdbConsoleExecCommand exec = (GdbConsoleExecCommand) cmd;
+				if (exec.getOutputTo() == Output.CAPTURE) {
+					return true;
+				}
+				return false;
+			}
+		}
+		return true;
+	}
+
 	@Override
 	public void threadSelected(GdbThread thread, GdbStackFrame frame, GdbCause cause) {
+		if (isFocusInternallyDriven(cause)) {
+			return;
+		}
 		GdbModelTargetInferior inf = inferiors.getTargetInferior(thread.getInferior());
 		GdbModelTargetThread t = inf.threads.getTargetThread(thread);
 		if (frame == null) {
@@ -228,7 +253,13 @@ public class GdbModelTargetSession extends DefaultTargetModelRoot
 		while (cur != null) {
 			if (cur instanceof GdbModelSelectableObject) {
 				GdbModelSelectableObject sel = (GdbModelSelectableObject) cur;
-				return sel.select();
+				/**
+				 * Have to call setFocus here, since the call to select() is considered
+				 * "internally-driven"
+				 */
+				return sel.select().thenRun(() -> {
+					setFocus(sel);
+				});
 			}
 			cur = cur.getParent();
 		}
@@ -239,20 +270,10 @@ public class GdbModelTargetSession extends DefaultTargetModelRoot
 		inferiors.invalidateMemoryAndRegisterCaches();
 	}
 
-	protected void freezeFocusUpdates() {
-		focusFreezeCount.incrementAndGet();
-	}
-
-	protected void thawFocusUpdates() {
-		focusFreezeCount.decrementAndGet();
-	}
-
 	protected void setFocus(GdbModelSelectableObject focus) {
-		if (focusFreezeCount.get() == 0) {
-			changeAttributes(List.of(), Map.of( //
-				FOCUS_ATTRIBUTE_NAME, this.focus = focus //
-			), "Focus changed");
-		}
+		changeAttributes(List.of(), Map.of( //
+			FOCUS_ATTRIBUTE_NAME, this.focus = focus //
+		), "Focus changed");
 	}
 
 	@Override
@@ -263,16 +284,29 @@ public class GdbModelTargetSession extends DefaultTargetModelRoot
 	@Override
 	public void inferiorStateChanged(GdbInferior inf, Collection<GdbThread> threads, GdbState state,
 			GdbThread thread, GdbCause cause, GdbReason reason) {
+		/**
+		 * TODO: It might be nice if the manager gave a manager-level callback for *stopped and
+		 * *running events. Without that, I can't really specify an action to execute, *after* all
+		 * inferiors have completed the stateChanged routines.
+		 */
 		GdbStateChangeRecord sco =
 			new GdbStateChangeRecord(inf, threads, state, thread, cause, reason);
-		GdbInferior toFocus = thread == null ? impl.gdb.currentInferior() : thread.getInferior();
-		freezeFocusUpdates();
+
 		CompletableFuture<Void> infUpdates = CompletableFuture.allOf(
 			breakpoints.stateChanged(sco),
 			inferiors.stateChanged(sco));
 		infUpdates.whenComplete((v, t) -> {
-			thawFocusUpdates();
-			toFocus.select();
+			if (thread == null) {
+				return;
+			}
+			/**
+			 * I have to do this for all inferiors, because I don't know in what order they will
+			 * complete.
+			 */
+			thread.select().exceptionally(ex -> {
+				impl.reportError(this, "Could not restore event thread", ex);
+				return null;
+			});
 		});
 	}
 }

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/model/impl/GdbModelTargetStack.java

@@ -74,6 +74,10 @@ public class GdbModelTargetStack extends
 		});
 	}
 
+	protected synchronized GdbModelTargetStackFrame getTargetFrameByLevel(int i) {
+		return framesByLevel.get(i);
+	}
+
 	protected void invalidateRegisterCaches() {
 		for (GdbModelTargetStackFrame frame : framesByLevel.values()) {
 			frame.invalidateRegisterCaches();

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/model/impl/GdbModelTargetStackFrameRegisterContainer.java

@@ -181,7 +181,13 @@ public class GdbModelTargetStackFrameRegisterContainer
 
 	public CompletableFuture<Void> stateChanged(GdbStateChangeRecord sco) {
 		return requestElements(false).exceptionally(ex -> {
-			impl.reportError(this, "Trouble updating registers on state change", ex);
+			if (!valid) {
+				Msg.info(this,
+					"Ignoring error when refreshing now-invalid thread registers: " + ex);
+			}
+			else {
+				impl.reportError(this, "Trouble updating registers on state change", ex);
+			}
 			return null;
 		});
 	}

Ghidra/Debug/Debugger-agent-gdb/src/main/java/agent/gdb/model/impl/GdbModelTargetThreadContainer.java

@@ -20,8 +20,7 @@ import java.util.Map;
 import java.util.concurrent.CompletableFuture;
 import java.util.stream.Collectors;
 
-import agent.gdb.manager.GdbInferior;
-import agent.gdb.manager.GdbThread;
+import agent.gdb.manager.*;
 import agent.gdb.manager.impl.cmd.GdbStateChangeRecord;
 import agent.gdb.manager.reason.GdbBreakpointHitReason;
 import ghidra.async.AsyncFence;
@@ -104,6 +103,15 @@ public class GdbModelTargetThreadContainer
 	}
 
 	public CompletableFuture<Void> stateChanged(GdbStateChangeRecord sco) {
+		/**
+		 * No sense refreshing anything unless we're stopped. Worse yet, because of fun timing
+		 * issues, we often see RUNNING just a little late, since the callbacks are all issued on a
+		 * separate thread. If that RUNNING is received after the manager has processed a
+		 * =thread-exited, we will wind up invalidating that thread early.
+		 */
+		if (sco.getState() != GdbState.STOPPED) {
+			return AsyncUtils.NIL;
+		}
 		return requestElements(false).thenCompose(__ -> {
 			AsyncFence fence = new AsyncFence();
 			for (GdbModelTargetThread modelThread : threadsById.values()) {

Ghidra/Debug/Debugger-agent-gdb/src/test/java/agent/gdb/model/AbstractModelForGdbThreadFocusTest.java

@@ -49,6 +49,6 @@ public abstract class AbstractModelForGdbThreadFocusTest extends AbstractDebugge
 
 	@Override
 	protected List<String> getExpectedDefaultFocus() {
-		return PathUtils.parse("Inferiors[2].Threads[2]");
+		return PathUtils.parse("Inferiors[2].Threads[2].Stack[0]");
 	}
 }

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/DefaultTraceRecorder.java

@@ -384,6 +384,10 @@ public class DefaultTraceRecorder implements TraceRecorder {
 		return curFocus;
 	}
 
+	public void setCurrentFocus(TargetObject focused) {
+		curFocus = focused;
+	}
+
 	@Override
 	public CompletableFuture<Boolean> requestFocus(TargetObject focus) {
 		if (!isSupportsFocus()) {

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceEventListener.java

@@ -258,7 +258,7 @@ public class TraceEventListener extends AnnotatedDebuggerAttributeListener {
 			return;
 		}
 		if (PathUtils.isAncestor(target.getPath(), focused.getPath())) {
-			curFocus = focused;
+			recorder.setCurrentFocus(focused);
 		}
 	}
 

Ghidra/Debug/Framework-Debugging/src/test/java/ghidra/dbg/test/AbstractDebuggerModelFocusTest.java

@@ -75,7 +75,8 @@ public abstract class AbstractDebuggerModelFocusTest extends AbstractDebuggerMod
 		TargetFocusScope scope = findFocusScope();
 		Set<TargetObject> focusable = getFocusableThings();
 		// The default must be one of the focusable objects
-		assertTrue(focusable.stream().anyMatch(f -> f.getPath().equals(expectedDefaultFocus)));
+		assertTrue(focusable.stream()
+				.anyMatch(f -> PathUtils.isAncestor(f.getPath(), expectedDefaultFocus)));
 		retryVoid(() -> {
 			assertEquals(expectedDefaultFocus, scope.getFocus().getPath());
 		}, List.of(AssertionError.class));

Ghidra/Debug/Framework-Debugging/src/test/java/ghidra/dbg/test/AbstractDebuggerModelTest.java

@@ -291,7 +291,8 @@ public abstract class AbstractDebuggerModelTest extends AbstractGhidraHeadlessIn
 		TargetDetachable detachable = m.suitable(TargetDetachable.class, process.getPath());
 		waitAcc(detachable);
 		waitOn(detachable.detach());
-		assertFalse(DebugModelConventions.isProcessAlive(process));
+		retryVoid(() -> assertFalse(DebugModelConventions.isProcessAlive(process)),
+			List.of(AssertionError.class));
 	}
 
 	protected void runTestKill(TargetObject container, DebuggerTestSpecimen specimen)
@@ -300,7 +301,8 @@ public abstract class AbstractDebuggerModelTest extends AbstractGhidraHeadlessIn
 		TargetKillable killable = m.suitable(TargetKillable.class, process.getPath());
 		waitAcc(killable);
 		waitOn(killable.kill());
-		assertFalse(DebugModelConventions.isProcessAlive(process));
+		retryVoid(() -> assertFalse(DebugModelConventions.isProcessAlive(process)),
+			List.of(AssertionError.class));
 	}
 
 	protected void runTestResumeTerminates(TargetObject container, DebuggerTestSpecimen specimen)

Ghidra/Debug/Framework-Debugging/src/test/java/ghidra/dbg/test/AbstractModelHost.java

@@ -35,7 +35,8 @@ public abstract class AbstractModelHost implements ModelHost, DebuggerModelTestU
 	public DebuggerConsole console;
 
 	protected boolean validateCallbacks = true;
-	protected boolean validateEvents = true;
+	// NB. GDB's modules actually aren't "unloaded" until the inferior's file is replaced
+	protected boolean validateEvents = false;
 	protected boolean provideConsole = true;
 
 	@Override
@@ -43,8 +44,7 @@ public abstract class AbstractModelHost implements ModelHost, DebuggerModelTestU
 		DebuggerModelFactory factory = getModelFactory();
 		for (Map.Entry<String, Object> opt : options.entrySet()) {
 			@SuppressWarnings("unchecked")
-			Property<Object> property =
-				(Property<Object>) factory.getOptions().get(opt.getKey());
+			Property<Object> property = (Property<Object>) factory.getOptions().get(opt.getKey());
 			property.setValue(opt.getValue());
 		}
 		DebuggerObjectModel model = waitOn(factory.build());

Ghidra/Debug/Framework-Debugging/src/test/java/ghidra/dbg/testutil/CallbackValidator.java

@@ -275,17 +275,17 @@ public class CallbackValidator implements DebuggerModelListener, AutoCloseable {
 				type + ",description=" + description + ",parameters=" + parameters + ")");
 		}
 		off.catching(() -> {
-			validateCallbackThread("event");
-			validateObject("event", object);
+			validateCallbackThread("event(" + type + ")");
+			validateObject("event(" + type + ")", object);
 			if (type == TargetEventType.THREAD_CREATED || type == TargetEventType.THREAD_EXITED) {
-				validateObject("event", eventThread);
+				validateObject("event(" + type + ")", eventThread);
 			}
 			else {
-				validateObjectOptional("event.eventThread", eventThread);
+				validateObjectOptional("event(" + type + ").eventThread", eventThread);
 			}
 			assertNotNull(type);
 			assertNotNull(description);
-			validateObjectsInCollection("event.parameters", parameters);
+			validateObjectsInCollection("event(" + type + ").parameters", parameters);
 		});
 	}