Merge remote-tracking branch 'origin/Ghidra_12.1'

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/StructureEditorModel.java

@@ -1328,91 +1328,47 @@ class StructureEditorModel extends CompEditorModel<Structure> {
 			else if (currentDataType instanceof Structure struct) {
 				numComps = struct.getNumComponents();
 				if (numComps > 0) {
-					// Remove the structure.
-					int currentOffset = currentComp.getOffset();
+
 
 					// TODO: may want to add this functionality into the API
 
-					Stack<DataTypeComponent> zeroDtcStack = new Stack<>();
-					int zeroStackOffset = -1;
-					int zeroStackOrdinal = -1;
-					int packedOrdinal = 0;
-
-					deleteComponent(rowIndex);
-
 					if (struct.isZeroLength()) {
+						deleteComponent(rowIndex); // remove zero-length structure component
 						return;
 					}
 
+					int currentOffset = currentComp.getOffset();
+					int nextOffset = currentComp.getEndOffset() + 1;
+
+					List<DataTypeComponent> zeroDtcStash = new ArrayList<>();
+					int packedOrdinal = 0;
+
 					if (rowIndex != 0 && !viewComposite.isPackingEnabled()) {
-						// Must consume any preceeding zero-length components at the same offset
-						// into the zeroDtcStack to prevent their movement on subsequent component
-						// inserts at the same offset
+						// Must stash zero-length components which occur after the structure 
+						// component to be unpacked so that we may preserve their placement
 						DataTypeComponent dtc =
-							viewComposite.getDefinedComponentAtOrAfterOffset(currentOffset);
-						if (dtc != null && dtc.getOffset() == currentOffset) {
-							// zero-length is assumed if offset matches
-							zeroStackOffset = 0;
-							int componentCount = viewComposite.getNumComponents();
-							while (dtc != null && dtc.getOffset() == currentOffset) {
-								int ordinal = dtc.getOrdinal();
-								zeroDtcStack.push(dtc);
-								viewComposite.delete(ordinal);
-								--componentCount;
-								dtc = ordinal < componentCount ? viewComposite.getComponent(ordinal)
-										: null;
-							}
+							viewComposite.getDefinedComponentAtOrAfterOffset(nextOffset);
+						while (dtc != null && dtc.getLength() == 0) {
+							// Stash zero-length component
+							zeroDtcStash.add(dtc);
+							viewComposite.delete(dtc.getOrdinal());
+							dtc =
+								viewComposite.getDefinedComponentAtOrAfterOffset(nextOffset);
 						}
 					}
 
+					deleteComponent(rowIndex); // remove structure component to be unpacked
+
 					// Add the structure's elements
 					for (DataTypeComponent dtc : struct.getDefinedComponents()) {
-
-						DataType compDt = dtc.getDataType();
-						int compLength = dtc.getLength();
-						int compOffset = dtc.getOffset();
-
-						if (compOffset != zeroStackOffset && !zeroDtcStack.isEmpty()) {
-							packedOrdinal += zeroDtcStack.size();
-							applyZeroDtcStack(viewComposite, currentOffset, componentOrdinal,
-								zeroDtcStack, zeroStackOffset, zeroStackOrdinal);
-						}
-
-						if (compLength == 0) {
-							// Defer adding zero-length component until after non-zero-length
-							// has been added
-							if (zeroStackOffset != compOffset) {
-								zeroStackOffset = compOffset;
-								zeroStackOrdinal = packedOrdinal;
-							}
-							zeroDtcStack.push(dtc);
-							continue;
-						}
-
-						if (!isPackingEnabled()) {
-							if (dtc.isBitFieldComponent()) {
-								BitFieldDataType bitfield = (BitFieldDataType) compDt;
-								viewComposite.insertBitFieldAt(currentOffset + compOffset,
-									compLength, bitfield.getBitOffset(), bitfield.getBaseDataType(),
-									bitfield.getDeclaredBitSize(), dtc.getFieldName(),
-									dtc.getComment());
-							}
-							else {
-								viewComposite.insertAtOffset(currentOffset + compOffset, compDt,
-									compLength, dtc.getFieldName(), dtc.getComment());
-							}
-						}
-						else {
-							viewComposite.insert(componentOrdinal + packedOrdinal, compDt,
-								compLength, dtc.getFieldName(), dtc.getComment());
-						}
-
+						// NOTE: ordinal is only used when packing is enabled
+						insertComponent(dtc, componentOrdinal + packedOrdinal, currentOffset);
 						++packedOrdinal;
 					}
 
-					if (!zeroDtcStack.isEmpty()) {
-						applyZeroDtcStack(viewComposite, currentOffset, componentOrdinal,
-							zeroDtcStack, zeroStackOffset, zeroStackOrdinal);
+					if (!zeroDtcStash.isEmpty()) {
+						applyZeroDtcStack(viewComposite, componentOrdinal,
+							struct.getNumDefinedComponents(), zeroDtcStash);
 					}
 				}
 			}
@@ -1437,19 +1393,45 @@ class StructureEditorModel extends CompEditorModel<Structure> {
 		selectionChanged();
 	}
 
-	private void applyZeroDtcStack(Structure viewStruct, int unpackOffset, int unpackOrdinal,
-			Stack<DataTypeComponent> zeroDtcStack, int zeroStackOffset, int zeroStackOrdinal) {
-
-		while (!zeroDtcStack.isEmpty()) {
-			DataTypeComponent zeroDtc = zeroDtcStack.pop();
-			if (!isPackingEnabled()) {
-				viewStruct.insertAtOffset(unpackOffset + zeroStackOffset, zeroDtc.getDataType(), 0,
-					zeroDtc.getFieldName(), zeroDtc.getComment());
+	private void insertComponent(DataTypeComponent dtc, int ordinal, int baseOffset)
+			throws InvalidDataTypeException {
+		DataType compDt = dtc.getDataType();
+		int compLength = dtc.getLength();
+		int compOffset = dtc.getOffset();
+		if (!isPackingEnabled()) {
+			if (dtc.isBitFieldComponent()) {
+				BitFieldDataType bitfield = (BitFieldDataType) compDt;
+				viewComposite.insertBitFieldAt(baseOffset + compOffset,
+					compLength, bitfield.getBitOffset(), bitfield.getBaseDataType(),
+					bitfield.getDeclaredBitSize(), dtc.getFieldName(),
+					dtc.getComment());
 			}
 			else {
-				viewStruct.insert(unpackOrdinal + zeroStackOrdinal, zeroDtc.getDataType(), 0,
-					zeroDtc.getFieldName(), zeroDtc.getComment());
+				viewComposite.insertAtOffset(baseOffset + compOffset, compDt,
+					compLength, dtc.getFieldName(), dtc.getComment());
 			}
 		}
+		else {
+			viewComposite.insert(ordinal, compDt,
+				compLength, dtc.getFieldName(), dtc.getComment());
+		}
+	}
+
+	private void applyZeroDtcStack(Structure viewStruct, int unpackOrdinal,
+			int unpackDefinedComponentCount, List<DataTypeComponent> zeroDtcStash)
+			throws InvalidDataTypeException {
+
+		int structLen = viewStruct.isZeroLength() ? 0 : viewStruct.getLength();
+
+		for (DataTypeComponent zeroDtc : zeroDtcStash) {
+			int offset = zeroDtc.getOffset();
+			if (!isPackingEnabled() && offset < structLen &&
+				viewStruct.getComponentAt(offset) == null) {
+				continue; // drop component if it conflicts
+			}
+			// NOTE: ordinal is only used when packing is enabled
+			int ordinal = zeroDtc.getOrdinal() - unpackOrdinal + unpackDefinedComponentCount;
+			insertComponent(zeroDtc, ordinal, 0);
+		}
 	}
 }

Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/compositeeditor/StructureEditorLockedActions5Test.java

@@ -0,0 +1,142 @@
+/* ###
+ * 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.
+ */
+package ghidra.app.plugin.core.compositeeditor;
+
+import static org.junit.Assert.*;
+
+import java.util.List;
+
+import org.junit.Test;
+
+import ghidra.program.model.data.*;
+
+public class StructureEditorLockedActions5Test extends AbstractStructureEditorTest {
+
+	@Test
+	public void testUnpackageComponentArray() throws Exception {
+
+		program.withTransaction("Modify structures", () -> {
+
+			complexStructure.setPackingEnabled(true);
+
+			// Add zero-length component after component to be unpacked
+			complexStructure.insert(12, new ArrayDataType(CharDataType.dataType, 0), 0, "z1", null);
+			complexStructure.insert(13, new ArrayDataType(CharDataType.dataType, 0), 0, "z2", null);
+		});
+
+		init(complexStructure, pgmTestCat);
+
+		Structure viewStruct =
+			(Structure) model.getViewDataTypeManager().getResolvedViewComposite();
+
+		List<DataTypeComponent> componentsAt96 = viewStruct.getComponentsContaining(96);
+		assertEquals(3, componentsAt96.size());
+		assertEquals("z1", componentsAt96.get(0).getFieldName());
+		assertEquals("z2", componentsAt96.get(1).getFieldName());
+		assertEquals("simpleStructure[3]", componentsAt96.get(2).getDataType().getName());
+
+		int num = model.getNumComponents();
+		int len = model.getLength();
+		DataType dt11 = getDataType(11);
+		assertTrue(dt11 instanceof Array);
+		DataType element = ((Array) dt11).getDataType();
+		int elementLen = ((Array) dt11).getElementLength();
+		setSelection(new int[] { 11 });
+		assertEquals("string[5]", getDataType(11).getDisplayName());
+		invoke(unpackageAction);
+
+		waitForSwing();
+
+		assertEquals(len, model.getLength());
+		assertEquals(num + 4, model.getNumComponents());
+		assertTrue(!getDataType(11).isEquivalent(simpleStructure));
+		for (int i = 0; i < 5; i++) {
+			DataTypeComponent dtc = model.getComponent(11 + i);
+			DataType sdt = dtc.getDataType();
+			assertTrue(sdt.isEquivalent(element));
+			assertEquals("string", sdt.getDisplayName());
+			assertEquals(elementLen, dtc.getLength());
+		}
+
+		componentsAt96 = viewStruct.getComponentsContaining(96);
+		assertEquals(3, componentsAt96.size());
+		assertEquals("z1", componentsAt96.get(0).getFieldName());
+		assertEquals("z2", componentsAt96.get(1).getFieldName());
+		assertEquals("simpleStructure[3]", componentsAt96.get(2).getDataType().getName());
+	}
+
+	@Test
+	public void testUnpackageComponentStructure() throws Exception {
+
+		program.withTransaction("Modify simpleStruct", () -> {
+
+			complexStructure.setPackingEnabled(true);
+
+			simpleStructure.setPackingEnabled(true); // simplifies adding bitfields
+			simpleStructure.delete(1); // remove word component where bitfields will be inserted
+			simpleStructure.insertBitField(1, 2, 0, WordDataType.dataType, 3, "bf012", null);
+			simpleStructure.insertBitField(1, 2, 3, WordDataType.dataType, 2, "bf34", null);
+			simpleStructure.insertBitField(1, 2, 7, WordDataType.dataType, 1, "bf7", null);
+
+			// Add zero-length component after component to be unpacked
+			complexStructure.insert(18, new ArrayDataType(CharDataType.dataType, 0), 0, "z1", null);
+			complexStructure.insert(19, new ArrayDataType(CharDataType.dataType, 0), 0, "z2", null);
+		});
+
+		waitForSwing();
+
+		init(complexStructure, pgmTestCat);
+
+		Structure viewStruct =
+			(Structure) model.getViewDataTypeManager().getResolvedViewComposite();
+
+		List<DataTypeComponent> componentsAt340 = viewStruct.getComponentsContaining(340);
+		assertEquals(3, componentsAt340.size());
+		assertEquals("z1", componentsAt340.get(0).getFieldName());
+		assertEquals("z2", componentsAt340.get(1).getFieldName());
+		assertEquals("refStructure *32", componentsAt340.get(2).getDataType().getName());
+
+		int num = model.getNumComponents();
+		int len = model.getLength();
+		int numComps = simpleStructure.getNumComponents();
+		setSelection(new int[] { 17 });
+		assertEquals("simpleStructure", getDataType(17).getDisplayName());
+		invoke(unpackageAction);
+
+		assertEquals(len, model.getLength());
+		assertEquals(num + numComps - 1, model.getNumComponents());
+		assertTrue(!getDataType(17).isEquivalent(simpleStructure));
+		for (int i = 0; i < numComps; i++) {
+			DataTypeComponent dtc = getComponent(17 + i);
+			DataType sdt = simpleStructure.getComponent(i).getDataType();
+			assertTrue("type mismatch: " + sdt.getDisplayName() + " at " + dtc.getOffset(),
+				dtc.getDataType().isEquivalent(sdt));
+			assertEquals("name mismatch: " + sdt.getDisplayName() + " at " + dtc.getOffset(),
+				sdt.getDisplayName(), dtc.getDataType().getDisplayName());
+		}
+
+		// When packing enabled, existing zero-length components moved based upon their char alignment of 1
+		List<DataTypeComponent> componentsAt337 = viewStruct.getComponentsContaining(337);
+		assertEquals(2, componentsAt337.size());
+		assertEquals("z1", componentsAt337.get(0).getFieldName());
+		assertEquals("z2", componentsAt337.get(1).getFieldName());
+
+		componentsAt340 = viewStruct.getComponentsContaining(340);
+		assertEquals(1, componentsAt340.size());
+		assertEquals("refStructure *32", componentsAt340.get(0).getDataType().getName());
+
+	}
+}

Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/compositeeditor/StructureEditorUnlockedActions5Test.java

@@ -20,6 +20,7 @@ import static org.junit.Assert.*;
 import java.awt.Component;
 import java.awt.Window;
 import java.awt.event.KeyEvent;
+import java.util.List;
 
 import javax.swing.JTextField;
 
@@ -710,8 +711,27 @@ public class StructureEditorUnlockedActions5Test extends AbstractStructureEditor
 
 	@Test
 	public void testUnpackageComponentArray() throws Exception {
+
+		program.withTransaction("Modify structures", () -> {
+
+			// Add zero-length component after component to be unpacked
+			complexStructure.insertAtOffset(92, new ArrayDataType(CharDataType.dataType, 0), 0,
+				"z1", null);
+			complexStructure.insertAtOffset(92, new ArrayDataType(CharDataType.dataType, 0), 0,
+				"z2", null);
+		});
+
 		init(complexStructure, pgmTestCat);
 
+		Structure viewStruct =
+			(Structure) model.getViewDataTypeManager().getResolvedViewComposite();
+
+		List<DataTypeComponent> componentsAt92 = viewStruct.getComponentsContaining(92);
+		assertEquals(3, componentsAt92.size());
+		assertEquals("z1", componentsAt92.get(0).getFieldName());
+		assertEquals("z2", componentsAt92.get(1).getFieldName());
+		assertEquals("simpleStructure[3]", componentsAt92.get(2).getDataType().getName());
+
 		int num = model.getNumComponents();
 		int len = model.getLength();
 		DataType dt15 = getDataType(15);
@@ -721,6 +741,9 @@ public class StructureEditorUnlockedActions5Test extends AbstractStructureEditor
 		setSelection(new int[] { 15 });
 		assertEquals("string[5]", getDataType(15).getDisplayName());
 		invoke(unpackageAction);
+
+		waitForSwing();
+
 		assertEquals(len, model.getLength());
 		assertEquals(num + 4, model.getNumComponents());
 		assertTrue(!getDataType(15).isEquivalent(simpleStructure));
@@ -731,25 +754,71 @@ public class StructureEditorUnlockedActions5Test extends AbstractStructureEditor
 			assertEquals("string", sdt.getDisplayName());
 			assertEquals(elementLen, dtc.getLength());
 		}
+
+		componentsAt92 = viewStruct.getComponentsContaining(92);
+		assertEquals(3, componentsAt92.size());
+		assertEquals("z1", componentsAt92.get(0).getFieldName());
+		assertEquals("z2", componentsAt92.get(1).getFieldName());
+		assertEquals("simpleStructure[3]", componentsAt92.get(2).getDataType().getName());
 	}
 
 	@Test
 	public void testUnpackageComponentStructure() throws Exception {
+
+		program.withTransaction("Modify structures", () -> {
+			simpleStructure.setPackingEnabled(true); // simplifies adding bitfields
+			simpleStructure.delete(1); // remove word component where bitfields will be inserted
+			simpleStructure.insertBitField(1, 2, 0, WordDataType.dataType, 3, "bf012", null);
+			simpleStructure.insertBitField(1, 2, 3, WordDataType.dataType, 2, "bf34", null);
+			simpleStructure.insertBitField(1, 2, 7, WordDataType.dataType, 1, "bf7", null);
+			simpleStructure.setPackingEnabled(false);
+			simpleStructure.setLength(29); // prune aligned length
+
+			// Add zero-length component after component to be unpacked
+			complexStructure.insertAtOffset(321, new ArrayDataType(CharDataType.dataType, 0), 0,
+				"z1", null);
+			complexStructure.insertAtOffset(321, new ArrayDataType(CharDataType.dataType, 0), 0,
+				"z2", null);
+
+		});
+
+		waitForSwing();
+
 		init(complexStructure, pgmTestCat);
 
+		Structure viewStruct =
+			(Structure) model.getViewDataTypeManager().getResolvedViewComposite();
+
+		List<DataTypeComponent> componentsAt321 = viewStruct.getComponentsContaining(321);
+		assertEquals(3, componentsAt321.size());
+		assertEquals("z1", componentsAt321.get(0).getFieldName());
+		assertEquals("z2", componentsAt321.get(1).getFieldName());
+		assertEquals("refStructure *32", componentsAt321.get(2).getDataType().getName());
+
 		int num = model.getNumComponents();
 		int len = model.getLength();
 		int numComps = simpleStructure.getNumComponents();
 		setSelection(new int[] { 21 });
 		assertEquals("simpleStructure", getDataType(21).getDisplayName());
 		invoke(unpackageAction);
+
 		assertEquals(len, model.getLength());
 		assertEquals(num + numComps - 1, model.getNumComponents());
 		assertTrue(!getDataType(21).isEquivalent(simpleStructure));
 		for (int i = 0; i < numComps; i++) {
+			DataTypeComponent dtc = getComponent(21 + i);
 			DataType sdt = simpleStructure.getComponent(i).getDataType();
-			assertTrue(getDataType(21 + i).isEquivalent(sdt));
-			assertEquals(sdt.getDisplayName(), getDataType(21 + i).getDisplayName());
+			assertTrue("type mismatch: " + sdt.getDisplayName() + " at " + dtc.getOffset(),
+				dtc.getDataType().isEquivalent(sdt));
+			assertEquals("name mismatch: " + sdt.getDisplayName() + " at " + dtc.getOffset(),
+				sdt.getDisplayName(), dtc.getDataType().getDisplayName());
 		}
+
+		componentsAt321 = viewStruct.getComponentsContaining(321);
+		assertEquals(3, componentsAt321.size());
+		assertEquals("z1", componentsAt321.get(0).getFieldName());
+		assertEquals("z2", componentsAt321.get(1).getFieldName());
+		assertEquals("refStructure *32", componentsAt321.get(2).getDataType().getName());
+
 	}
 }

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

@@ -74,8 +74,11 @@ public interface Composite extends DataType {
 	public abstract DataTypeComponent[] getDefinedComponents();
 
 	/**
-	 * Adds a new datatype to the end of this composite.  This is the preferred method
-	 * to use for adding components to an aligned structure for fixed-length dataTypes.
+	 * Adds a new datatype to the end of this composite.
+	 * <p>
+	 * Note: When packing is enabled the component's offset will get determined
+	 * automatically to provide the proper alignment.
+	 * 
 	 * @param dataType the datatype to add.
 	 * @return the DataTypeComponent created.
 	 * @throws IllegalArgumentException if the specified data type is not
@@ -86,9 +89,11 @@ public interface Composite extends DataType {
 	public DataTypeComponent add(DataType dataType) throws IllegalArgumentException;
 
 	/**
-	 * Adds a new datatype to the end of this composite. This is the preferred method
-	 * to use for adding components to an aligned structure for dynamic dataTypes such as
-	 * strings whose length must be specified.
+	 * Adds a new datatype to the end of this composite.
+	 * <p>
+	 * Note: When packing is enabled the component's offset will get determined
+	 * automatically to provide the proper alignment.
+	 * 
 	 * @param dataType the datatype to add.
 	 * @param length the length to associate with the datatype.
 	 * For fixed length types a length &lt;= 0 will use the length of the resolved dataType.
@@ -102,8 +107,11 @@ public interface Composite extends DataType {
 	public DataTypeComponent add(DataType dataType, int length) throws IllegalArgumentException;
 
 	/**
-	 * Adds a new datatype to the end of this composite.  This is the preferred method
-	 * to use for adding components to an aligned structure for fixed-length dataTypes.
+	 * Adds a new datatype to the end of this composite.
+	 * <p>
+	 * Note: When packing is enabled the component's offset will get determined
+	 * automatically to provide the proper alignment.
+	 * 
 	 * @param dataType the datatype to add.
 	 * @param name the field name to associate with this component.
 	 * @param comment the comment to associate with this component.
@@ -121,6 +129,7 @@ public interface Composite extends DataType {
 	 * to be used with packed structures/unions only where the bitfield will be
 	 * appropriately packed.  The minimum storage byte size will be applied.
 	 * It will not provide useful results for composites with packing disabled.
+	 * 
 	 * @param baseDataType the bitfield base datatype (certain restrictions apply).
 	 * @param bitSize the bitfield size in bits
 	 * @param componentName the field name to associate with this component.
@@ -134,9 +143,11 @@ public interface Composite extends DataType {
 			String comment) throws InvalidDataTypeException;
 
 	/**
-	 * Adds a new datatype to the end of this composite.  This is the preferred method
-	 * to use for adding components to an aligned structure for dynamic dataTypes such as
-	 * strings whose length must be specified.
+	 * Adds a new datatype to the end of this composite.
+	 * <p>
+	 * Note: When packing is enabled the component's offset will get determined
+	 * automatically to provide the proper alignment.
+	 * 
 	 * @param dataType the datatype to add.
 	 * @param length the length to associate with the datatype.
 	 * For fixed length types a length &lt;= 0 will use the length of the resolved dataType.
@@ -153,8 +164,10 @@ public interface Composite extends DataType {
 
 	/**
 	 * Inserts a new datatype at the specified ordinal position in this composite.
-	 * <BR>Note: For an aligned structure the ordinal position will get adjusted
+	 * <p>
+	 * Note: When packing is enabled the component's offset will get determined
 	 * automatically to provide the proper alignment.
+	 * 
 	 * @param ordinal the ordinal where the new datatype is to be inserted (numbering starts at 0).
 	 * @param dataType the datatype to insert.
 	 * @return the componentDataType created.
@@ -169,8 +182,10 @@ public interface Composite extends DataType {
 
 	/**
 	 * Inserts a new datatype at the specified ordinal position in this composite.
-	 * <BR>Note: For an aligned structure the ordinal position will get adjusted
+	 * <p>
+	 * Note: When packing is enabled the component's offset will get determined
 	 * automatically to provide the proper alignment.
+	 * 
 	 * @param ordinal the ordinal where the new datatype is to be inserted (numbering starts at 0).
 	 * @param dataType the datatype to insert.
 	 * @param length the length to associate with the datatype.
@@ -188,8 +203,10 @@ public interface Composite extends DataType {
 
 	/**
 	 * Inserts a new datatype at the specified ordinal position in this composite.
-	 * <BR>Note: For an aligned structure the ordinal position will get adjusted
+	 * <p>
+	 * Note: When packing is enabled the component's offset will get determined
 	 * automatically to provide the proper alignment.
+	 * 
 	 * @param ordinal the ordinal where the new datatype is to be inserted (numbering starts at 0).
 	 * @param dataType the datatype to insert.
 	 * @param length the length to associate with the datatype.