Merge remote-tracking branch 'origin/GP-4740_ghidra1_CompositeEditorUndoRedo--SQUASHED'

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

@@ -16,6 +16,7 @@
 package ghidra.program.database.data;
 
 import java.io.IOException;
+import java.util.Objects;
 
 import db.DBRecord;
 import ghidra.docking.settings.Settings;
@@ -212,6 +213,9 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal {
 		lock.acquire();
 		try {
 			checkDeleted();
+			if (Objects.equals(desc, record.getString(CompositeDBAdapter.COMPOSITE_COMMENT_COL))) {
+				return;
+			}
 			record.setString(CompositeDBAdapter.COMPOSITE_COMMENT_COL, desc);
 			compositeAdapter.updateRecord(record, true);
 			dataMgr.dataTypeChanged(this, false);
@@ -391,13 +395,17 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal {
 		return record.getLongValue(CompositeDBAdapter.COMPOSITE_LAST_CHANGE_TIME_COL);
 	}
 
+	void doSetLastChangeTime(long lastChangeTime) throws IOException {
+		record.setLongValue(CompositeDBAdapter.COMPOSITE_LAST_CHANGE_TIME_COL, lastChangeTime);
+		compositeAdapter.updateRecord(record, false);
+	}
+
 	@Override
 	public void setLastChangeTime(long lastChangeTime) {
 		lock.acquire();
 		try {
 			checkDeleted();
-			record.setLongValue(CompositeDBAdapter.COMPOSITE_LAST_CHANGE_TIME_COL, lastChangeTime);
-			compositeAdapter.updateRecord(record, false);
+			doSetLastChangeTime(lastChangeTime);
 			dataMgr.dataTypeChanged(this, false);
 		}
 		catch (IOException e) {

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

@@ -4,9 +4,9 @@
  * 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.
@@ -138,7 +138,11 @@ class DataTypeComponentDB implements InternalDataTypeComponent {
 		if (id == -1) {
 			return DataType.DEFAULT;
 		}
-		return dataMgr.getDataType(id);
+		DataType dt = dataMgr.getDataType(id);
+		if (dt == null) {
+			return BadDataType.dataType;
+		}
+		return dt;
 	}
 
 	@Override
@@ -191,6 +195,7 @@ class DataTypeComponentDB implements InternalDataTypeComponent {
 
 	@Override
 	public Settings getDefaultSettings() {
+
 		if (!hasSettings()) {
 			return SettingsImpl.NO_SETTINGS;
 		}

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

@@ -1693,8 +1693,12 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		// (preference is given to similar kind of datatype when checking existing conflict types)
 		DataType existingDataType = findDataTypeSameLocation(dataType);
 		if (existingDataType == null) {
-			return createDataType(dataType, getUnusedConflictName(dataType), sourceArchive,
-				currentHandler);
+			// create non-existing datatype - keep original name unless it is already used
+			String name = dataType.getName();
+			if (getDataType(dataType.getCategoryPath(), name) != null) {
+				name = getUnusedConflictName(dataType);
+			}
+			return createDataType(dataType, name, sourceArchive, currentHandler);
 		}
 
 		// So we have a dataType with the same path and name, but not equivalent, so use
@@ -2310,7 +2314,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		if (id <= 0) { // removal of certain special types not permitted
 			return false;
 		}
-		idsToDelete.add(Long.valueOf(id));
+		idsToDelete.add(id);
 		removeQueuedDataTypes();
 		return true;
 	}
@@ -3130,8 +3134,9 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 
 			structDB.doReplaceWith(struct, false);
 
-			// doReplaceWith may have updated the last change time so set it back to what we want.
-			structDB.setLastChangeTime(struct.getLastChangeTime());
+			// doReplaceWith may have updated the last change time so set it back to what we want
+			// without triggering change notification
+			structDB.doSetLastChangeTime(struct.getLastChangeTime());
 
 			return structDB;
 		}
@@ -3198,8 +3203,9 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 
 			unionDB.doReplaceWith(union, false);
 
-			// doReplaceWith updated the last change time so set it back to what we want.
-			unionDB.setLastChangeTime(union.getLastChangeTime());
+			// doReplaceWith may have updated the last change time so set it back to what we want
+			// without triggering change notification
+			unionDB.doSetLastChangeTime(union.getLastChangeTime());
 
 			return unionDB;
 		}
@@ -3717,7 +3723,7 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		}
 	}
 
-	void removeParentChildRecord(long parentID, long childID) {
+	protected void removeParentChildRecord(long parentID, long childID) {
 
 		if (isBulkRemoving) {
 			// we are in the process of bulk removing the given child; no need to call
@@ -3733,6 +3739,26 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		}
 	}
 
+	protected Set<Long> getChildIds(long parentID) {
+		try {
+			return parentChildAdapter.getChildIds(parentID);
+		}
+		catch (IOException e) {
+			dbError(e);
+		}
+		return Set.of();
+	}
+
+	protected boolean hasParent(long childID) {
+		try {
+			return parentChildAdapter.hasParent(childID);
+		}
+		catch (IOException e) {
+			dbError(e);
+		}
+		return false;
+	}
+
 	List<DataType> getParentDataTypes(long dataTypeId) {
 		lock.acquire();
 		try {

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

@@ -4,9 +4,9 @@
  * 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.
@@ -80,6 +80,16 @@ abstract class ParentChildAdapter {
 
 	abstract void removeRecord(long parentID, long childID) throws IOException;
 
+	/**
+	 * Get the unique set of child IDs associated with the specified parent ID.
+	 * Since a parent may have duplicate parent-child records, this method
+	 * avoids returning the same child more than once.
+	 * @param parentID parent datatype ID
+	 * @return set of child datatype IDs
+	 * @throws IOException if a DB IO error occurs
+	 */
+	abstract Set<Long> getChildIds(long parentID) throws IOException;
+
 	/**
 	 * Get the unique set of parent ID associated with the specified childID.
 	 * Since composite parents may have duplicate parent-child records, this method
@@ -90,6 +100,14 @@ abstract class ParentChildAdapter {
 	 */
 	abstract Set<Long> getParentIds(long childID) throws IOException;
 
+	/** 
+	 * Determine if there is one or more parents associated with the specified childID.
+	 * @param childID child datatype ID
+	 * @return true if a parent was identified, else false
+	 * @throws IOException if a DB IO error occurs
+	 */
+	abstract boolean hasParent(long childID) throws IOException;
+
 	abstract void removeAllRecordsForParent(long parentID) throws IOException;
 
 	abstract void removeAllRecordsForChild(long childID) throws IOException;

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

@@ -4,9 +4,9 @@
  * 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.
@@ -43,11 +43,21 @@ class ParentChildDBAdapterNoTable extends ParentChildAdapter {
 		throw new UnsupportedOperationException();
 	}
 
+	@Override
+	Set<Long> getChildIds(long parentID) throws IOException {
+		return Set.of();
+	}
+
 	@Override
 	Set<Long> getParentIds(long childID) throws IOException {
 		return Set.of();
 	}
 
+	@Override
+	boolean hasParent(long childID) throws IOException {
+		return false;
+	}
+
 	@Override
 	boolean needsInitializing() {
 		return false;

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

@@ -4,9 +4,9 @@
  * 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.
@@ -88,17 +88,33 @@ class ParentChildDBAdapterV0 extends ParentChildAdapter {
 		}
 	}
 
+	@Override
+	Set<Long> getChildIds(long parentID) throws IOException {
+		Field[] ids = table.findRecords(new LongField(parentID), PARENT_COL);
+		Set<Long> childIds = new HashSet<>(ids.length);
+		for (Field id : ids) {
+			DBRecord rec = table.getRecord(id);
+			childIds.add(rec.getLongValue(CHILD_COL));
+		}
+		return childIds;
+	}
+
 	@Override
 	Set<Long> getParentIds(long childID) throws IOException {
 		Field[] ids = table.findRecords(new LongField(childID), CHILD_COL);
 		Set<Long> parentIds = new HashSet<>(ids.length);
-		for (int i = 0; i < ids.length; i++) {
-			DBRecord rec = table.getRecord(ids[i]);
+		for (Field id : ids) {
+			DBRecord rec = table.getRecord(id);
 			parentIds.add(rec.getLongValue(PARENT_COL));
 		}
 		return parentIds;
 	}
 
+	@Override
+	boolean hasParent(long childID) throws IOException {
+		return table.hasRecord(new LongField(childID), CHILD_COL);
+	}
+
 	public void setNeedsInitializing() {
 		needsInitializing = true;
 	}

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

@@ -4,9 +4,9 @@
  * 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.
@@ -111,9 +111,6 @@ abstract class PointerDBAdapter implements RecordTranslator {
 		}
 	}
 
-	/**
-	 *
-	 */
 	abstract void deleteTable(DBHandle handle) throws IOException;
 
 	/**

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

@@ -2301,7 +2301,7 @@ class StructureDB extends CompositeDB implements StructureInternal {
 				checkAncestry(replacementDt);
 			}
 			catch (Exception e) {
-				// TODO: should we flag bad replacement
+				// Handle bad replacement with use of undefined component
 				replacementDt = isPackingEnabled() ? Undefined1DataType.dataType : DataType.DEFAULT;
 			}
 

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

@@ -4,9 +4,9 @@
  * 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.
@@ -15,6 +15,8 @@
  */
 package ghidra.program.model.data;
 
+import org.apache.commons.lang3.StringUtils;
+
 import ghidra.program.model.address.*;
 import ghidra.program.model.listing.*;
 import ghidra.program.model.mem.*;
@@ -36,7 +38,7 @@ public final class DataUtilities {
 	 * @return true if name is valid, else false
 	 */
 	public static boolean isValidDataTypeName(String name) {
-		if (name == null || name.length() == 0) {
+		if (StringUtils.isBlank(name)) {
 			return false;
 		}
 

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

@@ -872,6 +872,14 @@ public class StandAloneDataTypeManager extends DataTypeManagerDB implements Clos
 		return transaction.intValue();
 	}
 
+	/**
+	 * Get the number of active transactions
+	 * @return number of active transactions
+	 */
+	protected int getTransactionCount() {
+		return transactionCount;
+	}
+
 	@Override
 	public void endTransaction(int transactionID, boolean commit) {
 		boolean restored = false;
@@ -953,6 +961,8 @@ public class StandAloneDataTypeManager extends DataTypeManagerDB implements Clos
 	protected synchronized void clearUndo() {
 		undoList.clear();
 		redoList.clear();
+
+		// Flatten all checkpoints then restore undo stack size
 		dbHandle.setMaxUndos(0);
 		dbHandle.setMaxUndos(NUM_UNDOS);
 	}

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

@@ -1640,13 +1640,13 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 	 * @param dataType the data type of the new component
 	 * @param newOffset offset of replacement component which must fall within origComponents bounds
 	 * @param length the length of the new component
-	 * @param name the field name of the new component
+	 * @param fieldName the field name of the new component
 	 * @param comment the comment for the new component
 	 * @return the new component or null if only a clear operation was performed.
 	 * @throws IllegalArgumentException if unable to identify/make sufficient space 
 	 */
 	private DataTypeComponent replaceComponents(LinkedList<DataTypeComponentImpl> origComponents,
-			DataType dataType, int newOffset, int length, String name, String comment)
+			DataType dataType, int newOffset, int length, String fieldName, String comment)
 			throws IllegalArgumentException {
 
 		boolean clearOnly = false;
@@ -1721,8 +1721,8 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 		DataTypeComponentImpl newDtc = null;
 		if (!clearOnly) {
 			// insert new component
-			newDtc = new DataTypeComponentImpl(dataType, this, length, newOrdinal, newOffset, name,
-				comment);
+			newDtc = new DataTypeComponentImpl(dataType, this, length, newOrdinal, newOffset,
+				fieldName, comment);
 			components.add(index, newDtc);
 		}
 

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

@@ -4,9 +4,9 @@
  * 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.
@@ -32,7 +32,8 @@ import ghidra.util.task.TaskMonitorAdapter;
 public class StructureDBTest extends AbstractGenericTest {
 
 	private StructureDB struct;
-	private DataTypeManagerDB dataMgr;
+	private StandAloneDataTypeManager dataMgr;
+	private int txId;
 
 	@Before
 	public void setUp() throws Exception {
@@ -42,7 +43,7 @@ public class StructureDBTest extends AbstractGenericTest {
 		// default data organization is little-endian
 		// default BitFieldPackingImpl uses gcc conventions with type alignment enabled
 
-		dataMgr.startTransaction("Test");
+		txId = dataMgr.startTransaction("Test");
 
 		struct = createStructure("Test", 0);
 		struct.add(new ByteDataType(), "field1", "Comment1");
@@ -52,6 +53,14 @@ public class StructureDBTest extends AbstractGenericTest {
 
 	}
 
+	@After
+	public void tearDown() {
+		if (dataMgr != null) {
+			dataMgr.endTransaction(txId, true);
+			dataMgr.close();
+		}
+	}
+
 	private void transitionToBigEndian() {
 
 		Structure structClone = struct.clone(null);
@@ -1442,7 +1451,45 @@ public class StructureDBTest extends AbstractGenericTest {
 	}
 
 	@Test
-	public void testDeleteMany() throws InvalidDataTypeException {
+	public void testDeleteMany() {
+
+		struct.growStructure(20);
+		struct.insertAtOffset(12, WordDataType.dataType, -1, "A", null);
+		struct.insertAtOffset(16, WordDataType.dataType, -1, "B", null);
+
+		assertEquals(32, struct.getLength());
+		assertEquals(26, struct.getNumComponents());
+		assertEquals(6, struct.getNumDefinedComponents());
+
+		struct.delete(Sets.newHashSet(1, 4, 5));
+
+		assertEquals(28, struct.getLength());
+		assertEquals(23, struct.getNumComponents());
+		assertEquals(5, struct.getNumDefinedComponents());
+
+		DataTypeComponent[] comps = struct.getDefinedComponents();
+		assertEquals(WordDataType.class, comps[3].getDataType().getClass());
+		assertEquals(5, comps[3].getOrdinal());
+		assertEquals(8, comps[3].getOffset());
+
+		// Verify that records were properly updated by comitting and performing an undo/redo
+		dataMgr.endTransaction(txId, true);
+		dataMgr.undo();
+		dataMgr.redo();
+		txId = dataMgr.startTransaction("Continue Test");
+
+		assertEquals(28, struct.getLength());
+		assertEquals(23, struct.getNumComponents());
+		assertEquals(5, struct.getNumDefinedComponents());
+
+		comps = struct.getDefinedComponents();
+		assertEquals(WordDataType.class, comps[3].getDataType().getClass());
+		assertEquals(5, comps[3].getOrdinal());
+		assertEquals(8, comps[3].getOffset());
+	}
+
+	@Test
+	public void testDeleteManyBF() throws InvalidDataTypeException {
 
 		struct.insertBitFieldAt(2, 4, 0, IntegerDataType.dataType, 3, "bf1", "bf1Comment");
 		struct.insertBitFieldAt(2, 4, 3, IntegerDataType.dataType, 3, "bf2", "bf2Comment");

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

@@ -4,9 +4,9 @@
  * 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.
@@ -30,8 +30,9 @@ import ghidra.util.task.TaskMonitor;
  */
 public class UnionDBTest extends AbstractGenericTest {
 
-	private DataTypeManager dataMgr;
+	private StandAloneDataTypeManager dataMgr;
 	private UnionDB union;
+	private int txId;
 
 	@Before
 	public void setUp() throws Exception {
@@ -41,7 +42,7 @@ public class UnionDBTest extends AbstractGenericTest {
 		// default data organization is little-endian
 		// default BitFieldPackingImpl uses gcc conventions
 
-		dataMgr.startTransaction("Test");
+		txId = dataMgr.startTransaction("Test");
 
 		union = createUnion("TestUnion");
 		union.add(new ByteDataType(), "field1", "Comment1");
@@ -50,6 +51,14 @@ public class UnionDBTest extends AbstractGenericTest {
 		union.add(new ByteDataType(), "field4", "Comment4");
 	}
 
+	@After
+	public void tearDown() {
+		if (dataMgr != null) {
+			dataMgr.endTransaction(txId, true);
+			dataMgr.close();
+		}
+	}
+
 	private void transitionToBigEndian() {
 
 		Union unionClone = union.clone(null);
@@ -380,6 +389,7 @@ public class UnionDBTest extends AbstractGenericTest {
 		union.delete(Sets.newHashSet(2, 4));
 
 		assertEquals(2, union.getLength());
+
 		//@formatter:off
 		CompositeTestUtils.assertExpectedComposite(this, "/TestUnion\n" + 
 			"pack(disabled)\n" + 
@@ -390,6 +400,33 @@ public class UnionDBTest extends AbstractGenericTest {
 			"}\n" + 
 			"Length: 2 Alignment: 1", union);
 		//@formatter:on
+
+		DataTypeComponent[] comps = union.getDefinedComponents();
+		assertEquals(ByteDataType.class, comps[2].getDataType().getClass());
+		assertEquals(2, comps[2].getOrdinal());
+
+		// Verify that records were properly updated by comitting and performing an undo/redo
+		dataMgr.endTransaction(txId, true);
+		dataMgr.undo();
+		dataMgr.redo();
+		txId = dataMgr.startTransaction("Continue Test");
+
+		assertEquals(2, union.getLength());
+
+		//@formatter:off
+		CompositeTestUtils.assertExpectedComposite(this, "/TestUnion\n" + 
+			"pack(disabled)\n" + 
+			"Union TestUnion {\n" + 
+			"   0   byte   1   field1   \"Comment1\"\n" + 
+			"   0   word   2      \"Comment2\"\n" + 
+			"   0   byte   1   field4   \"Comment4\"\n" + 
+			"}\n" + 
+			"Length: 2 Alignment: 1", union);
+		//@formatter:on
+
+		comps = union.getDefinedComponents();
+		assertEquals(ByteDataType.class, comps[2].getDataType().getClass());
+		assertEquals(2, comps[2].getOrdinal());
 	}
 
 	@Test