GP-6027: Fix breakpoint GUI glitches when a breakpoint's address changes.

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/DebuggerLogicalBreakpointServicePlugin.java

@@ -231,7 +231,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 				return;
 			}
 			try {
-				info.trackTraceBreakpoint(c.a, tb, getMode(info.trace), false);
+				info.trackTraceBreakpoint(c, tb, getMode(info.trace), false);
 			}
 			catch (TrackedTooSoonException e) {
 				Msg.info(this,
@@ -244,7 +244,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 				return;
 			}
 			try {
-				info.trackTraceBreakpoint(c.a, tb, getMode(info.trace), true);
+				info.trackTraceBreakpoint(c, tb, getMode(info.trace), true);
 			}
 			catch (TrackedTooSoonException e) {
 				Msg.info(this,
@@ -269,7 +269,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 			}
 			else {
 				try {
-					info.trackTraceBreakpoint(c.a, tb, getMode(info.trace), false);
+					info.trackTraceBreakpoint(c, tb, getMode(info.trace), false);
 				}
 				catch (TrackedTooSoonException e) {
 					Msg.info(this, "Ignoring " + tb +
@@ -482,7 +482,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 
 		protected void reloadBreakpoints(ChangeCollector c) {
 			forgetTraceInvalidBreakpoints(c.r);
-			trackTraceBreakpoints(c.a);
+			trackTraceBreakpoints(c);
 		}
 
 		protected void forgetAllBreakpoints(RemoveCollector r) {
@@ -531,7 +531,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 			}
 		}
 
-		protected void trackTraceBreakpoints(AddCollector a) {
+		protected void trackTraceBreakpoints(ChangeCollector c) {
 			ControlMode mode = getMode(trace);
 			if (!mode.useEmulatedBreakpoints() && target == null) {
 				return;
@@ -541,10 +541,10 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 				visible.addAll(trace.getBreakpointManager()
 						.getBreakpointsIntersecting(Lifespan.at(snap), range));
 			}
-			trackTraceBreakpoints(a, visible, mode);
+			trackTraceBreakpoints(c, visible, mode);
 		}
 
-		protected void trackTraceBreakpoints(AddCollector a,
+		protected void trackTraceBreakpoints(ChangeCollector c,
 				Collection<TraceBreakpointLocation> breakpoints, ControlMode mode) {
 			for (TraceBreakpointLocation tb : breakpoints) {
 				try {
@@ -553,7 +553,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 					 * events that the manager punts to OBJECT_RESTORED. Thus, we have to set
 					 * forceUpdate here.
 					 */
-					trackTraceBreakpoint(a, tb, mode, true);
+					trackTraceBreakpoint(c, tb, mode, true);
 				}
 				catch (TrackedTooSoonException e) {
 					// This can still happen during reload (on OBJECT_RESTORED)
@@ -579,37 +579,44 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 				new DefaultTraceLocation(trace, null, Lifespan.at(snap), minAddress));
 		}
 
-		protected void trackTraceBreakpoint(AddCollector a, TraceBreakpointLocation tb,
-				ControlMode mode,
-				boolean forceUpdate) throws TrackedTooSoonException {
+		protected void trackTraceBreakpoint(ChangeCollector c, TraceBreakpointLocation tb,
+				ControlMode mode, boolean forceUpdate) throws TrackedTooSoonException {
 			if (!mode.useEmulatedBreakpoints() &&
 				(target == null || !target.isBreakpointValid(tb))) {
 				return;
 			}
 			Address traceAddr = tb.getMinAddress(snap);
 			if (traceAddr == null) {
+				LogicalBreakpointInternal oldLb = logicalByBreakpoint.remove(tb);
+				if (oldLb != null) {
+					// Happens when existing location's range is deleted
+					doRemoveFromLogicalBreakpoint(c.r, oldLb, tb);
+				}
 				return; // Will update via breakpointChanged when address is set
 			}
 			ProgramLocation progLoc = computeStaticLocation(tb);
 			LogicalBreakpointInternal lb;
 			if (progLoc != null) {
 				InfoPerProgram progInfo = programInfos.get(progLoc.getProgram());
-				lb = progInfo.getOrCreateLogicalBreakpointFor(a, progLoc.getByteAddress(), tb,
+				lb = progInfo.getOrCreateLogicalBreakpointFor(c.a, progLoc.getByteAddress(), tb,
 					snap);
 			}
 			else {
-				lb = getOrCreateLogicalBreakpointFor(a, traceAddr, tb, snap);
+				lb = getOrCreateLogicalBreakpointFor(c.a, traceAddr, tb, snap);
 			}
 			assert logicalByAddress.get(traceAddr).contains(lb);
-			logicalByBreakpoint.put(tb, lb);
+			LogicalBreakpointInternal oldLb = logicalByBreakpoint.put(tb, lb);
+			if (oldLb != lb && oldLb != null) {
+				// Happens when existing location's range changes
+				doRemoveFromLogicalBreakpoint(c.r, oldLb, tb);
+			}
 			if (lb.trackBreakpoint(tb) || forceUpdate) {
-				a.updated(lb);
+				c.a.updated(lb);
 			}
 		}
 
-		protected LogicalBreakpointInternal removeFromLogicalBreakpoint(RemoveCollector r,
-				TraceBreakpointLocation tb) {
-			LogicalBreakpointInternal lb = logicalByBreakpoint.remove(tb);
+		protected LogicalBreakpointInternal doRemoveFromLogicalBreakpoint(RemoveCollector r,
+				LogicalBreakpointInternal lb, TraceBreakpointLocation tb) {
 			if (lb == null || !lb.untrackBreakpoint(tb)) {
 				return null;
 			}
@@ -625,6 +632,12 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
 			return lb;
 		}
 
+		protected LogicalBreakpointInternal removeFromLogicalBreakpoint(RemoveCollector r,
+				TraceBreakpointLocation tb) {
+			LogicalBreakpointInternal lb = logicalByBreakpoint.remove(tb);
+			return doRemoveFromLogicalBreakpoint(r, lb, tb);
+		}
+
 		protected void forgetTraceBreakpoint(RemoveCollector r, TraceBreakpointLocation tb) {
 			LogicalBreakpointInternal lb = removeFromLogicalBreakpoint(r, tb);
 			if (lb == null) {

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/target/DBTraceObjectInterface.java

@@ -159,6 +159,51 @@ public interface DBTraceObjectInterface extends TraceObjectInterface, TraceUniqu
 					cast.getNewValue());
 				return new TraceChangeRecord<>(type, getSpace(life), iface, null, null);
 			}
+			if (rec.getEventType() == TraceEvents.VALUE_LIFESPAN_CHANGED) {
+				if (object.isDeleted()) {
+					return null;
+				}
+				TraceEvent<T, ?> type = getChangedType();
+				if (type == null) {
+					return null;
+				}
+				TraceChangeRecord<TraceObjectValue, Lifespan> cast =
+					TraceEvents.VALUE_LIFESPAN_CHANGED.cast(rec);
+				TraceObjectValue affected = cast.getAffectedObject();
+				String key = affected.getEntryKey();
+				if (!appliesToKey(key)) {
+					return null;
+				}
+				assert affected.getParent() == object;
+				if (object.getCanonicalParent(affected.getMaxSnap()) == null) {
+					return null; // Object is not complete
+				}
+				emitExtraValueChanged(affected.getLifespan(), key, null, null);
+				return new TraceChangeRecord<>(type, getSpace(life), iface, null, null);
+			}
+			if (rec.getEventType() == TraceEvents.VALUE_DELETED) {
+				if (object.isDeleted()) {
+					return null;
+				}
+				TraceEvent<T, ?> type = getChangedType();
+				if (type == null) {
+					return null;
+				}
+				TraceChangeRecord<TraceObjectValue, Void> cast =
+					TraceEvents.VALUE_DELETED.cast(rec);
+				TraceObjectValue affected = cast.getAffectedObject();
+				String key = affected.getEntryKey();
+				if (!appliesToKey(key)) {
+					return null;
+				}
+				assert affected.getParent() == object;
+				if (object.getCanonicalParent(affected.getMaxSnap()) == null) {
+					return null; // Object is not complete
+				}
+				emitExtraValueChanged(affected.getLifespan(), key, cast.getOldValue(),
+					cast.getNewValue());
+				return new TraceChangeRecord<>(type, getSpace(life), iface, null, null);
+			}
 			if (rec.getEventType() == TraceEvents.OBJECT_DELETED) {
 				return translateDeleted(life);
 			}

Ghidra/Test/DebuggerIntegrationTest/src/test/java/ghidra/app/plugin/core/debug/service/breakpoint/DebuggerRmiLogicalBreakpointServiceTest.java

@@ -16,22 +16,31 @@
 package ghidra.app.plugin.core.debug.service.breakpoint;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotSame;
 
 import java.util.*;
 
 import org.junit.Before;
+import org.junit.Test;
 
 import db.Transaction;
+import generic.Unique;
+import ghidra.app.plugin.core.debug.gui.breakpoint.DebuggerBreakpointsPlugin;
+import ghidra.app.plugin.core.debug.gui.model.DebuggerModelPlugin;
+import ghidra.app.plugin.core.debug.service.control.DebuggerControlServicePlugin;
 import ghidra.app.plugin.core.debug.service.modules.DebuggerStaticMappingUtils;
 import ghidra.app.plugin.core.debug.service.tracermi.TraceRmiTarget;
+import ghidra.app.services.DebuggerControlService;
+import ghidra.debug.api.breakpoint.LogicalBreakpoint;
+import ghidra.debug.api.control.ControlMode;
+import ghidra.debug.api.tracemgr.DebuggerCoordinates;
 import ghidra.program.model.address.Address;
 import ghidra.program.model.listing.Program;
 import ghidra.program.util.ProgramLocation;
 import ghidra.trace.database.ToyDBTraceBuilder;
 import ghidra.trace.model.*;
+import ghidra.trace.model.breakpoint.*;
 import ghidra.trace.model.breakpoint.TraceBreakpointKind.TraceBreakpointKindSet;
-import ghidra.trace.model.breakpoint.TraceBreakpointLocation;
-import ghidra.trace.model.breakpoint.TraceBreakpointSpec;
 import ghidra.trace.model.memory.TraceMemoryRegion;
 import ghidra.trace.model.modules.TraceStaticMapping;
 import ghidra.trace.model.target.TraceObject;
@@ -249,4 +258,203 @@ public class DebuggerRmiLogicalBreakpointServiceTest extends
 		assertEquals(Map.ofEntries(
 			Map.entry("breakpoint", expectedLoc.getSpecification().getObject())), args);
 	}
+
+	@Test
+	public void testAddTraceBreakpointThenModifyRange_Lone() throws Throwable {
+		// These are for interactive debugging
+		//addPlugin(tool, DebuggerModelPlugin.class);
+		//addPlugin(tool, DebuggerBreakpointsPlugin.class);
+
+		DebuggerControlService controlService =
+			addPlugin(tool, DebuggerControlServicePlugin.class);
+
+		createTrace();
+		traceManager.openTrace(tb.trace);
+		traceManager.activate(DebuggerCoordinates.NOWHERE.trace(tb.trace).snap(1));
+		// Needs to have a target or be emulated for the breakpoint service to care
+		controlService.setCurrentMode(tb.trace, ControlMode.RW_EMULATOR);
+
+		TraceBreakpointLocation bpt;
+		try (Transaction tid = tb.startTransaction()) {
+			tb.createRootObject(SCHEMA_CTX);
+			bpt = tb.trace.getBreakpointManager()
+					.addBreakpoint("Processes[1].Breakpoints[0][0]", Lifespan.nowOn(0),
+						tb.addr(0x55550123), Set.of(), Set.of(TraceBreakpointKind.SW_EXECUTE),
+						false, "");
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		LogicalBreakpoint lbBefore =
+			Unique.assertOne(breakpointService.getBreakpointsAt(tb.trace, tb.addr(0x55550123)));
+
+		try (Transaction tid = tb.startTransaction()) {
+			bpt.setRange(Lifespan.nowOn(1), tb.range(0x56660123));
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		LogicalBreakpoint lbAfter =
+			Unique.assertOne(breakpointService.getBreakpointsAt(tb.trace, tb.addr(0x56660123)));
+		assertNotSame(lbBefore, lbAfter);
+		assertEquals(Set.of(lbAfter), breakpointService.getAllBreakpoints());
+		assertEquals(Set.of(bpt), lbAfter.getTraceBreakpoints());
+	}
+
+	@Test
+	public void testAddTraceBreakpointThenModifyRange_LoneThenNullAndBack() throws Throwable {
+		// These are for interactive debugging
+		addPlugin(tool, DebuggerModelPlugin.class);
+		addPlugin(tool, DebuggerBreakpointsPlugin.class);
+
+		DebuggerControlService controlService =
+			addPlugin(tool, DebuggerControlServicePlugin.class);
+
+		createTrace();
+		traceManager.openTrace(tb.trace);
+		traceManager.activate(DebuggerCoordinates.NOWHERE.trace(tb.trace).snap(1));
+		// Needs to have a target or be emulated for the breakpoint service to care
+		controlService.setCurrentMode(tb.trace, ControlMode.RW_EMULATOR);
+
+		TraceBreakpointLocation bpt;
+		try (Transaction tid = tb.startTransaction()) {
+			tb.createRootObject(SCHEMA_CTX);
+			bpt = tb.trace.getBreakpointManager()
+					.addBreakpoint("Processes[1].Breakpoints[0][0]", Lifespan.nowOn(0),
+						tb.addr(0x55550123), Set.of(), Set.of(TraceBreakpointKind.SW_EXECUTE),
+						false, "");
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		Unique.assertOne(breakpointService.getBreakpointsAt(tb.trace, tb.addr(0x55550123)));
+
+		try (Transaction tid = tb.startTransaction()) {
+			bpt.getObject()
+					.setAttribute(Lifespan.nowOn(1), TraceBreakpointLocation.KEY_RANGE, null);
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		assertEquals(Set.of(), breakpointService.getAllBreakpoints());
+
+		try (Transaction tid = tb.startTransaction()) {
+			bpt.setRange(Lifespan.nowOn(1), tb.range(0x56660123));
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		LogicalBreakpoint lbAfter =
+			Unique.assertOne(breakpointService.getBreakpointsAt(tb.trace, tb.addr(0x56660123)));
+		assertEquals(Set.of(lbAfter), breakpointService.getAllBreakpoints());
+		assertEquals(Set.of(bpt), lbAfter.getTraceBreakpoints());
+	}
+
+	@Test
+	public void testAddTraceBreakpointThenModifyRange_MappedThenLone() throws Throwable {
+		// These are for interactive debugging
+		//addPlugin(tool, DebuggerModelPlugin.class);
+		//addPlugin(tool, DebuggerBreakpointsPlugin.class);
+
+		DebuggerControlService controlService =
+			addPlugin(tool, DebuggerControlServicePlugin.class);
+
+		createTrace();
+		traceManager.openTrace(tb.trace);
+		traceManager.activate(DebuggerCoordinates.NOWHERE.trace(tb.trace).snap(1));
+		// Needs to have a target or be emulated for the breakpoint service to care
+		controlService.setCurrentMode(tb.trace, ControlMode.RW_EMULATOR);
+
+		createProgramFromTrace();
+		intoProject(program);
+		programManager.openProgram(program);
+
+		TraceBreakpointLocation bpt;
+		try (Transaction tid = tb.startTransaction()) {
+			tb.createRootObject(SCHEMA_CTX);
+			bpt = tb.trace.getBreakpointManager()
+					.addBreakpoint("Processes[1].Breakpoints[0][0]", Lifespan.nowOn(0),
+						tb.addr(0x55550123), Set.of(), Set.of(TraceBreakpointKind.SW_EXECUTE),
+						false, "");
+			addTextMappingDead(0, program, tb);
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(mappingService.changesSettled());
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		LogicalBreakpoint lbBefore =
+			Unique.assertOne(breakpointService.getBreakpointsAt(tb.trace, tb.addr(0x55550123)));
+
+		try (Transaction tid = tb.startTransaction()) {
+			bpt.setRange(Lifespan.nowOn(1), tb.range(0x56660123));
+			// NOTE: New address is not mapped
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		LogicalBreakpoint lbAfter =
+			Unique.assertOne(breakpointService.getBreakpointsAt(tb.trace, tb.addr(0x56660123)));
+		assertNotSame(lbBefore, lbAfter);
+		assertEquals(Set.of(lbBefore, lbAfter), breakpointService.getAllBreakpoints());
+		assertEquals(Set.of(), lbBefore.getTraceBreakpoints());
+		assertEquals(Set.of(bpt), lbAfter.getTraceBreakpoints());
+	}
+
+	@Test
+	public void testAddTraceBreakpointThenModifyRange_Mapped() throws Throwable {
+		// These are for interactive debugging
+		//addPlugin(tool, DebuggerModelPlugin.class);
+		//addPlugin(tool, DebuggerBreakpointsPlugin.class);
+
+		DebuggerControlService controlService =
+			addPlugin(tool, DebuggerControlServicePlugin.class);
+
+		createTrace();
+		traceManager.openTrace(tb.trace);
+		traceManager.activate(DebuggerCoordinates.NOWHERE.trace(tb.trace).snap(1));
+		// Needs to have a target or be emulated for the breakpoint service to care
+		controlService.setCurrentMode(tb.trace, ControlMode.RW_EMULATOR);
+
+		createProgramFromTrace();
+		intoProject(program);
+		programManager.openProgram(program);
+
+		TraceBreakpointLocation bpt;
+		try (Transaction tid = tb.startTransaction()) {
+			tb.createRootObject(SCHEMA_CTX);
+			bpt = tb.trace.getBreakpointManager()
+					.addBreakpoint("Processes[1].Breakpoints[0][0]", Lifespan.nowOn(0),
+						tb.addr(0x55550123), Set.of(), Set.of(TraceBreakpointKind.SW_EXECUTE),
+						false, "");
+			addTextMappingDead(0, program, tb);
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(mappingService.changesSettled());
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		LogicalBreakpoint lbBefore =
+			Unique.assertOne(breakpointService.getBreakpointsAt(tb.trace, tb.addr(0x55550123)));
+
+		try (Transaction tid = tb.startTransaction()) {
+			bpt.setRange(Lifespan.nowOn(1), tb.range(0x55550124));
+		}
+		waitForDomainObject(tb.trace);
+		waitOn(breakpointService.changesSettled());
+		changeListener.assertAgreesWithService();
+
+		LogicalBreakpoint lbAfter =
+			Unique.assertOne(breakpointService.getBreakpointsAt(tb.trace, tb.addr(0x55550124)));
+		assertNotSame(lbBefore, lbAfter);
+		assertEquals(Set.of(lbBefore, lbAfter), breakpointService.getAllBreakpoints());
+		assertEquals(Set.of(), lbBefore.getTraceBreakpoints());
+		assertEquals(Set.of(bpt), lbAfter.getTraceBreakpoints());
+	}
 }