Merge remote-tracking branch 'origin/patch'

Conflicts:
	Ghidra/Debug/Framework-Debugging/build.gradle

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

@@ -608,6 +608,8 @@ public class GdbManagerImpl implements GdbManager {
 
 					cliThread = iniThread;
 					cliThread.setName("GDB Read CLI");
+					// Looks terrible, but we're already in this world
+					cliThread.writer.print("set confirm off" + newLine);
 					cliThread.writer
 							.print("new-ui mi2 " + mi2Pty.getChild().nullSession() + newLine);
 					cliThread.writer.flush();
@@ -1538,19 +1540,34 @@ public class GdbManagerImpl implements GdbManager {
 		}
 	}
 
+	public void sendInterruptNow(PtyThread thread, byte[] bytes) throws IOException {
+		Msg.info(this, "Interrupting by Ctrl-C on " + thread + "'s pty");
+		OutputStream os = thread.pty.getParent().getOutputStream();
+		os.write(bytes);
+		os.flush();
+	}
+
 	@Override
 	public void sendInterruptNow() throws IOException {
 		checkStarted();
-		Msg.info(this, "Interrupting");
+		/*Msg.info(this, "Interrupting while runningInterpreter = " + runningInterpreter);
+		if (runningInterpreter == Interpreter.MI2) {
+			if (cliThread != null) {
+				Msg.info(this, "Interrupting by 'interrupt' on CLI");
+				OutputStream os = cliThread.pty.getParent().getOutputStream();
+				os.write(("interrupt" + newLine).getBytes());
+				os.flush();
+			}
+			else {
+				sendInterruptNow(mi2Thread);
+			}
+		}
+		else*/
 		if (cliThread != null) {
-			OutputStream os = cliThread.pty.getParent().getOutputStream();
-			os.write(3);
-			os.flush();
+			sendInterruptNow(cliThread, (((char) 3) + "interrupt" + newLine).getBytes());
 		}
 		else if (mi2Thread != null) {
-			OutputStream os = mi2Thread.pty.getParent().getOutputStream();
-			os.write(3);
-			os.flush();
+			sendInterruptNow(mi2Thread, new byte[] { 3 });
 		}
 	}
 

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

@@ -18,38 +18,39 @@ package agent.gdb.manager.impl.cmd;
 import agent.gdb.manager.GdbThread;
 import agent.gdb.manager.evt.*;
 import agent.gdb.manager.impl.*;
+import agent.gdb.manager.impl.GdbManagerImpl.Interpreter;
 
-public abstract class AbstractLaunchGdbCommand extends AbstractGdbCommand<GdbThread> {
+public abstract class AbstractLaunchGdbCommand extends AbstractGdbCommand<GdbThread>
+		implements MixinResumeInCliGdbCommand<GdbThread> {
 
 	protected AbstractLaunchGdbCommand(GdbManagerImpl manager) {
 		super(manager);
 	}
 
+	@Override
+	public Interpreter getInterpreter() {
+		//return getInterpreter(manager);
+
+		/**
+		 * A lot of good event-handling logic is factored in the Mixin interface. However, errors
+		 * from CLI commands are catastrophically mishandled or just missed entirely, so we will
+		 * still use MI2 for these.
+		 */
+		return Interpreter.MI2;
+	}
+
 	@Override
 	public boolean handle(GdbEvent<?> evt, GdbPendingCommand<?> pending) {
 		evt = checkErrorViaCli(evt);
-		if (evt instanceof GdbCommandRunningEvent) {
-			pending.claim(evt);
-			return pending.hasAny(GdbRunningEvent.class);
-		}
-		else if (evt instanceof AbstractGdbCompletedCommandEvent) {
-			pending.claim(evt);
-			return true; // Not the expected Completed event
-		}
-		else if (evt instanceof GdbRunningEvent) {
-			// Event happens no matter which interpreter received the command
-			pending.claim(evt);
-			return pending.hasAny(GdbCommandRunningEvent.class);
-		}
-		else if (evt instanceof GdbThreadCreatedEvent) {
+		if (evt instanceof GdbThreadCreatedEvent) {
 			pending.claim(evt);
 		}
-		return false;
+		return handleExpectingRunning(evt, pending);
 	}
 
 	@Override
 	public GdbThread complete(GdbPendingCommand<?> pending) {
-		pending.checkCompletion(GdbCommandRunningEvent.class);
+		completeOnRunning(pending);
 
 		// Just take the first thread. Others are considered clones.
 		GdbThreadCreatedEvent created = pending.findFirstOf(GdbThreadCreatedEvent.class);

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

@@ -23,7 +23,7 @@ import agent.gdb.manager.impl.GdbManagerImpl.Interpreter;
  * Implementation of {@link GdbInferior#cont()}
  */
 public class GdbContinueCommand extends AbstractGdbCommandWithThreadId<Void>
-		implements MixinResumeInCliGdbCommand {
+		implements MixinResumeInCliGdbCommand<Void> {
 	public GdbContinueCommand(GdbManagerImpl manager, Integer threadId) {
 		super(manager, threadId);
 	}
@@ -49,6 +49,7 @@ public class GdbContinueCommand extends AbstractGdbCommandWithThreadId<Void>
 
 	@Override
 	public Void complete(GdbPendingCommand<?> pending) {
-		return completeOnRunning(pending);
+		completeOnRunning(pending);
+		return null;
 	}
 }

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

@@ -41,7 +41,7 @@ public class GdbInterruptCommand extends AbstractGdbCommand<Void> {
 	public String encode() {
 		Interpreter i = getInterpreter();
 		if (i == manager.getRunningInterpreter()) {
-			Msg.debug(this, "Using ^C to interrupt");
+			Msg.debug(this, "Using ^C to interrupt via " + i);
 			return "\u0003";
 		}
 		switch (i) {

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

@@ -28,13 +28,16 @@ public class GdbRunCommand extends AbstractLaunchGdbCommand {
 		super(manager);
 	}
 
-	@Override
+	/*@Override
 	public Interpreter getInterpreter() {
 		return Interpreter.MI2;
-	}
+	}*/
 
 	@Override
 	public String encode() {
+		if (getInterpreter() == Interpreter.CLI) {
+			return "run";
+		}
 		return "-exec-run";
 	}
 }

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

@@ -28,13 +28,16 @@ public class GdbStartCommand extends AbstractLaunchGdbCommand {
 		super(manager);
 	}
 
-	@Override
+	/*@Override
 	public Interpreter getInterpreter() {
 		return Interpreter.MI2;
-	}
+	}*/
 
 	@Override
 	public String encode() {
+		if (getInterpreter() == Interpreter.CLI) {
+			return "start";
+		}
 		return "-exec-run --start";
 	}
 }

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

@@ -28,13 +28,16 @@ public class GdbStartInstructionCommand extends AbstractLaunchGdbCommand {
 		super(manager);
 	}
 
-	@Override
+	/*@Override
 	public Interpreter getInterpreter() {
 		return Interpreter.MI2;
-	}
+	}*/
 
 	@Override
 	public String encode() {
+		if (getInterpreter() == Interpreter.CLI) {
+			return "starti";
+		}
 		return "-interpreter-exec console starti";
 	}
 }

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

@@ -24,7 +24,7 @@ import agent.gdb.manager.impl.GdbManagerImpl.Interpreter;
  * Implementation of {@link GdbThread#stepInstruction()}
  */
 public class GdbStepCommand extends AbstractGdbCommandWithThreadId<Void>
-		implements MixinResumeInCliGdbCommand {
+		implements MixinResumeInCliGdbCommand<Void> {
 	protected final StepCmd cmd;
 
 	public GdbStepCommand(GdbManagerImpl manager, Integer threadId, StepCmd cmd) {
@@ -53,6 +53,7 @@ public class GdbStepCommand extends AbstractGdbCommandWithThreadId<Void>
 
 	@Override
 	public Void complete(GdbPendingCommand<?> pending) {
-		return completeOnRunning(pending);
+		completeOnRunning(pending);
+		return null;
 	}
 }

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

@@ -29,7 +29,7 @@ import agent.gdb.manager.impl.GdbManagerImpl.Interpreter;
  * "interrupt" from the primary (console) interpreter. So, for these resumes, I need to issue the
  * command from the console, allowing ^C to work.
  */
-public interface MixinResumeInCliGdbCommand extends GdbCommand<Void> {
+public interface MixinResumeInCliGdbCommand<T> extends GdbCommand<T> {
 
 	default Interpreter getInterpreter(GdbManagerImpl manager) {
 		if (manager.hasCli()) {
@@ -55,8 +55,7 @@ public interface MixinResumeInCliGdbCommand extends GdbCommand<Void> {
 		return false;
 	}
 
-	default Void completeOnRunning(GdbPendingCommand<?> pending) {
+	default void completeOnRunning(GdbPendingCommand<?> pending) {
 		pending.checkCompletion(GdbCommandRunningEvent.class);
-		return null;
 	}
 }

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

@@ -17,6 +17,7 @@ package agent.gdb.model.impl;
 
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
+import java.util.function.Supplier;
 
 import agent.gdb.manager.GdbInferior;
 import agent.gdb.manager.GdbThread;
@@ -24,14 +25,29 @@ import ghidra.dbg.util.ShellUtils;
 
 public enum GdbModelImplUtils {
 	;
-	public static CompletableFuture<GdbThread> launch(GdbModelImpl impl, GdbInferior inferior,
-			List<String> args, boolean useStarti) {
+
+	/**
+	 * Perform the file-args-start sequence to launch a target in an inferior
+	 * 
+	 * @param inferior the inferior to assign the target
+	 * @param args the command-line arguments, including the executable at args[0]
+	 * @param useStarti true to use {@code starti}, false to use {@code start}
+	 * @param postFile a caller-specified routine to execute after the {@code file} part of the
+	 *            sequence. This is useful to grab auto-derived environment information
+	 * @return a future which completes with the initial thread of the target
+	 */
+	public static CompletableFuture<GdbThread> launch(GdbInferior inferior,
+			List<String> args, boolean useStarti, Supplier<CompletableFuture<Void>> postFile) {
 		// Queue all these up to avoid other commands getting between.
 		CompletableFuture<Void> feas = inferior.fileExecAndSymbols(args.get(0));
+		CompletableFuture<Void> post = postFile.get();
 		CompletableFuture<Void> sargs =
 			inferior.setVar("args", ShellUtils.generateLine(args.subList(1, args.size())));
 		CompletableFuture<GdbThread> start = useStarti ? inferior.starti() : inferior.start();
-		return feas.thenCombine(sargs, (v1, v2) -> v2).thenCombine(start, (v, t) -> t);
+		return feas
+				.thenCombine(post, (v, t) -> t)
+				.thenCombine(sargs, (v, t) -> t)
+				.thenCombine(start, (v, t) -> t);
 	}
 
 	public static <V> V noDupMerge(V first, V second) {

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

@@ -175,7 +175,9 @@ public class GdbModelTargetInferior
 			CmdLineParser.tokenize(TargetCmdLineLauncher.PARAMETER_CMDLINE_ARGS.get(args));
 		Boolean useStarti = PARAMETER_STARTI.get(args);
 		return impl.gateFuture(
-			GdbModelImplUtils.launch(impl, inferior, cmdLineArgs, useStarti).thenApply(__ -> null));
+			GdbModelImplUtils.launch(inferior, cmdLineArgs, useStarti, () -> {
+				return environment.refreshInternal();
+			}).thenApply(__ -> null));
 	}
 
 	@Override
@@ -249,8 +251,16 @@ public class GdbModelTargetInferior
 		parent.getListeners().fire.event(parent, null, TargetEventType.PROCESS_CREATED,
 			"Inferior " + inferior.getId() + " started " + inferior.getExecutable() + " pid=" + pid,
 			List.of(this));
+		/*System.err.println("inferiorStarted: realState = " + realState);
+		changeAttributes(List.of(), Map.ofEntries(
+			// This is hacky, but =inferior-started comes before ^running.
+			// Is it ever not followed by ^running, except on failure?
+			Map.entry(STATE_ATTRIBUTE_NAME, state = TargetExecutionState.RUNNING),
+			Map.entry(PID_ATTRIBUTE_NAME, pid),
+			Map.entry(DISPLAY_ATTRIBUTE_NAME, updateDisplay())),
+			"Refresh on started");*/
 		AsyncFence fence = new AsyncFence();
-		fence.include(modules.refreshInternal());
+		//fence.include(modules.refreshInternal());
 		//fence.include(registers.refreshInternal());
 		fence.include(environment.refreshInternal());
 		fence.include(impl.gdb.listInferiors()); // HACK to update inferior.getExecutable()
@@ -266,14 +276,14 @@ public class GdbModelTargetInferior
 				changeAttributes(List.of(), Map.ofEntries(
 					Map.entry(STATE_ATTRIBUTE_NAME, state = realState),
 					Map.entry(DISPLAY_ATTRIBUTE_NAME, updateDisplay())),
-					"Refresh on started");
+					"Refresh on initial break");
 			}
 			else {
 				changeAttributes(List.of(), Map.ofEntries(
 					Map.entry(STATE_ATTRIBUTE_NAME, state = realState),
 					Map.entry(PID_ATTRIBUTE_NAME, p),
 					Map.entry(DISPLAY_ATTRIBUTE_NAME, updateDisplay())),
-					"Refresh on started");
+					"Refresh on initial break");
 			}
 		});
 	}
@@ -442,7 +452,8 @@ public class GdbModelTargetInferior
 			List<Object> params = new ArrayList<>();
 			gatherThreads(params, sco.getAffectedThreads());
 			if (targetEventThread == null && !params.isEmpty()) {
-				targetEventThread = threads.getTargetThread(sco.getAffectedThreads().iterator().next());
+				targetEventThread =
+					threads.getTargetThread(sco.getAffectedThreads().iterator().next());
 			}
 			if (targetEventThread != null) {
 				impl.session.getListeners().fire.event(impl.session, targetEventThread,

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

@@ -107,7 +107,7 @@ public class GdbModelTargetProcessMemory
 			return AsyncUtils.NIL;
 		}
 		return inferior.listMappings().exceptionally(ex -> {
-			Msg.error(this, "Could not list regions", ex);
+			Msg.error(this, "Could not list regions. Using default.");
 			return Map.of(); // empty map will be replaced with default
 		}).thenAccept(this::updateUsingMappings);
 	}

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

@@ -209,7 +209,9 @@ public class GdbModelTargetSession extends DefaultTargetModelRoot
 			CmdLineParser.tokenize(TargetCmdLineLauncher.PARAMETER_CMDLINE_ARGS.get(args));
 		Boolean useStarti = GdbModelTargetInferior.PARAMETER_STARTI.get(args);
 		return impl.gateFuture(impl.gdb.availableInferior().thenCompose(inf -> {
-			return GdbModelImplUtils.launch(impl, inf, cmdLineArgs, useStarti);
+			return GdbModelImplUtils.launch(inf, cmdLineArgs, useStarti, () -> {
+				return inferiors.getTargetInferior(inf).environment.refreshInternal();
+			});
 		}).thenApply(__ -> null));
 	}
 
@@ -231,7 +233,7 @@ public class GdbModelTargetSession extends DefaultTargetModelRoot
 		//return impl.gdb.interrupt();
 		try {
 			impl.gdb.sendInterruptNow();
-			impl.gdb.cancelCurrentCommand();
+			//impl.gdb.cancelCurrentCommand();
 		}
 		catch (IOException e) {
 			Msg.error(this, "Could not interrupt", e);

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

@@ -18,8 +18,11 @@ package agent.gdb.model;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
+import org.junit.Ignore;
+import org.junit.Test;
+
 import agent.gdb.model.impl.GdbModelTargetInferior;
-import ghidra.dbg.target.TargetEnvironment;
+import ghidra.dbg.target.*;
 import ghidra.dbg.target.TargetMethod.TargetParameterMap;
 import ghidra.dbg.test.AbstractDebuggerModelLauncherTest;
 
@@ -43,4 +46,50 @@ public abstract class AbstractModelForGdbLauncherTest extends AbstractDebuggerMo
 		assertEquals("little", environment.getEndian());
 		assertTrue(environment.getDebugger().toLowerCase().contains("gdb"));
 	}
+
+	protected DebuggerTestSpecimen getLaunchStrippedSpecimen() {
+		return GdbLinuxSpecimen.SPIN_STRIPPED;
+	}
+
+	/**
+	 * Test a target which runs indefinitely, and for which GDB cannot get the temporary breakpoint
+	 * on main.
+	 */
+	@Test
+	@Ignore
+	public void testLaunchStrippedThenInterrupt() throws Throwable {
+		m.build();
+
+		ProcessCreatedDebugModelListener listener = new ProcessCreatedDebugModelListener();
+		// NB. I've intentionally omitted the reorderer here. The model should get it right.
+		m.getModel().addModelListener(listener);
+
+		DebuggerTestSpecimen specimen = getLaunchStrippedSpecimen();
+		TargetLauncher launcher = findLauncher();
+		waitAcc(launcher);
+		waitOn(launcher.launch(specimen.getLauncherArgs()));
+		//System.err.println("Launched");
+
+		/**
+		 * For the moment, we're stuck having to wait for the initial break in GDB before we
+		 * announce that the target is RUNNING, because we depend on the environment being correct
+		 * *before* that announcement. For launch, we can resolve the issue, because we can refresh
+		 * the environment between "file" and "start". However, for attach, we're still hosed. I
+		 * don't care to try to distinguish the two cases, because that's a lot of work, and still
+		 * only a partial fix.
+		 * 
+		 * Thus, we will not observe state=RUNNING until after we successfully interrupt the target.
+		 * This test still suffices to address the interrupt problem, but I don't know any way to
+		 * fix the state reporting problem until we fix the record-depends-on-language-mapping
+		 * issue, which is still some time away.
+		 */
+		TargetInterruptible interruptible =
+			m.suitable(TargetInterruptible.class, launcher.getPath());
+		Thread.sleep(1000); // HACK
+		waitOn(interruptible.interrupt());
+		//System.err.println("Interrupted");
+
+		waitOn(listener.observedCreated);
+		//System.err.println("Observed");
+	}
 }

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

@@ -59,6 +59,12 @@ public enum GdbLinuxSpecimen implements DebuggerTestSpecimen, DebuggerModelTestU
 			return DummyProc.which("expRegisters");
 		}
 	},
+	SPIN_STRIPPED {
+		@Override
+		String getCommandLine() {
+			return DummyProc.which("expSpin.stripped");
+		}
+	},
 	STACK {
 		@Override
 		String getCommandLine() {

Ghidra/Debug/Framework-Debugging/build.gradle

@@ -54,6 +54,13 @@ task testSpecimenLinux_x86_64 {
 	//dependsOn 'expTypesExecutable'//Linux_x86_64Executable'
 	dependsOn 'expRegistersLinux_x86_64Executable'
 	dependsOn 'expStackLinux_x86_64Executable'
+	
+	doLast {
+		exec {
+			workingDir "build/os/linux_x86_64"
+			commandLine "strip", "-o", "expSpin.stripped", "expSpin"
+		}
+	}
 }
 
 // TODO: testSpecimenMac_x86_64 (Intel)

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

@@ -35,6 +35,31 @@ import ghidra.dbg.target.TargetMethod.TargetParameterMap;
 public abstract class AbstractDebuggerModelLauncherTest extends AbstractDebuggerModelTest
 		implements RequiresLaunchSpecimen {
 
+	protected  class ProcessCreatedDebugModelListener extends AnnotatedDebuggerAttributeListener {
+		public final CompletableFuture<Void> observedCreated = new CompletableFuture<>();
+
+		public ProcessCreatedDebugModelListener() {
+			super(MethodHandles.lookup());
+		}
+
+		@AttributeCallback(TargetExecutionStateful.STATE_ATTRIBUTE_NAME)
+		public void stateChanged(TargetObject object, TargetExecutionState state) {
+			// We're only expecting one process, so this should be fine
+			TargetProcess process = DebugModelConventions.liveProcessOrNull(object);
+			if (process == null) {
+				return;
+			}
+			try {
+				TargetEnvironment env = findEnvironment(process.getPath());
+				assertEnvironment(env);
+				observedCreated.complete(null);
+			}
+			catch (Throwable e) {
+				observedCreated.completeExceptionally(e);
+			}
+		}
+	}
+
 	public List<String> getExpectedLauncherPath() {
 		return null;
 	}
@@ -80,26 +105,7 @@ public abstract class AbstractDebuggerModelLauncherTest extends AbstractDebugger
 	public void testLaunch() throws Throwable {
 		m.build();
 
-		var listener = new AnnotatedDebuggerAttributeListener(MethodHandles.lookup()) {
-			CompletableFuture<Void> observedCreated = new CompletableFuture<>();
-
-			@AttributeCallback(TargetExecutionStateful.STATE_ATTRIBUTE_NAME)
-			public void stateChanged(TargetObject object, TargetExecutionState state) {
-				// We're only expecting one process, so this should be fine
-				TargetProcess process = DebugModelConventions.liveProcessOrNull(object);
-				if (process == null) {
-					return;
-				}
-				try {
-					TargetEnvironment env = findEnvironment(process.getPath());
-					assertEnvironment(env);
-					observedCreated.complete(null);
-				}
-				catch (Throwable e) {
-					observedCreated.completeExceptionally(e);
-				}
-			}
-		};
+		ProcessCreatedDebugModelListener listener = new ProcessCreatedDebugModelListener();
 		// NB. I've intentionally omitted the reorderer here. The model should get it right.
 		m.getModel().addModelListener(listener);