GT-3292 - Decompiler - Secondary Highlights - Checkpoint 5 - Functional

review feedback; still need help and context menu rearrangement

Ghidra/Features/Decompiler/certification.manifest

@@ -6,7 +6,6 @@ Module.manifest||GHIDRA||||END|
 build.gradle||GHIDRA||||END|
 src/decompile/.cproject||GHIDRA||||END|
 src/decompile/.project||GHIDRA||||END|
-src/decompile/.settings/org.eclipse.jdt.core.prefs||GHIDRA||||END|
 src/decompile/build.gradle||GHIDRA||||END|
 src/decompile/cpp/.gitignore||GHIDRA||||END|
 src/decompile/cpp/Doxyfile||GHIDRA|||Most of this file is autogenerated by doxygen which falls under the GPL - output from GPL products are NOT GPL! - mjbell4|END|

Ghidra/Features/Decompiler/src/main/help/help/topics/DecompilePlugin/Decompiler.htm

@@ -572,9 +572,9 @@
     <P style="margin-left: 80px;">Ghidra will do stack analysis that will recover parameters and
     return types, but for many programs, the analysis the decompiler does is better.</P>
 
-    <P style="margin-left: 80px;"><IMG alt="" src="../../shared/note.png" border="0">There is a
-    prototype plug-in that automatically pulls in the decompiler derived information and applies it
-    to each function as the function is created.</P>
+    <P style="margin-left: 80px;"><IMG alt="" src="../../shared/note.png" border="0">The
+    Decompiler Parameter ID analyzer automatically pulls in the decompiler derived 
+    information and applies it to each function as the function is created.</P>
 
     <P style="margin-left: 80px;"><IMG alt="" src="../../shared/note.png" border="0"> If a variable
     displayed in the assembly window has an undefined type, the decompiler will still respect the
@@ -597,11 +597,12 @@
     on the stack, but for many programs, the analysis the decompiler does is better.<BR>
     </P>
 
-    <P style="margin-left: 80px;"><IMG alt="" src="../../shared/note.png" border="0"> There is a
-    prototype plug-in that automatically pulls in the decompiler derived information and applies it
-    to each function as the function is created.&nbsp; The plugin by default will not commit local
-    variable definitions, either stack or register locals.&nbsp; Committing locals automatically
-    can be turned on by changing the analysis options for the Decompiler Parameter ID plugin.&nbsp;
+    <P style="margin-left: 80px;"><IMG alt="" src="../../shared/note.png" border="0">he
+    Decompiler Parameter ID analyzer automatically pulls in the decompiler derived 
+    information and applies it to each function as the function is created.  The analyzer by default 
+    will not commit local variable definitions, either stack or register locals.  
+    Committing locals automatically 
+    can be turned on by changing the Decompiler Parameter ID analysis options.
     In most cases it is better to commit locals only for certain functions that you really care
     about, or after the data type definitions (structures, etc...) have settled down for the
     program you are Reverse Engineering.<BR>

Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/ClangHighlightController.java

@@ -145,8 +145,7 @@ public abstract class ClangHighlightController {
 		tokenHighlights.clear();
 	}
 
-	public void togglePrimaryMultiHighlight(ClangToken token, Color hlColor,
-			Supplier<List<ClangToken>> tokens) {
+	public void togglePrimaryHighlights(Color hlColor, Supplier<List<ClangToken>> tokens) {
 
 		boolean isAllHighlighted = true;
 		for (ClangToken otherToken : tokens.get()) {
@@ -176,9 +175,14 @@ public abstract class ClangHighlightController {
 		return secondaryHighlightTokens.contains(token);
 	}
 
-	public void removeSecondaryHighlights(ghidra.program.model.listing.Function function) {
-		Set<HighlightToken> oldHighlights =
-			secondaryHighlightTokens.removeTokensByFunction(function);
+	public Set<HighlightToken> getSecondaryHighlightsByFunction(
+			ghidra.program.model.listing.Function f) {
+		Set<HighlightToken> highlights = secondaryHighlightTokens.getHighlightsByFunction(f);
+		return highlights;
+	}
+
+	public void removeSecondaryHighlights(ghidra.program.model.listing.Function f) {
+		Set<HighlightToken> oldHighlights = secondaryHighlightTokens.removeHighlightsByFunction(f);
 		for (HighlightToken hl : oldHighlights) {
 			ClangToken token = hl.getToken();
 			updateHighlightColor(token);
@@ -186,13 +190,11 @@ public abstract class ClangHighlightController {
 		notifyListeners();
 	}
 
-	public HighlightToken removeSecondaryHighlight(ClangToken token) {
-		HighlightToken hlToken = secondaryHighlightTokens.get(token);
+	public void removeSecondaryHighlights(ClangToken token) {
 		secondaryHighlightTokens.remove(token);
-		return hlToken;
 	}
 
-	public void removeSecondaryMultiHighlight(Supplier<? extends Collection<ClangToken>> tokens) {
+	public void removeSecondaryHighlights(Supplier<? extends Collection<ClangToken>> tokens) {
 		for (ClangToken clangToken : tokens.get()) {
 			secondaryHighlightTokens.remove(clangToken);
 			updateHighlightColor(clangToken);
@@ -200,14 +202,13 @@ public abstract class ClangHighlightController {
 		notifyListeners();
 	}
 
-	public void addSecondaryMultiHighlight(ClangToken token,
+	public void addSecondaryHighlights(String tokenText,
 			Supplier<? extends Collection<ClangToken>> tokens) {
-		String text = token.getText();
-		Color highlightColor = secondaryHighlightColors.getColor(text);
-		addSecondaryMultiHighlight(tokens, highlightColor);
+		Color highlightColor = secondaryHighlightColors.getColor(tokenText);
+		addSecondaryHighlights(tokens, highlightColor);
 	}
 
-	public void addSecondaryMultiHighlight(Supplier<? extends Collection<ClangToken>> tokens,
+	public void addSecondaryHighlights(Supplier<? extends Collection<ClangToken>> tokens,
 			Color hlColor) {
 		Function<ClangToken, Color> colorProvider = token -> hlColor;
 		addTokensToHighlights(tokens.get(), colorProvider, secondaryHighlightTokens);

Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/DecompilerPanel.java

@@ -22,6 +22,7 @@ import java.util.*;
 import java.util.List;
 import java.util.Map.Entry;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
 import javax.swing.JComponent;
 import javax.swing.JPanel;
@@ -52,6 +53,7 @@ import ghidra.program.util.ProgramSelection;
 import ghidra.util.*;
 import ghidra.util.bean.field.AnnotatedTextFieldElement;
 import ghidra.util.task.SwingUpdateManager;
+import util.CollectionUtils;
 
 /**
  * Class to handle the display of a decompiled function
@@ -149,7 +151,7 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field
 			String tokenName = entry.getKey();
 			Color color = entry.getValue();
 			Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(tokenName);
-			highlightController.addSecondaryMultiHighlight(lazyTokens, color);
+			highlightController.addSecondaryHighlights(lazyTokens, color);
 		}
 	}
 
@@ -167,26 +169,40 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field
 	}
 
 	public void removeSecondaryHighlight(ClangToken token) {
-		Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(token.getText());
-		highlightController.removeSecondaryMultiHighlight(lazyTokens);
+		removeSecondaryHighlight(token.getText());
+	}
+
+	private void removeSecondaryHighlight(String tokenText) {
+		Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(tokenText);
+		highlightController.removeSecondaryHighlights(lazyTokens);
 	}
 
 	public void addSecondaryHighlight(ClangToken token) {
-		Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(token.getText());
-		highlightController.addSecondaryMultiHighlight(token, lazyTokens);
+		String tokenText = token.getText();
+		addSecondaryHighlight(tokenText);
+	}
+
+	private void addSecondaryHighlight(String tokenText) {
+		Supplier<List<ClangToken>> lazyTokens = () -> {
+			return findTokensByName(tokenText);
+		};
+		highlightController.addSecondaryHighlights(tokenText, lazyTokens);
 	}
 
 	public void addSecondaryHighlight(ClangToken token, Color color) {
-		Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(token.getText());
-		highlightController.addSecondaryMultiHighlight(lazyTokens, color);
+		addSecondaryHighlight(token.getText(), color);
+	}
+
+	private void addSecondaryHighlight(String tokenText, Color color) {
+		Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(tokenText);
+		highlightController.addSecondaryHighlights(lazyTokens, color);
 	}
 
 	private void togglePrimaryHighlight(FieldLocation location, Field field, Color highlightColor) {
 
 		ClangToken token = ((ClangTextField) field).getToken(location);
 		Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(token.getText());
-		highlightController.togglePrimaryMultiHighlight(token, middleMouseHighlightColor,
-			lazyTokens);
+		highlightController.togglePrimaryHighlights(middleMouseHighlightColor, lazyTokens);
 	}
 
 	@Override
@@ -229,17 +245,48 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field
 
 		decompilerHoverProvider.setProgram(decompileData.getProgram());
 
-		/*
-		 * Give user notice when seeing the decompile of a non-function.
-		 */
+		// give user notice when seeing the decompile of a non-function
 		useNonFunctionColor = function instanceof UndefinedFunction;
 		setBackground(originalBackgroundColor);
+
 		if (clipboard != null) {
 			clipboard.selectionChanged(null);
 		}
 
 		// don't highlight search results across functions
 		currentSearchLocation = null;
+
+		reapplySecondaryHighlights();
+	}
+
+	private void reapplySecondaryHighlights() {
+
+		Function function = decompileData.getFunction();
+		if (function == null) {
+			return;
+		}
+
+		// The existing highlights are based on the previously generated tokens, which no longer
+		// exist.  Use those tokens to highlight the current tokens, which are conceptually the 
+		// same tokens.
+		Set<HighlightToken> oldHighlights =
+			highlightController.getSecondaryHighlightsByFunction(function);
+
+		//@formatter:off
+		Map<String, List<ClangToken>> tokensByName =
+			CollectionUtils.asStream(oldHighlights)
+						   .map(ht -> ht.getToken())
+						   .collect(Collectors.groupingBy(t -> t.getText()))
+						   ;
+		//@formatter:on
+
+		Set<Entry<String, List<ClangToken>>> entries = tokensByName.entrySet();
+		for (Entry<String, List<ClangToken>> entry : entries) {
+			String name = entry.getKey();
+			List<ClangToken> oldTokens = entry.getValue();
+			highlightController.removeSecondaryHighlights(() -> oldTokens);
+			addSecondaryHighlight(name);
+		}
 	}
 
 	private void setLocation(DecompileData oldData, DecompileData newData) {
@@ -972,22 +1019,23 @@ public class DecompilerPanel extends JPanel implements FieldMouseListener, Field
 	 * to the new token.
 	 * 
 	 * @param token the token being renamed
-	 * @param name the new name of the token
+	 * @param newName the new name of the token
 	 */
-	public void tokenRenamed(ClangToken token, String name) {
+	public void tokenRenamed(ClangToken token, String newName) {
 
 		if (!highlightController.hasSecondaryHighlight(token)) {
 			return;
 		}
 
+		TokenHighlightColors colors = highlightController.getSecondaryHighlightColors();
+		String oldName = token.getText();
+		Color hlColor = colors.getColor(oldName);
+		highlightController.removeSecondaryHighlights(token);
+
 		controller.doWhenNotBusy(() -> {
 
-			HighlightToken hlToken = highlightController.removeSecondaryHighlight(token);
-
-			Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(name);
-			Color color = hlToken.getColor();
-			highlightController.addSecondaryMultiHighlight(lazyTokens, color);
-			repaint();
+			Supplier<List<ClangToken>> lazyTokens = () -> findTokensByName(newName);
+			highlightController.addSecondaryHighlights(lazyTokens, hlColor);
 		});
 	}
 

Ghidra/Features/Decompiler/src/main/java/ghidra/app/decompiler/component/TokenHighlights.java

@@ -24,8 +24,9 @@ import ghidra.program.model.listing.Function;
 import ghidra.program.model.pcode.HighFunction;
 
 /**
- * A simple class to manage {@link HighlightToken}s used to create secondary highlights in the
- * Decompiler
+ * A simple class to manage {@link HighlightToken}s used to create highlights in the Decompiler.
+ * This class allows clients to access highlights either by a {@link ClangToken} or a 
+ * {@link HighlightToken}.
  */
 public class TokenHighlights implements Iterable<HighlightToken> {
 
@@ -52,9 +53,6 @@ public class TokenHighlights implements Iterable<HighlightToken> {
 	}
 
 	private Function getFunction(ClangToken t) {
-
-		// TODO verify that we can always get a function
-
 		ClangFunction cFunction = t.getClangFunction();
 		if (cFunction == null) {
 			return null;
@@ -67,44 +65,87 @@ public class TokenHighlights implements Iterable<HighlightToken> {
 		return highFunction.getFunction();
 	}
 
+	/**
+	 * Returns true if there are not highlights
+	 * @return true if there are not highlights
+	 */
 	public boolean isEmpty() {
 		return size() == 0;
 	}
 
+	/**
+	 * Returns the number of highlights
+	 * @return the number of highlights
+	 */
 	public int size() {
 		return highlightsByToken.size();
 	}
 
+	/**
+	 * Adds the given highlight to this container
+	 * @param t the highlight
+	 */
 	public void add(HighlightToken t) {
 		highlightsByToken.put(getKey(t), t);
 	}
 
+	/**
+	 * Gets the current highlight for the given token
+	 * @param t the token
+	 * @return the highlight
+	 */
 	public HighlightToken get(ClangToken t) {
 		return highlightsByToken.get(getKey(t));
 	}
 
+	/**
+	 * Returns all highlights for the given function
+	 * 
+	 * @param f the function
+	 * @return the highlights
+	 */
+	public Set<HighlightToken> getHighlightsByFunction(Function f) {
+		Set<HighlightToken> results = new HashSet<>();
+		Set<TokenKey> keys = getHighlightKeys(f);
+		for (TokenKey key : keys) {
+			HighlightToken hl = highlightsByToken.get(key);
+			results.add(hl);
+		}
+
+		return results;
+	}
+
+	/**
+	 * Returns true if this class has a highlight for the given token
+	 * @param t the token
+	 * @return true if this class has a highlight for the given token
+	 */
 	public boolean contains(ClangToken t) {
 		return highlightsByToken.containsKey(getKey(t));
 	}
 
+	/**
+	 * Removes all highlights from this container
+	 */
 	public void clear() {
 		highlightsByToken.clear();
 	}
 
-	public boolean contains(HighlightToken t) {
-		return highlightsByToken.containsKey(getKey(t));
-	}
-
-	// TODO examine this method and others for removal
-	public void remove(HighlightToken t) {
-		highlightsByToken.remove(getKey(t));
-	}
-
+	/**
+	 * Removes the highlight for the given token
+	 * @param t the token
+	 */
 	public void remove(ClangToken t) {
 		highlightsByToken.remove(getKey(t));
 	}
 
-	public Set<HighlightToken> removeTokensByFunction(Function function) {
+	/**
+	 * Removes all highlights associated with the given function
+	 * 
+	 * @param function the function
+	 * @return the removed highlights; empty if no highlights existed
+	 */
+	public Set<HighlightToken> removeHighlightsByFunction(Function function) {
 		Set<HighlightToken> oldHighlights = new HashSet<>();
 		Set<TokenKey> keys = getHighlightKeys(function);
 		for (TokenKey key : keys) {

Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/SliceHighlightColorProvider.java

@@ -24,7 +24,12 @@ import ghidra.app.decompiler.component.DecompilerUtils;
 import ghidra.program.model.pcode.PcodeOp;
 import ghidra.program.model.pcode.Varnode;
 
-// TODO docme
+/**
+ * A class to provider a color for highlight a variable using one of the 'slice' actions
+ * 
+ * @see ForwardSliceAction
+ * @see BackwardsSliceAction
+ */
 public class SliceHighlightColorProvider implements TokenHighlightColorProvider {
 
 	private Set<Varnode> varnodes;

Ghidra/Features/Decompiler/src/main/java/ghidra/app/plugin/core/decompile/actions/TokenHighlightColorProvider.java

@@ -24,6 +24,11 @@ import ghidra.app.decompiler.ClangToken;
  */
 public interface TokenHighlightColorProvider {
 
-	// TODO null if no match;  d0cme
+	/**
+	 * Returns a color for the given token
+	 * 
+	 * @param token the token
+	 * @return the color
+	 */
 	public Color getColor(ClangToken token);
 }

Ghidra/Features/Decompiler/src/test.slow/java/ghidra/app/plugin/core/decompile/DecompilerClangTest.java

@@ -841,10 +841,99 @@ public class DecompilerClangTest extends AbstractDecompilerTest {
 		assertEquals(myColor, hlColor2);
 	}
 
+	@Test
+	public void testSecondaryHighlighting_GetsReappliedAfterRefresh() {
+
+		/*
+		
+		 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();
+
+		Color color = highlight();
+		assertAllFieldsSecondaryHighlighted(token, color);
+
+		refresh();
+
+		setDecompilerLocation(line, charPosition);
+		token = getToken();
+		assertAllFieldsSecondaryHighlighted(token, color);
+	}
+
+	@Test
+	public void testSecondaryHighlighting_GetsReappliedAfterReturningToPreviousFunction() {
+
+		/*
+		
+		 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();
+
+		Color color = highlight();
+		assertAllFieldsSecondaryHighlighted(token, color);
+
+		decompile("100000bf0"); // 'main'
+		assertNoFieldsSecondaryHighlighted(token.getText());
+
+		decompile("100000d60"); // '_call_structure_A'
+		setDecompilerLocation(line, charPosition);
+		token = getToken();
+		assertAllFieldsSecondaryHighlighted(token, color);
+	}
+
 //==================================================================================================
 // Private Methods
 //==================================================================================================
 
+	private void refresh() {
+
+		DockingActionIf action = getAction(decompiler, "Refresh");
+		performAction(action);
+		waitForDecompiler();
+	}
+
 	private DecompilerProvider cloneDecompiler() {
 
 		DockingActionIf action = getAction(decompiler, "Decompile Clone");
@@ -1093,7 +1182,6 @@ public class DecompilerClangTest extends AbstractDecompilerTest {
 		DecompilerController controller = theProvider.getController();
 		DecompilerPanel panel = controller.getDecompilerPanel();
 		List<ClangToken> tokensWithName = panel.findTokensByName(name);
-		assertFalse(tokensWithName.isEmpty());
 		for (ClangToken otherToken : tokensWithName) {
 			if (ignore.test(otherToken)) {
 				continue;