GP-4078 added delayed pointer resolve logic to FunctionDefinitionDB

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CompositeDB.java

@@ -213,12 +213,11 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal {
 		try {
 			checkDeleted();
 			record.setString(CompositeDBAdapter.COMPOSITE_COMMENT_COL, desc);
-			try {
+			compositeAdapter.updateRecord(record, true);
-				compositeAdapter.updateRecord(record, true);
+			dataMgr.dataTypeChanged(this, false);
-			}
+		}
-			catch (IOException e) {
+		catch (IOException e) {
-				dataMgr.dbError(e);
+			dataMgr.dbError(e);
-			}
 		}
 		finally {
 			lock.release();

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeManagerDB.java

@@ -1372,25 +1372,23 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 			SourceArchive sourceArchive) throws DataTypeDependencyException {
 
 		try {
-			if (existingDataType instanceof StructureDB) {
+			if (existingDataType instanceof StructureDB existingStruct) {
-				if (!(dataType instanceof StructureInternal)) {
+				if (!(dataType instanceof StructureInternal replacementStruct)) {
 					return false;
 				}
-				StructureDB existingStruct = (StructureDB) existingDataType;
+				existingStruct.doReplaceWith(replacementStruct, true);
-				existingStruct.doReplaceWith((StructureInternal) dataType, true);
 			}
-			else if (existingDataType instanceof UnionDB) {
+			else if (existingDataType instanceof UnionDB existingUnion) {
-				if (!(dataType instanceof UnionInternal)) {
+				if (!(dataType instanceof UnionInternal replacementUnion)) {
 					return false;
 				}
-				UnionDB existingUnion = (UnionDB) existingDataType;
+				existingUnion.doReplaceWith(replacementUnion, true);
-				existingUnion.doReplaceWith((UnionInternal) dataType, true);
 			}
-			else if (existingDataType instanceof FunctionDefinitionDB) {
+			else if (existingDataType instanceof FunctionDefinitionDB existingFuncDef) {
-				if (!(dataType instanceof FunctionDefinition)) {
+				if (!(dataType instanceof FunctionDefinition replacementFuncDef)) {
 					return false;
 				}
-				existingDataType.replaceWith(dataType);
+				existingFuncDef.doReplaceWith(replacementFuncDef, true);
 			}
 			else if (existingDataType instanceof EnumDB) {
 				if (!(dataType instanceof Enum)) {
@@ -3015,7 +3013,6 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 			category.dataTypeAdded(structDB);
 
 			structDB.doReplaceWith(struct, false);
-			structDB.setDescription(struct.getDescription());
 
 			// doReplaceWith may have updated the last change time so set it back to what we want.
 			structDB.setLastChangeTime(struct.getLastChangeTime());
@@ -3079,7 +3076,6 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 			category.dataTypeAdded(unionDB);
 
 			unionDB.doReplaceWith(union, false);
-			unionDB.setDescription(union.getDescription());
 
 			// doReplaceWith updated the last change time so set it back to what we want.
 			unionDB.setLastChangeTime(union.getLastChangeTime());
@@ -3326,8 +3322,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 			// Make sure category knows about function definition before args/return resolved
 			cat.dataTypeAdded(funDefDb);
 
-			funDefDb.setArguments(funDef.getArguments());
+			funDefDb.doReplaceWith(funDef, false);
-			funDefDb.setReturnType(funDef.getReturnType());
 
 			// setArguments updated the last change time so set it back to what we want.
 			funDefDb.setLastChangeTime(funDef.getLastChangeTime());

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/FunctionDefinitionDB.java

@@ -16,7 +16,8 @@
 package ghidra.program.database.data;
 
 import java.io.IOException;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collections;
 
 import db.DBRecord;
 import db.Field;
@@ -153,10 +154,10 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 	}
 
 	@Override
-	public ParameterDefinition[] getArguments() {
+	public ParameterDefinitionDB[] getArguments() {
 		lock.acquire();
 		try {
-			ParameterDefinition[] vars = new ParameterDefinition[parameters.size()];
+			ParameterDefinitionDB[] vars = new ParameterDefinitionDB[parameters.size()];
 			return parameters.toArray(vars);
 		}
 		finally {
@@ -183,41 +184,77 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 
 	@Override
 	public void replaceWith(DataType dataType) {
-		if (!(dataType instanceof FunctionDefinition)) {
+		if (!(dataType instanceof FunctionDefinition functionDefinition)) {
 			throw new IllegalArgumentException();
 		}
-		doReplaceWith((FunctionDefinition) dataType);
-	}
-
-	private void doReplaceWith(FunctionDefinition functionDefinition) {
-
 		lock.acquire();
+		boolean isResolveCacheOwner = dataMgr.activateResolveCache();
 		try {
 			checkDeleted();
-			setArguments(functionDefinition.getArguments());
+			doReplaceWith(functionDefinition, true);
-			try {
-				setReturnType(functionDefinition.getReturnType());
-			}
-			catch (IllegalArgumentException e) {
-				setReturnType(DEFAULT);
-			}
-			setVarArgs(functionDefinition.hasVarArgs());
-			setNoReturn(functionDefinition.hasNoReturn());
-			try {
-				setCallingConvention(functionDefinition.getCallingConventionName(), false);
-			}
-			catch (InvalidInputException e) {
-				// will not happen
-			}
 		}
 		catch (IOException e) {
 			dataMgr.dbError(e);
 		}
 		finally {
+			if (isResolveCacheOwner) {
+				dataMgr.flushResolveQueue(true);
+			}
 			lock.release();
 		}
 	}
 
+	void doReplaceWith(FunctionDefinition functionDefinition, boolean notify) throws IOException {
+		doSetArguments(functionDefinition.getArguments(), true, false);
+		try {
+			doSetReturnType(functionDefinition.getReturnType(), true, false);
+		}
+		catch (IllegalArgumentException e) {
+			setReturnType(DEFAULT);
+		}
+		doSetVarArgs(functionDefinition.hasVarArgs(), false);
+		doSetNoReturn(functionDefinition.hasNoReturn(), false);
+		try {
+			doSetCallingConvention(functionDefinition.getCallingConventionName(), false, false);
+		}
+		catch (InvalidInputException e) {
+			// will not happen
+		}
+
+		record.setString(FunctionDefinitionDBAdapter.FUNCTION_DEF_COMMENT_COL,
+			functionDefinition.getComment());
+		funDefAdapter.updateRecord(record, false);
+
+		if (notify) {
+			dataMgr.dataTypeChanged(this, false);
+		}
+
+		if (pointerPostResolveRequired) {
+			dataMgr.queuePostResolve(this, functionDefinition);
+		}
+	}
+
+	@Override
+	protected void postPointerResolve(DataType definitionDt, DataTypeConflictHandler handler) {
+		FunctionDefinition funcDef = (FunctionDefinition) definitionDt;
+		ParameterDefinition[] definedArguments = funcDef.getArguments();
+		ParameterDefinitionDB[] myArguments = getArguments();
+		if (definedArguments.length != myArguments.length) {
+			throw new IllegalArgumentException("mismatched definition datatype");
+		}
+		for (int i = 0; i < definedArguments.length; i++) {
+			ParameterDefinition arg = definedArguments[i];
+			DataType dt = arg.getDataType();
+			if (dt instanceof Pointer) {
+				myArguments[i].doSetDataType(dt, false);
+			}
+		}
+		DataType dt = funcDef.getReturnType();
+		if (dt instanceof Pointer) {
+			doSetReturnType(dt, false, false);
+		}
+	}
+
 	@Override
 	public String getComment() {
 		lock.acquire();
@@ -274,29 +311,20 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 		return getPrototypeString();
 	}
 
+	private DataType doCheckedResolve(DataType dt) {
+		if (dt instanceof Pointer) {
+			pointerPostResolveRequired = true;
+			return resolve(((Pointer) dt).newPointer(DataType.DEFAULT));
+		}
+		return resolve(dt);
+	}
+
 	@Override
 	public void setArguments(ParameterDefinition[] args) {
 		lock.acquire();
 		try {
 			checkDeleted();
-			Iterator<ParameterDefinitionDB> it = parameters.iterator();
+			doSetArguments(args, false, true);
-			while (it.hasNext()) {
-				ParameterDefinitionDB param = it.next();
-				param.getDataType().removeParent(this);
-				paramAdapter.removeRecord(param.getKey());
-			}
-			parameters.clear();
-			for (int i = 0; i < args.length; i++) {
-				DataType type =
-					ParameterDefinitionImpl.validateDataType(args[i].getDataType(), dataMgr, false);
-				DataType resolvedDt = resolve(type);
-				paramAdapter.createRecord(dataMgr.getID(resolvedDt), key, i, args[i].getName(),
-					args[i].getComment(), args[i].getLength());
-				resolvedDt.addParent(this);
-			}
-			loadParameters();
-			funDefAdapter.updateRecord(record, true); // update last change time
-			dataMgr.dataTypeChanged(this, false);
 		}
 		catch (IOException e) {
 			dataMgr.dbError(e);
@@ -306,29 +334,60 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 		}
 	}
 
+	private void doSetArguments(ParameterDefinition[] args, boolean checkedResolveReqd,
+			boolean notify) throws IOException {
+
+		for (ParameterDefinitionDB param : parameters) {
+			param.getDataType().removeParent(this);
+			paramAdapter.removeRecord(param.getKey());
+		}
+		parameters.clear();
+		for (int i = 0; i < args.length; i++) {
+			DataType type =
+				ParameterDefinitionImpl.validateDataType(args[i].getDataType(), dataMgr, false);
+			DataType resolvedDt = checkedResolveReqd ? doCheckedResolve(type) : resolve(type);
+			paramAdapter.createRecord(dataMgr.getID(resolvedDt), key, i, args[i].getName(),
+				args[i].getComment(), args[i].getLength());
+			resolvedDt.addParent(this);
+		}
+		loadParameters();
+		funDefAdapter.updateRecord(record, true); // update last change time
+		if (notify) {
+			dataMgr.dataTypeChanged(this, false);
+		}
+	}
+
 	@Override
 	public void setReturnType(DataType type) {
 		type = ParameterDefinitionImpl.validateDataType(type, dataMgr, true);
 		lock.acquire();
 		try {
 			checkDeleted();
+			doSetReturnType(type, false, true);
+		}
+		finally {
+			lock.release();
+		}
+	}
+
+	private void doSetReturnType(DataType type, boolean checkedResolveReqd, boolean notify) {
+		try {
 			getReturnType().removeParent(this);
 			if (type == null) {
 				type = DataType.DEFAULT;
 			}
-			DataType resolvedDt = resolve(type);
+			DataType resolvedDt = checkedResolveReqd ? doCheckedResolve(type) : resolve(type);
 			record.setLongValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_RETURN_ID_COL,
 				dataMgr.getID(resolvedDt));
 			funDefAdapter.updateRecord(record, true);
 			resolvedDt.addParent(this);
-			dataMgr.dataTypeChanged(this, false);
+			if (notify) {
+				dataMgr.dataTypeChanged(this, false);
+			}
 		}
 		catch (IOException e) {
 			dataMgr.dbError(e);
 		}
-		finally {
-			lock.release();
-		}
 	}
 
 	@Override
@@ -558,59 +617,79 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 		lock.acquire();
 		try {
 			checkDeleted();
-			byte flags = record.getByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_FLAGS_COL);
+			doSetVarArgs(hasVarArgs, true);
-			if (hasVarArgs) {
+		}
-				flags |= FunctionDefinitionDBAdapter.FUNCTION_DEF_VARARG_FLAG;
+		catch (IOException e) {
-			}
+			dataMgr.dbError(e);
-			else {
-				flags &= ~FunctionDefinitionDBAdapter.FUNCTION_DEF_VARARG_FLAG;
-			}
-			record.setByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_FLAGS_COL, flags);
-			try {
-				funDefAdapter.updateRecord(record, true);
-				dataMgr.dataTypeChanged(this, false);
-			}
-			catch (IOException e) {
-				dataMgr.dbError(e);
-			}
 		}
 		finally {
 			lock.release();
 		}
 	}
 
+	private void doSetVarArgs(boolean hasVarArgs, boolean notify) throws IOException {
+
+		if (hasVarArgs == hasVarArgs()) {
+			return;
+		}
+
+		byte flags = record.getByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_FLAGS_COL);
+		if (hasVarArgs) {
+			flags |= FunctionDefinitionDBAdapter.FUNCTION_DEF_VARARG_FLAG;
+		}
+		else {
+			flags &= ~FunctionDefinitionDBAdapter.FUNCTION_DEF_VARARG_FLAG;
+		}
+		record.setByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_FLAGS_COL, flags);
+		funDefAdapter.updateRecord(record, true);
+
+		if (notify) {
+			dataMgr.dataTypeChanged(this, false);
+		}
+	}
+
 	@Override
 	public void setNoReturn(boolean hasNoReturn) {
 		lock.acquire();
 		try {
 			checkDeleted();
-			byte flags = record.getByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_FLAGS_COL);
+			doSetNoReturn(hasNoReturn, true);
-			if (hasNoReturn) {
+		}
-				flags |= FunctionDefinitionDBAdapter.FUNCTION_DEF_NORETURN_FLAG;
+		catch (IOException e) {
-			}
+			dataMgr.dbError(e);
-			else {
-				flags &= ~FunctionDefinitionDBAdapter.FUNCTION_DEF_NORETURN_FLAG;
-			}
-			record.setByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_FLAGS_COL, flags);
-			try {
-				funDefAdapter.updateRecord(record, true);
-				dataMgr.dataTypeChanged(this, false);
-			}
-			catch (IOException e) {
-				dataMgr.dbError(e);
-			}
 		}
 		finally {
 			lock.release();
 		}
 	}
 
+	private void doSetNoReturn(boolean hasNoReturn, boolean notify) throws IOException {
+
+		if (hasNoReturn == hasNoReturn()) {
+			return;
+		}
+
+		byte flags = record.getByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_FLAGS_COL);
+		if (hasNoReturn) {
+			flags |= FunctionDefinitionDBAdapter.FUNCTION_DEF_NORETURN_FLAG;
+		}
+		else {
+			flags &= ~FunctionDefinitionDBAdapter.FUNCTION_DEF_NORETURN_FLAG;
+		}
+		record.setByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_FLAGS_COL, flags);
+		funDefAdapter.updateRecord(record, true);
+
+		if (notify) {
+			dataMgr.dataTypeChanged(this, false);
+		}
+	}
+
 	@Override
 	public void setGenericCallingConvention(GenericCallingConvention genericCallingConvention) {
 		lock.acquire();
 		try {
 			checkDeleted();
-			setCallingConvention(genericCallingConvention.name(), false);
+			doSetCallingConvention(genericCallingConvention.name(), false, true);
 		}
 		catch (IOException e) {
 			dataMgr.dbError(e);
@@ -628,7 +707,7 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 		lock.acquire();
 		try {
 			checkDeleted();
-			setCallingConvention(conventionName, true);
+			doSetCallingConvention(conventionName, true, true);
 		}
 		catch (IOException e) {
 			dataMgr.dbError(e);
@@ -638,12 +717,14 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 		}
 	}
 
-	private void setCallingConvention(String conventionName, boolean restrictive)
+	private void doSetCallingConvention(String conventionName, boolean restrictive, boolean notify)
 			throws InvalidInputException, IOException {
 		byte id = dataMgr.getCallingConventionID(conventionName, restrictive);
 		record.setByteValue(FunctionDefinitionDBAdapter.FUNCTION_DEF_CALLCONV_COL, id);
 		funDefAdapter.updateRecord(record, true);
-		dataMgr.dataTypeChanged(this, false);
+		if (notify) {
+			dataMgr.dataTypeChanged(this, false);
+		}
 	}
 
 	@Override

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/ParameterDefinitionDB.java

@@ -61,19 +61,32 @@ final class ParameterDefinitionDB implements ParameterDefinition {
 
 	@Override
 	public void setDataType(DataType type) {
-		type = ParameterDefinitionImpl.validateDataType(type, dataMgr, false);
+		// TODO: This is not a DatabaseObject so it lacks ability to refresh properly
+		// and parameter objects are not singletons.  It also lacks locking.
+		// There is risk of using a stale or deleted parameter object which could lead 
+		// to corruption of a function definition and datatype parent tracking.
 
-		getDataType().removeParent(parent);
+		doSetDataType(type, true);
+	}
 
-		type = dataMgr.resolve(type, null);
+	void doSetDataType(DataType type, boolean notify) {
-		type.addParent(parent);
-
-		record.setLongValue(FunctionParameterAdapter.PARAMETER_DT_ID_COL,
-			dataMgr.getResolvedID(type));
-		record.setIntValue(FunctionParameterAdapter.PARAMETER_DT_LENGTH_COL, type.getLength());
 		try {
+
+			type = ParameterDefinitionImpl.validateDataType(type, dataMgr, false);
+
+			getDataType().removeParent(parent);
+
+			type = dataMgr.resolve(type, null);
+			type.addParent(parent);
+
+			record.setLongValue(FunctionParameterAdapter.PARAMETER_DT_ID_COL,
+				dataMgr.getResolvedID(type));
+			record.setIntValue(FunctionParameterAdapter.PARAMETER_DT_LENGTH_COL, type.getLength());
 			adapter.updateRecord(record);
-			dataMgr.dataTypeChanged(parent, false);
+
+			if (notify) {
+				dataMgr.dataTypeChanged(parent, false);
+			}
 		}
 		catch (IOException e) {
 			dataMgr.dbError(e);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/StructureDB.java

@@ -1645,6 +1645,7 @@ class StructureDB extends CompositeDB implements StructureInternal {
 		record.setIntValue(CompositeDBAdapter.COMPOSITE_NUM_COMPONENTS_COL, numComponents);
 		record.setIntValue(CompositeDBAdapter.COMPOSITE_LENGTH_COL, structLength);
 		record.setIntValue(CompositeDBAdapter.COMPOSITE_ALIGNMENT_COL, structAlignment);
+		record.setString(CompositeDBAdapter.COMPOSITE_COMMENT_COL, struct.getDescription());
 		compositeAdapter.updateRecord(record, true); // updates timestamp
 
 		if (notify) {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/UnionDB.java

@@ -344,6 +344,9 @@ class UnionDB extends CompositeDB implements UnionInternal {
 			doAdd(resolvedDts[i], dtc.getLength(), dtc.getFieldName(), dtc.getComment(), false);
 		}
 
+		record.setString(CompositeDBAdapter.COMPOSITE_COMMENT_COL, union.getDescription());
+		compositeAdapter.updateRecord(record, false);
+
 		repack(false, false); // updates timestamp
 
 		if (notify) {