From 6266ecbea499adb25b70a0bfb51ca32b29b45598 Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Thu, 22 Sep 2022 16:58:32 -0400 Subject: [PATCH] GP-2243: Add some status messages when breakpoints don't enable. --- .../DebuggerBreakpointMarkerPlugin.java | 11 ++++- .../DebuggerBreakpointsProvider.java | 21 ++++++++-- .../gui/breakpoint/LogicalBreakpointRow.java | 14 ++++++- ...ebuggerLogicalBreakpointServicePlugin.java | 40 ++++++++++++++++++- .../breakpoint/LoneLogicalBreakpoint.java | 8 ++++ .../breakpoint/MappedLogicalBreakpoint.java | 19 +++++++++ .../DebuggerLogicalBreakpointService.java | 32 +++++++++++++++ .../app/services/LogicalBreakpoint.java | 12 ++++++ 8 files changed, 149 insertions(+), 8 deletions(-) diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointMarkerPlugin.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointMarkerPlugin.java index eeaff15f38..c3d1c31f1b 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointMarkerPlugin.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointMarkerPlugin.java @@ -532,7 +532,12 @@ public class DebuggerBreakpointMarkerPlugin extends Plugin } ProgramLocation location = getLocationFromContext(context); Set col = breakpointService.getBreakpointsAt(location); - breakpointService.enableAll(col, getTraceFromContext(context)).exceptionally(ex -> { + Trace trace = getTraceFromContext(context); + String status = breakpointService.generateStatusEnable(col, trace); + if (status != null) { + tool.setStatusInfo(status, true); + } + breakpointService.enableAll(col, trace).exceptionally(ex -> { breakpointError(NAME, "Could not enable breakpoint", ex); return null; }); @@ -869,6 +874,10 @@ public class DebuggerBreakpointMarkerPlugin extends Plugin if (loc == null) { return; } + String status = breakpointService.generateStatusToggleAt(loc); + if (status != null) { + tool.setStatusInfo(status, true); + } breakpointService.toggleBreakpointsAt(loc, () -> { Set supported = getSupportedKindsFromContext(context); if (supported.isEmpty()) { diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointsProvider.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointsProvider.java index e085a8f41d..97d7197a6b 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointsProvider.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/DebuggerBreakpointsProvider.java @@ -267,7 +267,12 @@ public class DebuggerBreakpointsProvider extends ComponentProviderAdapter DebuggerLogicalBreakpointsActionContext ctx = (DebuggerLogicalBreakpointsActionContext) context; Collection sel = ctx.getBreakpoints(); - breakpointService.enableAll(sel, null).exceptionally(ex -> { + Trace trace = isFilterByCurrentTrace() ? currentTrace : null; + String status = breakpointService.generateStatusEnable(sel, trace); + if (status != null) { + tool.setStatusInfo(status, true); + } + breakpointService.enableAll(sel, trace).exceptionally(ex -> { breakpointError("Enable Breakpoints", "Could not enable breakpoints", ex); return null; }); @@ -309,7 +314,12 @@ public class DebuggerBreakpointsProvider extends ComponentProviderAdapter @Override public void actionPerformed(ActionContext context) { Set all = breakpointService.getAllBreakpoints(); - breakpointService.enableAll(all, null).exceptionally(ex -> { + Trace trace = isFilterByCurrentTrace() ? currentTrace : null; + String status = breakpointService.generateStatusEnable(all, trace); + if (status != null) { + tool.setStatusInfo(status, true); + } + breakpointService.enableAll(all, trace).exceptionally(ex -> { breakpointError("Enable All Breakpoints", "Could not enable breakpoints", ex); return null; }); @@ -996,7 +1006,12 @@ public class DebuggerBreakpointsProvider extends ComponentProviderAdapter new DebuggerBreakpointStateTableCellEditor<>(breakpointFilterPanel) { @Override protected State getToggledState(LogicalBreakpointRow row, State current) { - return current.getToggled(row.isMapped()); + boolean mapped = row.isMapped(); + if (!mapped) { + tool.setStatusInfo( + "Breakpoint has no locations. Only toggling its bookmark.", true); + } + return current.getToggled(mapped); } }); bptEnCol.setMaxWidth(24); diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/LogicalBreakpointRow.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/LogicalBreakpointRow.java index 21124be93e..1656787b39 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/LogicalBreakpointRow.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/breakpoint/LogicalBreakpointRow.java @@ -60,8 +60,14 @@ public class LogicalBreakpointRow { } public void setEnabled(boolean enabled) { + boolean filter = provider.isFilterByCurrentTrace(); if (enabled) { - CompletableFuture future = provider.isFilterByCurrentTrace() + String status = lb.generateStatusEnable(filter ? provider.currentTrace : null); + if (status != null) { + provider.getTool().setStatusInfo(status, true); + } + lb.enableForProgram(); + CompletableFuture future = filter ? lb.enableForTrace(provider.currentTrace) : lb.enable(); future.exceptionally(ex -> { @@ -70,7 +76,8 @@ public class LogicalBreakpointRow { }); } else { - CompletableFuture future = provider.isFilterByCurrentTrace() + lb.disableForProgram(); + CompletableFuture future = filter ? lb.disableForTrace(provider.currentTrace) : lb.disable(); future.exceptionally(ex -> { @@ -139,6 +146,9 @@ public class LogicalBreakpointRow { */ public boolean isMapped() { if (provider.isFilterByCurrentTrace()) { + if (provider.currentTrace == null) { + return false; + } return lb.getMappedTraces().contains(provider.currentTrace); } return !lb.getMappedTraces().isEmpty(); diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/DebuggerLogicalBreakpointServicePlugin.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/DebuggerLogicalBreakpointServicePlugin.java index 93786b7ec4..db59ba4989 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/DebuggerLogicalBreakpointServicePlugin.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/DebuggerLogicalBreakpointServicePlugin.java @@ -1131,15 +1131,25 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin if (trace == null || participants.isEmpty() || participants.equals(Set.of(trace))) { consumerForProgram.accept(lb); } - if (!(lb instanceof LogicalBreakpointInternal)) { + if (!(lb instanceof LogicalBreakpointInternal lbi)) { continue; } - LogicalBreakpointInternal lbi = (LogicalBreakpointInternal) lb; consumerForTarget.accept(actions, lbi); } return actions.execute(); } + @Override + public String generateStatusEnable(Collection col, Trace trace) { + String message; + for (LogicalBreakpoint lb : col) { + if ((message = lb.generateStatusEnable(trace)) != null) { + return message; + } + } + return null; + } + @Override public CompletableFuture enableAll(Collection col, Trace trace) { return actOnAll(col, trace, LogicalBreakpoint::enableForProgram, @@ -1205,6 +1215,32 @@ public class DebuggerLogicalBreakpointServicePlugin extends Plugin }); } + @Override + public String generateStatusToggleAt(ProgramLocation loc) { + Set bs = getBreakpointsAt(loc); + if (bs == null || bs.isEmpty()) { + return null; + } + State state = computeState(bs, loc); + Trace trace = + DebuggerLogicalBreakpointService.programOrTrace(loc, (p, a) -> null, (t, a) -> t); + /** + * TODO: If we have a trace here, then there are mapped breakpoints, no? We should never + * expect a status message in that case. I don't suppose it hurts to check, though, since + * the rules could change later. + */ + boolean mapped = anyMapped(bs, trace); + if (!mapped) { + return "No breakpoint at this location is mapped to a live trace. " + + "Cannot toggle on target. Is there a target? Check your module map."; + } + State toggled = state.getToggled(mapped); + if (!toggled.isEnabled()) { + return null; + } + return generateStatusEnable(bs, trace); + } + @Override public CompletableFuture> toggleBreakpointsAt(ProgramLocation loc, Supplier>> placer) { diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/LoneLogicalBreakpoint.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/LoneLogicalBreakpoint.java index 9312271557..9b7be1ca93 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/LoneLogicalBreakpoint.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/LoneLogicalBreakpoint.java @@ -211,6 +211,14 @@ public class LoneLogicalBreakpoint implements LogicalBreakpointInternal { breaks.planEnable(actions, length, kinds); } + @Override + public String generateStatusEnable(Trace trace) { + if (trace == null || trace == breaks.getTrace()) { + return null; + } + return "A breakpoint is not in this trace. Is there a target? Check your module map."; + } + @Override public CompletableFuture enable() { BreakpointActionSet actions = new BreakpointActionSet(); diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/MappedLogicalBreakpoint.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/MappedLogicalBreakpoint.java index 537a566298..94b2cc3d4f 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/MappedLogicalBreakpoint.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/service/breakpoint/MappedLogicalBreakpoint.java @@ -192,6 +192,25 @@ public class MappedLogicalBreakpoint implements LogicalBreakpointInternal { return enableForTraces(); } + @Override + public String generateStatusEnable(Trace trace) { + if (trace == null) { + for (TraceBreakpointSet breaks : traceBreaks.values()) { + if (!breaks.isEmpty()) { + return null; + } + } + return "A breakpoint is not mapped to any live trace. Cannot enable it on target. " + + "Is there a target? Check your module map."; + } + TraceBreakpointSet breaks = traceBreaks.get(trace); + if (breaks != null && !breaks.isEmpty()) { + return null; + } + return "A breakpoint is not mapped to the trace, or the trace is not live. " + + "Cannot enable it on target. Is there a target? Check your module map."; + } + @Override public CompletableFuture enable() { enableForProgram(); diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/DebuggerLogicalBreakpointService.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/DebuggerLogicalBreakpointService.java index 60994fb840..dfd8947e6b 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/DebuggerLogicalBreakpointService.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/DebuggerLogicalBreakpointService.java @@ -292,6 +292,23 @@ public interface DebuggerLogicalBreakpointService { CompletableFuture placeBreakpointAt(ProgramLocation loc, long length, Collection kinds, String name); + /** + * Generate an informational status message when enabling the selected breakpoints + * + *

+ * Breakpoint enabling may fail for a variety of reasons. Some of those reasons deal with the + * trace database and GUI rather than with the target. When enabling will not likely behave in + * the manner expected by the user, this should provide a message explaining why. For example, + * if a breakpoint has no locations on a target, then we already know "enable" will not work. + * This should explain the situation to the user. If enabling is expected to work, then this + * should return null. + * + * @param col the collection we're about to enable + * @param trace a trace, if the command will be limited to the given trace + * @return the status message, or null + */ + String generateStatusEnable(Collection col, Trace trace); + /** * Enable a collection of logical breakpoints on target, if applicable * @@ -353,6 +370,21 @@ public interface DebuggerLogicalBreakpointService { */ CompletableFuture deleteLocs(Collection col); + /** + * Generate an informational message when toggling the breakpoints at the given location + * + *

+ * This works in the same manner as {@link #generateStatusEnable(Collection)}, except it is for + * toggling breakpoints at a given location. If there are no breakpoints at the location, this + * should return null, since the usual behavior in that case is to prompt to place a new + * breakpoint. + * + * @see #generateStatusEnable(Collection) + * @param loc the location + * @return the status message, or null + */ + String generateStatusToggleAt(ProgramLocation loc); + /** * Toggle the breakpoints at the given location * diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/LogicalBreakpoint.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/LogicalBreakpoint.java index 5528ad0389..b770d1c814 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/LogicalBreakpoint.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/services/LogicalBreakpoint.java @@ -790,6 +790,18 @@ public interface LogicalBreakpoint { */ CompletableFuture deleteForTrace(Trace trace); + /** + * Generate a status message for enabling this breakpoint + * + *

+ * If this breakpoint has no locations in the given trace, then the status message should + * explain that it cannot actually enable the breakpoint. + * + * @param trace optional to limit scope of message to locations in the given trace + * @return the status message, or null + */ + String generateStatusEnable(Trace trace); + /** * Enable (or create) this breakpoint everywhere in the tool. *