GP-6801 - Data Type Export - Fixed potential infinity with a large number of composites

Ghidra/Features/Decompiler/src/main/java/ghidra/app/util/exporter/CppExporter.java

@@ -295,7 +295,7 @@ public class CppExporter extends Exporter {
 			DataTypeWriter dataTypeWriter =
 				new DataTypeWriter(dtm, headerWriter, isUseCppStyleComments);
 			headerWriter.write(getFakeCTypeDefinitions(dtm.getDataOrganization()));
-			dataTypeWriter.write(dtm, monitor);
+			dataTypeWriter.write(monitor);
 
 			headerWriter.println("");
 			headerWriter.println("");
@@ -308,7 +308,7 @@ public class CppExporter extends Exporter {
 			DataTypeManager dtm = program.getDataTypeManager();
 			DataTypeWriter dataTypeWriter =
 				new DataTypeWriter(dtm, cFileWriter, isUseCppStyleComments);
-			dataTypeWriter.write(dtm, monitor);
+			dataTypeWriter.write(monitor);
 		}
 
 		if (cFileWriter != null) {

Ghidra/Framework/Generic/src/main/java/ghidra/util/graph/AbstractDependencyGraph.java

@@ -237,7 +237,7 @@ public abstract class AbstractDependencyGraph<T> {
 	 * graph, dependencies will be removed and additional values will be eligible to be returned
 	 * by this method.  Once a value has been retrieved using this method, it will be considered
 	 * "visited" and future calls to this method will not include those values.  To continue
-	 * processing the values in the graph, all values return from this method should eventually
+	 * processing the values in the graph, all values returned from this method should eventually
 	 * be deleted from the graph to "free up" other values.  NOTE: values retrieved by this method
 	 * will no longer be eligible for return by the pop() method. 
 	 *

Ghidra/Framework/Generic/src/main/java/ghidra/util/graph/DeterministicDependencyGraph.java

@@ -25,8 +25,9 @@ import org.apache.commons.collections4.set.ListOrderedSet;
  * memory than {@link DependencyGraph}, and if memory is not an issue, it also seems to be
  * slightly faster as well.
  * <P>
- * This class was implemented to provide determinism while doing
+ * This class was implemented to provide determinism while doing developmental debugging.
- * developmental debugging.
+ * <P>
+ * Note: the types used in this graph must be comparable in order to provider consistent ordering.
  *
  * @param <T> the type of value.
  * 

Ghidra/Framework/Generic/src/test/java/ghidra/util/datastruct/DependencyGraphTest.java

@@ -19,7 +19,6 @@ import static org.junit.Assert.*;
 
 import java.util.*;
 
-import org.junit.Assert;
 import org.junit.Test;
 
 import ghidra.util.graph.*;
@@ -186,7 +185,7 @@ public class DependencyGraphTest {
 			while (!graph.isEmpty()) {
 				graph.pop();
 			}
-			Assert.fail("Expected cycle exception");
+			fail("Expected cycle exception");
 		}
 		catch (IllegalStateException e) {
 			// expected
@@ -285,28 +284,26 @@ public class DependencyGraphTest {
 	/**
 	 * Given a dependency map, does the captured linear order of execution 
 	 * satisfy the ordering constraints?
-	 * @param <T> the type of the keys being compared
 	 * @param dependencyGraph a map where keys are predecessors and values 
 	 *        are successors that depend on the respective key
 	 * @param visitedOrder the actual execution order to be tested
-	 * @return
 	 */
 	private void checkOrderSatisfiesDependencies(AbstractDependencyGraph<String> dependencyGraph,
 			List<String> visitedOrder) {
 
 		if (visitedOrder.size() > dependencyGraph.size()) {
-			Assert.fail("More items were visited than the number of items in the graph");
+			fail("More items were visited than the number of items in the graph");
 		}
 		if (visitedOrder.size() < dependencyGraph.size()) {
-			Assert.fail("Not all items in the graph were visited");
+			fail("Not all items in the graph were visited");
 		}
 
-		HashSet<String> items = new HashSet<>(visitedOrder);
+		Set<String> items = new HashSet<>(visitedOrder);
 		if (items.size() != visitedOrder.size()) {
-			Assert.fail("duplicate item(s) in linearOrder\n");
+			fail("duplicate item(s) in linearOrder\n");
 		}
 
-		HashMap<String, Integer> visitedOrderMap = new HashMap<>();
+		Map<String, Integer> visitedOrderMap = new HashMap<>();
 		for (int i = 0; i < visitedOrder.size(); i++) {
 			visitedOrderMap.put(visitedOrder.get(i), i);
 		}
@@ -314,21 +311,21 @@ public class DependencyGraphTest {
 		for (String key : dependencyGraph.getValues()) {
 			Integer visitedOrdinal = visitedOrderMap.get(key);
 			if (visitedOrdinal == null) {
-				Assert.fail("dependencyGraph key " + key + " not in linearOrder\n");
+				fail("dependencyGraph key " + key + " not in linearOrder\n");
 			}
 
 			Set<String> dependents = dependencyGraph.getDependentValues(key);
 			for (String dependent : dependents) {
 				if (key.equals(dependent)) {
-					Assert.fail("dependencyGraph key " + key + " depends on itself\n");
+					fail("dependencyGraph key " + key + " depends on itself\n");
 				}
 				Integer dependentVisitedOrdinal = visitedOrderMap.get(dependent);
 				if (dependentVisitedOrdinal == null) {
-					Assert.fail("dependent " + dependent + " of dependencyGraph key " + key +
+					fail("dependent " + dependent + " of dependencyGraph key " + key +
 						" not in linearOrder\n");
 				}
 				if (dependentVisitedOrdinal <= visitedOrdinal) {
-					Assert.fail("dependent " + dependent + " of dependencyGraph key " + key +
+					fail("dependent " + dependent + " of dependencyGraph key " + key +
 						" came first (" + dependentVisitedOrdinal + " < " + visitedOrdinal + ")\n");
 				}
 			}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/DataTypeWriter.java

@@ -23,6 +23,7 @@ import org.apache.commons.lang3.StringUtils;
 
 import ghidra.util.Msg;
 import ghidra.util.exception.CancelledException;
+import ghidra.util.graph.DeterministicDependencyGraph;
 import ghidra.util.task.TaskMonitor;
 
 /**
@@ -43,9 +44,11 @@ public class DataTypeWriter {
 
 	private Set<DataType> resolved = new HashSet<>();
 	private Map<String, DataType> resolvedTypeMap = new HashMap<>();
-	private Set<Composite> deferredCompositeDeclarations = new HashSet<>();
+
-	private ArrayDeque<DataType> deferredTypeFIFO = new ArrayDeque<>();
+	private DeterministicDependencyGraph<CompositeNode> compositeDependencyGraph =
-	private Set<DataType> deferredTypes = new HashSet<>();
+		new DeterministicDependencyGraph<>();
+	private LinkedHashSet<DataType> deferredCompositeInternalTypes = new LinkedHashSet<>();
+
 	private int writerDepth = 0;
 	private Writer writer;
 	private DataTypeManager dtm;
@@ -126,14 +129,13 @@ public class DataTypeWriter {
 
 	/**
 	 * Converts all data types in the data type manager into ANSI-C code.
-	 * @param dataTypeManager the manager containing the data types to write
 	 * @param monitor the task monitor
 	 * @throws IOException if there is an exception writing the output
 	 * @throws CancelledException if the action is cancelled by the user
 	 */
-	public void write(DataTypeManager dataTypeManager, TaskMonitor monitor)
+	public void write(TaskMonitor monitor)
 			throws IOException, CancelledException {
-		write(dataTypeManager.getRootCategory(), monitor);
+		write(dtm.getRootCategory(), monitor);
 	}
 
 	/**
@@ -199,9 +201,8 @@ public class DataTypeWriter {
 	}
 
 	private void deferWrite(DataType dt) {
-		if (!resolved.contains(dt) && !deferredTypes.contains(dt)) {
+		if (!resolved.contains(dt)) {
-			deferredTypes.add(dt);
+			deferredCompositeInternalTypes.add(dt);
-			deferredTypeFIFO.addLast(dt);
 		}
 	}
 
@@ -222,6 +223,9 @@ public class DataTypeWriter {
 	 */
 	private void doWrite(DataType dt, TaskMonitor monitor, boolean throwExceptionOnInvalidType)
 			throws IOException, CancelledException {
+
+		monitor.checkCancelled();
+
 		if (dt == null) {
 			return;
 		}
@@ -280,27 +284,25 @@ public class DataTypeWriter {
 			writer.write(EOL);
 			writer.write(EOL);
 		}
-		else if (dt instanceof Dynamic) {
+		else if (dt instanceof Dynamic dynamic) {
-			writeDynamicBuiltIn((Dynamic) dt, monitor);
+			writeDynamicBuiltIn(dynamic, monitor);
 		}
-		else if (dt instanceof Structure) {
+		else if (dt instanceof Structure struct) {
-			Structure struct = (Structure) dt;
 			writeCompositePreDeclaration(struct, monitor);
-			deferredCompositeDeclarations.add(struct);
+			addCompositeToDependencyGraph(struct, monitor);
 		}
-		else if (dt instanceof Union) {
+		else if (dt instanceof Union union) {
-			Union union = (Union) dt;
 			writeCompositePreDeclaration(union, monitor);
-			deferredCompositeDeclarations.add(union);
+			addCompositeToDependencyGraph(union, monitor);
 		}
-		else if (dt instanceof Enum) {
+		else if (dt instanceof Enum enumm) {
-			writeEnum((Enum) dt, monitor);
+			writeEnum(enumm, monitor);
 		}
-		else if (dt instanceof TypeDef) {
+		else if (dt instanceof TypeDef typeDef) {
-			writeTypeDef((TypeDef) dt, monitor);
+			writeTypeDef(typeDef, monitor);
 		}
-		else if (dt instanceof BuiltInDataType) {
+		else if (dt instanceof BuiltInDataType bidt) {
-			writeBuiltIn((BuiltInDataType) dt, monitor);
+			writeBuiltIn(bidt, monitor);
 		}
 		else if (dt instanceof BitFieldDataType) {
 			// skip
@@ -319,15 +321,34 @@ public class DataTypeWriter {
 		--writerDepth;
 	}
 
+	private void addCompositeToDependencyGraph(Composite composite, TaskMonitor monitor)
+			throws CancelledException {
+
+		// Each composite will be a node in the graph. Composite dependencies are added below.
+		compositeDependencyGraph.addValue(new CompositeNode(composite));
+
+		for (DataTypeComponent component : composite.getDefinedComponents()) {
+
+			monitor.checkCancelled();
+
+			DataType dt = component.getDataType();
+			if (dt instanceof Composite childComposite) {
+				CompositeNode start = new CompositeNode(composite);
+				CompositeNode end = new CompositeNode(childComposite);
+				compositeDependencyGraph.addDependency(start, end);
+			}
+		}
+	}
+
 	private void writeDeferredDeclarations(TaskMonitor monitor)
 			throws IOException, CancelledException {
-		while (!deferredTypes.isEmpty()) {
+
-			DataType dt = deferredTypeFIFO.removeFirst();
+		while (!deferredCompositeInternalTypes.isEmpty()) {
-			deferredTypes.remove(dt);
+			DataType dt = deferredCompositeInternalTypes.removeFirst();
 			write(dt, monitor);
 		}
+
 		writeDeferredCompositeDeclarations(monitor);
-		deferredCompositeDeclarations.clear();
 	}
 
 	private DataType getBaseArrayTypedefType(DataType dt) {
@@ -345,81 +366,38 @@ public class DataTypeWriter {
 		return dt;
 	}
 
-	private boolean containsComposite(Composite container, Composite contained) {
-		for (DataTypeComponent component : container.getDefinedComponents()) {
-			DataType dt = getBaseArrayTypedefType(component.getDataType());
-			if (dt instanceof Composite && dt.getName().equals(contained.getName()) &&
-				dt.isEquivalent(contained)) {
-				return true;
-			}
-		}
-		return false;
-	}
-
 	private void writeDeferredCompositeDeclarations(TaskMonitor monitor)
 			throws IOException, CancelledException {
-		int cnt = deferredCompositeDeclarations.size();
-		if (cnt == 0) {
-			return;
-		}
 
-		LinkedList<Composite> list = new LinkedList<>(deferredCompositeDeclarations);
+		CompositeNode node = compositeDependencyGraph.pop();
-		if (list.size() > 1) {
+		while (node != null) {
-			// Establish dependency ordering
+			writeCompositeBody(node.composite, monitor);
-			int sortChange = 1;
+			node = compositeDependencyGraph.pop();
-			while (sortChange != 0) {
-				sortChange = 0;
-				for (int i = cnt - 1; i > 0; i--) {
-					if (resortComposites(list, i)) {
-						++sortChange;
-					}
-				}
-			}
 		}
-
-		for (Composite composite : list) {
-			writeCompositeBody(composite, monitor);
-		}
-	}
-
-	private boolean resortComposites(List<Composite> list, int index) {
-		int listSize = list.size();
-		if (listSize <= 0) {
-			return false;
-		}
-		Composite composite = list.get(index);
-		for (int i = 0; i < index; i++) {
-			Composite other = list.get(i);
-			if (containsComposite(other, composite)) {
-				list.remove(index);
-				list.add(i, composite);
-				composite = null;
-				return true;
-			}
-		}
-		return false;
 	}
 
 	private String getDynamicComponentString(Dynamic dynamicType, String fieldName, int length) {
-		if (dynamicType.canSpecifyLength()) {
+		if (!dynamicType.canSpecifyLength()) {
-			DataType replacementBaseType = dynamicType.getReplacementBaseType();
+			return null;
-			if (replacementBaseType != null) {
-				replacementBaseType = replacementBaseType.clone(dtm);
-				int elementLen = replacementBaseType.getLength();
-				if (elementLen <= 0) {
-					Msg.error(this,
-						dynamicType.getClass().getSimpleName() +
-							" returned bad replacementBaseType: " +
-							replacementBaseType.getClass().getSimpleName());
-				}
-				else {
-					int elementCnt = (length + elementLen - 1) / elementLen;
-					return replacementBaseType.getDisplayName() + " " + fieldName + "[" +
-						elementCnt + "]";
-				}
-			}
 		}
-		return null;
+
+		DataType replacementBaseType = dynamicType.getReplacementBaseType();
+		if (replacementBaseType == null) {
+			return null;
+		}
+
+		replacementBaseType = replacementBaseType.clone(dtm);
+		int elementLen = replacementBaseType.getLength();
+		if (elementLen <= 0) {
+			String dynamicName = dynamicType.getClass().getSimpleName();
+			String replacementName = replacementBaseType.getClass().getSimpleName();
+			Msg.error(this, dynamicName + " returned bad replacementBaseType: " + replacementName);
+			return null;
+		}
+
+		int elementCnt = (length + elementLen - 1) / elementLen;
+		return replacementBaseType.getDisplayName() + " " + fieldName + "[" +
+			elementCnt + "]";
 	}
 
 	private void writeCompositePreDeclaration(Composite composite, TaskMonitor monitor)
@@ -433,16 +411,12 @@ public class DataTypeWriter {
 		writer.write(EOL);
 		writer.write(EOL);
 
-		for (DataTypeComponent component : composite.getComponents()) {
+		for (DataTypeComponent component : composite.getDefinedComponents()) {
-			if (monitor.isCancelled()) {
+			monitor.checkCancelled();
-				break;
+
-			}
 			// force resolution of field datatype
 			DataType componentType = component.getDataType();
 			deferWrite(componentType);
-
-			// TODO the return value of this is not used--delete?
-			getTypeDeclaration(null, componentType, component.getLength(), true, monitor);
 		}
 	}
 
@@ -642,7 +616,8 @@ public class DataTypeWriter {
 					return;
 				}
 			}
-			// TODO: A comment explaining the special 'P' case would be helpful!!  Smells like fish.
+
+			// auto-pointer-typedef generated with composite
 			else if (baseType instanceof Pointer && typedefName.startsWith("P")) {
 				DataType dt = ((Pointer) baseType).getDataType();
 				if (dt instanceof TypeDef) {
@@ -891,4 +866,47 @@ public class DataTypeWriter {
 		buf.append(")");
 		return buf.toString();
 	}
+
+	// A simple Composite class to use in the dependency graph to speed up the equals() call
+	private class CompositeNode implements Comparable<CompositeNode> {
+
+		private Composite composite;
+
+		CompositeNode(Composite composite) {
+			this.composite = composite;
+		}
+
+		@Override
+		public int compareTo(CompositeNode o) {
+			String pn1 = composite.getPathName();
+			String pn2 = o.composite.getPathName();
+			return pn1.compareTo(pn2);
+		}
+
+		@Override
+		public int hashCode() {
+			return composite.hashCode();
+		}
+
+		@Override
+		public boolean equals(Object obj) {
+			if (this == obj) {
+				return true;
+			}
+			if (obj == null) {
+				return false;
+			}
+			if (getClass() != obj.getClass()) {
+				return false;
+			}
+
+			CompositeNode other = (CompositeNode) obj;
+			return composite.getUniversalID().equals(other.composite.getUniversalID());
+		}
+
+		@Override
+		public String toString() {
+			return composite.toString();
+		}
+	}
 }