Merge remote-tracking branch

'origin/GP-4434_ghidra1_FixDecompilerReturnParamCommit--SQUASHED'
(Closes #6318)

Ghidra/Features/Decompiler/ghidra_scripts/StringParameterPropagator.java

@@ -36,6 +36,7 @@ import ghidra.program.model.data.*;
 import ghidra.program.model.lang.PrototypeModel;
 import ghidra.program.model.listing.*;
 import ghidra.program.model.pcode.*;
+import ghidra.program.model.pcode.HighFunctionDBUtil.ReturnCommitOption;
 import ghidra.program.model.symbol.*;
 import ghidra.util.exception.*;
 
@@ -195,9 +196,10 @@ public class StringParameterPropagator extends GhidraScript {
 				int maxParams = funcInfo.getMaxParamsSeen();
 				boolean couldBeVararg = !funcInfo.numParamsAgree();
 				if (!funcInfo.numParamsAgree()) {
-					currentProgram.getBookmarkManager().setBookmark(calledFunc.getEntryPoint(),
+					currentProgram.getBookmarkManager()
-						BookmarkType.NOTE, this.getClass().getName(),
+							.setBookmark(calledFunc.getEntryPoint(), BookmarkType.NOTE,
-						"Number of parameters disagree min: " + minParams + " max: " + maxParams);
+								this.getClass().getName(), "Number of parameters disagree min: " +
+									minParams + " max: " + maxParams);
 
 					println("WARNING : Number of params disagree for " + calledFunc.getName() +
 						" @ " + entry);
@@ -317,9 +319,8 @@ public class StringParameterPropagator extends GhidraScript {
 						ReferenceIterator dataRefIter = rData.getReferenceIteratorTo();
 						while (dataRefIter.hasNext()) {
 							Reference dataRef = dataRefIter.next();
-							func =
+							func = currentProgram.getFunctionManager()
-								currentProgram.getFunctionManager().getFunctionContaining(
+									.getFunctionContaining(dataRef.getFromAddress());
-									dataRef.getFromAddress());
 							if (func == null) {
 								continue;
 							}
@@ -337,9 +338,8 @@ public class StringParameterPropagator extends GhidraScript {
 	private void collectDataRefenceLocations(HashSet<Address> dataItemLocationSet,
 			HashSet<Address> referringFuncLocationSet) {
 		int count = 0;
-		ReferenceIterator iter =
+		ReferenceIterator iter = currentProgram.getReferenceManager()
-			currentProgram.getReferenceManager().getReferenceIterator(
+				.getReferenceIterator(currentProgram.getMinAddress());
-				currentProgram.getMinAddress());
 		while (iter.hasNext() && !monitor.isCancelled()) {
 			Reference ref = iter.next();
 
@@ -412,7 +412,8 @@ public class StringParameterPropagator extends GhidraScript {
 		if (convention == null) {
 			convention = currentProgram.getCompilerSpec().getDefaultCallingConvention();
 		}
-		if (initialConvention != null && !convention.getName().equals(initialConvention.getName())) {
+		if (initialConvention != null &&
+			!convention.getName().equals(initialConvention.getName())) {
 			return true;
 		}
 
@@ -452,8 +453,9 @@ public class StringParameterPropagator extends GhidraScript {
 		if (param == null) {
 			return false;
 		}
-		currentProgram.getBookmarkManager().setBookmark(func.getEntryPoint(), BookmarkType.NOTE,
+		currentProgram.getBookmarkManager()
-			this.getClass().getName(), "Created " + dt.getName() + " parameter");
+				.setBookmark(func.getEntryPoint(), BookmarkType.NOTE, this.getClass().getName(),
+					"Created " + dt.getName() + " parameter");
 		return false;
 	}
 
@@ -476,7 +478,8 @@ public class StringParameterPropagator extends GhidraScript {
 		}
 		if (minParams == numParams) {
 			try {
-				HighFunctionDBUtil.commitParamsToDatabase(hfunction, true, SourceType.USER_DEFINED);
+				HighFunctionDBUtil.commitParamsToDatabase(hfunction, true,
+					ReturnCommitOption.NO_COMMIT, SourceType.USER_DEFINED);
 			}
 			catch (DuplicateNameException e) {
 				throw new AssertException("Unexpected exception", e);
@@ -497,9 +500,8 @@ public class StringParameterPropagator extends GhidraScript {
 			if (i < f.getParameterCount()) {
 				continue;
 			}
-			VariableStorage storage =
+			VariableStorage storage = convention.getArgLocation(i - 1, f.getParameters(),
-				convention.getArgLocation(i - 1, f.getParameters(), DataType.DEFAULT,
+				DataType.DEFAULT, currentProgram);
-					currentProgram);
 			if (storage.isUnassignedStorage()) {
 				break;
 			}
@@ -576,7 +578,8 @@ public class StringParameterPropagator extends GhidraScript {
 					}
 
 					long mask =
-						0xffffffffffffffffL >>> ((8 - entry.getAddressSpace().getPointerSize()) * 8);
+						0xffffffffffffffffL >>> ((8 - entry.getAddressSpace().getPointerSize()) *
+							8);
 					Address possibleAddr = entry.getNewAddress(mask & value);
 					if (stringLocationSet.contains(possibleAddr)) {
 						markStringParam(constUse, possibleAddr, calledFuncAddr, i - 1,
@@ -637,9 +640,8 @@ public class StringParameterPropagator extends GhidraScript {
 			return true;
 
 		try {
-			DecompileResults decompRes =
+			DecompileResults decompRes = decompInterface.decompileFunction(f,
-				decompInterface.decompileFunction(f,
+				decompInterface.getOptions().getDefaultTimeout(), monitor);
-					decompInterface.getOptions().getDefaultTimeout(), monitor);
 
 			hfunction = decompRes.getHighFunction();
 		}

Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerParameterIdCmd.java

@@ -32,6 +32,7 @@ import ghidra.program.model.lang.CompilerSpec;
 import ghidra.program.model.listing.*;
 import ghidra.program.model.mem.MemoryBlock;
 import ghidra.program.model.pcode.*;
+import ghidra.program.model.pcode.HighFunctionDBUtil.ReturnCommitOption;
 import ghidra.program.model.symbol.SourceType;
 import ghidra.program.model.util.AcyclicCallGraphBuilder;
 import ghidra.util.Msg;
@@ -215,17 +216,10 @@ public class DecompilerParameterIdCmd extends BackgroundCommand<Program> {
 					if (hfunc == null) {
 						return;
 					}
-					HighFunctionDBUtil.commitParamsToDatabase(hfunc, true, SourceType.ANALYSIS);
+					ReturnCommitOption returnCommit = commitVoidReturn ? ReturnCommitOption.COMMIT
-					boolean commitReturn = true;
+							: ReturnCommitOption.COMMIT_NO_VOID;
-					if (!commitVoidReturn) {
+					HighFunctionDBUtil.commitParamsToDatabase(hfunc, true, returnCommit,
-						DataType returnType = hfunc.getFunctionPrototype().getReturnType();
+						SourceType.ANALYSIS);
-						if (returnType instanceof VoidDataType) {
-							commitReturn = false;
-						}
-					}
-					if (commitReturn) {
-						HighFunctionDBUtil.commitReturnToDatabase(hfunc, SourceType.ANALYSIS);
-					}
 					goodInfo = true;
 				}
 				else {

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

@@ -24,6 +24,7 @@ import ghidra.app.util.HelpTopics;
 import ghidra.program.model.listing.Program;
 import ghidra.program.model.pcode.HighFunction;
 import ghidra.program.model.pcode.HighFunctionDBUtil;
+import ghidra.program.model.pcode.HighFunctionDBUtil.ReturnCommitOption;
 import ghidra.program.model.symbol.SourceType;
 import ghidra.util.HelpLocation;
 import ghidra.util.Msg;
@@ -59,8 +60,8 @@ public class CommitParamsAction extends AbstractDecompilerAction {
 				source = SourceType.USER_DEFINED;
 			}
 
-			HighFunctionDBUtil.commitReturnToDatabase(hfunc, source);
+			HighFunctionDBUtil.commitParamsToDatabase(hfunc, true, ReturnCommitOption.COMMIT,
-			HighFunctionDBUtil.commitParamsToDatabase(hfunc, true, source);
+				source);
 		}
 		catch (DuplicateNameException e) {
 			throw new AssertException("Unexpected exception", e);

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

@@ -21,6 +21,7 @@ import ghidra.framework.plugintool.PluginTool;
 import ghidra.program.model.listing.Function;
 import ghidra.program.model.listing.Program;
 import ghidra.program.model.pcode.*;
+import ghidra.program.model.pcode.HighFunctionDBUtil.ReturnCommitOption;
 import ghidra.program.model.symbol.SourceType;
 import ghidra.util.exception.DuplicateNameException;
 import ghidra.util.exception.InvalidInputException;
@@ -49,10 +50,8 @@ public class RenameVariableTask extends RenameTask {
 	@Override
 	public void commit() throws DuplicateNameException, InvalidInputException {
 		if (commitRequired) {
-			HighFunctionDBUtil.commitParamsToDatabase(hfunction, false, signatureSrcType);
+			HighFunctionDBUtil.commitParamsToDatabase(hfunction, false,
-			if (signatureSrcType != SourceType.DEFAULT) {
+				ReturnCommitOption.NO_COMMIT, signatureSrcType);
-				HighFunctionDBUtil.commitReturnToDatabase(hfunction, signatureSrcType);
-			}
 		}
 		HighFunctionDBUtil.updateDBVariable(highSymbol, newName, null, srctype);
 	}

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

@@ -29,6 +29,7 @@ import ghidra.program.model.data.DataTypeManager;
 import ghidra.program.model.listing.Function;
 import ghidra.program.model.listing.Program;
 import ghidra.program.model.pcode.*;
+import ghidra.program.model.pcode.HighFunctionDBUtil.ReturnCommitOption;
 import ghidra.program.model.symbol.SourceType;
 import ghidra.util.*;
 import ghidra.util.exception.*;
@@ -96,11 +97,7 @@ public class RetypeLocalAction extends AbstractDecompilerAction {
 					hfunction.getFunction().getSignatureSource() != SourceType.DEFAULT;
 				try {
 					HighFunctionDBUtil.commitParamsToDatabase(hfunction, useDataTypes,
-						SourceType.USER_DEFINED);
+						ReturnCommitOption.NO_COMMIT, SourceType.USER_DEFINED);
-					if (useDataTypes) {
-						HighFunctionDBUtil.commitReturnToDatabase(hfunction,
-							SourceType.USER_DEFINED);
-					}
 				}
 				catch (DuplicateNameException e) {
 					throw new AssertException("Unexpected exception", e);

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

@@ -32,6 +32,7 @@ import ghidra.program.model.listing.Function;
 import ghidra.program.model.listing.Program;
 import ghidra.program.model.pcode.HighFunction;
 import ghidra.program.model.pcode.HighFunctionDBUtil;
+import ghidra.program.model.pcode.HighFunctionDBUtil.ReturnCommitOption;
 import ghidra.program.model.symbol.SourceType;
 import ghidra.util.*;
 import ghidra.util.exception.*;
@@ -97,7 +98,7 @@ public class RetypeReturnAction extends AbstractDecompilerAction {
 			if (commitRequired) {
 				try {
 					HighFunctionDBUtil.commitParamsToDatabase(highFunction, true,
-						SourceType.USER_DEFINED);
+						ReturnCommitOption.NO_COMMIT, SourceType.USER_DEFINED);
 				}
 				catch (DuplicateNameException e) {
 					throw new AssertException("Unexpected exception", e);

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

@@ -72,54 +72,109 @@ public class HighFunctionDBUtil {
 		return modelName;
 	}
 
-	/**
+	public enum ReturnCommitOption {
-	 * Commit the decompiler's version of the function return data-type to the database.
+		/**
-	 * The decompiler's version of the prototype model is committed as well
+		 * {@link #NO_COMMIT} - keep functions existing return parameter
-	 * @param highFunction is the decompiler's model of the function
+		 */
-	 * @param source is the desired SourceType for the commit
+		NO_COMMIT,
-	 */
-	public static void commitReturnToDatabase(HighFunction highFunction, SourceType source) {
-		try {
 
-			// Change calling convention if needed
+		/**
-			Function function = highFunction.getFunction();
+		 * {@link #COMMIT} - commit return parameter as defined by {@link HighFunction}
-			String convention = function.getCallingConventionName();
+		 */
-			String modelName = getPrototypeModelForCommit(highFunction);
+		COMMIT,
-			if (modelName != null && !modelName.equals(convention)) {
-				function.setCallingConvention(modelName);
-			}
 
-			VariableStorage storage = highFunction.getFunctionPrototype().getReturnStorage();
+		/**
-			DataType dataType = highFunction.getFunctionPrototype().getReturnType();
+		 * {@value #COMMIT_NO_VOID} - commit return parameter as defined by {@link HighFunction}
-			if (dataType == null) {
+		 * unless it is {@link VoidDataType} in which case keep existing function return parameter.
-				dataType = DefaultDataType.dataType;
+		 */
-				source = SourceType.DEFAULT;
+		COMMIT_NO_VOID;
-			}
-			function.setReturn(dataType, storage, source);
-		}
-		catch (InvalidInputException e) {
-			Msg.error(HighFunctionDBUtil.class, e.getMessage());
-		}
 	}
 
 	/**
-	 * Commit all parameters associated with HighFunction to the underlying database.
+	 * Commit all parameters, including optional return, associated with HighFunction to the 
+	 * underlying database.
 	 * @param highFunction is the associated HighFunction
 	 * @param useDataTypes is true if the HighFunction's parameter data-types should be committed
+	 * @param returnCommit controls optional commit of return parameter
 	 * @param source is the signature source type to set
 	 * @throws DuplicateNameException if commit of parameters caused conflict with other
 	 * local variable/label.
 	 * @throws InvalidInputException if specified storage is invalid
 	 */
 	public static void commitParamsToDatabase(HighFunction highFunction, boolean useDataTypes,
-			SourceType source) throws DuplicateNameException, InvalidInputException {
+			ReturnCommitOption returnCommit, SourceType source)
+			throws DuplicateNameException, InvalidInputException {
 		Function function = highFunction.getFunction();
 
+		Parameter returnParam;
+		if (returnCommit == ReturnCommitOption.NO_COMMIT) {
+			returnParam = function.getReturn();
+		}
+		else {
+			returnParam = getReturnParameter(highFunction, useDataTypes, returnCommit);
+		}
+
 		List<Parameter> params = getParameters(highFunction, useDataTypes);
+		boolean hasVarArgs = highFunction.getFunctionPrototype().isVarArg();
 
 		String modelName = getPrototypeModelForCommit(highFunction);
-		commitParamsToDatabase(function, modelName, params,
+
-			highFunction.getFunctionPrototype().isVarArg(), true, source);
+		try {
+			function.updateFunction(modelName, returnParam, params,
+				FunctionUpdateType.DYNAMIC_STORAGE_ALL_PARAMS, true, source);
+		}
+		catch (DuplicateNameException e) {
+			for (Variable param : params) {
+				changeConflictingSymbolNames(param.getName(), returnParam, function);
+			}
+			function.updateFunction(modelName, null, params,
+				FunctionUpdateType.DYNAMIC_STORAGE_ALL_PARAMS, true, source);
+		}
+
+		boolean customStorageReqd =
+			!VariableUtilities.storageMatches(params, function.getParameters());
+		if (returnCommit != ReturnCommitOption.NO_COMMIT) {
+			customStorageReqd |=
+				!returnParam.getVariableStorage().equals(function.getReturn().getVariableStorage());
+		}
+		if (customStorageReqd) {
+			// try again if dynamic storage assignment does not match decompiler's
+			// force into custom storage mode
+			function.updateFunction(modelName, returnParam, params,
+				FunctionUpdateType.CUSTOM_STORAGE, true, source);
+		}
+
+		if (function.hasVarArgs() != hasVarArgs) {
+			function.setVarArgs(hasVarArgs);
+		}
+	}
+
+	private static Parameter getReturnParameter(HighFunction highFunction, boolean useDataTypes,
+			ReturnCommitOption returnCommit) throws InvalidInputException {
+		Function function = highFunction.getFunction();
+		if (returnCommit == ReturnCommitOption.NO_COMMIT) {
+			return function.getReturn();
+		}
+
+		Program program = function.getProgram();
+		DataTypeManager dtm = program.getDataTypeManager();
+
+		VariableStorage returnStorage = highFunction.getFunctionPrototype().getReturnStorage();
+		DataType returnDt = highFunction.getFunctionPrototype().getReturnType();
+
+		if (useDataTypes && returnCommit == ReturnCommitOption.COMMIT_NO_VOID &&
+			(returnDt instanceof VoidDataType)) {
+			return function.getReturn(); // retain current return
+		}
+
+		if (returnDt == null) {
+			returnDt = DefaultDataType.dataType;
+			returnStorage = VariableStorage.UNASSIGNED_STORAGE;
+		}
+		else if (!useDataTypes) {
+			returnDt = Undefined.getUndefinedDataType(returnDt.getLength()).clone(dtm);
+		}
+		return new ReturnParameterImpl(returnDt, returnStorage, program);
 	}
 
 	private static List<Parameter> getParameters(HighFunction highFunction, boolean useDataTypes)
@@ -146,54 +201,6 @@ public class HighFunctionDBUtil {
 		return params;
 	}
 
-	/**
-	 * Commit a specified set of parameters for the given function to the database.
-	 * The name, data-type, and storage is committed for each parameter.  The parameters are
-	 * provided along with a formal PrototypeModel.  If the parameters fit the model, they are
-	 * committed using "dynamic" storage. Otherwise, they are committed using "custom" storage.
-	 * @param function is the Function being modified
-	 * @param modelName is the name of the underlying PrototypeModel
-	 * @param params is the formal list of parameter objects
-	 * @param hasVarArgs is true if the prototype can take variable arguments
-	 * @param renameConflicts if true any name conflicts will be resolved
-	 * by renaming the conflicting local variable/label
-	 * @param source source type
-	 * @throws DuplicateNameException if commit of parameters caused conflict with other
-	 * local variable/label.  Should not occur if renameConflicts is true.
-	 * @throws InvalidInputException for invalid variable names or for parameter data-types that aren't fixed length
-	 * @throws DuplicateNameException is there are collisions between variable names in the function's scope 
-	 */
-	public static void commitParamsToDatabase(Function function, String modelName,
-			List<Parameter> params, boolean hasVarArgs, boolean renameConflicts, SourceType source)
-			throws DuplicateNameException, InvalidInputException {
-
-		try {
-			function.updateFunction(modelName, null, params,
-				FunctionUpdateType.DYNAMIC_STORAGE_ALL_PARAMS, true, source);
-		}
-		catch (DuplicateNameException e) {
-			if (!renameConflicts) {
-				throw e;
-			}
-			for (Variable param : params) {
-				changeConflictingSymbolNames(param.getName(), null, function);
-			}
-			function.updateFunction(modelName, null, params,
-				FunctionUpdateType.DYNAMIC_STORAGE_ALL_PARAMS, true, source);
-		}
-
-		if (!VariableUtilities.storageMatches(params, function.getParameters())) {
-			// try again if dynamic storage assignment does not match decompiler's
-			// force into custom storage mode
-			function.updateFunction(modelName, null, params, FunctionUpdateType.CUSTOM_STORAGE,
-				true, source);
-		}
-
-		if (function.hasVarArgs() != hasVarArgs) {
-			function.setVarArgs(hasVarArgs);
-		}
-	}
-
 	private static void changeConflictingSymbolNames(String name, Variable ignoreVariable,
 			Function func) {
 
@@ -454,7 +461,8 @@ public class HighFunctionDBUtil {
 		if (slot >= parameters.length ||
 			!parameters[slot].getVariableStorage().equals(param.getStorage())) {
 			try {
-				commitParamsToDatabase(highFunction, true, SourceType.ANALYSIS);
+				commitParamsToDatabase(highFunction, true, ReturnCommitOption.NO_COMMIT,
+					SourceType.ANALYSIS);
 			}
 			catch (DuplicateNameException e) {
 				throw new AssertException("Unexpected exception", e);