diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/StructureEditorModel.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/StructureEditorModel.java index 41638a7041..415e0e701e 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/StructureEditorModel.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/compositeeditor/StructureEditorModel.java @@ -1328,91 +1328,47 @@ class StructureEditorModel extends CompEditorModel { 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 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 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 { selectionChanged(); } - private void applyZeroDtcStack(Structure viewStruct, int unpackOffset, int unpackOrdinal, - Stack 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 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); + } } } diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/compositeeditor/StructureEditorLockedActions5Test.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/compositeeditor/StructureEditorLockedActions5Test.java new file mode 100644 index 0000000000..cc10eb5f47 --- /dev/null +++ b/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 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 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 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()); + + } +} diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/compositeeditor/StructureEditorUnlockedActions5Test.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/compositeeditor/StructureEditorUnlockedActions5Test.java index ec51927f73..1c4eef4fe7 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/app/plugin/core/compositeeditor/StructureEditorUnlockedActions5Test.java +++ b/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 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 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()); + } } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Composite.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Composite.java index 50bdc84356..2895b51a70 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Composite.java +++ b/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. + *

+ * 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. + *

+ * 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 <= 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. + *

+ * 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. + *

+ * 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 <= 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. - *
Note: For an aligned structure the ordinal position will get adjusted + *

+ * 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. - *
Note: For an aligned structure the ordinal position will get adjusted + *

+ * 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. - *
Note: For an aligned structure the ordinal position will get adjusted + *

+ * 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.