From 30db29d1ce2a65c34f778032a3a2778ff42a397d Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Fri, 20 Mar 2020 12:56:56 -0400 Subject: [PATCH] GT-3542 - Symbol Tree - fixed flashing of the symbol tree while analyzing; fixed bad progress of Decompiler Parameter ID analyzer; fixed odd progress updating of ConcurrentQ --- .../misc/MyProgramChangesDisplayPlugin.java | 2 +- .../core/symboltree/SymbolTreeProvider.java | 35 ++++++++++++++++--- .../function/DecompilerParameterIdCmd.java | 31 ++++++++-------- .../DecompilerCallConventionAnalyzer.java | 26 +++++++------- .../analysis/DecompilerFunctionAnalyzer.java | 2 +- .../java/docking/widgets/tree/GTreeTask.java | 4 ++- .../java/ghidra/util/task/TaskRunner.java | 2 +- .../generic/concurrent/ConcurrentGraphQ.java | 11 +++--- .../java/generic/concurrent/ConcurrentQ.java | 12 ++++++- .../plugintool/mgr/EventManager.java | 2 +- .../model/util/AcyclicCallGraphBuilder.java | 12 ++++--- .../src/main/java/ghidra/util/Swing.java | 17 ++++----- 12 files changed, 98 insertions(+), 58 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/misc/MyProgramChangesDisplayPlugin.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/misc/MyProgramChangesDisplayPlugin.java index 5266188453..a7eebb21ba 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/misc/MyProgramChangesDisplayPlugin.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/misc/MyProgramChangesDisplayPlugin.java @@ -255,7 +255,7 @@ public class MyProgramChangesDisplayPlugin extends ProgramPlugin implements Doma */ private void updateChangeMarkers() { - Swing.assertThisIsTheSwingThread( + Swing.assertSwingThread( "Change markers must be manipulated on the Swing thread"); if (currentProgram == null) { diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/SymbolTreeProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/SymbolTreeProvider.java index 320b06bab3..349011a304 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/SymbolTreeProvider.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/symboltree/SymbolTreeProvider.java @@ -76,6 +76,7 @@ public class SymbolTreeProvider extends ComponentProviderAdapter { private List bufferedTasks = new ArrayList<>(); private SwingUpdateManager domainChangeUpdateManager = new SwingUpdateManager(1000, SwingUpdateManager.DEFAULT_MAX_DELAY, "Symbol Tree Provider", () -> { + if (bufferedTasks.isEmpty()) { return; } @@ -404,7 +405,7 @@ public class SymbolTreeProvider extends ComponentProviderAdapter { private void addTask(GTreeTask task) { // Note: if we want to call this method from off the Swing thread, then we have to // synchronize on the list that we are adding to here. - Swing.assertThisIsTheSwingThread( + Swing.assertSwingThread( "Adding tasks must be done on the Swing thread," + "since they are put into a list that is processed on the Swing thread. "); @@ -535,7 +536,10 @@ public class SymbolTreeProvider extends ComponentProviderAdapter { public void run(TaskMonitor monitor) throws CancelledException { TreePath[] selectionPaths = tree.getSelectionPaths(); doRun(monitor); - tree.setSelectionPaths(selectionPaths); + + if (selectionPaths.length != 0) { + tree.setSelectionPaths(selectionPaths); + } } @Override @@ -612,8 +616,9 @@ public class SymbolTreeProvider extends ComponentProviderAdapter { @Override public void runBulk(TaskMonitor monitor) throws CancelledException { + if (tasks.size() > MAX_TASK_COUNT) { - SystemUtilities.runSwingLater(() -> rebuildTree()); + Swing.runLater(() -> rebuildTree()); return; } @@ -628,7 +633,7 @@ public class SymbolTreeProvider extends ComponentProviderAdapter { @Override public void domainObjectChanged(DomainObjectChangedEvent event) { - if (!tool.isVisible(SymbolTreeProvider.this)) { + if (ignoreEvents()) { return; } @@ -686,5 +691,27 @@ public class SymbolTreeProvider extends ComponentProviderAdapter { } } + + private boolean ignoreEvents() { + if (!isVisible()) { + return true; + } + return treeIsCollapsed(); + } + + private boolean treeIsCollapsed() { + // note: the root's children are visible by default + GTreeNode root = tree.getViewRoot(); + if (!root.isExpanded()) { + return true; + } + List children = root.getChildren(); + for (GTreeNode node : children) { + if (node.isExpanded()) { + return false; + } + } + return true; + } } } diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerParameterIdCmd.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerParameterIdCmd.java index 755a1a8533..752b31ce7d 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerParameterIdCmd.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerParameterIdCmd.java @@ -50,9 +50,10 @@ public class DecompilerParameterIdCmd extends BackgroundCommand { private boolean commitVoidReturn; private int decompilerTimeoutSecs; - public DecompilerParameterIdCmd(AddressSetView entries, SourceType sourceTypeClearLevel, + public DecompilerParameterIdCmd(String name, AddressSetView entries, + SourceType sourceTypeClearLevel, boolean commitDataTypes, boolean commitVoidReturn, int decompilerTimeoutSecs) { - super("Create Function Stack Variables", true, true, false); + super(name, true, true, false); entryPoints.add(entries); this.sourceTypeClearLevel = sourceTypeClearLevel; this.commitDataTypes = commitDataTypes; @@ -65,13 +66,13 @@ public class DecompilerParameterIdCmd extends BackgroundCommand { program = (Program) obj; CachingPool decompilerPool = - new CachingPool(new DecompilerFactory()); + new CachingPool<>(new DecompilerFactory()); QRunnable
runnable = new ParallelDecompileRunnable(decompilerPool); ConcurrentGraphQ
queue = null; try { - monitor.setMessage("Analyzing Call Hierarchy..."); + monitor.setMessage(getName() + " - creating dependency graph..."); AcyclicCallGraphBuilder builder = new AcyclicCallGraphBuilder(program, entryPoints, true); AbstractDependencyGraph
graph = builder.getDependencyGraph(monitor); @@ -80,10 +81,11 @@ public class DecompilerParameterIdCmd extends BackgroundCommand { } GThreadPool pool = AutoAnalysisManager.getSharedAnalsysThreadPool(); - queue = new ConcurrentGraphQ
(runnable, graph, pool, monitor); + queue = new ConcurrentGraphQ<>(runnable, graph, pool, monitor); resetFunctionSourceTypes(graph.getValues()); - monitor.setMessage("Analyzing..."); + monitor.setMessage(getName() + " - analyzing..."); + monitor.initialize(graph.size()); queue.execute(); } @@ -114,7 +116,8 @@ public class DecompilerParameterIdCmd extends BackgroundCommand { */ private boolean funcIsExternalGlue(Function func) { String blockName = program.getMemory().getBlock(func.getEntryPoint()).getName(); - return (blockName.equals("EXTERNAL") || blockName.equals(".plt") || blockName.equals("__stub_helper")); + return (blockName.equals("EXTERNAL") || blockName.equals(".plt") || + blockName.equals("__stub_helper")); } private void resetFunctionSourceTypes(Set
set) { @@ -147,10 +150,8 @@ public class DecompilerParameterIdCmd extends BackgroundCommand { } } catch (InvalidInputException e) { - Msg.warn( - this, - "Error changing signature SourceType (should not since Input is the same) on--" + - func.getName(), e); + Msg.warn(this, + "Error changing signature SourceType on--" + func.getName(), e); } } } @@ -248,10 +249,10 @@ public class DecompilerParameterIdCmd extends BackgroundCommand { } /** - * Check for consistency of returned results. Trying to propogate, don't want to propagate garbage. + * Check for consistency of returned results. Trying to propagate, don't want to propagate garbage. * - * @param decompRes - * @return + * @param decompRes the decompile result + * @return true if inconsistent results */ private boolean hasInconsistentResults(DecompileResults decompRes) { HighFunction hfunc = decompRes.getHighFunction(); @@ -382,7 +383,7 @@ public class DecompilerParameterIdCmd extends BackgroundCommand { private void doWork(Function function, DecompInterface decompiler, TaskMonitor monitor) throws CancelledException { monitor.checkCanceled(); - monitor.setMessage("Decompile " + function.getName()); + monitor.setMessage(getName() + " - decompile " + function.getName()); analyzeFunction(decompiler, function, monitor); } diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/analysis/DecompilerCallConventionAnalyzer.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/analysis/DecompilerCallConventionAnalyzer.java index fab980247d..2e6b9e334b 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/analysis/DecompilerCallConventionAnalyzer.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/analysis/DecompilerCallConventionAnalyzer.java @@ -37,11 +37,10 @@ import ghidra.util.graph.AbstractDependencyGraph; import ghidra.util.task.TaskMonitor; public class DecompilerCallConventionAnalyzer extends AbstractAnalyzer { - private static final String NAME = "Call Convention Identification"; + private static final String NAME = "Call Convention ID"; private static final String DESCRIPTION = "Uses decompiler to figure out unknown calling conventions."; - private static final String STD_NAMESPACE = "std"; private static final String COULD_NOT_RECOVER_CALLING_CONVENTION = "Could not recover calling convention"; private static final String OPTION_NAME_DECOMPILER_TIMEOUT_SECS = @@ -53,8 +52,6 @@ public class DecompilerCallConventionAnalyzer extends AbstractAnalyzer { private boolean ignoreBookmarks = false; - private Program program; - //================================================================================================== // Interface Methods //================================================================================================== @@ -68,11 +65,11 @@ public class DecompilerCallConventionAnalyzer extends AbstractAnalyzer { @Override public boolean canAnalyze(Program program) { - boolean cando = program.getLanguage().supportsPcode(); + boolean can = program.getLanguage().supportsPcode(); // for a single entry compiler convention, the convention is always used, so no need to identify it - cando &= program.getCompilerSpec().getCallingConventions().length > 1; - return cando; + can &= program.getCompilerSpec().getCallingConventions().length > 1; + return can; } @Override @@ -92,8 +89,6 @@ public class DecompilerCallConventionAnalyzer extends AbstractAnalyzer { public boolean added(Program program, AddressSetView set, TaskMonitor monitor, MessageLog log) throws CancelledException { - this.program = program; // set program for use by ParallelDecompilerCallback - ignoreBookmarks = set.hasSameAddresses(program.getMemory()); try { @@ -128,14 +123,14 @@ public class DecompilerCallConventionAnalyzer extends AbstractAnalyzer { CachingPool decompilerPool = new CachingPool<>(new DecompilerFactory(program)); - QRunnable
callback = new ParallelDecompilerCallback(decompilerPool); + QRunnable
callback = new ParallelDecompilerCallback(decompilerPool, program); ConcurrentGraphQ
queue = null; monitor.initialize(functionEntries.getNumAddresses()); try { - monitor.setMessage("Analyzing Call Hierarchy..."); + monitor.setMessage(NAME + " - creating dependency graph..."); AcyclicCallGraphBuilder builder = new AcyclicCallGraphBuilder(program, functionEntries, true); AbstractDependencyGraph
graph = builder.getDependencyGraph(monitor); @@ -146,7 +141,8 @@ public class DecompilerCallConventionAnalyzer extends AbstractAnalyzer { GThreadPool pool = AutoAnalysisManager.getSharedAnalsysThreadPool(); queue = new ConcurrentGraphQ<>(callback, graph, pool, monitor); - monitor.setMessage("Analyzing Call Conventions..."); + monitor.setMessage(NAME + " - analyzing call conventions..."); + monitor.initialize(graph.size()); queue.execute(); } @@ -272,9 +268,11 @@ public class DecompilerCallConventionAnalyzer extends AbstractAnalyzer { private class ParallelDecompilerCallback implements QRunnable
{ private CachingPool pool; + private Program program; - ParallelDecompilerCallback(CachingPool decompilerPool) { + ParallelDecompilerCallback(CachingPool decompilerPool, Program program) { this.pool = decompilerPool; + this.program = program; } @Override @@ -287,10 +285,12 @@ public class DecompilerCallConventionAnalyzer extends AbstractAnalyzer { try { Function function = program.getFunctionManager().getFunctionAt(address); + monitor.setMessage(getName() + " - decompile " + function.getName()); performConventionAnalysis(function, decompiler, monitor); } finally { pool.release(decompiler); + monitor.incrementProgress(1); } return; } diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/analysis/DecompilerFunctionAnalyzer.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/analysis/DecompilerFunctionAnalyzer.java index 912faa9022..8a4dcb5674 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/analysis/DecompilerFunctionAnalyzer.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/analysis/DecompilerFunctionAnalyzer.java @@ -71,7 +71,7 @@ public class DecompilerFunctionAnalyzer extends AbstractAnalyzer { @Override public boolean added(Program program, AddressSetView set, TaskMonitor monitor, MessageLog log) { - DecompilerParameterIdCmd cmd = new DecompilerParameterIdCmd(set, + DecompilerParameterIdCmd cmd = new DecompilerParameterIdCmd(NAME, set, sourceTypeClearLevelOption, commitDataTypesOption, commitVoidReturnOption, decompilerTimeoutSecondsOption); cmd.applyTo(program, monitor); diff --git a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeTask.java b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeTask.java index 3935a7dfba..58078d9c53 100644 --- a/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeTask.java +++ b/Ghidra/Framework/Docking/src/main/java/docking/widgets/tree/GTreeTask.java @@ -19,7 +19,6 @@ import javax.swing.JTree; import javax.swing.tree.TreePath; import ghidra.util.SystemUtilities; -import ghidra.util.exception.CancelledException; import ghidra.util.task.TaskMonitor; import ghidra.util.worker.PriorityJob; @@ -53,6 +52,9 @@ public abstract class GTreeTask extends PriorityJob { * values on later calls to getSelectedPaths(). So, to handle that 'feature' of the JTree, we * need to translate the given path to the equivalent path in the current tree (this code may * not be needed in all uses of this task, but it protects us from the aforementioned case). + * @param path the path to translate + * @param monitor the monitor + * @return the translated path */ protected TreePath translatePath(TreePath path, TaskMonitor monitor) { diff --git a/Ghidra/Framework/Docking/src/main/java/ghidra/util/task/TaskRunner.java b/Ghidra/Framework/Docking/src/main/java/ghidra/util/task/TaskRunner.java index de756e7177..6740625048 100644 --- a/Ghidra/Framework/Docking/src/main/java/ghidra/util/task/TaskRunner.java +++ b/Ghidra/Framework/Docking/src/main/java/ghidra/util/task/TaskRunner.java @@ -124,7 +124,7 @@ class TaskRunner { private void showTaskDialog(WrappingTaskMonitor monitor) { - Swing.assertThisIsTheSwingThread("Must be on the Swing thread build the Task Dialog"); + Swing.assertSwingThread("Must be on the Swing thread build the Task Dialog"); taskDialog = buildTaskDialog(parent, monitor); monitor.setDelegate(taskDialog); // initialize the dialog to the current state of the monitor diff --git a/Ghidra/Framework/Generic/src/main/java/generic/concurrent/ConcurrentGraphQ.java b/Ghidra/Framework/Generic/src/main/java/generic/concurrent/ConcurrentGraphQ.java index 04e05b63ec..2c58067bf4 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/concurrent/ConcurrentGraphQ.java +++ b/Ghidra/Framework/Generic/src/main/java/generic/concurrent/ConcurrentGraphQ.java @@ -15,25 +15,26 @@ */ package generic.concurrent; +import java.util.Set; + import ghidra.util.graph.AbstractDependencyGraph; import ghidra.util.task.TaskMonitor; -import java.util.Set; - public class ConcurrentGraphQ { private ConcurrentQ queue; private AbstractDependencyGraph graph; - public ConcurrentGraphQ(QRunnable runnable, AbstractDependencyGraph graph, GThreadPool pool, + public ConcurrentGraphQ(QRunnable runnable, AbstractDependencyGraph graph, + GThreadPool pool, TaskMonitor monitor) { this.graph = graph; // @formatter:off queue = new ConcurrentQBuilder() .setCollectResults(false) - .setThreadPool(pool) + .setThreadPool(pool) .setMonitor(monitor) .setListener(new MyItemListener()) - .build(new QRunnableAdapter(runnable)); + .build(new QRunnableAdapter<>(runnable)); // @formatter:on } diff --git a/Ghidra/Framework/Generic/src/main/java/generic/concurrent/ConcurrentQ.java b/Ghidra/Framework/Generic/src/main/java/generic/concurrent/ConcurrentQ.java index f9da6e3646..3fcff98639 100644 --- a/Ghidra/Framework/Generic/src/main/java/generic/concurrent/ConcurrentQ.java +++ b/Ghidra/Framework/Generic/src/main/java/generic/concurrent/ConcurrentQ.java @@ -791,7 +791,17 @@ public class ConcurrentQ { @Override public void taskEnded(long id, I Item, long total, long progress) { if (!jobsReportProgress) { - if (total != monitor.getMaximum()) { + // + // This code works in 2 ways. The default case is that clients place items on + // the queue. As the amount of work grows, so too does the max progress value. + // This obviates the need for clients to manager progress. (The downside to this + // is that the progress may keep getting pushed back as it approaches the + // current maximum value.) The second case is where the client has specified a + // true maximum value. In that case, this code will not change the maxmimum + // (assuming that the client does not put more items into the queue than they + // specified). + // + if (total > monitor.getMaximum()) { monitor.setMaximum(total); } monitor.setProgress(progress); diff --git a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/mgr/EventManager.java b/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/mgr/EventManager.java index 859ef0dd8c..62c68e9aac 100644 --- a/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/mgr/EventManager.java +++ b/Ghidra/Framework/Project/src/main/java/ghidra/framework/plugintool/mgr/EventManager.java @@ -248,7 +248,7 @@ public class EventManager { private void sendEvents() { - Swing.assertThisIsTheSwingThread("Events must be sent on the Swing thread"); + Swing.assertSwingThread("Events must be sent on the Swing thread"); synchronized (eventQ) { currentEvent = eventQ.poll(); diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/util/AcyclicCallGraphBuilder.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/util/AcyclicCallGraphBuilder.java index 3c1ed66c1c..20fa4ca9a5 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/util/AcyclicCallGraphBuilder.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/util/AcyclicCallGraphBuilder.java @@ -91,7 +91,7 @@ public class AcyclicCallGraphBuilder { AbstractDependencyGraph
graph = new DependencyGraph<>(); Deque
startPoints = findStartPoints(); Set
unprocessed = new TreeSet<>(functionSet); // reliable processing order - + monitor.initialize(unprocessed.size()); while (!unprocessed.isEmpty()) { monitor.checkCanceled(); Address functionEntry = getNextStartFunction(startPoints, unprocessed); @@ -123,7 +123,7 @@ public class AcyclicCallGraphBuilder { return startPoints; } - private void initializeNode(StackNode node, Set
unprocessed) { + private void initializeNode(StackNode node) { FunctionManager fmanage = program.getFunctionManager(); Function function = fmanage.getFunctionAt(node.address); if (function.isThunk()) { @@ -163,14 +163,16 @@ public class AcyclicCallGraphBuilder { Address startFunction, TaskMonitor monitor) throws CancelledException { VisitStack stack = new VisitStack(startFunction); StackNode curnode = stack.peek(); - initializeNode(curnode, unprocessed); + initializeNode(curnode); graph.addValue(curnode.address); while (!stack.isEmpty()) { + monitor.checkCanceled(); + curnode = stack.peek(); if (curnode.nextchild >= curnode.children.length) { // Node more to children to traverse for this node unprocessed.remove(curnode.address); + monitor.incrementProgress(1); stack.pop(); - monitor.checkCanceled(); } else { Address childAddr = curnode.children[curnode.nextchild++]; @@ -178,7 +180,7 @@ public class AcyclicCallGraphBuilder { if (unprocessed.contains(childAddr)) { stack.push(childAddr); StackNode nextnode = stack.peek(); - initializeNode(nextnode, unprocessed); + initializeNode(nextnode); childAddr = nextnode.address; graph.addValue(nextnode.address); } diff --git a/Ghidra/Framework/Utility/src/main/java/ghidra/util/Swing.java b/Ghidra/Framework/Utility/src/main/java/ghidra/util/Swing.java index 3512de086f..46b5fe04c4 100644 --- a/Ghidra/Framework/Utility/src/main/java/ghidra/util/Swing.java +++ b/Ghidra/Framework/Utility/src/main/java/ghidra/util/Swing.java @@ -84,21 +84,18 @@ public class Swing { } /** - * A development/testing time method to make sure the current thread is the swing thread. - * @param errorMessage The message to display when the assert fails + * Logs a stack trace if the current calling thread is not the Swing thread + * @param errorMessage The message to display when not on the Swing thread + * @return true if the calling thread is the Swing thread */ - public static void assertThisIsTheSwingThread(String errorMessage) { - boolean isProductionMode = - !SystemUtilities.isInTestingMode() && !SystemUtilities.isInDevelopmentMode(); - if (isProductionMode) { - return; // squash during production mode - } - + public static boolean assertSwingThread(String errorMessage) { if (!isSwingThread()) { Throwable t = ReflectionUtilities.filterJavaThrowable(new AssertException(errorMessage)); - Msg.error(SystemUtilities.class, errorMessage, t); + Msg.error(Swing.class, errorMessage, t); + return false; } + return true; } /**