GP-1008: Fixed debugger table issues.

This commit is contained in:
Dan 2021-06-04 16:10:59 -04:00
parent 10990723b1
commit 60a68e64e6
14 changed files with 154 additions and 61 deletions

View file

@ -768,7 +768,6 @@ public class DebuggerBreakpointsProvider extends ComponentProviderAdapter
@Override
public void breakpointsRemoved(Collection<LogicalBreakpoint> clb) {
Swing.runIfSwingOrRunLater(() -> {
Msg.debug(this, "LBs removed: " + clb);
breakpointTableModel.deleteAllItems(clb);
contextChanged();
});

View file

@ -34,6 +34,11 @@ public class LogicalBreakpointRow {
this.lb = lb;
}
@Override
public String toString() {
return "<Row " + lb + ">";
}
public LogicalBreakpoint getLogicalBreakpoint() {
return lb;
}

View file

@ -52,27 +52,26 @@ import ghidra.util.Swing;
import ghidra.util.datastruct.CollectionChangeListener;
import ghidra.util.datastruct.ListenerSet;
@PluginInfo( //
shortDescription = "Debugger logical breakpoints service plugin", //
description = "Aggregates breakpoints from open programs and live traces", //
category = PluginCategoryNames.DEBUGGER, //
packageName = DebuggerPluginPackage.NAME, //
status = PluginStatus.RELEASED, //
eventsConsumed = { //
ProgramOpenedPluginEvent.class, //
ProgramClosedPluginEvent.class, //
TraceOpenedPluginEvent.class, //
TraceClosedPluginEvent.class, //
}, //
servicesRequired = { //
DebuggerTraceManagerService.class, //
DebuggerModelService.class, //
DebuggerStaticMappingService.class, //
}, //
servicesProvided = { //
DebuggerLogicalBreakpointService.class, //
} //
)
@PluginInfo(
shortDescription = "Debugger logical breakpoints service plugin",
description = "Aggregates breakpoints from open programs and live traces",
category = PluginCategoryNames.DEBUGGER,
packageName = DebuggerPluginPackage.NAME,
status = PluginStatus.RELEASED,
eventsConsumed = {
ProgramOpenedPluginEvent.class,
ProgramClosedPluginEvent.class,
TraceOpenedPluginEvent.class,
TraceClosedPluginEvent.class,
},
servicesRequired = {
DebuggerTraceManagerService.class,
DebuggerModelService.class,
DebuggerStaticMappingService.class,
},
servicesProvided = {
DebuggerLogicalBreakpointService.class,
})
public class DebuggerLogicalBreakpointServicePlugin extends Plugin
implements DebuggerLogicalBreakpointService {
@ -154,8 +153,9 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
protected class TrackMappingsListener implements DebuggerStaticMappingChangeListener {
@Override
public void mappingsChanged(Set<Trace> affectedTraces, Set<Program> affectedPrograms) {
Msg.debug(this, "Mappings changed: " + affectedTraces + "," + affectedPrograms);
// Msg.debug(this, "Mappings changed: " + affectedTraces + "," + affectedPrograms);
synchronized (lock) {
// Msg.debug(this, "Processing map change");
Set<Trace> additionalTraces = new HashSet<>(affectedTraces);
Set<Program> additionalPrograms = new HashSet<>(affectedPrograms);
try (RemoveCollector r = new RemoveCollector(changeListeners.fire)) {
@ -209,20 +209,21 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
}
private void objectRestored() {
Msg.debug(this, "Restored: " + info.trace);
// Msg.debug(this, "Restored: " + info.trace);
synchronized (lock) {
info.reloadBreakpoints();
}
}
private void breakpointAdded(TraceBreakpoint breakpoint) {
Msg.debug(this, "Breakpoint added: " + breakpoint);
// Msg.debug(this, "Breakpoint added: " + breakpoint);
if (!breakpoint.getLifespan().contains(info.recorder.getSnap())) {
// NOTE: User/script probably added historical breakpoint
return;
}
try (AddCollector c = new AddCollector(changeListeners.fire)) {
synchronized (lock) {
// Msg.debug(this, "Processing breakpoint add");
info.trackTraceBreakpoint(breakpoint, c, false);
}
}
@ -241,7 +242,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
private void breakpointLifespanChanged(TraceAddressSpace spaceIsNull,
TraceBreakpoint breakpoint, Range<Long> oldSpan, Range<Long> newSpan) {
Msg.debug(this, "Breakpoint span: " + breakpoint);
// Msg.debug(this, "Breakpoint span: " + breakpoint);
// NOTE: User/script probably modified historical breakpoint
boolean isInOld = oldSpan.contains(info.recorder.getSnap());
boolean isInNew = newSpan.contains(info.recorder.getSnap());
@ -265,7 +266,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
}
private void breakpointDeleted(TraceBreakpoint breakpoint) {
Msg.debug(this, "Breakpoint deleted: " + breakpoint);
// Msg.debug(this, "Breakpoint deleted: " + breakpoint);
if (!breakpoint.getLifespan().contains(info.recorder.getSnap())) {
// NOTE: User/script probably removed historical breakpoint
assert false;
@ -295,7 +296,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
}
private void objectRestored() {
Msg.debug(this, "Restored: " + info.program);
// Msg.debug(this, "Restored: " + info.program);
synchronized (lock) {
info.reloadBreakpoints();
// NOTE: logical breakpoints should know which traces are affected
@ -315,8 +316,10 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
}
private void breakpointBookmarkAdded(Bookmark bookmark) {
// Msg.debug(this, "Breakpoint bookmark added: " + bookmark);
try (AddCollector c = new AddCollector(changeListeners.fire)) {
synchronized (lock) {
// Msg.debug(this, "Processing breakpoint bookmark add");
info.trackProgramBreakpoint(bookmark, c);
}
}
@ -821,7 +824,7 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin
}
private void programOpened(Program program) {
Msg.debug(this, "Opened Program: " + program);
// Msg.debug(this, "Opened Program: " + program);
synchronized (lock) {
if (program instanceof TraceProgramView) {
// TODO: Not a good idea for user/script, but not prohibited

View file

@ -44,6 +44,11 @@ public class LoneLogicalBreakpoint implements LogicalBreakpointInternal {
this.justThisTrace = Set.of(recorder.getTrace());
}
@Override
public String toString() {
return String.format("<%s trace=%s>", getClass().getSimpleName(), breaks);
}
@Override
public boolean isEmpty() {
return breaks.isEmpty();

View file

@ -51,7 +51,7 @@ public class MappedLogicalBreakpoint implements LogicalBreakpointInternal {
@Override
public String toString() {
return String.format("<MappedLogicalBreakpoint prog=%s, traces=%s>", progBreak,
return String.format("<%s prog=%s, traces=%s>", getClass().getSimpleName(), progBreak,
traceBreaks.values());
}

View file

@ -191,7 +191,7 @@ public class DefaultBreakpointRecorder implements ManagedBreakpointRecorder {
Collection<TraceBreakpointKind> kinds) {
for (TargetBreakpointLocation bl : bpts) {
String path = PathUtils.toString(bl.getPath());
recorder.parTx.execute("Breakpoint " + path + " toggled", () -> {
recorder.parTx.execute("Breakpoint " + path + " changed", () -> {
TraceBreakpoint traceBpt = recorder.getTraceBreakpoint(bl);
if (traceBpt == null) {
Msg.warn(this, "Cannot find toggled trace breakpoint for " + path);
@ -210,7 +210,7 @@ public class DefaultBreakpointRecorder implements ManagedBreakpointRecorder {
spec.getLocations().thenAccept(bpts -> {
doBreakpointSpecChanged(snap, bpts, enabled, kinds);
}).exceptionally(ex -> {
Msg.error(this, "Error recording toggled breakpoint spec: " + spec.getJoinedPath("."),
Msg.error(this, "Error recording changed breakpoint spec: " + spec.getJoinedPath("."),
ex);
return null;
});

View file

@ -502,7 +502,6 @@ public class DefaultTraceRecorder implements TraceRecorder {
return objectManager.getEventListener();
}
@Override
public ListenerSet<TraceRecorderListener> getListeners() {
return objectManager.getListeners();
}
@ -564,4 +563,8 @@ public class DefaultTraceRecorder implements TraceRecorder {
return true;
}
@Override
public CompletableFuture<Void> flushTransactions() {
return parTx.flush();
}
}

View file

@ -17,6 +17,7 @@ package ghidra.app.plugin.core.debug.service.model;
import java.util.HashMap;
import java.util.concurrent.*;
import java.util.stream.Stream;
import org.apache.commons.lang3.concurrent.BasicThreadFactory;
@ -74,4 +75,12 @@ public class PermanentTransactionExecutor {
return null;
});
}
public CompletableFuture<Void> flush() {
Runnable nop = () -> {
};
return CompletableFuture.allOf(Stream.of(threads)
.map(t -> CompletableFuture.runAsync(nop, t))
.toArray(CompletableFuture[]::new));
}
}

View file

@ -19,7 +19,6 @@ import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.CompletableFuture;
import ghidra.app.services.TraceRecorder;
import ghidra.async.AsyncLazyMap;
import ghidra.async.AsyncLazyMap.KeyedFuture;
import ghidra.async.AsyncUtils;
@ -32,7 +31,7 @@ import ghidra.util.TriConsumer;
public class RecorderComposedRegisterSet {
private TraceRecorder recorder;
private DefaultTraceRecorder recorder;
protected final TriConsumer<Boolean, Boolean, Void> listenerRegAccChanged =
this::registerAccessibilityChanged;
@ -61,7 +60,7 @@ public class RecorderComposedRegisterSet {
});
}
public RecorderComposedRegisterSet(TraceRecorder recorder) {
public RecorderComposedRegisterSet(DefaultTraceRecorder recorder) {
this.recorder = recorder;
}

View file

@ -457,7 +457,7 @@ public class DebuggerStaticMappingServicePlugin extends Plugin
}
private void staticMappingAdded(TraceStaticMapping mapping) {
Msg.debug(this, "Trace Mapping added: " + mapping);
// Msg.debug(this, "Trace Mapping added: " + mapping);
synchronized (lock) {
MappingEntry me = new MappingEntry(mapping);
putOutboundAndInboundEntries(me);

View file

@ -40,7 +40,6 @@ import ghidra.trace.model.stack.TraceStackFrame;
import ghidra.trace.model.thread.TraceThread;
import ghidra.trace.model.time.TraceSnapshot;
import ghidra.trace.model.time.TraceTimeManager;
import ghidra.util.datastruct.ListenerSet;
import ghidra.util.task.TaskMonitor;
/**
@ -480,5 +479,16 @@ public interface TraceRecorder {
@Internal
TraceEventListener getListenerForRecord();
ListenerSet<TraceRecorderListener> getListeners();
/**
* Wait for pending transactions finish execution.
*
* <p>
* The returned future will complete when queued transactions have been executed. There are no
* guarantees regarding transactions submitted after this future is returned. Furthermore, it
* may still be necessary to wait for the trace to finish invoking its domain object change
* listeners.
*
* @return the future which completes when pending transactions finish execution.
*/
CompletableFuture<Void> flushTransactions();
}

View file

@ -36,6 +36,7 @@ import ghidra.app.services.*;
import ghidra.dbg.model.TestDebuggerModelBuilder;
import ghidra.dbg.target.TargetBreakpointSpec.TargetBreakpointKind;
import ghidra.dbg.target.TargetBreakpointSpecContainer;
import ghidra.dbg.target.TargetTogglable;
import ghidra.dbg.testutil.DebuggerModelTestUtils;
import ghidra.program.model.address.Address;
import ghidra.program.model.listing.Program;
@ -44,6 +45,7 @@ import ghidra.test.ToyProgramBuilder;
import ghidra.trace.model.DefaultTraceLocation;
import ghidra.trace.model.Trace;
import ghidra.trace.model.breakpoint.TraceBreakpoint;
import ghidra.util.Msg;
import ghidra.util.database.UndoableTransaction;
import ghidra.util.task.TaskMonitor;
import help.screenshot.GhidraScreenShotGenerator;
@ -86,6 +88,17 @@ public class DebuggerBreakpointsPluginScreenShots extends GhidraScreenShotGenera
@After
public void tearDownMine() {
Msg.debug(this, "Tearing down");
Msg.debug(this, "Service breakpoints:");
for (LogicalBreakpoint lb : breakpointService.getAllBreakpoints()) {
Msg.debug(this, " bp: " + lb);
}
DebuggerBreakpointsProvider provider =
waitForComponentProvider(DebuggerBreakpointsProvider.class);
Msg.debug(this, "Provider breakpoints:");
for (LogicalBreakpointRow row : provider.breakpointTableModel.getModelData()) {
Msg.debug(this, " bp: " + row.getLogicalBreakpoint());
}
if (program != null) {
program.release(this);
}
@ -112,9 +125,9 @@ public class DebuggerBreakpointsPluginScreenShots extends GhidraScreenShotGenera
traceManager.openTrace(trace1);
traceManager.openTrace(trace3);
mb.testProcess1.addRegion("echo:.text", mb.rng(0x00400000, 0x0040fff), "rx");
mb.testProcess1.addRegion("echo:.data", mb.rng(0x00600000, 0x0060fff), "rw");
mb.testProcess3.addRegion("echo:.text", mb.rng(0x7fac0000, 0x7facfff), "rx");
mb.testProcess1.addRegion("echo:.text", mb.rng(0x00400000, 0x00400fff), "rx");
mb.testProcess1.addRegion("echo:.data", mb.rng(0x00600000, 0x00600fff), "rw");
mb.testProcess3.addRegion("echo:.text", mb.rng(0x7fac0000, 0x7fac0fff), "rx");
try (UndoableTransaction tid = UndoableTransaction.start(trace1, "Add mapping", true)) {
mappingService.addMapping(
@ -137,14 +150,13 @@ public class DebuggerBreakpointsPluginScreenShots extends GhidraScreenShotGenera
waitFor(() -> Unique.assertAtMostOne(recorder3.collectBreakpointContainers(null)),
"No container");
waitOn(bc3.placeBreakpoint(mb.addr(0x7fac1234), Set.of(TargetBreakpointKind.SW_EXECUTE)));
TargetTogglable bp3 = (TargetTogglable) waitForValue(
() -> Unique.assertAtMostOne(bc3.getCachedElements().values()));
waitOn(bp3.disable());
TraceBreakpoint bpt = waitForValue(() -> Unique.assertAtMostOne(
trace3.getBreakpointManager()
.getBreakpointsAt(recorder3.getSnap(), addr(trace3, 0x7fac1234))));
try (UndoableTransaction tid =
UndoableTransaction.start(trace3, "Disable breakpoint", true)) {
bpt.setEnabled(false);
}
try (UndoableTransaction tid = UndoableTransaction.start(program, "Add breakpoint", true)) {
program.getBookmarkManager()
@ -156,9 +168,20 @@ public class DebuggerBreakpointsPluginScreenShots extends GhidraScreenShotGenera
}
waitForPass(() -> {
assertEquals(3, breakpointService.getAllBreakpoints().size());
Set<LogicalBreakpoint> allBreakpoints = breakpointService.getAllBreakpoints();
assertEquals(3, allBreakpoints.size());
});
waitForPass(() -> {
assertFalse(bpt.isEnabled());
});
/**
* TODO: Might be necessary to debounce and wait for service callbacks to settle. Sometimes,
* there are 3 for just a moment, and then additional callbacks mess things up.
*/
waitForPass(() -> {
assertEquals(3, provider.breakpointTable.getRowCount());
assertEquals(3, provider.locationTable.getRowCount());
});
captureIsolatedProvider(provider, 600, 600);
}

View file

@ -110,6 +110,7 @@ public class DBTraceBreakpoint
}
}
enabled = (flagsByte & ENABLED_MASK) != 0;
// Msg.debug(this, "trace: breakpoint " + this + " enabled=" + enabled + ", because doFresh");
}
@Override
@ -154,6 +155,7 @@ public class DBTraceBreakpoint
this.comment = comment;
update(PATH_COLUMN, NAME_COLUMN, THREADS_COLUMN, FLAGS_COLUMN, COMMENT_COLUMN);
this.enabled = enabled;
// Msg.debug(this, "trace: breakpoint " + this + " enabled=" + enabled + ", because set");
}
public void set(String path, String name, long[] threadKeys,
@ -342,11 +344,15 @@ public class DBTraceBreakpoint
this.kinds.clear();
this.kinds.addAll(kinds);
this.enabled = enabled;
// Msg.debug(this,
// "trace: breakpoint " + this + " enabled=" + enabled + ", because doSetFlags");
update(FLAGS_COLUMN);
}
protected void doSetEnabled(boolean enabled) {
this.enabled = enabled;
// Msg.debug(this,
// "trace: breakpoint " + this + " enabled=" + enabled + ", because doSetEnabled");
if (enabled) {
flagsByte |= ENABLED_MASK;
}

View file

@ -21,6 +21,7 @@ import java.util.stream.Collectors;
import java.util.stream.Stream;
import docking.widgets.table.DefaultEnumeratedColumnTableModel.EnumeratedTableColumn;
import ghidra.util.Msg;
/**
* A table model where the columns are enumerated, and the rows are wrappers on the objects being
@ -44,11 +45,16 @@ public class RowWrappedEnumeratedColumnTableModel<C extends Enum<C> & Enumerated
this.wrapper = wrapper;
}
protected synchronized R rowFor(T t) {
return map.computeIfAbsent(keyFunc.apply(t), k -> wrapper.apply(t));
protected synchronized R addRowFor(T t) {
R row = wrapper.apply(t);
R exists = map.put(keyFunc.apply(t), row);
if (exists != null) {
Msg.warn(this, "Replaced existing row! row=" + exists);
}
return row;
}
protected synchronized R delFor(T t) {
protected synchronized R delRowFor(T t) {
return delKey(keyFunc.apply(t));
}
@ -56,50 +62,75 @@ public class RowWrappedEnumeratedColumnTableModel<C extends Enum<C> & Enumerated
return map.remove(k);
}
protected synchronized List<R> rowsFor(Stream<? extends T> s) {
return s.map(this::rowFor).collect(Collectors.toList());
protected synchronized List<R> addRowsFor(Stream<? extends T> s) {
return s.map(this::addRowFor).collect(Collectors.toList());
}
protected synchronized List<R> rowsFor(Collection<? extends T> c) {
return rowsFor(c.stream());
protected synchronized List<R> addRowsFor(Collection<? extends T> c) {
return addRowsFor(c.stream());
}
public synchronized R getRow(T t) {
return map.get(keyFunc.apply(t));
}
protected synchronized List<R> getRows(Stream<? extends T> s) {
return s.map(this::getRow).filter(r -> r != null).collect(Collectors.toList());
}
protected synchronized List<R> getRows(Collection<? extends T> c) {
return getRows(c.stream());
}
public synchronized void addItem(T t) {
if (map.containsKey(keyFunc.apply(t))) {
return;
}
add(rowFor(t));
add(addRowFor(t));
}
public synchronized void addAllItems(Collection<? extends T> c) {
Stream<? extends T> s = c.stream().filter(t -> !map.containsKey(keyFunc.apply(t)));
addAll(rowsFor(s));
Stream<? extends T> s = c.stream().filter(t -> {
K k = keyFunc.apply(t);
if (map.containsKey(k)) {
return false;
}
return true;
});
addAll(addRowsFor(s));
}
public void updateItem(T t) {
notifyUpdated(rowFor(t));
R row = getRow(t);
if (row == null) {
return;
}
notifyUpdated(row);
}
public void updateAllItems(Collection<T> c) {
notifyUpdatedWith(rowsFor(c)::contains);
notifyUpdatedWith(getRows(c)::contains);
}
public void deleteItem(T t) {
delete(delFor(t));
R row = delRowFor(t);
if (row == null) {
return;
}
delete(row);
}
public R deleteKey(K k) {
R r = delKey(k);
if (r == null) {
return null;
}
delete(r);
return r;
}
public synchronized void deleteAllItems(Collection<T> c) {
deleteWith(rowsFor(c)::contains);
deleteWith(getRows(c)::contains);
map.keySet().removeAll(c.stream().map(keyFunc).collect(Collectors.toList()));
}