Skip to content

Commit

Permalink
GP-4334: Remove 'Synchronize Target Activation' toggle. Prohibit time…
Browse files Browse the repository at this point in the history 
… navigation in Target mode.
  • Loading branch information
@nsadeveloper789
nsadeveloper789 committed Feb 20, 2024
1 parent f5008f9 commit 5f7df08
Show file tree
Hide file tree
Showing 7 changed files with 72 additions and 230 deletions.
30 changes: 1 addition & 29 deletions Ghidra/Debug/Debugger-api/src/main/java/ghidra/app/services/DebuggerTraceManagerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ default void activate(DebuggerCoordinates coordinates) {
*
* <p>
* 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
Expand Down Expand Up @@ -484,34 +484,6 @@ default void activateObject(TraceObject object) {
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
*
Expand Down
89 changes: 32 additions & 57 deletions Ghidra/Debug/Debugger-api/src/main/java/ghidra/debug/api/control/ControlMode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,23 +90,10 @@ public boolean useEmulatedBreakpoints() {
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.
Expand Down Expand Up @@ -159,23 +146,10 @@ public boolean useEmulatedBreakpoints() {
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,
Expand Down Expand Up @@ -391,6 +365,38 @@ private ControlMode(String name, Icon icon) {
*/
public abstract boolean followsPresent();

/**
* Validate and/or adjust the given coordinates pre-activation
*
* <p>
* 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
*
Expand Down Expand Up @@ -444,37 +450,6 @@ public boolean isSelectable(DebuggerCoordinates coordinates) {
return true;
}

/**
* If the mode can no longer be selected for new coordinates, get the new mode
*
* <p>
* 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
*
* <p>
* 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
*
Expand Down
29 changes: 17 additions & 12 deletions ...er/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,8 @@ protected DataType resolveSelection(DataType dataType) {
@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;
Expand Down Expand Up @@ -847,15 +849,7 @@ void writeRegisterValue(RegisterValue rv) {
CompletableFuture<Void> 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;
Expand Down Expand Up @@ -1278,9 +1272,7 @@ protected CompletableFuture<Void> readRegistersIfLiveAndAccessible() {
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);
}
Expand All @@ -1301,4 +1293,17 @@ public void readDataState(SaveState saveState) {
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);
}
}
}
49 changes: 2 additions & 47 deletions ...bugger/src/main/java/ghidra/app/plugin/core/debug/gui/thread/DebuggerThreadsProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,10 @@

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;
Expand Down Expand Up @@ -97,18 +94,14 @@ private void snapDeleted() {
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;
Expand All @@ -118,15 +111,8 @@ private void snapDeleted() {
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;
Expand All @@ -149,17 +135,7 @@ public DebuggerThreadsProvider(final DebuggerThreadsPlugin plugin) {

@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();
}

Expand Down Expand Up @@ -242,27 +218,6 @@ protected void buildMainPanel() {
}

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
Expand Down
31 changes: 2 additions & 29 deletions .../main/java/ghidra/app/plugin/core/debug/service/control/DebuggerControlServicePlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@
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.*;
Expand All @@ -46,7 +46,6 @@
status = PluginStatus.RELEASED,
eventsConsumed = {
TraceOpenedPluginEvent.class,
TraceActivatedPluginEvent.class,
TraceClosedPluginEvent.class,
},
servicesRequired = {
Expand Down Expand Up @@ -263,29 +262,6 @@ public StateEditingMemoryHandler createStateEditor(TraceProgramView view) {
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) {
Expand Down Expand Up @@ -346,9 +322,6 @@ public void processEvent(PluginEvent event) {
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());
}
Expand Down
Loading

0 comments on commit 5f7df08

Please sign in to comment.