Renaming adjustments

Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.cc

@@ -2457,6 +2457,14 @@ void ActionNameVars::linkSpacebaseSymbol(Varnode *vn,Funcdata &data,vector<Varno
   }
 }
 
+/// \brief Link formal Symbols to their HighVariable representative in the given Function
+///
+/// Run through all HighVariables for the given function and set up the explicit mapping with
+/// existing Symbol objects.  If there is no matching Symbol for a given HighVariable, a new
+/// Symbol is created. Any Symbol that does not have a name is added to a list for further
+/// name resolution.
+/// \param data is the given function
+/// \param namerec is the container for collecting Symbols with a name
 void ActionNameVars::linkSymbols(Funcdata &data,vector<Varnode *> &namerec)
 
 {
@@ -3870,8 +3878,6 @@ int4 ActionDynamicSymbols::apply(Funcdata &data)
   while(iter != enditer) {
     SymbolEntry *entry = &(*iter);
     ++iter;
-    // FIXME: Setting the symbol here (for the first time) gives the type no time to propagate properly
-    // Currently the casts aren't set properly
     if (data.attemptDynamicMappingLate(entry, dhash))
       count += 1;
   }
@@ -4857,6 +4863,7 @@ void universal_action(Architecture *conf)
   act->addAction( new ActionMergeMultiEntry("merge") );
   act->addAction( new ActionMergeCopy("merge") );
   act->addAction( new ActionDominantCopy("merge") );
+  act->addAction( new ActionDynamicSymbols("dynamic") );
   act->addAction( new ActionMarkIndirectOnly("merge") ); // Must come after required merges but before speculative
   act->addAction( new ActionMergeAdjacent("merge") );
   act->addAction( new ActionMergeType("merge") );

Ghidra/Features/Decompiler/src/decompile/cpp/funcdata.hh

@@ -362,7 +362,6 @@ public:
   bool ancestorOpUse(int4 maxlevel,const Varnode *invn,const PcodeOp *op,ParamTrial &trial) const;
   bool syncVarnodesWithSymbols(const ScopeLocal *lm,bool typesyes);
   void transferVarnodeProperties(Varnode *vn,Varnode *newVn,int4 lsbOffset);
-  void splitVarnode(Varnode *vn,int4 lowsize,Varnode *&vnlo,Varnode *& vnhi);
   bool fillinReadOnly(Varnode *vn);		///< Replace the given Varnode with its (constant) value in the load image
   bool replaceVolatile(Varnode *vn);		///< Replace accesses of the given Varnode with \e volatile operations
   void markIndirectOnly(void);			///< Mark \e illegal \e input Varnodes used only in INDIRECTs

Ghidra/Features/Decompiler/src/decompile/cpp/funcdata_op.cc

@@ -525,6 +525,7 @@ Varnode *Funcdata::opStackLoad(AddrSpace *spc,uintb off,uint4 sz,PcodeOp *op,Var
 /// CPUI_INT_MULT PcodeOp. If finalization is requested and a new PcodeOp is needed, the output
 /// Varnode is marked as \e implicit and has its data-type set
 /// \param op is the given PTRADD
+/// \param finalize is \b true if finalization is needed for any new PcodeOp
 void Funcdata::opUndoPtradd(PcodeOp *op,bool finalize)
 
 {

Ghidra/Features/Decompiler/src/decompile/cpp/merge.cc

@@ -282,7 +282,7 @@ void Merge::mergeOpcode(OpCode opc)
 	vn2 = op->getIn(j);
 	if (!mergeTestBasic(vn2)) continue;
 	if (mergeTestRequired(vn1->getHigh(),vn2->getHigh()))
-	  merge(vn1->getHigh(),vn2->getHigh(),true);	// Treat as speculative
+	  merge(vn1->getHigh(),vn2->getHigh(),false);	// This is a required merge
       }
     }
   }

Ghidra/Features/Decompiler/src/decompile/cpp/pcodeinject.hh

@@ -97,7 +97,7 @@ public:
   InjectPayload(const string &nm,int4 tp) { name=nm; type=tp; paramshift=0; dynamic = false; incidentalCopy = false; }	///< Construct for use with restoreXml
   int4 getParamShift(void) const { return paramshift; }	///< Get the number of parameters shifted
   bool isDynamic(void) const { return dynamic; }	///< Return \b true if p-code in the injection is generated dynamically
-  bool isIncidentalCopy(void) const { return incidentalCopy; }
+  bool isIncidentalCopy(void) const { return incidentalCopy; }	///< Return \b true if any injected COPY is considered \e incidental
   int4 sizeInput(void) const { return inputlist.size(); }	///< Return the number of input parameters
   int4 sizeOutput(void) const { return output.size(); }		///< Return the number of output parameters
   InjectParameter &getInput(int4 i) { return inputlist[i]; }	///< Get the i-th input parameter

Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc

@@ -1884,9 +1884,9 @@ int4 RuleDoubleShift::applyOp(PcodeOp *op,Funcdata &data)
 }
 
 /// \class RuleDoubleArithShift
-/// \brief Simply two sequential INT_SRIGHT: `(x s>> #c) s>> #d   =>  x s>> saturate(#c + #d)`
+/// \brief Simplify two sequential INT_SRIGHT: `(x s>> #c) s>> #d   =>  x s>> saturate(#c + #d)`
 ///
-/// Optimized division optimization in particular can produce a sequence of signed right shifts.
+/// Division optimization in particular can produce a sequence of signed right shifts.
 /// The shift amounts add up to the point where the sign bit has saturated the entire result.
 void RuleDoubleArithShift::getOpList(vector<uint4> &oplist) const
 

Ghidra/Features/Decompiler/src/decompile/cpp/transform.hh

@@ -97,12 +97,12 @@ public:
     LanedIterator(const LanedRegister *lanedR) { size = 0; mask = lanedR->sizeBitMask; normalize(); }	///< Constructor
     LanedIterator(void) { size = -1; mask = 0; }	///< Constructor for ending iterator
     LanedIterator &operator++(void) { size += 1; normalize(); return *this; }	///< Preincrement operator
-    int4 operator*(void) const { return size; }		/// Dereference operator
+    int4 operator*(void) const { return size; }		///< Dereference operator
     LanedIterator &operator=(const LanedIterator &op2) { size = op2.size; mask = op2.mask; return *this; }	///< Assignment
     bool operator==(const LanedIterator &op2) const { return (size == op2.size); }	///< Equal operator
     bool operator!=(const LanedIterator &op2) const { return (size != op2.size); }	///< Not-equal operator
   };
-  typedef LanedIterator const_iterator;
+  typedef LanedIterator const_iterator;		///< Iterator over possible lane sizes for this register
 private:
   int4 wholeSize;		///< Size of the whole register
   uint4 sizeBitMask;		///< A 1-bit for every permissible lane size

Ghidra/Features/Decompiler/src/decompile/cpp/variable.cc

@@ -23,10 +23,11 @@ HighVariable::HighVariable(Varnode *vn)
 
 {
   numMergeClasses = 1;
-  highflags = flagsdirty | typedirty | coverdirty;
+  highflags = flagsdirty | namerepdirty | typedirty | coverdirty;
   flags = 0;
   type = (Datatype *)0;
   symbol = (Symbol *)0;
+  nameRepresentative = (Varnode *)0;
   symboloffset = -1;
   inst.push_back(vn);
   vn->setHigh( this, numMergeClasses-1 );
@@ -66,7 +67,7 @@ void HighVariable::setSymbol(Varnode *vn) const
 /// is a constant address reference to the Symbol and the Varnode holds the reference, not
 /// the actual value of the Symbol.
 /// \param sym is the given Symbol to attach
-/// \off is the byte offset into the Symbol of the reference
+/// \param off is the byte offset into the Symbol of the reference
 void HighVariable::setSymbolReference(Symbol *sym,int4 off)
 
 {
@@ -229,18 +230,22 @@ bool HighVariable::compareName(Varnode *vn1,Varnode *vn2)
 Varnode *HighVariable::getNameRepresentative(void) const
 
 {
+  if ((highflags & namerepdirty)==0)
+    return nameRepresentative;		// Name representative is up to date
+  highflags &= ~namerepdirty;
+
   vector<Varnode *>::const_iterator iter;
-  Varnode *rep,*vn;
+  Varnode *vn;
 
   iter = inst.begin();
-  rep = *iter;
+  nameRepresentative = *iter;
   ++iter;
   for(;iter!=inst.end();++iter) {
     vn = *iter;
-    if (compareName(rep,vn))
+    if (compareName(nameRepresentative,vn))
-      rep = vn;
+      nameRepresentative = vn;
   }
-  return rep;
+  return nameRepresentative;
 }
 
 /// Search for the given Varnode and cut it out of the list, marking all properties as \e dirty.
@@ -254,7 +259,7 @@ void HighVariable::remove(Varnode *vn)
   for(;iter!=inst.end();++iter) {
     if (*iter == vn) {
       inst.erase(iter);
-      highflags |= (flagsdirty|coverdirty|typedirty);
+      highflags |= (flagsdirty|namerepdirty|coverdirty|typedirty);
       if (vn->getSymbolEntry() != (SymbolEntry *)0)
 	highflags |= symboldirty;
       return;
@@ -296,7 +301,7 @@ void HighVariable::merge(HighVariable *tv2,bool isspeculative)
 
   if (tv2 == this) return;
 
-  highflags |= (flagsdirty|typedirty);
+  highflags |= (flagsdirty|namerepdirty|typedirty);
   if (tv2->symbol != (Symbol *)0) {		// Check if we inherit a Symbol
     if ((tv2->highflags & symboldirty)==0) {
       symbol = tv2->symbol;			// Overwrite our Symbol (assume it is the same)

Ghidra/Features/Decompiler/src/decompile/cpp/variable.hh

@@ -44,13 +44,14 @@ public:
   /// So we keep track of when these inherited values are \e dirty
   enum {
     flagsdirty = 1,		///< Boolean properties for the HighVariable are dirty
-    typedirty = 2,		///< The data-type for the HighVariable is dirty
+    namerepdirty = 2,		///< The name representative for the HighVariable is dirty
-    coverdirty = 4,		///< The cover for the HighVariable is dirty
+    typedirty = 4,		///< The data-type for the HighVariable is dirty
-    symboldirty = 8,		///< The symbol attachment is dirty
+    coverdirty = 8,		///< The cover for the HighVariable is dirty
-    copy_in1 = 8,		///< There exists at least 1 COPY into \b this HighVariable from other HighVariables
+    symboldirty = 0x10,		///< The symbol attachment is dirty
-    copy_in2 = 16,		///< There exists at least 2 COPYs into \b this HighVariable from other HighVariables
+    copy_in1 = 0x20,		///< There exists at least 1 COPY into \b this HighVariable from other HighVariables
-    type_finalized = 32,	///< Set if a final data-type is locked in and dirtying is disabled
+    copy_in2 = 0x40,		///< There exists at least 2 COPYs into \b this HighVariable from other HighVariables
-    unmerged = 64		///< Set if part of a multi-entry Symbol but did not get merged with other SymbolEntrys
+    type_finalized = 0x80,	///< Set if a final data-type is locked in and dirtying is disabled
+    unmerged = 0x100		///< Set if part of a multi-entry Symbol but did not get merged with other SymbolEntrys
   };
 private:
   friend class Varnode;
@@ -59,7 +60,8 @@ private:
   int4 numMergeClasses;			///< Number of different speculative merge classes in \b this
   mutable uint4 highflags;		///< Dirtiness flags
   mutable uint4 flags;			///< Boolean properties inherited from Varnode members
-  mutable Datatype *type;		///< The data-type for this
+  mutable Datatype *type;		///< The data-type for \b this
+  mutable Varnode *nameRepresentative;	///< The storage location used to generate a Symbol name
   mutable Cover wholecover;		///< The ranges of code addresses covered by this HighVariable
   mutable Symbol *symbol;		///< The Symbol \b this HighVariable is tied to
   mutable int4 symboloffset;		///< -1=perfect symbol match >=0, offset
@@ -77,9 +79,9 @@ private:
   void merge(HighVariable *tv2,bool isspeculative);	///< Merge another HighVariable into \b this
   void setSymbol(Varnode *vn) const;		///< Update Symbol information for \b this from the given member Varnode
   void setSymbolReference(Symbol *sym,int4 off);	///< Attach a reference to a Symbol to \b this
-  void flagsDirty(void) const { highflags |= HighVariable::flagsdirty; }	///< Mark the boolean properties as \e dirty
+  void flagsDirty(void) const { highflags |= flagsdirty | namerepdirty; }	///< Mark the boolean properties as \e dirty
-  void coverDirty(void) const { highflags |= HighVariable::coverdirty; }	///< Mark the cover as \e dirty
+  void coverDirty(void) const { highflags |= coverdirty; }	///< Mark the cover as \e dirty
-  void typeDirty(void) const { highflags |= HighVariable::typedirty; }		///< Mark the data-type as \e dirty
+  void typeDirty(void) const { highflags |= typedirty; }	///< Mark the data-type as \e dirty
   void setUnmerged(void) const { highflags |= unmerged; }	///< Mark \b this as having merge problems
 public:
   HighVariable(Varnode *vn);		///< Construct a HighVariable with a single member Varnode

Ghidra/Features/Decompiler/src/decompile/cpp/varmap.hh

@@ -21,6 +21,11 @@
 
 #include "database.hh"
 
+/// \brief A symbol name recommendation with its associated storage location
+///
+/// The name is associated with a static Address and use point in the code. Symbols
+/// present at the end of function decompilation without a name can acquire \b this name
+/// if their storage matches.
 class NameRecommend {
   Address addr;			///< The starting address of the storage location
   Address useaddr;		///< The code address at the point of use
@@ -37,7 +42,7 @@ public:
   uint8 getSymbolId(void) const { return symbolId; }		///< Get the original Symbol id
 };
 
-/// \brief A name recommendation for a particular dynamic location
+/// \brief A name recommendation for a particular dynamic storage location
 ///
 /// A recommendation for a symbol name whose storage is dynamic. The storage
 /// is identified using the DynamicHash mechanism and may or may not exist.

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

@@ -23,15 +23,19 @@ import org.junit.Test;
 
 import docking.widgets.fieldpanel.field.Field;
 import docking.widgets.fieldpanel.support.FieldLocation;
+import ghidra.app.cmd.function.CreateFunctionCmd;
+import ghidra.app.cmd.function.DeleteFunctionCmd;
 import ghidra.app.decompiler.ClangToken;
 import ghidra.app.decompiler.ClangVariableToken;
 import ghidra.app.decompiler.component.ClangTextField;
 import ghidra.app.decompiler.component.DecompilerPanel;
-import ghidra.app.plugin.core.decompile.actions.RenameGlobalVariableTask;
+import ghidra.app.plugin.core.decompile.actions.*;
-import ghidra.app.plugin.core.decompile.actions.RenameVariableTask;
+import ghidra.framework.options.Options;
 import ghidra.program.database.symbol.CodeSymbol;
 import ghidra.program.model.address.Address;
-import ghidra.program.model.listing.Data;
+import ghidra.program.model.data.DataType;
+import ghidra.program.model.data.Undefined;
+import ghidra.program.model.listing.*;
 import ghidra.program.model.pcode.*;
 import ghidra.program.model.symbol.SourceType;
 import ghidra.program.model.symbol.Symbol;
@@ -85,6 +89,29 @@ public class HighSymbolTest extends AbstractDecompilerTest {
 		waitForDecompiler();
 	}
 
+	private void deleteFunction(String address) {
+		modifyProgram(p -> {
+			Address addr = p.getAddressFactory().getAddress(address);
+			DeleteFunctionCmd deleteCmd = new DeleteFunctionCmd(addr);
+			deleteCmd.applyTo(p);
+		});
+	}
+
+	private void createFunction(String address) {
+		modifyProgram(p -> {
+			Address addr = p.getAddressFactory().getAddress(address);
+			CreateFunctionCmd createCmd = new CreateFunctionCmd(addr);
+			createCmd.applyTo(p);
+		});
+	}
+
+	private void turnOffAnalysis() {
+		modifyProgram(p -> {
+			Options options = p.getOptions(Program.ANALYSIS_PROPERTIES);
+			options.setBoolean("Decompiler Parameter ID", false);
+			options.setBoolean("Stack", false);
+		});
+	}
 	private void renameVariable(HighSymbol highSymbol, ClangToken tokenAtCursor, String newName) {
 		RenameVariableTask rename =
 			new RenameVariableTask(provider.getTool(), highSymbol.getProgram(),
@@ -96,6 +123,16 @@ public class HighSymbolTest extends AbstractDecompilerTest {
 		waitForDecompiler();
 	}
 
+	private void isolateVariable(HighSymbol highSymbol, ClangToken tokenAtCursor, String newName) {
+		IsolateVariableTask isolate = new IsolateVariableTask(provider.getTool(), program,
+			provider.getDecompilerPanel(), tokenAtCursor, highSymbol, SourceType.USER_DEFINED);
+		assertTrue(isolate.isValid(newName));
+		modifyProgram(p -> {
+			isolate.commit();
+		});
+		waitForDecompiler();
+	}
+
 	private void renameExisting(HighSymbol highSymbol, ClangToken tokenAtCursor, String newName) {
 		SymbolEntry oldEntry = highSymbol.getFirstWholeMap();
 		long oldId = highSymbol.getId();
@@ -303,4 +340,64 @@ public class HighSymbolTest extends AbstractDecompilerTest {
 		entry = highSymbol.getFirstWholeMap();
 		assertEquals(usepoint, entry.getPCAdress());		// Make sure the same usepoint comes back
 	}
+
+	@Test
+	public void testHighSymbol_freeParameter() {
+		deleteFunction("10015a6");
+		turnOffAnalysis();
+		createFunction("10015a6");
+		decompile("10015a6");
+		ClangTextField line = getLineContaining("param_4 +");
+		FieldLocation loc = loc(line.getLineNumber(), 23);
+		ClangToken token = line.getToken(loc);
+		assertTrue(token instanceof ClangVariableToken);
+		HighSymbol highSymbol = token.getHighVariable().getSymbol();
+		renameVariable(highSymbol, token, "newParam");
+		line = getLineContaining("newParam +");
+		token = line.getToken(loc);
+		assertTrue(token instanceof ClangVariableToken);
+		HighVariable variable = token.getHighVariable();
+		assertTrue(variable instanceof HighParam);
+		assertEquals(((HighParam) variable).getSlot(), 3);
+		highSymbol = variable.getSymbol();
+		assertTrue(highSymbol.isNameLocked());
+		assertFalse(highSymbol.isTypeLocked());
+		Function function = highSymbol.getHighFunction().getFunction();
+		Parameter[] parameters = function.getParameters();
+		assertEquals(parameters.length, 4);
+		for (int i = 0; i < 4; ++i) {
+			DataType dt = parameters[i].getDataType();
+			assertTrue(Undefined.isUndefined(dt));
+			assertEquals(dt.getLength(), 4);
+		}
+		assertEquals(parameters[3].getName(), "newParam");
+	}
+
+	@Test
+	public void testHighSymbol_isolate() {
+		decompile("1002a22");
+		ClangTextField line = getLineContaining(" >> 1");
+		FieldLocation loc = loc(line.getLineNumber(), 2);
+		ClangToken token = line.getToken(loc);
+		assertTrue(token instanceof ClangVariableToken);
+		HighVariable variable = token.getHighVariable();
+		Varnode[] instances = variable.getInstances();
+		short maxMerge = 0;
+		for (Varnode vn : instances) {
+			if (vn.getMergeGroup() > maxMerge) {
+				maxMerge = vn.getMergeGroup();
+			}
+		}
+		assertEquals(maxMerge, 1);		// Make sure there are 2 merge groups
+		String name = token.getText();
+		isolateVariable(variable.getSymbol(), token, name);
+		line = getLineContaining(" >> 1");
+		token = line.getToken(loc);
+		variable = token.getHighVariable();
+		assertEquals(variable.getInstances().length, 1);
+		HighSymbol highSymbol = variable.getSymbol();
+		assertEquals(highSymbol.getName(), name);
+		assertTrue(highSymbol.isNameLocked());
+		assertTrue(highSymbol.isTypeLocked());
+	}
 }

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/pcode/DynamicEntry.java

@@ -89,24 +89,20 @@ public class DynamicEntry extends SymbolEntry {
 	}
 
 	/**
-	 * Build the dynamic storage object for a new DynamicEntry, given the underlying temporary
+	 * Build a new DynamicEntry, given the underlying temporary
-	 * Varnode and its function model.  The hash is created from local information in the
+	 * Varnode attached to a symbol.  The hash is created from local information in the
 	 * syntax tree near the Varnode.
 	 * @param vn is the underlying Varnode
-	 * @param high is the function model
+	 * @return the new DynamicEntry
-	 * @return the dynamic storage object
 	 */
-	public static VariableStorage buildDynamicStorage(Varnode vn, HighFunction high) {
+	public static DynamicEntry build(Varnode vn) {
-		DynamicHash dynamicHash = new DynamicHash(vn, high);
+		HighVariable highVariable = vn.getHigh();
-		Program program = high.getFunction().getProgram();
+		HighSymbol highSymbol = highVariable.getSymbol();
-		long ourHash = dynamicHash.getHash();
+		HighFunction highFunction = highSymbol.getHighFunction();
-		try {
+		DynamicHash dynamicHash = new DynamicHash(vn, highFunction);
-			return new VariableStorage(program, AddressSpace.HASH_SPACE.getAddress(ourHash),
+		DynamicEntry entry =
-				vn.getSize());
+			new DynamicEntry(highSymbol, dynamicHash.getAddress(), dynamicHash.getHash());
-		}
+		return entry;
-		catch (InvalidInputException e) {
-			throw new AssertException("Unexpected exception", e);
-		}
 	}
 
 	@Override

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/pcode/HighFunctionDBUtil.java

@@ -510,8 +510,9 @@ public class HighFunctionDBUtil {
 			HighVariable tmpHigh = highSymbol.getHighVariable();
 			if (!storage.isHashStorage() && tmpHigh != null &&
 				tmpHigh.requiresDynamicStorage()) {
-				storage =
+				DynamicEntry entry = DynamicEntry.build(tmpHigh.getRepresentative());
-					DynamicEntry.buildDynamicStorage(tmpHigh.getRepresentative(), highFunction);
+				storage = entry.getStorage();
+				pcAddr = entry.getPCAdress();	// The address may change from original Varnode
 			}
 			else {
 				Variable var = clearConflictingLocalVariables(function, storage, pcAddr);