From 5f7df08b676cffbef2deba685c6757b050f3ea71 Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Tue, 20 Feb 2024 12:40:40 -0500 Subject: [PATCH] GP-4334: Remove 'Synchronize Target Activation' toggle. Prohibit time navigation in Target mode. --- .../services/DebuggerTraceManagerService.java | 30 +------ .../ghidra/debug/api/control/ControlMode.java | 89 +++++++------------ .../register/DebuggerRegistersProvider.java | 29 +++--- .../gui/thread/DebuggerThreadsProvider.java | 49 +--------- .../control/DebuggerControlServicePlugin.java | 31 +------ .../DebuggerTraceManagerServicePlugin.java | 57 ++++-------- .../DebuggerTraceManagerServiceTest.java | 17 ---- 7 files changed, 72 insertions(+), 230 deletions(-) diff --git a/Ghidra/Debug/Debugger-api/src/main/java/ghidra/app/services/DebuggerTraceManagerService.java b/Ghidra/Debug/Debugger-api/src/main/java/ghidra/app/services/DebuggerTraceManagerService.java index a78c3ab4ab..3b8c369845 100644 --- a/Ghidra/Debug/Debugger-api/src/main/java/ghidra/app/services/DebuggerTraceManagerService.java +++ b/Ghidra/Debug/Debugger-api/src/main/java/ghidra/app/services/DebuggerTraceManagerService.java @@ -303,7 +303,7 @@ public interface DebuggerTraceManagerService { * *

* If asynchronous notification is needed, use - * {@link #activateAndNotify(DebuggerCoordinates, boolean)}. + * {@link #activateAndNotify(DebuggerCoordinates, ActivationCause)}. * * @param coordinates the desired coordinates * @param cause the cause of activation @@ -484,34 +484,6 @@ public interface DebuggerTraceManagerService { activate(resolveObject(object)); } - /** - * Control whether trace activation is synchronized with debugger activation - * - * @param enabled true to synchronize, false otherwise - */ - void setSynchronizeActive(boolean enabled); - - /** - * Check whether trace activation is synchronized with debugger activation - * - * @return true if synchronized, false otherwise - */ - boolean isSynchronizeActive(); - - /** - * Add a listener for changes to activation synchronization enablement - * - * @param listener the listener to receive change notifications - */ - void addSynchronizeActiveChangeListener(BooleanChangeAdapter listener); - - /** - * Remove a listener for changes to activation synchronization enablement - * - * @param listener the listener receiving change notifications - */ - void removeSynchronizeActiveChangeListener(BooleanChangeAdapter listener); - /** * Control whether traces should be saved by default * diff --git a/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/control/ControlMode.java b/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/control/ControlMode.java index 823bb8be77..45e9b4dfcf 100644 --- a/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/control/ControlMode.java +++ b/Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/control/ControlMode.java @@ -90,23 +90,10 @@ public enum ControlMode { return false; } - @Override - public ControlMode modeOnChange(DebuggerCoordinates coordinates) { - if (coordinates.isAliveAndPresent()) { - return this; - } - return getAlternative(coordinates); - } - @Override public boolean isSelectable(DebuggerCoordinates coordinates) { return coordinates.isAlive(); } - - @Override - public ControlMode getAlternative(DebuggerCoordinates coordinates) { - return RO_TRACE; - } }, /** * Control actions, breakpoint commands, and state edits are all directed to the target. @@ -159,23 +146,10 @@ public enum ControlMode { return false; } - @Override - public ControlMode modeOnChange(DebuggerCoordinates coordinates) { - if (coordinates.isAliveAndPresent()) { - return this; - } - return getAlternative(coordinates); - } - @Override public boolean isSelectable(DebuggerCoordinates coordinates) { return coordinates.isAlive(); } - - @Override - public ControlMode getAlternative(DebuggerCoordinates coordinates) { - return RW_EMULATOR; - } }, /** * Control actions activate trace snapshots, breakpoint commands are directed to the emulator, @@ -391,6 +365,38 @@ public enum ControlMode { */ public abstract boolean followsPresent(); + /** + * Validate and/or adjust the given coordinates pre-activation + * + *

+ * This is called by the trace manager whenever there is a request to activate new coordinates. + * The control mode may adjust or reject the request before the trace manager actually performs + * and notifies the activation. + * + * @param tool the tool for displaying status messages + * @param coordinates the requested coordinates + * @param cause the cause of the activation + * @return the effective coordinates or null to reject + */ + public DebuggerCoordinates validateCoordinates(PluginTool tool, + DebuggerCoordinates coordinates, ActivationCause cause) { + if (!followsPresent()) { + return coordinates; + } + Target target = coordinates.getTarget(); + if (target == null) { + return coordinates; + } + if (cause == ActivationCause.USER && + (!coordinates.getTime().isSnapOnly() || coordinates.getSnap() != target.getSnap())) { + tool.setStatusInfo( + "Cannot navigate time in %s mode. Switch to Trace or Emulate mode first." + .formatted(name), + true); + } + return coordinates.snap(target.getSnap()); + } + /** * Check if (broadly speaking) the mode supports editing the given coordinates * @@ -444,37 +450,6 @@ public enum ControlMode { return true; } - /** - * If the mode can no longer be selected for new coordinates, get the new mode - * - *

- * For example, if a target terminates while the mode is {@link #RO_TARGET}, this specifies the - * new mode. - * - * @param coordinates the new coordinates - * @return the new mode - */ - public ControlMode getAlternative(DebuggerCoordinates coordinates) { - throw new AssertionError("INTERNAL: Non-selectable mode must provide alternative"); - } - - /** - * Find the new mode (or same) mode when activating the given coordinates - * - *

- * The default is implemented using {@link #isSelectable(DebuggerCoordinates)} followed by - * {@link #getAlternative(DebuggerCoordinates)}. - * - * @param coordinates the new coordinates - * @return the mode - */ - public ControlMode modeOnChange(DebuggerCoordinates coordinates) { - if (isSelectable(coordinates)) { - return this; - } - return getAlternative(coordinates); - } - /** * Indicates whether this mode controls the target * diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java index 298fdce36a..bf7ba68498 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java @@ -480,6 +480,8 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter @AutoServiceConsumed private DebuggerControlService controlService; @AutoServiceConsumed + private DebuggerConsoleService consoleService; + @AutoServiceConsumed private MarkerService markerService; // TODO: Mark address types (separate plugin?) @SuppressWarnings("unused") private final AutoService.Wiring autoServiceWiring; @@ -847,15 +849,7 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter CompletableFuture future = editor.setRegister(rv); future.exceptionally(ex -> { ex = AsyncUtils.unwrapThrowable(ex); - if (ex instanceof DebuggerModelAccessException) { - Msg.error(this, "Could not write target register", ex); - plugin.getTool() - .setStatusInfo("Could not write target register: " + ex.getMessage()); - } - else { - Msg.showError(this, getComponent(), "Edit Register", - "Could not write target register", ex); - } + reportError("Edit Register", "Could not write target register", ex); return null; }); return; @@ -1278,9 +1272,7 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter current.getThread(), current.getFrame(), registers); return future.exceptionally(ex -> { ex = AsyncUtils.unwrapThrowable(ex); - String msg = "Could not read target registers for selected thread: " + ex.getMessage(); - Msg.info(this, msg); - plugin.getTool().setStatusInfo(msg); + reportError(null, "Could not read target registers for selected thread", ex); return ExceptionUtils.rethrow(ex); }).thenApply(__ -> null); } @@ -1301,4 +1293,17 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter public DebuggerCoordinates getCurrent() { return current; } + + private void reportError(String title, String message, Throwable ex) { + plugin.getTool().setStatusInfo(message + ": " + ex.getMessage()); + if (title != null && !(ex instanceof DebuggerModelAccessException)) { + Msg.showError(this, getComponent(), title, message, ex); + } + else if (consoleService != null) { + consoleService.log(DebuggerResources.ICON_LOG_ERROR, message, ex); + } + else { + Msg.error(this, message, ex); + } + } } diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/thread/DebuggerThreadsProvider.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/thread/DebuggerThreadsProvider.java index f2b146ab23..216f208c81 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/thread/DebuggerThreadsProvider.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/thread/DebuggerThreadsProvider.java @@ -25,13 +25,10 @@ import org.apache.commons.lang3.ArrayUtils; import docking.ActionContext; import docking.WindowPosition; -import docking.action.*; +import docking.action.DockingActionIf; import ghidra.app.plugin.core.debug.DebuggerPluginPackage; import ghidra.app.plugin.core.debug.gui.DebuggerResources; -import ghidra.app.plugin.core.debug.gui.DebuggerResources.SynchronizeTargetAction; -import ghidra.app.plugin.core.debug.gui.DebuggerResources.ToToggleSelectionListener; import ghidra.app.services.*; -import ghidra.app.services.DebuggerTraceManagerService.BooleanChangeAdapter; import ghidra.debug.api.tracemgr.DebuggerCoordinates; import ghidra.framework.model.DomainObjectChangeRecord; import ghidra.framework.model.DomainObjectEvent; @@ -97,18 +94,14 @@ public class DebuggerThreadsProvider extends ComponentProviderAdapter { final DebuggerThreadsPlugin plugin; DebuggerCoordinates current = DebuggerCoordinates.NOWHERE; - Trace currentTrace; // Copy for transition @AutoServiceConsumed DebuggerTargetService targetService; - // @AutoServiceConsumed by method + // @AutoServiceConsumed // via method private DebuggerTraceManagerService traceManager; @SuppressWarnings("unused") private final AutoService.Wiring autoServiceWiring; - private final BooleanChangeAdapter synchronizeTargetChangeListener = - this::changedSynchronizeTarget; - private final ForSnapsListener forSnapsListener = new ForSnapsListener(); private JPanel mainPanel; @@ -118,15 +111,8 @@ public class DebuggerThreadsProvider extends ComponentProviderAdapter { DebuggerThreadsPanel panel; DebuggerLegacyThreadsPanel legacyPanel; - DockingAction actionSaveTrace; - // TODO: This should probably be moved to ModelProvider - ToggleDockingAction actionSyncTarget; - ActionContext myActionContext; - // strong ref - ToToggleSelectionListener toToggleSelectionListener; - public DebuggerThreadsProvider(final DebuggerThreadsPlugin plugin) { super(plugin.getTool(), DebuggerResources.TITLE_PROVIDER_THREADS, plugin.getName()); this.plugin = plugin; @@ -149,17 +135,7 @@ public class DebuggerThreadsProvider extends ComponentProviderAdapter { @AutoServiceConsumed public void setTraceManager(DebuggerTraceManagerService traceManager) { - if (this.traceManager != null) { - this.traceManager - .removeSynchronizeActiveChangeListener(synchronizeTargetChangeListener); - } this.traceManager = traceManager; - if (traceManager != null) { - traceManager.addSynchronizeActiveChangeListener(synchronizeTargetChangeListener); - if (actionSyncTarget != null) { - actionSyncTarget.setSelected(traceManager.isSynchronizeActive()); - } - } contextChanged(); } @@ -242,27 +218,6 @@ public class DebuggerThreadsProvider extends ComponentProviderAdapter { } protected void createActions() { - actionSyncTarget = SynchronizeTargetAction.builder(plugin) - .selected(traceManager != null && traceManager.isSynchronizeActive()) - .enabledWhen(c -> traceManager != null) - .onAction(c -> toggleSyncFocus(actionSyncTarget.isSelected())) - .buildAndInstallLocal(this); - traceManager.addSynchronizeActiveChangeListener( - toToggleSelectionListener = new ToToggleSelectionListener(actionSyncTarget)); - } - - private void changedSynchronizeTarget(boolean value) { - if (actionSyncTarget == null || actionSyncTarget.isSelected()) { - return; - } - actionSyncTarget.setSelected(value); - } - - private void toggleSyncFocus(boolean enabled) { - if (traceManager == null) { - return; - } - traceManager.setSynchronizeActive(enabled); } @Override diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/control/DebuggerControlServicePlugin.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/control/DebuggerControlServicePlugin.java index 0ed085db2b..50e8942ac0 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/control/DebuggerControlServicePlugin.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/control/DebuggerControlServicePlugin.java @@ -22,9 +22,9 @@ import java.util.concurrent.*; import ghidra.app.plugin.PluginCategoryNames; import ghidra.app.plugin.core.debug.AbstractDebuggerPlugin; import ghidra.app.plugin.core.debug.DebuggerPluginPackage; -import ghidra.app.plugin.core.debug.event.*; +import ghidra.app.plugin.core.debug.event.TraceClosedPluginEvent; +import ghidra.app.plugin.core.debug.event.TraceOpenedPluginEvent; import ghidra.app.services.*; -import ghidra.app.services.DebuggerTraceManagerService.ActivationCause; import ghidra.debug.api.control.ControlMode; import ghidra.debug.api.tracemgr.DebuggerCoordinates; import ghidra.framework.plugintool.*; @@ -46,7 +46,6 @@ import ghidra.util.datastruct.ListenerSet; status = PluginStatus.RELEASED, eventsConsumed = { TraceOpenedPluginEvent.class, - TraceActivatedPluginEvent.class, TraceClosedPluginEvent.class, }, servicesRequired = { @@ -263,29 +262,6 @@ public class DebuggerControlServicePlugin extends AbstractDebuggerPlugin return new FollowsViewStateEditor(view); } - protected void coordinatesActivated(DebuggerCoordinates coordinates, ActivationCause cause) { - if (cause != ActivationCause.USER) { - return; - } - Trace trace = coordinates.getTrace(); - if (trace == null) { - return; - } - ControlMode oldMode; - ControlMode newMode; - synchronized (currentModes) { - oldMode = currentModes.getOrDefault(trace, ControlMode.DEFAULT); - newMode = oldMode.modeOnChange(coordinates); - if (newMode != oldMode) { - currentModes.put(trace, newMode); - } - } - if (newMode != oldMode) { - listeners.invoke().modeChanged(trace, newMode); - tool.contextChanged(null); - } - } - protected void installMemoryEditor(TraceProgramView view) { TraceProgramViewMemory memory = view.getMemory(); if (memory.getLiveMemoryHandler() != null) { @@ -346,9 +322,6 @@ public class DebuggerControlServicePlugin extends AbstractDebuggerPlugin if (event instanceof TraceOpenedPluginEvent ev) { installAllMemoryEditors(ev.getTrace()); } - else if (event instanceof TraceActivatedPluginEvent ev) { - coordinatesActivated(ev.getActiveCoordinates(), ev.getCause()); - } else if (event instanceof TraceClosedPluginEvent ev) { uninstallAllMemoryEditors(ev.getTrace()); } diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java index 576f9e86ce..e71cd90dd0 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServicePlugin.java @@ -253,7 +253,7 @@ public class DebuggerTraceManagerServicePlugin extends Plugin } DebuggerCoordinates coords = current; TraceObjectKeyPath focus = curTarget.getFocus(); - if (focus != null && synchronizeActive.get()) { + if (focus != null) { coords = coords.path(focus); } coords = coords.snap(curTarget.getSnap()); @@ -274,8 +274,6 @@ public class DebuggerTraceManagerServicePlugin extends Plugin @AutoConfigStateField(codec = BooleanAsyncConfigFieldCodec.class) protected final AsyncReference saveTracesByDefault = new AsyncReference<>(true); @AutoConfigStateField(codec = BooleanAsyncConfigFieldCodec.class) - protected final AsyncReference synchronizeActive = new AsyncReference<>(true); - @AutoConfigStateField(codec = BooleanAsyncConfigFieldCodec.class) protected final AsyncReference autoCloseOnTerminate = new AsyncReference<>(true); // @AutoServiceConsumed via method @@ -569,6 +567,7 @@ public class DebuggerTraceManagerServicePlugin extends Plugin newCurrent = newCurrent.snap(target.getSnap()); } } + newCurrent = validateCoordiantes(newCurrent, cause); if (!doSetCurrent(newCurrent)) { return null; } @@ -583,10 +582,21 @@ public class DebuggerTraceManagerServicePlugin extends Plugin tool.contextChanged(null); } + private ControlMode getEffectiveControlMode(Trace trace) { + if (trace == null) { + return ControlMode.RO_TRACE; + } + return controlService == null ? ControlMode.DEFAULT : controlService.getCurrentMode(trace); + } + + private DebuggerCoordinates validateCoordiantes(DebuggerCoordinates coordinates, + ActivationCause cause) { + ControlMode mode = getEffectiveControlMode(coordinates.getTrace()); + return mode.validateCoordinates(tool, coordinates, cause); + } + private boolean isFollowsPresent(Trace trace) { - ControlMode mode = controlService == null - ? ControlMode.DEFAULT - : controlService.getCurrentMode(trace); + ControlMode mode = getEffectiveControlMode(trace); return mode.followsPresent(); } @@ -628,9 +638,6 @@ public class DebuggerTraceManagerServicePlugin extends Plugin return; } DebuggerCoordinates toActivate = current.target(target); - if (isFollowsPresent(current.getTrace())) { - toActivate = toActivate.snap(target.getSnap()); - } activate(toActivate, ActivationCause.FOLLOW_PRESENT); } @@ -1043,19 +1050,12 @@ public class DebuggerTraceManagerServicePlugin extends Plugin } } - if (!synchronizeActive.get() && cause == ActivationCause.SYNC_MODEL) { - return AsyncUtils.nil(); - } if (cause == ActivationCause.FOLLOW_PRESENT) { if (!isFollowsPresent(newTrace)) { return AsyncUtils.nil(); } if (current.getTrace() != newTrace) { - /** - * The snap needs to match upon re-activating this trace, lest it look like the user - * intentionally navigated to the past. That may cause the control mode to switch - * off of "Target." - */ + // The snap needs to match upon re-activating this trace. try { newTrace.getProgramView().setSnap(coordinates.getViewSnap()); } @@ -1082,7 +1082,7 @@ public class DebuggerTraceManagerServicePlugin extends Plugin return null; }); - if (!synchronizeActive.get() || cause != ActivationCause.USER) { + if (cause != ActivationCause.USER) { return future; } Target target = resolved.getTarget(); @@ -1152,27 +1152,6 @@ public class DebuggerTraceManagerServicePlugin extends Plugin return current.object(object); } - @Override - public void setSynchronizeActive(boolean enabled) { - synchronizeActive.set(enabled, null); - // TODO: Which action to take here, if any? - } - - @Override - public boolean isSynchronizeActive() { - return synchronizeActive.get(); - } - - @Override - public void addSynchronizeActiveChangeListener(BooleanChangeAdapter listener) { - synchronizeActive.addChangeListener(listener); - } - - @Override - public void removeSynchronizeActiveChangeListener(BooleanChangeAdapter listener) { - synchronizeActive.removeChangeListener(listener); - } - @Override public void setSaveTracesByDefault(boolean enabled) { saveTracesByDefault.set(enabled, null); diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServiceTest.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServiceTest.java index e507856185..c9d5830177 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServiceTest.java +++ b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/service/tracemgr/DebuggerTraceManagerServiceTest.java @@ -397,8 +397,6 @@ public class DebuggerTraceManagerServiceTest extends AbstractGhidraHeadedDebugge @Test public void testSynchronizeFocusTraceToModel() throws Throwable { - assertTrue(traceManager.isSynchronizeActive()); - createTestModel(); mb.createTestProcessesAndThreads(); @@ -448,18 +446,10 @@ public class DebuggerTraceManagerServiceTest extends AbstractGhidraHeadedDebugge waitForSwing(); waitForPass(() -> assertEquals(frame0, mb.testModel.session.getFocus())); - - traceManager.setSynchronizeActive(false); - traceManager.activateFrame(1); - waitForSwing(); - - waitForPass(() -> assertEquals(frame0, mb.testModel.session.getFocus())); } @Test public void testSynchronizeFocusModelToTrace() throws Throwable { - assertTrue(traceManager.isSynchronizeActive()); - createTestModel(); mb.createTestProcessesAndThreads(); @@ -503,12 +493,5 @@ public class DebuggerTraceManagerServiceTest extends AbstractGhidraHeadedDebugge waitOn(mb.testModel.session.requestFocus(frame0)); waitForPass(() -> assertEquals(0, traceManager.getCurrentFrame())); - - traceManager.setSynchronizeActive(false); - waitOn(mb.testModel.session.requestFocus(frame1)); - // Not super reliable, but at least wait for it to change in case it does - Thread.sleep(200); - - assertEquals(0, traceManager.getCurrentFrame()); } }