GP-260 Added ability to fixup composites due to primitive datatype size

change such as a data origanization may cause.

Ghidra/Features/Base/ghidra_scripts/FixupCompositeDataTypesScript.java

@@ -0,0 +1,47 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+//
+// Fixes up all composite datatypes within the current program to account for 
+// any changes to primitive datatype sizes or alignment rules as defined
+// by the associated data organization.
+//
+// This script requires exclusive access to the program to avoid the possibilty
+// of excessive change conflicts.
+//
+// This script can be run multiple times without harm
+//@category Data Types
+import ghidra.app.script.GhidraScript;
+import ghidra.program.database.data.DataTypeManagerDB;
+
+public class FixupCompositeDataTypesScript extends GhidraScript {
+
+	@Override
+	protected void run() throws Exception {
+
+		if (currentProgram == null) {
+			return;
+		}
+		
+		if (!currentProgram.hasExclusiveAccess()) {
+			popup("This script requires an exclusive checkout of the program");
+			return;
+		}
+
+		DataTypeManagerDB dtm = (DataTypeManagerDB) currentProgram.getDataTypeManager();
+		dtm.fixupComposites(monitor);
+	}
+
+}

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/datamgr/actions/AlignAllDataTypesAction.java

@@ -89,6 +89,7 @@ public class AlignAllDataTypesAction extends DockingAction {
 						"Are you sure you want to align all of the data types in " +
 							dataTypeManager.getName() +
 							"?\nBoth structures and unions that are currently unaligned will become aligned.\n" +
+							"This could cause component offsets to change and datatype sizes to change.\n" +
 							"Do you want to continue?", "Continue", OptionDialog.WARNING_MESSAGE);
 				if (result == OptionDialog.CANCEL_OPTION) {
 					return;
@@ -125,7 +126,7 @@ public class AlignAllDataTypesAction extends DockingAction {
 
 	private void alignEachStructure(DataTypeManager dataTypeManager,
 			DataOrganization dataOrganization) {
-		Iterator<Composite> allComposites = dataTypeManager.getAllComposites();
+		Iterator<? extends Composite> allComposites = dataTypeManager.getAllComposites();
 		while (allComposites.hasNext()) {
 			Composite composite = allComposites.next();
 			composite.setInternallyAligned(true);

Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/ApplyDataTypes.java

@@ -24,7 +24,6 @@ import ghidra.app.util.bin.format.pdb.PdbParser.PdbXmlMember;
 import ghidra.app.util.importer.MessageLog;
 import ghidra.graph.*;
 import ghidra.graph.algo.GraphNavigator;
-import ghidra.graph.jung.JungDirectedGraph;
 import ghidra.program.model.data.Composite;
 import ghidra.program.model.data.DataType;
 import ghidra.program.model.symbol.SymbolUtilities;
@@ -67,8 +66,8 @@ public class ApplyDataTypes {
 	private List<CompositeDefinition> getCompositeDefinitionsInPostDependencyOrder(
 			TaskMonitor monitor) {
 
-		JungDirectedGraph<CompositeDefinition, GEdge<CompositeDefinition>> graph =
-			new JungDirectedGraph<>();
+		GDirectedGraph<CompositeDefinition, GEdge<CompositeDefinition>> graph =
+			GraphFactory.createDirectedGraph();
 		for (CompositeDefinition compositeDefinition : compositeQueue.values()) {
 			graph.addVertex(compositeDefinition);
 			for (PdbMember m : compositeDefinition.memberList) {

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

@@ -69,26 +69,20 @@ abstract class CompositeDB extends DataTypeDB implements Composite {
 	protected abstract void initialize();
 
 	/**
-	 * Get the preferred length for a new component. For Unions and internally
-	 * aligned structures the preferred component length for a fixed-length dataType
-	 * will be the length of that dataType. Otherwise the length returned will be no
-	 * larger than the specified length.
-	 * 
+	 * Get the preferred length for a new component. Constraining length of fixed-length datatype
+	 * may not be sustainable in response to datatype size changes over time.
 	 * @param dataType new component datatype
-	 * @param length   constrained length or -1 to force use of dataType size.
-	 *                 Dynamic types such as string must have a positive length
-	 *                 specified.
+	 * @param length   specified length required for Dynamic types such as string 
+	 * 				   which must have a positive length specified.
 	 * @return preferred component length
 	 */
 	protected int getPreferredComponentLength(DataType dataType, int length) {
-		if ((isInternallyAligned() || (this instanceof Union)) && !(dataType instanceof Dynamic)) {
-			length = -1; // force use of datatype size
+		if (length > 0 && (dataType instanceof Composite) &&
+			((Composite) dataType).isNotYetDefined()) {
+			return length;
 		}
 		int dtLength = dataType.getLength();
-		if (length <= 0) {
-			length = dtLength;
-		}
-		else if (dtLength > 0 && dtLength < length) {
+		if (dtLength > 0) {
 			length = dtLength;
 		}
 		if (length <= 0) {
@@ -789,4 +783,13 @@ abstract class CompositeDB extends DataTypeDB implements Composite {
 		}
 		return " pack(" + packingValue + ")";
 	}
+
+	/**
+	 * Perform any neccessary component adjustments based on
+	 * sizes and alignment of components differing from their 
+	 * specification which may be influenced by the data organization.
+	 * If this composite changes parents will not be
+	 * notified - handling this is the caller's responsibility.
+	 */
+	protected abstract void fixupComponents();
 }

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

@@ -28,6 +28,8 @@ import generic.jar.ResourceFile;
 import ghidra.app.plugin.core.datamgr.archive.BuiltInSourceArchive;
 import ghidra.framework.store.db.PackedDBHandle;
 import ghidra.framework.store.db.PackedDatabase;
+import ghidra.graph.*;
+import ghidra.graph.algo.GraphNavigator;
 import ghidra.program.database.*;
 import ghidra.program.database.map.AddressMap;
 import ghidra.program.model.address.Address;
@@ -2711,7 +2713,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 
 	class StructureIterator implements Iterator<Structure> {
 		private RecordIterator it;
-		private Structure nextStruct;
+		private StructureDB nextStruct;
 
 		StructureIterator() throws IOException {
 			it = compositeAdapter.getRecords();
@@ -2731,9 +2733,9 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		}
 
 		@Override
-		public Structure next() {
+		public StructureDB next() {
 			if (hasNext()) {
-				Structure s = nextStruct;
+				StructureDB s = nextStruct;
 				nextStruct = null;
 				return s;
 			}
@@ -2746,7 +2748,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 					Record rec = it.next();
 					DataType dt = getDataType(rec.getKey(), rec);
 					if (dt instanceof Structure) {
-						nextStruct = (Structure) dt;
+						nextStruct = (StructureDB) dt;
 						return;
 					}
 				}
@@ -2759,7 +2761,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 
 	class CompositeIterator implements Iterator<Composite> {
 		private RecordIterator it;
-		private Composite nextComposite;
+		private CompositeDB nextComposite;
 
 		CompositeIterator() throws IOException {
 			it = compositeAdapter.getRecords();
@@ -2779,9 +2781,9 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		}
 
 		@Override
-		public Composite next() {
+		public CompositeDB next() {
 			if (hasNext()) {
-				Composite c = nextComposite;
+				CompositeDB c = nextComposite;
 				nextComposite = null;
 				return c;
 			}
@@ -2792,7 +2794,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 			try {
 				if (it.hasNext()) {
 					Record rec = it.next();
-					nextComposite = (Composite) getDataType(rec.getKey(), rec);
+					nextComposite = (CompositeDB) getDataType(rec.getKey(), rec);
 				}
 			}
 			catch (IOException e) {
@@ -3787,6 +3789,99 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		}
 	}
 
+	/**
+	 * Fixup all composites and thier components which may be affected by a data organization
+	 * change include primitive type size changes and alignment changes.  It is highly recommended
+	 * that this program be open with exclusive access before invoking this method to avoid 
+	 * excessive merge conflicts with other users.
+	 * @param monitor task monitor
+	 * @throws CancelledException if operation is cancelled
+	 */
+	public void fixupComposites(TaskMonitor monitor) throws CancelledException {
+		lock.acquire();
+		try {
+
+			// NOTE: Any composite could be indirectly affected by a component size change
+			// based upon type relationships
+
+			// NOTE: Composites brought in from archive may have incorrect component size
+			// if not aligned and should not be used to guage a primitive size change
+
+			// Unfortunately parent table does not track use of primitives so a brute
+			// force search is required.  Since all composites must be checked, this 
+			// is combined with the composite graph generation to get ordered list
+			// of composites for subsequent size change operation.
+
+			List<CompositeDB> orderedComposites = getAllCompositesInPostDependencyOrder(monitor);
+
+			monitor.setProgress(0);
+			monitor.setMaximum(orderedComposites.size());
+			monitor.setMessage("Updating Datatype Sizes...");
+			
+			int count = 0;
+			for (CompositeDB c : orderedComposites) {
+				monitor.checkCanceled();
+				c.fixupComponents();
+				monitor.setProgress(++count);
+			}
+
+		}
+		finally {
+			lock.release();
+		}
+	}
+
+	/**
+	 * Get composite base type which corresponds to a specified datatype.
+	 * Pointers to composites are ignored.  This method is intended to be
+	 * used by the {@link #getAllCompositesInPostDependencyOrder} method only.
+	 * @param dt datatype
+	 * @return base datatype if dt corresponds to a composite or array of composites, 
+	 * otherwise null is returned
+	 */
+	private CompositeDB getCompositeBaseType(DataType dt) {
+		while ((dt instanceof Array) || (dt instanceof TypeDef)) {
+			if (dt instanceof Array) {
+				dt = ((Array) dt).getDataType();
+			}
+			else {
+				dt = ((TypeDef) dt).getBaseDataType();
+			}
+		}
+		return (dt instanceof CompositeDB) ? (CompositeDB) dt : null;
+	}
+
+	/*
+	 * Graph all composites return an ordered list with leaves returned first and detect
+	 * primitve size changes based upon specified primitiveTypeIds.  It is assumed TypeDef
+	 * use of primitives have already be handled elsewhere.
+	 * All pointers are ignored and not followed during graph generation.
+	 * This method is intended to facilitate datatype size change propogation in an 
+	 * orderly fashion to reduce size change propogation.
+	
+	 * @param monitor task monitor
+	 * @return order list of composites
+	 * @throws CancelledException if task cancelled
+	 */
+	private List<CompositeDB> getAllCompositesInPostDependencyOrder(TaskMonitor monitor)
+			throws CancelledException {
+
+		GDirectedGraph<CompositeDB, GEdge<CompositeDB>> graph = GraphFactory.createDirectedGraph();
+		Iterator<Composite> allComposites = getAllComposites();
+		while (allComposites.hasNext()) {
+			monitor.checkCanceled();
+			CompositeDB c = (CompositeDB) allComposites.next();
+			graph.addVertex(c);
+			for (DataTypeComponent m : c.getDefinedComponents()) {
+				CompositeDB refC = getCompositeBaseType(m.getDataType());
+				if (refC != null) {
+					graph.addEdge(new DefaultGEdge<CompositeDB>(c, refC));
+				}
+			}
+		}
+		return GraphAlgorithms.getVerticesInPostOrder(graph, GraphNavigator.topDownNavigator());
+	}
+
 	/**
 	 * Activate resolveCache and associated resolveQueue if not already active. If
 	 * this method returns true caller is responsible for flushing resolveQueue and

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

@@ -590,8 +590,7 @@ class StructureDB extends CompositeDB implements Structure {
 			if (equals(dataType)) {
 				return true;
 			}
-			for (int i = 0; i < components.size(); i++) {
-				DataTypeComponent dtc = components.get(i);
+			for (DataTypeComponentDB dtc : components) {
 				DataType subDt = dtc.getDataType();
 				if (subDt instanceof Composite) {
 					if (((Composite) subDt).isPartOf(dataType)) {
@@ -1198,8 +1197,7 @@ class StructureDB extends CompositeDB implements Structure {
 		int oldLength = structLength;
 		int oldMinAlignment = getMinimumAlignment();
 
-		for (int i = 0; i < components.size(); i++) {
-			DataTypeComponentDB dtc = components.get(i);
+		for (DataTypeComponentDB dtc : components) {
 			dtc.getDataType().removeParent(this);
 			componentAdapter.removeRecord(dtc.getKey());
 		}
@@ -1364,37 +1362,44 @@ class StructureDB extends CompositeDB implements Structure {
 		try {
 			checkDeleted();
 			if (isInternallyAligned()) {
-				adjustInternalAlignment(true);
+				adjustComponents(true); // notifies parents
 				return;
 			}
 			boolean didChange = false;
+			boolean warn = false;
 			int n = components.size();
 			for (int i = 0; i < n; i++) {
 				DataTypeComponentDB dtc = components.get(i);
-				int nextIndex = i + 1;
 				if (dtc.getDataType() == dt) {
 					// assume no impact to bitfields since base types
 					// should not change size
-					int dtLen = dt.getLength();
 					int dtcLen = dtc.getLength();
-					if (dtLen < dtcLen) {
-						dtc.setLength(dtLen, true);
-						shiftOffsets(nextIndex, dtcLen - dtLen, 0);
+					int length = getPreferredComponentLength(dt, dtcLen);
+					if (length < dtcLen) {
+						dtc.setLength(length, true);
+						shiftOffsets(i + 1, dtcLen - length, 0);
 						didChange = true;
 					}
-					else if (dtLen > dtcLen) {
-						int consumed = consumeBytesAfter(i, dtLen - dtcLen);
+					else if (length > dtcLen) {
+						int consumed = consumeBytesAfter(i, length - dtcLen);
 						if (consumed > 0) {
 							dtc.updateRecord();
-							shiftOffsets(nextIndex, -consumed, 0);
+							shiftOffsets(i + 1, -consumed, 0);
 							didChange = true;
 						}
 					}
+					if (dtc.getLength() != length) {
+						warn = true;
+					}
 				}
 			}
 			if (didChange) {
-				adjustInternalAlignment(true);
-				notifySizeChanged();
+				adjustInternalAlignment(false);
+				notifySizeChanged(); // notifies parents
+			}
+			if (warn) {
+				Msg.warn(this,
+					"Failed to resize one or more structure components: " + getPathName());
 			}
 		}
 		finally {
@@ -1402,6 +1407,56 @@ class StructureDB extends CompositeDB implements Structure {
 		}
 	}
 
+	@Override
+	protected void fixupComponents() {
+		if (isInternallyAligned()) {
+			// Do not notify parents
+			if (adjustComponents(false)) {
+				dataMgr.dataTypeChanged(this);
+			}
+			return;
+		}
+		boolean didChange = false;
+		boolean warn = false;
+		int n = components.size();
+		for (int i = 0; i < n; i++) {
+			DataTypeComponentDB dtc = components.get(i);
+			DataType dt = dtc.getDataType();
+			if (dt instanceof BitFieldDataType) {
+				// TODO: could get messy
+				continue;
+			}
+			int dtcLen = dtc.getLength();
+			int length = getPreferredComponentLength(dt, dtcLen);
+			if (dtcLen != length) {
+				if (length < dtcLen) {
+					dtc.setLength(length, true);
+					shiftOffsets(i + 1, dtcLen - length, 0);
+					didChange = true;
+				}
+				else if (length > dtcLen) {
+					int consumed = consumeBytesAfter(i, length - dtcLen);
+					if (consumed > 0) {
+						dtc.updateRecord();
+						shiftOffsets(i + 1, -consumed, 0);
+						didChange = true;
+					}
+				}
+				if (dtc.getLength() != length) {
+					warn = true;
+				}
+			}
+		}
+		if (didChange) {
+			// Do not notify parents
+			adjustInternalAlignment(false);
+			dataMgr.dataTypeChanged(this);
+		}
+		if (warn) {
+			Msg.warn(this, "Failed to resize one or more structure components: " + getPathName());
+		}
+	}
+
 	@Override
 	public void dataTypeAlignmentChanged(DataType dt) {
 		lock.acquire();
@@ -1808,8 +1863,7 @@ class StructureDB extends CompositeDB implements Structure {
 				flexibleArrayComponent = null;
 			}
 
-			for (int i = 0; i < components.size(); i++) {
-				DataTypeComponentDB dtc = components.get(i);
+			for (DataTypeComponentDB dtc : components) {
 				dtc.getDataType().removeParent(this);
 				try {
 					componentAdapter.removeRecord(dtc.getKey());
@@ -1885,12 +1939,14 @@ class StructureDB extends CompositeDB implements Structure {
 			changed |= updateComposite(packResult.numComponents, packResult.structureLength,
 				packResult.alignment, false);
 
-			if (notify & changed) {
-				if (oldLength != structLength) {
-					notifySizeChanged();
-				}
-				else {
-					dataMgr.dataTypeChanged(this);
+			if (changed) {
+				if (notify) {
+					if (oldLength != structLength) {
+						notifySizeChanged();
+					}
+					else {
+						dataMgr.dataTypeChanged(this);
+					}
 				}
 				return true;
 			}

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

@@ -277,8 +277,7 @@ class UnionDB extends CompositeDB implements Union {
 		int oldLength = unionLength;
 		int oldMinAlignment = getMinimumAlignment();
 
-		for (int i = 0; i < components.size(); i++) {
-			DataTypeComponentDB dtc = components.get(i);
+		for (DataTypeComponentDB dtc : components) {
 			dtc.getDataType().removeParent(this);
 			removeComponent(dtc.getKey());
 		}
@@ -432,7 +431,7 @@ class UnionDB extends CompositeDB implements Union {
 				}
 			}
 			if (changed) {
-				adjustLength(true, false);
+				adjustLength(true, false);  // notifies parents
 			}
 		}
 		finally {
@@ -440,6 +439,30 @@ class UnionDB extends CompositeDB implements Union {
 		}
 	}
 
+	@Override
+	protected void fixupComponents() {
+		boolean changed = false;
+		for (DataTypeComponentDB dtc : components) {
+			DataType dt = dtc.getDataType();
+			if (dt instanceof BitFieldDataType) {
+				dt = adjustBitField(dt); // in case base type changed
+			}
+			int dtcLen = dtc.getLength();
+			int length = getPreferredComponentLength(dt, dtcLen);
+			if (length != dtcLen) {
+				dtc.setLength(length, true);
+				changed = true;
+			}
+		}
+		if (changed || isInternallyAligned()) {
+			// NOTE: since we do not retain our external alignment we have no way of knowing if
+			// it has changed, so we must assume it has if we are an aligned union
+			// Do not notify parents
+			adjustLength(false, false);
+			dataMgr.dataTypeChanged(this);
+		}
+	}
+
 	@Override
 	public void dataTypeAlignmentChanged(DataType dt) {
 		adjustInternalAlignment(true);
@@ -614,7 +637,9 @@ class UnionDB extends CompositeDB implements Union {
 			catch (IOException e) {
 				dataMgr.dbError(e);
 			}
-			notifySizeChanged();
+			if (notify) {
+				notifySizeChanged();
+			}
 		}
 		else if (notify) {
 			dataMgr.dataTypeChanged(this);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/listing/Program.java

@@ -82,7 +82,7 @@ public interface Program extends DataTypeManagerDomainObject {
 	 * Returns the program's datatype manager.
 	 */
 	@Override
-	public DataTypeManager getDataTypeManager();
+	public ProgramBasedDataTypeManager getDataTypeManager();
 
 	/**
 	 * Returns the programs function manager.

Ghidra/Framework/SoftwareModeling/src/test/java/ghidra/program/database/data/StructureDBTest.java

@@ -85,6 +85,30 @@ public class StructureDBTest extends AbstractGTest {
 		return (Pointer) dataMgr.resolve(new Pointer32DataType(dataType), null);
 	}
 
+	@Test
+	public void testEmpty() throws Exception {
+		Structure s = new StructureDataType("foo", 0);
+		assertTrue(s.isNotYetDefined());
+		assertEquals(0, s.getNumComponents());
+		assertEquals(0, s.getNumDefinedComponents());
+		Structure s2 = (Structure) dataMgr.resolve(s, null);
+		assertTrue(s2.isNotYetDefined());
+		assertEquals(0, s2.getNumComponents());
+		assertEquals(0, s.getNumDefinedComponents());
+	}
+
+	@Test
+	public void testSizeOne() throws Exception {
+		Structure s = new StructureDataType("foo", 1);
+		assertFalse(s.isNotYetDefined());
+		assertEquals(1, s.getNumComponents());
+		assertEquals(0, s.getNumDefinedComponents());
+		Structure s2 = (Structure) dataMgr.resolve(s, null);
+		assertFalse(s2.isNotYetDefined());
+		assertEquals(1, s2.getNumComponents());
+		assertEquals(0, s2.getNumDefinedComponents());
+	}
+
 	@Test
 	public void testAdd() throws Exception {
 		assertEquals(8, struct.getLength());

certification.local.manifest

@@ -9,5 +9,4 @@ LICENSE||GHIDRA||||END|
 NOTICE||GHIDRA||||END|
 README.md||GHIDRA||||END|
 build.gradle||GHIDRA||||END|
-ghidra.repos.config||GHIDRA||||END|
 settings.gradle||GHIDRA||||END|