From 28d973e5f2f0486fda769df59610677e44fae7bd Mon Sep 17 00:00:00 2001 From: dragonmacher <48328597+dragonmacher@users.noreply.github.com> Date: Tue, 23 Nov 2021 18:02:32 -0500 Subject: [PATCH] GP-1533 - Decompiler - fixed stack trace related to removing secondary highlights; fixed secondary highlights being applied to multiple functions; fixed rename of functions not getting re-highlighted --- .../java/ghidra/app/util/AddEditDialog.java | 1 + .../component/ClangHighlightController.java | 59 ++++-- .../decompiler/component/DecompilerPanel.java | 34 +-- .../RemoveAllSecondaryHighlightsAction.java | 7 +- .../actions/RenameFunctionAction.java | 18 +- .../component/DecompilerClangTest.java | 193 +++++++++++++++++- 6 files changed, 276 insertions(+), 36 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/util/AddEditDialog.java b/Ghidra/Features/Base/src/main/java/ghidra/app/util/AddEditDialog.java index 632f88416a..48c847933d 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/util/AddEditDialog.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/util/AddEditDialog.java @@ -522,6 +522,7 @@ public class AddEditDialog extends DialogComponentProvider { */ private JPanel create() { labelNameChoices = new GhidraComboBox<>(); + labelNameChoices.setName("label.name.choices"); GhidraComboBox comboBox = new GhidraComboBox<>(); comboBox.setEnterKeyForwarding(true); namespaceChoices = comboBox; diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/ClangHighlightController.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/ClangHighlightController.java index 825d35b496..ad67cffac0 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/ClangHighlightController.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/ClangHighlightController.java @@ -27,6 +27,7 @@ import docking.widgets.fieldpanel.field.Field; import docking.widgets.fieldpanel.support.FieldLocation; import ghidra.app.decompiler.*; import ghidra.program.model.listing.Function; +import ghidra.program.model.pcode.HighFunction; import ghidra.program.model.pcode.PcodeOp; import ghidra.util.ColorUtils; import util.CollectionUtils; @@ -46,7 +47,7 @@ import util.CollectionUtils; * matching braces) * *
  • Secondary Highlights - triggered by the user to show all occurrences of a particular - * variable; they will stay until they are manually cleared by a user action. The user can \ + * variable; they will stay until they are manually cleared by a user action. The user can * apply multiple secondary highlights at the same time, with different colors for each * highlight. * These highlights apply to the function in use when the highlight is created. Thus, @@ -144,8 +145,8 @@ public abstract class ClangHighlightController { return getSecondaryHighlight(token) != null; } - public boolean hasSecondaryHighlights() { - return !secondaryHighlighters.isEmpty(); + public boolean hasSecondaryHighlights(Function function) { + return !secondaryHighlightersbyFunction.get(function).isEmpty(); } public Color getSecondaryHighlight(ClangToken token) { @@ -174,7 +175,7 @@ public abstract class ClangHighlightController { * @param function the function * @return the highlighters */ - public Set getSecondaryHighlightersByFunction(Function function) { + public Set getSecondaryHighlighters(Function function) { return new HashSet<>(secondaryHighlightersbyFunction.get(function)); } @@ -285,12 +286,14 @@ public abstract class ClangHighlightController { * @param f the function */ public void removeSecondaryHighlights(Function f) { + List highlighters = secondaryHighlightersbyFunction.get(f); for (ClangDecompilerHighlighter highlighter : highlighters) { TokenHighlights highlights = highlighterHighlights.get(highlighter); Consumer clearHighlight = token -> updateHighlightColor(token); doClearHighlights(highlights, clearHighlight); } + highlighters.clear(); notifyListeners(); } @@ -320,9 +323,18 @@ public abstract class ClangHighlightController { } public void removeHighlighter(DecompilerHighlighter highlighter) { + removeHighlighterHighlights(highlighter); - secondaryHighlighters.remove(highlighter); highlighterHighlights.remove(highlighter); + secondaryHighlighters.remove(highlighter); + + Collection> lists = + secondaryHighlightersbyFunction.values(); + for (List highlighters : lists) { + if (highlighters.remove(highlighter)) { + break; + } + } } /** @@ -330,6 +342,7 @@ public abstract class ClangHighlightController { * @param highlighter the highlighter */ public void removeHighlighterHighlights(DecompilerHighlighter highlighter) { + TokenHighlights highlighterTokens = highlighterHighlights.get(highlighter); if (highlighterTokens == null) { return; @@ -348,21 +361,19 @@ public abstract class ClangHighlightController { */ public void addSecondaryHighlighter(Function function, ClangDecompilerHighlighter highlighter) { - // note: this highlighter has likely already been added the the this class, but has not + // Note: this highlighter has likely already been added the the this class, but has not // yet been bound to the given function. secondaryHighlightersbyFunction.get(function).add(highlighter); secondaryHighlighters.add(highlighter); highlighterHighlights.putIfAbsent(highlighter, new TokenHighlights()); } - /** - * Adds the given highlighter, but does not create any highlights - * @param highlighter the highlighter - */ + // Note: this is used for all highlight types, secondary and highlighter service highlighters public void addHighlighter(ClangDecompilerHighlighter highlighter) { highlighterHighlights.putIfAbsent(highlighter, new TokenHighlights()); } + // Note: this is used for all highlight types, secondary and highlighter service highlights public void addHighlighterHighlights(ClangDecompilerHighlighter highlighter, Supplier> tokens, ColorProvider colorProvider) { @@ -488,9 +499,17 @@ public abstract class ClangHighlightController { private Color blendHighlighterColors(ClangToken token) { + Function function = getFunction(token); + if (function == null) { + return null; // not sure if this can happen + } + + Set global = getGlobalHighlighters(); + Set secondary = getSecondaryHighlighters(function); + Iterable it = CollectionUtils.asIterable(global, secondary); Color lastColor = null; - Collection allHighlights = highlighterHighlights.values(); - for (TokenHighlights highlights : allHighlights) { + for (ClangDecompilerHighlighter highlighter : it) { + TokenHighlights highlights = highlighterHighlights.get(highlighter); HighlightToken hlToken = highlights.get(token); if (hlToken == null) { continue; @@ -508,9 +527,21 @@ public abstract class ClangHighlightController { return lastColor; } + private Function getFunction(ClangToken t) { + ClangFunction cFunction = t.getClangFunction(); + if (cFunction == null) { + return null; + } + + HighFunction highFunction = cFunction.getHighFunction(); + if (highFunction == null) { + return null; + } + return highFunction.getFunction(); + } + /** - * If input token is a parenthesis, highlight all - * tokens between it and its match + * If input token is a parenthesis, highlight all tokens between it and its match * @param tok potential parenthesis token * @param highlightColor the highlight color * @return a list of all tokens that were highlighted. diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/DecompilerPanel.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/DecompilerPanel.java index f4073f9e58..74b8d6eeb6 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/DecompilerPanel.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/DecompilerPanel.java @@ -149,8 +149,8 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field return highlightController.getSecondaryHighlightColors(); } - public boolean hasSecondaryHighlights() { - return highlightController.hasSecondaryHighlights(); + public boolean hasSecondaryHighlights(Function function) { + return highlightController.hasSecondaryHighlights(function); } public boolean hasSecondaryHighlight(ClangToken token) { @@ -166,14 +166,14 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field } private Set getSecondaryHighlihgtersByFunction(Function function) { - return highlightController.getSecondaryHighlightersByFunction(function); + return highlightController.getSecondaryHighlighters(function); } /** * Removes all secondary highlights for the current function + * @param function the function containing the secondary highlights */ - public void removeSecondaryHighlights() { - Function function = controller.getFunction(); + public void removeSecondaryHighlights(Function function) { highlightController.removeSecondaryHighlights(function); } @@ -283,9 +283,14 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field } /** - * This is function is used to alert the panel that a token was renamed. - * If the token that is being renamed had a secondary highlight, we must re-apply the highlight - * to the new token. + * This function is used to alert the panel that a token was renamed. If the token being renamed + * had a secondary highlight, we must re-apply the highlight to the new token. + * + *

    This is not needed for highlighter service highlights, since they get called again to + * re-apply highlights. It is up to that highlighter to determine if highlighting still applies + * to the new token name. Alternatively, for secondary highlights, we know the user chose the + * highlight based upon name. Thus, when the name changes, we need to take action to update + * the secondary highlight. * * @param token the token being renamed * @param newName the new name of the token @@ -307,9 +312,9 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field private void cloneGlobalHighlighters(DecompilerPanel sourcePanel) { - Set allHighlighters = + Set globalHighlighters = sourcePanel.highlightController.getGlobalHighlighters(); - for (ClangDecompilerHighlighter otherHighlighter : allHighlighters) { + for (ClangDecompilerHighlighter otherHighlighter : globalHighlighters) { ClangDecompilerHighlighter newHighlighter = otherHighlighter.clone(this); highlightersById.put(newHighlighter.getId(), newHighlighter); @@ -411,18 +416,19 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field currentSearchLocation = null; reapplySecondaryHighlights(); - reapplyHighlighterHighlights(); + reapplyGlobalHighlights(); } - private void reapplyHighlighterHighlights() { + private void reapplyGlobalHighlights() { Function function = decompileData.getFunction(); if (function == null) { return; } - Collection values = highlightersById.values(); - for (ClangDecompilerHighlighter highlighter : values) { + Set globalHighlighters = + highlightController.getGlobalHighlighters(); + for (ClangDecompilerHighlighter highlighter : globalHighlighters) { highlighter.clearHighlights(); highlighter.applyHighlights(); } diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/RemoveAllSecondaryHighlightsAction.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/RemoveAllSecondaryHighlightsAction.java index 822fa1afd3..35bda851f9 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/RemoveAllSecondaryHighlightsAction.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/RemoveAllSecondaryHighlightsAction.java @@ -20,6 +20,7 @@ import ghidra.app.decompiler.component.ClangHighlightController; import ghidra.app.decompiler.component.DecompilerPanel; import ghidra.app.plugin.core.decompile.DecompilerActionContext; import ghidra.app.util.HelpTopics; +import ghidra.program.model.listing.Function; import ghidra.util.HelpLocation; /** @@ -46,12 +47,14 @@ public class RemoveAllSecondaryHighlightsAction extends AbstractDecompilerAction } DecompilerPanel panel = context.getDecompilerPanel(); - return panel.hasSecondaryHighlights(); + Function function = context.getFunction(); + return panel.hasSecondaryHighlights(function); } @Override protected void decompilerActionPerformed(DecompilerActionContext context) { DecompilerPanel panel = context.getDecompilerPanel(); - panel.removeSecondaryHighlights(); + Function function = context.getFunction(); + panel.removeSecondaryHighlights(function); } } diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/RenameFunctionAction.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/RenameFunctionAction.java index 80d0f1b53c..b342f072df 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/RenameFunctionAction.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/RenameFunctionAction.java @@ -16,6 +16,7 @@ package ghidra.app.plugin.core.decompile.actions; import java.awt.event.KeyEvent; +import java.util.Objects; import docking.action.KeyBindingData; import docking.action.MenuData; @@ -23,12 +24,14 @@ import ghidra.app.decompiler.ClangFuncNameToken; import ghidra.app.decompiler.ClangToken; import ghidra.app.decompiler.component.DecompilerUtils; import ghidra.app.plugin.core.decompile.DecompilerActionContext; +import ghidra.app.plugin.core.decompile.DecompilerProvider; import ghidra.app.util.AddEditDialog; import ghidra.app.util.HelpTopics; import ghidra.program.model.listing.Function; import ghidra.program.model.listing.Program; import ghidra.program.model.pcode.HighFunctionShellSymbol; import ghidra.program.model.pcode.HighSymbol; +import ghidra.program.model.symbol.Symbol; import ghidra.util.HelpLocation; import ghidra.util.UndefinedFunction; @@ -41,6 +44,7 @@ public class RenameFunctionAction extends AbstractDecompilerAction { setPopupMenuData(new MenuData(new String[] { "Rename Function" }, "Decompile")); } + @Override protected Function getFunction(DecompilerActionContext context) { Program program = context.getProgram(); ClangToken tokenAtCursor = context.getTokenAtCursor(); @@ -68,6 +72,18 @@ public class RenameFunctionAction extends AbstractDecompilerAction { Program program = context.getProgram(); Function function = getFunction(context); AddEditDialog dialog = new AddEditDialog("Edit Function Name", context.getTool()); - dialog.editLabel(function.getSymbol(), program); + Symbol symbol = function.getSymbol(); + String originalName = symbol.getName(); + dialog.editLabel(symbol, program); + + String currentName = symbol.getName(); + if (Objects.equals(originalName, currentName)) { + return; // no change + } + + DecompilerProvider provider = context.getComponentProvider(); + ClangToken tokenAtCursor = context.getTokenAtCursor(); + provider.tokenRenamed(tokenAtCursor, currentName); + } } diff --git a/Ghidra/Features/Decompiler/src/test.slow/java/ghidra/app/decompiler/component/DecompilerClangTest.java b/Ghidra/Features/Decompiler/src/test.slow/java/ghidra/app/decompiler/component/DecompilerClangTest.java index 794af85ad5..60eebdface 100644 --- a/Ghidra/Features/Decompiler/src/test.slow/java/ghidra/app/decompiler/component/DecompilerClangTest.java +++ b/Ghidra/Features/Decompiler/src/test.slow/java/ghidra/app/decompiler/component/DecompilerClangTest.java @@ -17,14 +17,14 @@ package ghidra.app.decompiler.component; import static org.junit.Assert.*; -import java.awt.Color; -import java.awt.Window; +import java.awt.*; import java.awt.event.MouseEvent; import java.util.*; +import java.util.List; import java.util.function.Predicate; import java.util.stream.Collectors; -import javax.swing.JButton; +import javax.swing.*; import org.junit.Before; import org.junit.Test; @@ -41,6 +41,7 @@ import ghidra.app.decompiler.DecompileOptions.NamespaceStrategy; import ghidra.app.plugin.core.decompile.AbstractDecompilerTest; import ghidra.app.plugin.core.decompile.DecompilerProvider; import ghidra.app.plugin.core.decompile.actions.*; +import ghidra.app.util.AddEditDialog; import ghidra.framework.options.ToolOptions; import ghidra.framework.plugintool.util.OptionsService; import ghidra.program.model.listing.CodeUnit; @@ -456,6 +457,79 @@ public class DecompilerClangTest extends AbstractDecompilerTest { assertNoFieldsSecondaryHighlighted(text2); } + @Test + public void testSecondaryHighlighting_ClearAll_DoesNotAffectOtherFunctions() { + + /* + + Decomp of '_call_structure_A': + + 1| + 2| void _call_structure_A(A *a) + 3| + 4| { + 5| _printf("call_structure_A: %s\n",a->name); + 6| _printf("call_structure_A: %s\n",(a->b).name); + 7| _printf("call_structure_A: %s\n",(a->b).c.name); + 8| _printf("call_structure_A: %s\n",(a->b).c.d.name); + 9| _printf("call_structure_A: %s\n",(a->b).c.d.e.name); + 10| _call_structure_B(&a->b); + 11| return; + 12| } + + + Decomp of '_call_structure_B': + + 1| + 2| void _call_structure_B(B *b) + 3| + 4| { + 5| _printf("call_structure_B: %s\n",b->name); + 6| _call_structure_C(&b->c); + 7| return; + 8| } + + + + */ + + decompile("100000d60"); // '_call_structure_A' + + // 5:2 "_printf" + int line1 = 5; + int charPosition1 = 2; + setDecompilerLocation(line1, charPosition1); + ClangToken token1 = getToken(); + + Color color1 = highlight(); + assertAllFieldsSecondaryHighlighted(token1, color1); + + decompile("100000e10"); // '_call_structure_B' + + // 5:2 "_printf" + int line2 = 5; + int charPosition2 = 2; + setDecompilerLocation(line2, charPosition2); + ClangToken token2 = getToken(); + + Color color2 = highlight(); + assertAllFieldsSecondaryHighlighted(token2, color2); + + decompile("100000d60"); // '_call_structure_A' + + // 5:2 "_printf" + setDecompilerLocation(line1, charPosition1); + clearAllHighlights(); + + // token 1 cleared; token 2 still highlighted + assertNoFieldsSecondaryHighlighted(token1.getText()); + + decompile("100000e10"); // '_call_structure_B' + setDecompilerLocation(line2, charPosition2); + token2 = getToken(); + assertAllFieldsSecondaryHighlighted(token2, color2); + } + @Test public void testSecondaryHighlighting_RenameHighlightedVariable() { @@ -499,6 +573,49 @@ public class DecompilerClangTest extends AbstractDecompilerTest { assertAllFieldsSecondaryHighlighted(token, color); } + @Test + public void testSecondaryHighlighting_RenameHighlightedFunction() { + + /* + + Decomp of '_call_structure_A': + + 1| + 2| void _call_structure_A(A *a) + 3| + 4| { + 5| _printf("call_structure_A: %s\n",a->name); + 6| _printf("call_structure_A: %s\n",(a->b).name); + 7| _printf("call_structure_A: %s\n",(a->b).c.name); + 8| _printf("call_structure_A: %s\n",(a->b).c.d.name); + 9| _printf("call_structure_A: %s\n",(a->b).c.d.e.name); + 10| _call_structure_B(&a->b); + 11| return; + 12| } + + */ + + decompile("100000d60"); // '_call_structure_A' + + // 5:2 "_printf" + int line = 5; + int charPosition = 2; + setDecompilerLocation(line, charPosition); + + ClangToken token = getToken(); + String text = token.getText(); + assertEquals("_printf", text); + + Color color = highlight(); + + renameFunction("bob"); + + token = getToken(); + text = token.getText(); + assertEquals("bob", text); + assertAllFieldsSecondaryHighlighted(token, color); + } + @Test public void testSecondaryHighlighting_HighlightColorGetsReused() { @@ -938,6 +1055,56 @@ public class DecompilerClangTest extends AbstractDecompilerTest { assertAllFieldsSecondaryHighlighted(token, color); } + @Test + public void testSecondaryHighlighting_DoesNotApplyToOtherFunctions() { + + /* + + Decomp of '_call_structure_A': + + 1| + 2| void _call_structure_A(A *a) + 3| + 4| { + 5| _printf("call_structure_A: %s\n",a->name); + 6| _printf("call_structure_A: %s\n",(a->b).name); + 7| _printf("call_structure_A: %s\n",(a->b).c.name); + 8| _printf("call_structure_A: %s\n",(a->b).c.d.name); + 9| _printf("call_structure_A: %s\n",(a->b).c.d.e.name); + 10| _call_structure_B(&a->b); + 11| return; + 12| } + + Decomp of '_call_structure_B': + + 1| + 2| void _call_structure_B(B *b) + 3| + 4| { + 5| _printf("call_structure_B: %s\n",b->name); + 6| _call_structure_C(&b->c); + 7| return; + 8| } + + + + */ + + decompile("100000d60"); // '_call_structure_A' + + // 5:2 "_printf" + int line = 5; + int charPosition = 2; + setDecompilerLocation(line, charPosition); + ClangToken token = getToken(); + + Color color = highlight(); + assertAllFieldsSecondaryHighlighted(token, color); + + decompile("100000e10"); // '_call_structure_B' + assertNoFieldsSecondaryHighlighted(token.getText()); + } + @Test public void testHighlightService() { @@ -1788,6 +1955,22 @@ public class DecompilerClangTest extends AbstractDecompilerTest { waitForDecompiler(); } + private void renameFunction(String newName) { + DockingActionIf action = getAction(decompiler, "Rename Function"); + performAction(action, provider.getActionContext(null), false); + + AddEditDialog dialog = waitForDialogComponent(AddEditDialog.class); + runSwing(() -> { + JComboBox comboBox = + (JComboBox) findComponentByName(dialog, "label.name.choices"); + Component comp = comboBox.getEditor().getEditorComponent(); + ((JTextField) comp).setText(newName); + }); + + pressButtonByText(dialog, "OK"); + waitForDecompiler(); + } + private void clearAllHighlights() { DockingActionIf highlightAction = @@ -1903,7 +2086,8 @@ public class DecompilerClangTest extends AbstractDecompilerTest { Color combinedColor = getCombinedHighlightColor(token); ColorMatcher cm = new ColorMatcher(color, combinedColor); Color actual = token.getHighlight(); - assertTrue("Token is not highlighted: '" + token + "'" + "\n\texpected: " + cm + + String tokenString = token.toString() + " at line " + token.getLineParent().getLineNumber(); + assertTrue("Token is not highlighted: '" + tokenString + "'" + "\n\texpected: " + cm + "; found: " + toString(actual), cm.matches(actual)); } @@ -2123,7 +2307,6 @@ public class DecompilerClangTest extends AbstractDecompilerTest { ColorMatcher(Color... colors) { // note: we allow null - for (Color c : colors) { myColors.add(c); }