GP-853: Responding to CLI-driven memory changes in dbgeng

Ghidra/Debug/Debugger-agent-dbgeng/src/main/java/agent/dbgeng/manager/evt/DbgDebuggeeStateChangeEvent.java

@@ -0,0 +1,48 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package agent.dbgeng.manager.evt;
+
+import agent.dbgeng.dbgeng.DebugClient.ChangeDebuggeeState;
+import ghidra.comm.util.BitmaskSet;
+
+/**
+ * The event corresponding with ChangedDebuggeeState
+ */
+public class DbgDebuggeeStateChangeEvent extends AbstractDbgEvent<Integer> {
+	private final BitmaskSet<ChangeDebuggeeState> flags;
+	private final long argument;
+
+	/**
+	 * The selected flags must be specified by dbgeng.
+	 * 
+	 * @param flags dbgeng-provided id
+	 * @param argument event-specific argument
+	 */
+	public DbgDebuggeeStateChangeEvent(BitmaskSet<ChangeDebuggeeState> flags, long argument) {
+		super((int) flags.getBitmask());
+		this.flags = flags;
+		this.argument = argument;
+	}
+
+	public BitmaskSet<ChangeDebuggeeState> getFlags() {
+		return flags;
+	}
+
+	public long getArgument() {
+		return argument;
+	}
+
+}

Ghidra/Debug/Debugger-agent-dbgeng/src/main/java/agent/dbgeng/manager/impl/DbgDebugEventCallbacksAdapter.java

@@ -18,8 +18,7 @@ package agent.dbgeng.manager.impl;
 import java.nio.file.Paths;
 
 import agent.dbgeng.dbgeng.*;
-import agent.dbgeng.dbgeng.DebugClient.ChangeEngineState;
-import agent.dbgeng.dbgeng.DebugClient.DebugStatus;
+import agent.dbgeng.dbgeng.DebugClient.*;
 import agent.dbgeng.dbgeng.util.DebugEventCallbacksAdapter;
 import agent.dbgeng.manager.DbgState;
 import agent.dbgeng.manager.evt.*;
@@ -132,13 +131,14 @@ public class DbgDebugEventCallbacksAdapter extends DebugEventCallbacksAdapter {
 		return checkInterrupt(DebugStatus.NO_CHANGE);
 	}
 
-	/*
 	@Override
 	public DebugStatus changeDebuggeeState(BitmaskSet<ChangeDebuggeeState> flags, long argument) {
-		System.err.println("CHANGE_DEBUGGEE_STATE: " + flags + ":" + argument);
-		return DebugStatus.NO_CHANGE;
+		//System.err.println("CHANGE_DEBUGGEE_STATE: " + flags + ":" + argument);
+		return checkInterrupt(
+			manager.processEvent(new DbgDebuggeeStateChangeEvent(flags, argument)));
 	}
 
+	/*
 	@Override
 	public DebugStatus sessionStatus(SessionStatus status) {
 		System.err.println("SESSION_STATUS: " + status);

Ghidra/Debug/Debugger-agent-dbgeng/src/main/java/agent/dbgeng/manager/impl/DbgManagerImpl.java

@@ -15,7 +15,7 @@
  */
 package agent.dbgeng.manager.impl;
 
-import static ghidra.async.AsyncUtils.*;
+import static ghidra.async.AsyncUtils.sequence;
 
 import java.util.*;
 import java.util.concurrent.CompletableFuture;
@@ -594,6 +594,7 @@ public class DbgManagerImpl implements DbgManager {
 		handlerMap.put(DbgStateChangedEvent.class, this::processStateChanged);
 		handlerMap.put(DbgSessionSelectedEvent.class, this::processSessionSelected);
 		handlerMap.put(DbgSystemsEvent.class, this::processSystemsEvent);
+		handlerMap.putVoid(DbgDebuggeeStateChangeEvent.class, this::processDebuggeeStateChanged);
 		handlerMap.putVoid(DbgCommandDoneEvent.class, this::processDefault);
 		handlerMap.putVoid(DbgStoppedEvent.class, this::processDefault);
 		handlerMap.putVoid(DbgRunningEvent.class, this::processDefault);
@@ -1034,6 +1035,12 @@ public class DbgManagerImpl implements DbgManager {
 		return statusMap.get(evt.getClass());
 	}
 
+	protected void processDebuggeeStateChanged(DbgDebuggeeStateChangeEvent evt, Void v) {
+		if (evt.getFlags().contains(ChangeDebuggeeState.DATA)) {
+			getEventListeners().fire.memoryChanged(currentProcess, 0L, 0, evt.getCause());
+		}
+	}
+
 	protected void processConsoleOutput(DbgConsoleOutputEvent evt, Void v) {
 		getEventListeners().fire.consoleOutput(evt.getInfo(), evt.getMask());
 	}

Ghidra/Debug/Debugger-agent-dbgeng/src/main/java/agent/dbgeng/model/impl/DbgModelTargetProcessImpl.java

@@ -224,6 +224,13 @@ public class DbgModelTargetProcessImpl extends DbgModelTargetObjectImpl
 		}
 	}
 
+	@Override
+	public void memoryChanged(DbgProcess proc, long addr, int len, DbgCause cause) {
+		if (proc.equals(this.process)) {
+			listeners.fire.invalidateCacheRequested(memory);
+		}
+	}
+
 	@Override
 	public CompletableFuture<Void> setActive() {
 		DbgManagerImpl manager = getManager();

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

@@ -111,7 +111,6 @@ public class GdbModelImpl extends AbstractDebuggerObjectModel {
 				break;
 			}
 			case RUNNING: {
-				session.invalidateMemoryAndRegisterCaches();
 				session.setAccessible(false);
 				break;
 			}

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

@@ -441,9 +441,13 @@ public class GdbModelTargetInferior
 			inferiorRunning(sco.getReason());
 			List<Object> params = new ArrayList<>();
 			gatherThreads(params, sco.getAffectedThreads());
+			if (targetEventThread == null && !params.isEmpty()) {
+				targetEventThread = threads.getTargetThread(sco.getAffectedThreads().iterator().next());
+			}
 			if (targetEventThread != null) {
 				impl.session.getListeners().fire.event(impl.session, targetEventThread,
 					TargetEventType.RUNNING, "Running", params);
+				invalidateMemoryAndRegisterCaches();
 			}
 		}
 		if (sco.getState() != GdbState.STOPPED) {

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

@@ -181,6 +181,8 @@ public class GdbModelTargetProcessMemory
 		});
 	}
 
+	// TODO: Seems this is only called when sco.getState() == STOPPED.
+	// Maybe should name it such
 	public CompletableFuture<Void> stateChanged(GdbStateChangeRecord sco) {
 		return requestElements(false).thenCompose(__ -> {
 			AsyncFence fence = new AsyncFence();

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/listing/DebuggerListingProvider.java

@@ -77,6 +77,7 @@ import ghidra.program.util.ProgramSelection;
 import ghidra.trace.model.*;
 import ghidra.trace.model.Trace.*;
 import ghidra.trace.model.memory.TraceMemoryRegion;
+import ghidra.trace.model.memory.TraceMemoryState;
 import ghidra.trace.model.modules.*;
 import ghidra.trace.model.program.TraceProgramView;
 import ghidra.trace.model.program.TraceVariableSnapProgramView;
@@ -257,6 +258,8 @@ public class DebuggerListingProvider extends CodeViewerProvider implements Listi
 			listenFor(TraceSectionChangeType.ADDED, this::sectionChanged);
 			listenFor(TraceSectionChangeType.CHANGED, this::sectionChanged);
 			listenFor(TraceSectionChangeType.DELETED, this::sectionChanged);
+
+			listenFor(TraceMemoryStateChangeType.CHANGED, this::memStateChanged);
 		}
 
 		private void snapshotAdded(TraceSnapshot snapshot) {
@@ -301,6 +304,22 @@ public class DebuggerListingProvider extends CodeViewerProvider implements Listi
 		private void sectionChanged(TraceSection section) {
 			updateLabelDebouncer.contact(null);
 		}
+
+		private void memStateChanged(TraceAddressSnapRange range, TraceMemoryState oldIsNull,
+				TraceMemoryState newState) {
+			if (current.getView() == null) {
+				return;
+			}
+			if (!range.getLifespan().contains(current.getSnap())) {
+				return;
+			}
+			// TODO: Debounce this?
+			getListingPanel().getFieldPanel().repaint();
+
+			if (newState == TraceMemoryState.UNKNOWN) {
+				doAutoReadMemory();
+			}
+		}
 	}
 
 	protected class ForAccessRecorderListener implements TraceRecorderListener {

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

@@ -26,8 +26,7 @@ import ghidra.async.AsyncUtils;
 import ghidra.dbg.target.*;
 import ghidra.dbg.target.TargetExecutionStateful.TargetExecutionState;
 import ghidra.dbg.util.PathUtils;
-import ghidra.program.model.address.Address;
-import ghidra.program.model.address.AddressRange;
+import ghidra.program.model.address.*;
 import ghidra.program.model.data.Pointer;
 import ghidra.program.model.lang.*;
 import ghidra.trace.model.Trace;
@@ -331,6 +330,24 @@ public class DefaultThreadRecorder implements ManagedThreadRecorder {
 		}, getTargetThread().getJoinedPath("."));
 	}
 
+	@Override
+	public void invalidateRegisterValues(TargetRegisterBank bank) {
+		int frameLevel = stackRecorder.getSuccessorFrameLevel(bank);
+		long snap = recorder.getSnap();
+		String path = bank.getJoinedPath(".");
+
+		recorder.parTx.execute("Registers invalidated: " + path, () -> {
+			TraceMemoryRegisterSpace regSpace =
+				memoryManager.getMemoryRegisterSpace(traceThread, frameLevel, false);
+			if (regSpace == null) {
+				return;
+			}
+			AddressSpace as = regSpace.getAddressSpace();
+			regSpace.setState(snap, as.getMinAddress(), as.getMaxAddress(),
+				TraceMemoryState.UNKNOWN);
+		}, path);
+	}
+
 	public CompletableFuture<Void> writeThreadRegisters(int frameLevel,
 			Map<Register, RegisterValue> values) {
 		if (!regMapper.getRegistersOnTarget().containsAll(values.keySet())) {

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

@@ -28,8 +28,7 @@ import ghidra.dbg.target.TargetEventScope.TargetEventType;
 import ghidra.dbg.target.TargetExecutionStateful.TargetExecutionState;
 import ghidra.dbg.util.DebuggerCallbackReorderer;
 import ghidra.dbg.util.PathUtils;
-import ghidra.program.model.address.Address;
-import ghidra.program.model.address.AddressRange;
+import ghidra.program.model.address.*;
 import ghidra.trace.model.Trace;
 import ghidra.trace.model.memory.TraceMemoryManager;
 import ghidra.trace.model.memory.TraceMemoryState;
@@ -50,6 +49,8 @@ public class TraceEventListener extends AnnotatedDebuggerAttributeListener {
 	protected final DebuggerCallbackReorderer reorderer = new DebuggerCallbackReorderer(this);
 	protected final PrivatelyQueuedListener<DebuggerModelListener> queue;
 
+	private boolean ignoreInvalidation = false;
+
 	public TraceEventListener(TraceObjectManager collection) {
 		super(MethodHandles.lookup());
 		this.recorder = collection.getRecorder();
@@ -97,13 +98,6 @@ public class TraceEventListener extends AnnotatedDebuggerAttributeListener {
 
 	private boolean eventApplies(TargetObject eventThread, TargetEventType type,
 			List<Object> parameters) {
-		if (type == TargetEventType.RUNNING) {
-			return false;
-			/**
-			 * TODO: Perhaps some configuration for this later. It's kind of interesting to record
-			 * the RUNNING event time, but it gets pedantic when these exist between steps.
-			 */
-		}
 		if (eventThread != null) {
 			return successor(eventThread);
 		}
@@ -131,8 +125,21 @@ public class TraceEventListener extends AnnotatedDebuggerAttributeListener {
 		if (!eventApplies(eventThread, type, parameters)) {
 			return;
 		}
+		if (type == TargetEventType.RUNNING) {
+			/**
+			 * Do not permit the current snapshot to be invalidated on account of the target
+			 * running. When the STOP occurs, a new (completely UNKNOWN) snapshot is generated.
+			 */
+			ignoreInvalidation = true;
+			return;
+			/**
+			 * TODO: Perhaps some configuration for this later. It's kind of interesting to record
+			 * the RUNNING event time, but it gets pedantic when these exist between steps.
+			 */
+		}
 		ManagedThreadRecorder rec = recorder.getThreadRecorder(eventThread);
 		recorder.createSnapshot(description, rec == null ? null : rec.getTraceThread(), null);
+		ignoreInvalidation = false;
 
 		if (type == TargetEventType.MODULE_LOADED) {
 			long snap = recorder.getSnap();
@@ -180,6 +187,30 @@ public class TraceEventListener extends AnnotatedDebuggerAttributeListener {
 		}
 	}
 
+	@Override
+	public void invalidateCacheRequested(TargetObject object) {
+		if (!valid) {
+			return;
+		}
+		if (ignoreInvalidation) {
+			return;
+		}
+		if (object instanceof TargetRegisterBank) {
+			ManagedThreadRecorder rec = recorder.getThreadRecorderForSuccessor(object);
+			if (rec != null) {
+				rec.invalidateRegisterValues((TargetRegisterBank) object);
+			}
+		}
+		if (object instanceof TargetMemory) {
+			long snap = recorder.getSnap();
+			String path = object.getJoinedPath(".");
+			recorder.parTx.execute("Memory invalidated: " + path, () -> {
+				AddressSet set = trace.getBaseLanguage().getAddressFactory().getAddressSet();
+				memoryManager.setState(snap, set, TraceMemoryState.UNKNOWN);
+			}, path);
+		}
+	}
+
 	@Override
 	public void registersUpdated(TargetObject bank, Map<String, byte[]> updates) {
 		if (!valid) {

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

@@ -39,6 +39,8 @@ public interface ManagedThreadRecorder extends AbstractTraceRecorder {
 
 	public void recordRegisterValues(TargetRegisterBank bank, Map<String, byte[]> updates);
 
+	public void invalidateRegisterValues(TargetRegisterBank bank);
+
 	public boolean objectRemoved(TargetObject removed);
 
 	public void stateChanged(TargetExecutionState state);

Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/DebuggerModelListener.java

@@ -304,7 +304,7 @@ public interface DebuggerModelListener {
 	 * it need only report reads or writes which updated that cache. However, that cache must be
 	 * invalidated whenever any other event occurs which could change register values, e.g., the
 	 * target stepping or running. See {@link #invalidateCacheRequested(TargetObject)}. If the
-	 * implementation doe not employ a cache, then it must report <em>every</em> successful
+	 * implementation does not employ a cache, then it must report <em>every</em> successful
 	 * client-driven read or write. If the implementation can detect <em>debugger-driven</em>
 	 * register reads and writes, then it recommended to call this method for those events. However,
 	 * this method <em>must not</em> be called for <em>target-driven</em> register changes, except

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

@@ -15,10 +15,15 @@
  */
 package ghidra.dbg.test;
 
-import static org.junit.Assert.*;
-import static org.junit.Assume.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeNotNull;
 
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
 import java.util.concurrent.CompletableFuture;
 
 import org.junit.Test;
@@ -27,9 +32,15 @@ import ghidra.async.AsyncDebouncer;
 import ghidra.async.AsyncTimer;
 import ghidra.dbg.DebugModelConventions.AsyncState;
 import ghidra.dbg.DebuggerModelListener;
-import ghidra.dbg.target.*;
 import ghidra.dbg.target.TargetEventScope.TargetEventType;
+import ghidra.dbg.target.TargetExecutionStateful;
 import ghidra.dbg.target.TargetExecutionStateful.TargetExecutionState;
+import ghidra.dbg.target.TargetMemory;
+import ghidra.dbg.target.TargetObject;
+import ghidra.dbg.target.TargetRegisterBank;
+import ghidra.dbg.target.TargetSteppable;
+import ghidra.dbg.target.TargetThread;
+import ghidra.util.Msg;
 
 /**
  * Tests the functionality of a single-stepping a target
@@ -116,7 +127,8 @@ public abstract class AbstractDebuggerModelSteppableTest extends AbstractDebugge
 		EVENT_RUNNING,
 		EVENT_STOPPED,
 		REGS_UPDATED,
-		CACHE_INVALIDATED;
+		REGS_CACHE_INVALIDATED,
+		MEM_CACHE_INVALIDATED;
 	}
 
 	/**
@@ -172,8 +184,14 @@ public abstract class AbstractDebuggerModelSteppableTest extends AbstractDebugge
 			@Override
 			public void invalidateCacheRequested(TargetObject object) {
 				synchronized (callbacks) {
-					callbacks.add(CallbackType.CACHE_INVALIDATED);
-					log.add("invalidateCacheRequested()");
+					if (object instanceof TargetRegisterBank) {
+						callbacks.add(CallbackType.REGS_CACHE_INVALIDATED);
+						log.add("invalidateCacheRequested(TargetRegisterBank)");
+					}
+					else if (object instanceof TargetMemory) {
+						callbacks.add(CallbackType.MEM_CACHE_INVALIDATED);
+						log.add("invalidateCacheRequested(TargetMemory)");
+					}
 				}
 				debouncer.contact(null);
 			}
@@ -215,17 +233,99 @@ public abstract class AbstractDebuggerModelSteppableTest extends AbstractDebugge
 			callbacks = List.copyOf(listener.callbacks);
 		}
 
-		int stoppedIdx = callbacks.indexOf(CallbackType.EVENT_STOPPED);
-		assertNotEquals(-1, stoppedIdx);
-		List<CallbackType> follows = callbacks.subList(stoppedIdx + 1, callbacks.size());
-		assertFalse("Observed multiple event(STOPPED/OTHER) callbacks for one step",
-			follows.contains(CallbackType.EVENT_STOPPED));
-		int regsUpdatedIdx = callbacks.indexOf(CallbackType.REGS_UPDATED);
-		assertNotEquals("Did not observe a registersUpdated() callback", -1, regsUpdatedIdx);
-		assertTrue("registersUpdated() must follow event(STOPPED/OTHER)",
-			regsUpdatedIdx > stoppedIdx);
-		int invalidatedIdx = follows.indexOf(CallbackType.CACHE_INVALIDATED);
-		assertTrue("Observed an invalidateCacheRequest() after registersUpdated()",
-			invalidatedIdx < regsUpdatedIdx); // absent or precedes
+		Msg.info(this, "Observations: " + callbacks);
+		
+		boolean observedRunning = false;
+		boolean observedStopped = false;
+		boolean observedRegsUpdated = false;
+		boolean observedInvalidateRegs = false;
+		boolean observedInvalidateMem = false;
+		for (CallbackType cb : callbacks) {
+			switch (cb) {
+				case EVENT_RUNNING:
+					if (observedRunning) {
+						fail("Observed a second event(RUNNING).");
+					}
+					observedRunning = true;
+					break;
+				case EVENT_STOPPED:
+					if (observedStopped) {
+						fail("Observed a second event(STOPPED).");
+					}
+					observedStopped = true;
+					break;
+				case REGS_UPDATED:
+					if (!observedStopped) {
+						fail("Observed registersUpdated() before event(STOPPED).");
+					}
+					observedRegsUpdated = true;
+					break;
+				case REGS_CACHE_INVALIDATED:
+					if (!observedStopped) {
+						if (observedRunning) {
+							// Recorder will ignore, but it still counts for flushing model
+							break;
+						}
+						/**
+						 * Spurious cache invalidations are not permitted before the recorder would
+						 * create a new snap. It knows to ignore invalidations that occur between
+						 * event(RUNNING) and event(STOPPED).
+						 */
+						fail("Observed a spurious invalidateCacheRequested(Regs).");
+					}
+					else if (observedInvalidateRegs) {
+						Msg.warn(this, 
+							"Observed an extra invalidateCacheRequested(Regs) after event(STOPPED).");
+					}
+					else if (observedRegsUpdated) {
+						// The invalidate would effectively undo the update
+						fail("Observed invalidateCacheRequested(Regs) after registersUpdated().");
+					}
+					else {
+						observedInvalidateRegs = true;
+					}
+					break;
+				case MEM_CACHE_INVALIDATED:
+					if (!observedStopped) {
+						if (observedRunning) {
+							// Recorder will ignore, but it still counts for flushing model
+							observedInvalidateMem = true;
+							break;
+						}
+						/**
+						 * Spurious cache invalidations are not permitted before the recorder would
+						 * create a new snap. It knows to ignore invalidations that occur between
+						 * event(RUNNING) and event(STOPPED).
+						 */
+						fail("Observed a spurious invalidateCacheRequested(Mem).");
+					}
+					else if (observedInvalidateMem) {
+						Msg.warn(this, 
+							"Observed an extra invalidateCacheRequested(Mem) after event(STOPPED).");
+					}
+					else {
+						observedInvalidateMem = true;
+					}
+					break;
+				default:
+					throw new AssertionError();
+			}
+		}
+
+		if (!observedStopped) {
+			fail("Never observed event(STOPPED).");
+		}
+		if (!observedRegsUpdated && !observedInvalidateRegs) {
+			fail("Observed neither invalidateCacheRequested(Regs) nor registersUpdated().");
+		}
+		if (!observedInvalidateMem) {
+			/**
+			 * NOTE: even though STOPPED advances the snapshot, we still require
+			 * invalidateCacheRequested(Mem) to ensure the model's memory cache is not stale.
+			 * Whether or not your model employs an internal cache, this is required, because your
+			 * model may be accessed via a proxy model (e.g., GADP) that uses a cache.
+			 */
+			fail("Never observed invalidateCacheRequested(Mem).");
+		}
 	}
 }

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/memory/DBTraceMemorySpace.java

@@ -328,9 +328,19 @@ public class DBTraceMemorySpace implements Unfinished, TraceMemorySpace, DBTrace
 	}
 
 	protected void checkState(TraceMemoryState state) {
-		if (state == null || state == TraceMemoryState.UNKNOWN) {
+		/**
+		 * TODO: I don't remember why I prohibited this originally. It seems some technicality in
+		 * calling something "last known?" We might revisit and specify that definition, in the face
+		 * of memory being invalidated while the target is suspended. It seems appropriate to leave
+		 * the stale bytes in the trace, but change the state to UNKNOWN. Do those stale bytes still
+		 * count as last known? Aside from getBytes, I don't see anything that requires a precise
+		 * distinction. We might be careful how this interacts with removeBytes, though.... Well,
+		 * AFAICT, it doesn't depend on state markings. For now, I'm going to allow it. We'll see
+		 * what happens. Changing this had no effect on the unit tests :/ .
+		 */
+		/*if (state == null || state == TraceMemoryState.UNKNOWN) {
 			throw new IllegalArgumentException("Cannot erase memory state without removing bytes");
-		}
+		}*/
 	}
 
 	@Override