GP-896: Recording changes in breakpoint spec kinds

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

@@ -186,8 +186,9 @@ public class DefaultBreakpointRecorder implements ManagedBreakpointRecorder {
 		}, path);
 	}
 
-	protected void doBreakpointToggled(long snap,
+	protected void doBreakpointSpecChanged(long snap,
-			Collection<? extends TargetBreakpointLocation> bpts, boolean enabled) {
+			Collection<? extends TargetBreakpointLocation> bpts, boolean enabled,
+			Collection<TraceBreakpointKind> kinds) {
 		for (TargetBreakpointLocation bl : bpts) {
 			String path = PathUtils.toString(bl.getPath());
 			recorder.parTx.execute("Breakpoint " + path + " toggled", () -> {
@@ -197,16 +198,17 @@ public class DefaultBreakpointRecorder implements ManagedBreakpointRecorder {
 					return;
 				}
 				// Verify attributes match? Eh. If they don't, someone has fiddled with it.
-				traceBpt.splitWithEnabled(snap, enabled);
+				traceBpt.splitAndSet(snap, enabled, kinds);
 			}, path);
 		}
 	}
 
 	@Override
-	public void breakpointToggled(TargetBreakpointSpec spec, boolean enabled) {
+	public void breakpointSpecChanged(TargetBreakpointSpec spec, boolean enabled,
+			Collection<TraceBreakpointKind> kinds) {
 		long snap = recorder.getSnap();
 		spec.getLocations().thenAccept(bpts -> {
-			doBreakpointToggled(snap, bpts, enabled);
+			doBreakpointSpecChanged(snap, bpts, enabled, kinds);
 		}).exceptionally(ex -> {
 			Msg.error(this, "Error recording toggled breakpoint spec: " + spec.getJoinedPath("."),
 				ex);

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

@@ -23,6 +23,7 @@ import java.util.stream.Collectors;
 
 import ghidra.app.plugin.core.debug.mapping.*;
 import ghidra.app.plugin.core.debug.service.model.interfaces.*;
+import ghidra.app.services.TraceRecorder;
 import ghidra.app.services.TraceRecorderListener;
 import ghidra.async.AsyncLazyMap;
 import ghidra.dbg.target.*;
@@ -30,6 +31,7 @@ import ghidra.dbg.util.PathUtils;
 import ghidra.dbg.util.PathUtils.PathComparator;
 import ghidra.program.model.address.Address;
 import ghidra.trace.model.breakpoint.TraceBreakpoint;
+import ghidra.trace.model.breakpoint.TraceBreakpointKind;
 import ghidra.trace.model.memory.TraceMemoryRegion;
 import ghidra.trace.model.modules.TraceModule;
 import ghidra.trace.model.modules.TraceSection;
@@ -471,10 +473,13 @@ public class TraceObjectManager {
 	}
 
 	public void attributesChangedBreakpointSpec(TargetObject bpt, Map<String, ?> added) {
-		if (added.containsKey(TargetBreakpointSpec.ENABLED_ATTRIBUTE_NAME)) {
+		if (added.containsKey(TargetBreakpointSpec.ENABLED_ATTRIBUTE_NAME) ||
+			added.containsKey(TargetBreakpointSpec.KINDS_ATTRIBUTE_NAME)) {
 			TargetBreakpointSpec spec = (TargetBreakpointSpec) bpt;
-			boolean enabled = (Boolean) added.get(TargetBreakpointSpec.ENABLED_ATTRIBUTE_NAME);
+			boolean enabled = spec.isEnabled();
-			recorder.breakpointRecorder.breakpointToggled(spec, enabled);
+			Set<TraceBreakpointKind> traceKinds =
+				TraceRecorder.targetToTraceBreakpointKinds(spec.getKinds());
+			recorder.breakpointRecorder.breakpointSpecChanged(spec, enabled, traceKinds);
 		}
 	}
 

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

@@ -15,11 +15,13 @@
  */
 package ghidra.app.plugin.core.debug.service.model.interfaces;
 
+import java.util.Collection;
 import java.util.Set;
 
 import ghidra.dbg.target.*;
 import ghidra.program.model.address.Address;
 import ghidra.trace.model.breakpoint.TraceBreakpoint;
+import ghidra.trace.model.breakpoint.TraceBreakpointKind;
 import ghidra.trace.model.thread.TraceThread;
 
 public interface ManagedBreakpointRecorder {
@@ -37,7 +39,7 @@ public interface ManagedBreakpointRecorder {
 	TraceBreakpoint getTraceBreakpoint(TargetBreakpointLocation bpt);
 
 	/**
-	 * The length of a breakpoint location has changed
+	 * The range of a breakpoint location has changed
 	 * 
 	 * @param length the new length
 	 * @param traceAddr the address of the location in the trace
@@ -45,5 +47,13 @@ public interface ManagedBreakpointRecorder {
 	 */
 	void breakpointLocationChanged(int length, Address traceAddr, String path);
 
-	void breakpointToggled(TargetBreakpointSpec spec, boolean enabled);
+	/**
+	 * A breakpoint specification has changed (typically, toggled)
+	 * 
+	 * @param spec the specification that changed
+	 * @param enabled whether or not the spec is enabled
+	 * @param kinds the kinds of the spec
+	 */
+	void breakpointSpecChanged(TargetBreakpointSpec spec, boolean enabled,
+			Collection<TraceBreakpointKind> kinds);
 }

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/breakpoint/DBTraceBreakpoint.java

@@ -284,7 +284,8 @@ public class DBTraceBreakpoint
 	}
 
 	@Override
-	public DBTraceBreakpoint splitWithEnabled(long snap, boolean en) {
+	public DBTraceBreakpoint splitAndSet(long snap, boolean en,
+			Collection<TraceBreakpointKind> kinds) {
 		DBTraceBreakpoint that;
 		Range<Long> oldLifespan;
 		Range<Long> newLifespan;
@@ -292,7 +293,7 @@ public class DBTraceBreakpoint
 			if (!lifespan.contains(snap)) {
 				throw new IllegalArgumentException("snap = " + snap);
 			}
-			if (en == enabled) {
+			if (flagsByte == computeFlagsByte(enabled, kinds)) {
 				return this;
 			}
 			if (snap == getPlacedSnap()) {
@@ -302,7 +303,7 @@ public class DBTraceBreakpoint
 
 			that = doCopy();
 			that.doSetLifespan(DBTraceUtils.toRange(snap, getClearedSnap()));
-			that.doSetEnabled(en);
+			that.doSetFlags(en, kinds);
 			oldLifespan = lifespan;
 			newLifespan = DBTraceUtils.toRange(getPlacedSnap(), snap - 1);
 			this.doSetLifespan(newLifespan);
@@ -315,7 +316,26 @@ public class DBTraceBreakpoint
 		return that;
 	}
 
-	protected void doSetEnabled(@SuppressWarnings("hiding") boolean enabled) {
+	protected static byte computeFlagsByte(boolean enabled, Collection<TraceBreakpointKind> kinds) {
+		byte flags = 0;
+		for (TraceBreakpointKind k : kinds) {
+			flags |= k.getBits();
+		}
+		if (enabled) {
+			flags |= ENABLED_MASK;
+		}
+		return flags;
+	}
+
+	protected void doSetFlags(boolean enabled, Collection<TraceBreakpointKind> kinds) {
+		this.flagsByte = computeFlagsByte(enabled, kinds);
+		this.kinds.clear();
+		this.kinds.addAll(kinds);
+		this.enabled = enabled;
+		update(FLAGS_COLUMN);
+	}
+
+	protected void doSetEnabled(boolean enabled) {
 		this.enabled = enabled;
 		if (enabled) {
 			flagsByte |= ENABLED_MASK;
@@ -326,6 +346,20 @@ public class DBTraceBreakpoint
 		update(FLAGS_COLUMN);
 	}
 
+	protected void doSetKinds(Collection<TraceBreakpointKind> kinds) {
+		for (TraceBreakpointKind k : TraceBreakpointKind.values()) {
+			if (kinds.contains(k)) {
+				this.flagsByte |= k.getBits();
+				this.kinds.add(k);
+			}
+			else {
+				this.flagsByte &= ~k.getBits();
+				this.kinds.remove(k);
+			}
+		}
+		update(FLAGS_COLUMN);
+	}
+
 	@Override
 	public void setEnabled(boolean enabled) {
 		try (LockHold hold = LockHold.lock(space.lock.writeLock())) {
@@ -342,6 +376,15 @@ public class DBTraceBreakpoint
 		}
 	}
 
+	@Override
+	public void setKinds(Collection<TraceBreakpointKind> kinds) {
+		try (LockHold hold = LockHold.lock(space.lock.writeLock())) {
+			doSetKinds(kinds);
+		}
+		space.trace.setChanged(
+			new TraceChangeRecord<>(TraceBreakpointChangeType.CHANGED, space, this));
+	}
+
 	@Override
 	public Set<TraceBreakpointKind> getKinds() {
 		try (LockHold hold = LockHold.lock(space.lock.readLock())) {

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/model/breakpoint/TraceBreakpoint.java

@@ -15,6 +15,7 @@
  */
 package ghidra.trace.model.breakpoint;
 
+import java.util.Collection;
 import java.util.Set;
 
 import com.google.common.collect.Range;
@@ -130,18 +131,18 @@ public interface TraceBreakpoint {
 	long getClearedSnap();
 
 	/**
-	 * Split this breakpoint at the given snap, the later having the given "enabled" value.
+	 * Split this breakpoint at the given snap, and set the later's fields.
 	 * 
 	 * <p>
 	 * This breakpoint's lifespan must contain the given snap. This method first creates a copy of
-	 * this breakpoint, replacing the copy's placed snap and "enabled" value with those given. Then,
+	 * this breakpoint, replacing the copy's placed snap and additional fields. Then, it sets this
-	 * it sets this breakpoint's cleared snap to one less than the given snap, so that the two
+	 * breakpoint's cleared snap to one less than the given snap, so that the two breakpoints do not
-	 * breakpoints do not overlap.
+	 * overlap.
 	 * 
 	 * <p>
 	 * Note the following special cases: 1) If the given snap is equal to the placed snap, this
-	 * method behaves like {@link #setEnabled(boolean)} and return this breakpoint. 2) If "enabled"
+	 * method simply sets the fields on this breakpoint and returns this. 2) If the field values
-	 * indicates no change, this method does nothing and returns this breakpoint.
+	 * indicate no change, this method does nothing and returns this breakpoint.
 	 * 
 	 * @implNote Listeners on breakpoint changes will see the added record before the lifespan
 	 *           change of the old record, despite those two records having the same path and
@@ -150,9 +151,10 @@ public interface TraceBreakpoint {
 	 * 
 	 * @param snap the placed snap for the later breakpoint
 	 * @param enabled true if the later breakpoint is enabled, false if disabled
+	 * @param kinds the kinds of the later breakpoint
 	 * @return the new breakpoint, or this breakpoint (see special case)
 	 */
-	TraceBreakpoint splitWithEnabled(long snap, boolean enabled);
+	TraceBreakpoint splitAndSet(long snap, boolean enabled, Collection<TraceBreakpointKind> kinds);
 
 	/**
 	 * Set whether this breakpoint was enabled or disabled
@@ -160,7 +162,7 @@ public interface TraceBreakpoint {
 	 * <p>
 	 * This change applies to the entire lifespan of this record. If a breakpoint is enabled for
 	 * some duration and then later disabled, this breakpoint should be split instead. See
-	 * {@link #splitWithEnabled(long,boolean)}.
+	 * {@link #splitAndSet(long,boolean, Collection)}.
 	 * 
 	 * @param enabled true if enabled, false if disabled
 	 */
@@ -173,12 +175,25 @@ public interface TraceBreakpoint {
 	 */
 	boolean isEnabled();
 
+	/**
+	 * Set the kinds included in this breakpoint
+	 * 
+	 * <p>
+	 * See {@link #getKinds()}. Note that it is unusual for a breakpoint to change kinds during its
+	 * life. Nevertheless, in the course of recording a trace, it may happen, or at least appear to
+	 * happen. Rather than require the client to delete and re-create the breakpoint, this allows
+	 * the record to be updated. See also {@link #splitAndSet(long, boolean, Collection)}.
+	 * 
+	 * @param kinds the set of kinds
+	 */
+	void setKinds(Collection<TraceBreakpointKind> kinds);
+
 	/**
 	 * Get the kinds included in this breakpoint
 	 * 
 	 * <p>
 	 * For example, an "access breakpoint" or "access watchpoint," depending on terminology, would
-	 * have include both {@link TraceBreakpointKind#READ} and {@link TraceBreakpointKind#WRITE}.
+	 * include both {@link TraceBreakpointKind#READ} and {@link TraceBreakpointKind#WRITE}.
 	 * 
 	 * @return the set of kinds
 	 */