From f1b200fbfdb8dab9bf322f7101f1f79ab7c511f1 Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Tue, 27 Apr 2021 09:40:43 -0400 Subject: [PATCH] GP-896: Recording changes in breakpoint spec kinds --- .../model/DefaultBreakpointRecorder.java | 12 +++-- .../service/model/TraceObjectManager.java | 11 ++-- .../interfaces/ManagedBreakpointRecorder.java | 14 ++++- .../breakpoint/DBTraceBreakpoint.java | 51 +++++++++++++++++-- .../model/breakpoint/TraceBreakpoint.java | 33 ++++++++---- 5 files changed, 98 insertions(+), 23 deletions(-) diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/DefaultBreakpointRecorder.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/DefaultBreakpointRecorder.java index dd714c115f..46eda1ff07 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/DefaultBreakpointRecorder.java +++ b/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, - Collection bpts, boolean enabled) { + protected void doBreakpointSpecChanged(long snap, + Collection bpts, boolean enabled, + Collection 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 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); diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectManager.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectManager.java index d855c4b27a..a7c0dce570 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/TraceObjectManager.java +++ b/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 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); - recorder.breakpointRecorder.breakpointToggled(spec, enabled); + boolean enabled = spec.isEnabled(); + Set traceKinds = + TraceRecorder.targetToTraceBreakpointKinds(spec.getKinds()); + recorder.breakpointRecorder.breakpointSpecChanged(spec, enabled, traceKinds); } } diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/interfaces/ManagedBreakpointRecorder.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/interfaces/ManagedBreakpointRecorder.java index a36d94ccbd..6c772fdcf0 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/model/interfaces/ManagedBreakpointRecorder.java +++ b/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 kinds); } diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/breakpoint/DBTraceBreakpoint.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/breakpoint/DBTraceBreakpoint.java index fed07b8555..832588ff53 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/breakpoint/DBTraceBreakpoint.java +++ b/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 kinds) { DBTraceBreakpoint that; Range oldLifespan; Range 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 kinds) { + byte flags = 0; + for (TraceBreakpointKind k : kinds) { + flags |= k.getBits(); + } + if (enabled) { + flags |= ENABLED_MASK; + } + return flags; + } + + protected void doSetFlags(boolean enabled, Collection 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 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 kinds) { + try (LockHold hold = LockHold.lock(space.lock.writeLock())) { + doSetKinds(kinds); + } + space.trace.setChanged( + new TraceChangeRecord<>(TraceBreakpointChangeType.CHANGED, space, this)); + } + @Override public Set getKinds() { try (LockHold hold = LockHold.lock(space.lock.readLock())) { diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/model/breakpoint/TraceBreakpoint.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/model/breakpoint/TraceBreakpoint.java index 3e2d025f29..38ebdd06ba 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/model/breakpoint/TraceBreakpoint.java +++ b/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. * *

* 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, - * it sets this breakpoint's cleared snap to one less than the given snap, so that the two - * breakpoints do not overlap. + * this breakpoint, replacing the copy's placed snap and additional fields. Then, it sets this + * breakpoint's cleared snap to one less than the given snap, so that the two breakpoints do not + * overlap. * *

* 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" - * indicates no change, this method does nothing and returns this breakpoint. + * method simply sets the fields on this breakpoint and returns this. 2) If the field values + * 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 kinds); /** * Set whether this breakpoint was enabled or disabled @@ -160,7 +162,7 @@ public interface TraceBreakpoint { *

* 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 + * + *

+ * 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 kinds); + /** * Get the kinds included in this breakpoint * *

* 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 */