GP-5421 Ensure merge inserts structure components in the correct

sequence to account for zero-length overlapping components. Refactor how
dataTypeDeleted and dataTypeReplaced are handled.  Use blocking error
message popup during most Merge operations.

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/BitFieldDBDataType.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.
@@ -56,7 +56,8 @@ class BitFieldDBDataType extends BitFieldDataType {
 	 * bit size may be reduced based upon the specified base datatype size.
 	 * @param bitOffset right shift factor within storage unit when viewed as a big-endian dd
 	 * scalar value.  Based upon minimal storage bitOffset should be in the range 0 to 7.
-	 * @throws InvalidDataTypeException
+	 * @throws InvalidDataTypeException if invalid base datatype has been specified or an 
+	 * invalid bitSize or bitOffset has been specified
 	 */
 	BitFieldDBDataType(DataType baseDataType, int bitSize, int bitOffset)
 			throws InvalidDataTypeException {
@@ -66,6 +67,7 @@ class BitFieldDBDataType extends BitFieldDataType {
 
 	private static enum BaseDatatypeKind {
 		NONE(0), TYPEDEF(1), ENUM(2), INTEGER(3);
+
 		final int id;
 
 		BaseDatatypeKind(int id) {

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

@@ -156,25 +156,23 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal {
 	 * @param oldDt             affected datatype which has been removed or replaced
 	 * @param newDt             replacement datatype
 	 * @return                  true if bitfield component was modified
-	 * @throws InvalidDataTypeException if bitfield was based upon oldDt but new
-	 *                                  datatype is invalid for a bitfield
 	 */
 	protected boolean updateBitFieldDataType(DataTypeComponentDB bitfieldComponent, DataType oldDt,
-			DataType newDt) throws InvalidDataTypeException {
+			DataType newDt) {
 		if (!bitfieldComponent.isBitFieldComponent()) {
 			throw new AssertException("expected bitfield component");
 		}
 
 		BitFieldDBDataType bitfieldDt = (BitFieldDBDataType) bitfieldComponent.getDataType();
-		if (bitfieldDt.getBaseDataType() != oldDt) {
+		if (bitfieldDt.getBaseDataType() != oldDt || !BitFieldDataType.isValidBaseDataType(newDt)) {
 			return false;
 		}
 
 		if (newDt != null) {
-			BitFieldDataType.checkBaseDataType(newDt);
 			int maxBitSize = 8 * newDt.getLength();
 			if (bitfieldDt.getBitSize() > maxBitSize) {
-				throw new InvalidDataTypeException("Replacement datatype too small for bitfield");
+				// Replacement datatype too small for bitfield
+				return false;
 			}
 		}
 
@@ -186,7 +184,7 @@ abstract class CompositeDB extends DataTypeDB implements CompositeInternal {
 			newDt.addParent(this);
 		}
 		catch (InvalidDataTypeException e) {
-			throw new AssertException("unexpected");
+			throw new AssertException(e); // unexpected
 		}
 
 		return true;

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeDB.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.
@@ -19,6 +19,8 @@ import java.io.IOException;
 import java.net.URL;
 import java.util.Collection;
 
+import org.apache.commons.lang3.StringUtils;
+
 import db.DBRecord;
 import ghidra.docking.settings.*;
 import ghidra.program.database.DBObjectCache;
@@ -614,4 +616,12 @@ abstract class DataTypeDB extends DatabaseObject implements DataType {
 		return existingDataType.isEquivalent(otherDataType);
 	}
 
+	static String prependComment(String additionalComment, String oldComment) {
+		String comment = additionalComment;
+		if (!StringUtils.isBlank(oldComment)) {
+			comment += "; " + oldComment;
+		}
+		return comment;
+	}
+
 }

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

@@ -156,6 +156,13 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 	private LinkedList<Pair<DataType, DataType>> typesToReplace = new LinkedList<>();
 	private List<DataType> favoritesList = new ArrayList<>();
 
+	/**
+	 * Set of {@link AbstractIntegerDataType} IDs whose removal has been blocked
+	 * to allow persistence of defined bitfields.  
+	 * See {@link #blockDataTypeRemoval(AbstractIntegerDataType)}
+	 */
+	private Set<Long> blockedRemovalsByID;
+
 	// TODO: idsToDataTypeMap may have issue since there could be a one to many mapping
 	// (e.g., type with same UniversalID could be in multiple categories unless specifically 
 	// prevented during resolve)
@@ -2299,6 +2306,26 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 		}
 	}
 
+	/**
+	 * Register a {@link AbstractIntegerDataType}, during an invocation of 
+	 * {@link StructureDB#dataTypeDeleted(DataType)} or {@link UnionDB#dataTypeDeleted(DataType)},
+	 * to block final removal of the specified datatype since it is required for persistence of a 
+	 * defined bitfield. It is required that this be done within the same thread where this manager 
+	 * has initiated the removal and callbacks.
+	 * @param dt integer datatype which should be retained and not deleted
+	 */
+	void blockDataTypeRemoval(AbstractIntegerDataType dt) {
+		long id = getID(dt);
+		if (id == NULL_DATATYPE_ID) {
+			throw new IllegalArgumentException(
+				"Datatype instance is not associated with this manager");
+		}
+		if (blockedRemovalsByID == null) {
+			blockedRemovalsByID = new HashSet<>();
+		}
+		blockedRemovalsByID.add(id);
+	}
+
 	/**
 	 * Remove the given datatype from this manager (assumes the lock has already been acquired).
 	 * 
@@ -2335,8 +2362,15 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 
 		// perform actual database updates (e.g., record removal, change notifications, etc.)
 		for (long id : deletedIds) {
+			if (blockedRemovalsByID != null && blockedRemovalsByID.contains(id)) {
+				DataType dt = getDataType(id);
+				Msg.warn(this, "The datatype '" + dt.getDisplayName() +
+					"' has been retained for use by defined bitfields");
+				continue;
+			}
 			deleteDataType(id);
 		}
+		blockedRemovalsByID = null;
 	}
 
 	/**
@@ -2367,14 +2401,11 @@ abstract public class DataTypeManagerDB implements DataTypeManager {
 	public boolean remove(DataType dataType, TaskMonitor monitor) {
 		lock.acquire();
 		try {
-			if (contains(dataType)) {
-				return removeInternal(dataType);
-			}
+			return removeInternal(dataType);
 		}
 		finally {
 			lock.release();
 		}
-		return false;
 	}
 
 	@Override

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/FunctionDefinitionDB.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.
@@ -321,7 +321,7 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 	}
 
 	@Override
-	public void setArguments(ParameterDefinition[] args) {
+	public void setArguments(ParameterDefinition... args) {
 		lock.acquire();
 		try {
 			checkDeleted();
@@ -413,15 +413,26 @@ class FunctionDefinitionDB extends DataTypeDB implements FunctionDefinition {
 		lock.acquire();
 		try {
 			checkDeleted();
+			boolean changed = false;
 			int n = parameters.size();
 			for (int i = 0; i < n; i++) {
 				ParameterDefinitionDB param = parameters.get(i);
 				if (param.getDataType() == dt) {
-					param.setDataType(DataType.DEFAULT);
+					param.doSetDataType(DataType.DEFAULT, false);
+					param.doSetComment(
+						prependComment("Type '" + dt.getDisplayName() + "' was deleted",
+							param.getComment()),
+						false);
+					changed = true;
 				}
 			}
 			if (dt == getReturnType()) {
-				setReturnType(DataType.DEFAULT);
+				// NOTE: Not sure how to reflect in a comment
+				doSetReturnType(DataType.DEFAULT, false, false);
+				changed = true;
+			}
+			if (changed) {
+				dataMgr.dataTypeChanged(this, true);
 			}
 		}
 		finally {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/ParameterDefinitionDB.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.
@@ -133,10 +133,16 @@ final class ParameterDefinitionDB implements ParameterDefinition {
 
 	@Override
 	public void setComment(String comment) {
+		doSetComment(comment, true);
+	}
+
+	void doSetComment(String comment, boolean notify) {
 		record.setString(FunctionParameterAdapter.PARAMETER_COMMENT_COL, comment);
 		try {
 			adapter.updateRecord(record);
-			dataMgr.dataTypeChanged(parent, false);
+			if (notify) {
+				dataMgr.dataTypeChanged(parent, false);
+			}
 		}
 		catch (IOException e) {
 			dataMgr.dbError(e);

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

@@ -251,7 +251,7 @@ class StructureDB extends CompositeDB implements StructureInternal {
 				// identify index of first defined-component to be removed
 				int index = Collections.binarySearch(components, Integer.valueOf(len),
 					OffsetComparator.INSTANCE);
-				
+
 				if (index < 0) {
 					index = -index - 1;
 				}
@@ -280,7 +280,7 @@ class StructureDB extends CompositeDB implements StructureInternal {
 			lock.release();
 		}
 	}
-	
+
 	@Override
 	public void growStructure(int amount) {
 		if (amount < 0) {
@@ -1836,20 +1836,30 @@ class StructureDB extends CompositeDB implements StructureInternal {
 			int n = components.size();
 			for (int i = n - 1; i >= 0; i--) {
 				DataTypeComponentDB dtc = components.get(i);
-				boolean removeBitFieldComponent = false;
 				if (dtc.isBitFieldComponent()) {
+					// Do not allow bitfield to be destroyed
+					// If base type is removed - revert to primitive type
 					BitFieldDataType bitfieldDt = (BitFieldDataType) dtc.getDataType();
-					removeBitFieldComponent = bitfieldDt.getBaseDataType() == dt;
+					if (bitfieldDt.getBaseDataType() == dt) {
+						AbstractIntegerDataType primitiveDt = bitfieldDt.getPrimitiveBaseDataType();
+						dataMgr.blockDataTypeRemoval(primitiveDt);
+						if (primitiveDt != dt && updateBitFieldDataType(dtc, dt, primitiveDt)) {
+							dtc.setComment(
+								prependComment("Type '" + dt.getDisplayName() + "' was deleted",
+									dtc.getComment()));
+							changed = true;
+						}
+					}
 				}
-				if (removeBitFieldComponent || dtc.getDataType() == dt) {
-					doDelete(i);
-					// for non-packed offsets of remaining components will not change
-					shiftOffsets(i, dtc.getLength() - 1, 0); // ordinals only
-					--numComponents; // may be revised by repack
+				else if (dtc.getDataType() == dt) {
+					setComponentDataType(dtc, BadDataType.dataType, i);
+					dtc.setComment(prependComment("Type '" + dt.getDisplayName() + "' was deleted",
+						dtc.getComment()));
 					changed = true;
 				}
 			}
-			if (changed && !repack(false, true)) {
+			// repack not needed for non-packed structure - nothing should move
+			if (changed && (!isPackingEnabled() || !repack(false, true))) {
 				dataMgr.dataTypeChanged(this, false);
 			}
 		}
@@ -2344,7 +2354,7 @@ class StructureDB extends CompositeDB implements StructureInternal {
 			checkDeleted();
 			DataType replacementDt = newDt;
 			try {
-				validateDataType(replacementDt);
+				replacementDt = validateDataType(replacementDt); // blocks DEFAULT use for packed
 				replacementDt = resolve(replacementDt);
 				checkAncestry(replacementDt);
 			}
@@ -2355,39 +2365,12 @@ class StructureDB extends CompositeDB implements StructureInternal {
 
 			boolean changed = false;
 			for (int i = components.size() - 1; i >= 0; i--) {
-
 				DataTypeComponentDB comp = components.get(i);
-
-				boolean remove = false;
 				if (comp.isBitFieldComponent()) {
-					try {
-						changed |= updateBitFieldDataType(comp, oldDt, replacementDt);
-					}
-					catch (InvalidDataTypeException e) {
-						Msg.error(this,
-							"Invalid bitfield replacement type " + newDt.getName() +
-								", removing bitfield " + comp.getDataType().getName() + ": " +
-								getPathName());
-						remove = true;
-					}
+					changed |= updateBitFieldDataType(comp, oldDt, replacementDt);
 				}
 				else if (comp.getDataType() == oldDt) {
-					if (replacementDt == DEFAULT && isPackingEnabled()) {
-						Msg.error(this,
-							"Invalid replacement type " + newDt.getName() +
-								", removing component " + comp.getDataType().getName() + ": " +
-								getPathName());
-						remove = true;
-					}
-					else {
-						setComponentDataType(comp, replacementDt, i);
-						changed = true;
-					}
-				}
-				if (remove) {
-					// error case - remove component
-					doDelete(i);
-					shiftOffsets(i, comp.getLength() - 1, 0); // ordinals only
+					setComponentDataType(comp, replacementDt, i);
 					changed = true;
 				}
 			}

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

@@ -25,7 +25,6 @@ import ghidra.program.database.DBObjectCache;
 import ghidra.program.model.data.*;
 import ghidra.program.model.data.DataTypeConflictHandler.ConflictResult;
 import ghidra.program.model.mem.MemBuffer;
-import ghidra.util.Msg;
 
 /**
  * Database implementation for the Union data type.
@@ -715,20 +714,32 @@ class UnionDB extends CompositeDB implements UnionInternal {
 			boolean changed = false;
 			for (int i = components.size() - 1; i >= 0; i--) { // reverse order
 				DataTypeComponentDB dtc = components.get(i);
-				boolean removeBitFieldComponent = false;
 				if (dtc.isBitFieldComponent()) {
+
+					// Do not allow bitfield to be destroyed
+					// If base type is removed - revert to primitive type
 					BitFieldDataType bitfieldDt = (BitFieldDataType) dtc.getDataType();
-					removeBitFieldComponent = bitfieldDt.getBaseDataType() == dt;
+					if (bitfieldDt.getBaseDataType() == dt) {
+						AbstractIntegerDataType primitiveDt = bitfieldDt.getPrimitiveBaseDataType();
+						dataMgr.blockDataTypeRemoval(primitiveDt);
+						if (primitiveDt != dt && updateBitFieldDataType(dtc, dt, primitiveDt)) {
+							dtc.setComment(
+								prependComment("Type '" + dt.getDisplayName() + "' was deleted",
+									dtc.getComment()));
+							changed = true;
+						}
+					}
 				}
-				if (removeBitFieldComponent || dtc.getDataType() == dt) {
+				else if (dtc.getDataType() == dt) {
 					dt.removeParent(this);
-					components.remove(i);
-					removeComponentRecord(dtc.getKey());
-					shiftOrdinals(i, -1);
+					dtc.setDataType(BadDataType.dataType); // updates record
+					dataMgr.getSettingsAdapter().removeAllSettingsRecords(dtc.getKey());
+					dtc.setComment(prependComment("Type '" + dt.getDisplayName() + "' was deleted",
+						dtc.getComment()));
 					changed = true;
 				}
 			}
-			if (changed && !repack(false, true)) {
+			if (changed && (!isPackingEnabled() || !repack(false, true))) {
 				dataMgr.dataTypeChanged(this, false);
 			}
 		}
@@ -821,58 +832,26 @@ class UnionDB extends CompositeDB implements UnionInternal {
 			checkDeleted();
 			DataType replacementDt = newDt;
 			try {
-				validateDataType(replacementDt);
-				if (!(replacementDt instanceof DataTypeDB) ||
-					(replacementDt.getDataTypeManager() != getDataTypeManager())) {
-					replacementDt = resolve(replacementDt);
-				}
+				replacementDt = validateDataType(replacementDt); // blocks DEFAULT use
+				replacementDt = replacementDt.clone(dataMgr);
 				checkAncestry(replacementDt);
 			}
 			catch (Exception e) {
-				// TODO: should we flag bad replacement
 				replacementDt = Undefined1DataType.dataType;
 			}
 			boolean changed = false;
 			for (int i = components.size() - 1; i >= 0; i--) {
-
 				DataTypeComponentDB dtc = components.get(i);
-
-				boolean remove = false;
 				if (dtc.isBitFieldComponent()) {
-					try {
-						changed |= updateBitFieldDataType(dtc, oldDt, replacementDt);
-					}
-					catch (InvalidDataTypeException e) {
-						Msg.error(this,
-							"Invalid bitfield replacement type " + newDt.getName() +
-								", removing bitfield " + dtc.getDataType().getName() + ": " +
-								getPathName());
-						remove = true;
-					}
+					changed |= updateBitFieldDataType(dtc, oldDt, replacementDt);
 				}
 				else if (dtc.getDataType() == oldDt) {
-					if (replacementDt == DEFAULT) {
-						Msg.error(this,
-							"Invalid replacement type " + newDt.getName() +
-								", removing component " + dtc.getDataType().getName() + ": " +
-								getPathName());
-						remove = true;
-					}
-					else {
-						int len = getPreferredComponentLength(newDt, dtc.getLength());
-						dtc.setLength(len, false);
-						oldDt.removeParent(this);
-						dtc.setDataType(replacementDt); // updates record
-						dataMgr.getSettingsAdapter().removeAllSettingsRecords(dtc.getKey());
-						replacementDt.addParent(this);
-						changed = true;
-					}
-				}
-				if (remove) {
+					int len = getPreferredComponentLength(newDt, dtc.getLength());
+					dtc.setLength(len, false);
 					oldDt.removeParent(this);
-					components.remove(i);
-					removeComponentRecord(dtc.getKey());
-					shiftOrdinals(i, -1);
+					dtc.setDataType(replacementDt); // updates record
+					dataMgr.getSettingsAdapter().removeAllSettingsRecords(dtc.getKey());
+					replacementDt.addParent(this);
 					changed = true;
 				}
 			}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/BitFieldDataType.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.
@@ -61,7 +61,8 @@ public class BitFieldDataType extends AbstractDataType {
 	 * bit size may be reduced based upon the specified base datatype size.
 	 * @param bitOffset right shift factor within storage unit when viewed as a big-endian dd
 	 * scalar value.  Based upon minimal storage bitOffset should be in the range 0 to 7.
-	 * @throws InvalidDataTypeException 
+	 * @throws InvalidDataTypeException if invalid base datatype has been specified or an 
+	 * invalid bitSize or bitOffset has been specified
 	 */
 	protected BitFieldDataType(DataType baseDataType, int bitSize, int bitOffset)
 			throws InvalidDataTypeException {
@@ -92,7 +93,7 @@ public class BitFieldDataType extends AbstractDataType {
 	protected BitFieldDataType(DataType baseDataType, int bitSize) throws InvalidDataTypeException {
 		this(baseDataType, bitSize, 0);
 	}
-	
+
 	@Override
 	public boolean isZeroLength() {
 		return bitSize == 0;
@@ -234,8 +235,8 @@ public class BitFieldDataType extends AbstractDataType {
 	public AbstractIntegerDataType getPrimitiveBaseDataType() {
 		// assumes proper enforcement during construction
 		DataType dt = baseDataType;
-		if (baseDataType instanceof TypeDef) {
-			dt = ((TypeDef) baseDataType).getBaseDataType();
+		while (dt instanceof TypeDef typeDef) {
+			dt = typeDef.getBaseDataType();
 		}
 		if (dt instanceof Enum) {
 			// TODO: uncertain if we should use signed or unsigned, although size
@@ -417,8 +418,8 @@ public class BitFieldDataType extends AbstractDataType {
 			return ((Enum) dt).getRepresentation(big, settings, effectiveBitSize);
 		}
 		AbstractIntegerDataType intDT = (AbstractIntegerDataType) dt;
-		if (intDT.getFormatSettingsDefinition().getFormat(
-			settings) == FormatSettingsDefinition.CHAR) {
+		if (intDT.getFormatSettingsDefinition()
+				.getFormat(settings) == FormatSettingsDefinition.CHAR) {
 			if (big.signum() < 0) {
 				big = big.add(BigInteger.valueOf(2).pow(effectiveBitSize));
 			}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/CompositeDataTypeImpl.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.
@@ -233,24 +233,23 @@ public abstract class CompositeDataTypeImpl extends GenericDataType implements C
 	 * @param oldDt             affected datatype which has been removed or replaced
 	 * @param newDt             replacement datatype
 	 * @return true if bitfield component was modified
-	 * @throws InvalidDataTypeException if new datatype is not
 	 */
 	protected boolean updateBitFieldDataType(DataTypeComponentImpl bitfieldComponent,
-			DataType oldDt, DataType newDt) throws InvalidDataTypeException {
+			DataType oldDt, DataType newDt) {
 		if (!bitfieldComponent.isBitFieldComponent()) {
 			throw new AssertException("expected bitfield component");
 		}
 
 		BitFieldDataType bitfieldDt = (BitFieldDataType) bitfieldComponent.getDataType();
-		if (bitfieldDt.getBaseDataType() != oldDt) {
+		if (bitfieldDt.getBaseDataType() != oldDt || !BitFieldDataType.isValidBaseDataType(newDt)) {
 			return false;
 		}
 
 		if (newDt != null) {
-			BitFieldDataType.checkBaseDataType(newDt);
 			int maxBitSize = 8 * newDt.getLength();
 			if (bitfieldDt.getBitSize() > maxBitSize) {
-				throw new InvalidDataTypeException("Replacement datatype too small for bitfield");
+				// Replacement datatype too small for bitfield
+				return false;
 			}
 		}
 
@@ -262,7 +261,7 @@ public abstract class CompositeDataTypeImpl extends GenericDataType implements C
 			newDt.addParent(this);
 		}
 		catch (InvalidDataTypeException e) {
-			throw new AssertException("unexpected");
+			throw new AssertException(e); // unexpected
 		}
 
 		return true;

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

@@ -332,6 +332,9 @@ public class DataTypeComponentImpl implements InternalDataTypeComponent, Seriali
 		if (DataTypeComponent.usesZeroLengthComponent(dataType)) {
 			return 0;
 		}
+		if ((dataType instanceof Dynamic dynamic) && dynamic.canSpecifyLength()) {
+			return length;
+		}
 		int dtLength = dataType.getLength();
 		if (length <= 0) {
 			length = dtLength;

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

@@ -308,7 +308,14 @@ public interface DataTypeManager {
 	public void removeInvalidatedListener(InvalidatedListener listener);
 
 	/**
-	 * Remove the given datatype from this manager
+	 * Remove the given datatype from this manager.
+	 * <br>
+	 * NOTE: Any use of the specified datatype within a {@link FunctionDefinition} will be 
+	 * converted to the {@link DataType#DEFAULT default 'undefined' datatype}.  Any use within
+	 * a {@link Structure} or {@link Union} will be converted to the {@link BadDataType} as
+	 * a placeholder to retain the component's field name and length (the comment will be prefixed
+	 * with a message indicating the remval of the old datatype.
+	 * 
 	 * @param dataType the dataType to be removed
 	 * @param monitor the task monitor
 	 * @return true if the data type existed and was removed

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/FunctionDefinition.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.
@@ -29,7 +29,7 @@ public interface FunctionDefinition extends DataType, FunctionSignature {
 	 * Set the arguments to this function.
 	 * @param args array of parameter definitions to be used as arguments to this function
 	 */
-	public void setArguments(ParameterDefinition[] args);
+	public void setArguments(ParameterDefinition... args);
 
 	/**
 	 * Set the return data type for this function

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

@@ -141,7 +141,7 @@ public class FunctionDefinitionDataType extends GenericDataType implements Funct
 	}
 
 	@Override
-	public void setArguments(ParameterDefinition[] args) {
+	public void setArguments(ParameterDefinition... args) {
 		params = new ParameterDefinition[args.length];
 		for (int i = 0; i < args.length; i++) {
 			DataType dt = args[i].getDataType();

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

@@ -20,7 +20,6 @@ import java.util.*;
 import ghidra.docking.settings.Settings;
 import ghidra.program.model.data.AlignedStructurePacker.StructurePackResult;
 import ghidra.program.model.mem.MemBuffer;
-import ghidra.util.Msg;
 import ghidra.util.UniversalID;
 import ghidra.util.exception.AssertException;
 
@@ -654,7 +653,7 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 		repack(false);
 		notifySizeChanged();
 	}
-	
+
 	@Override
 	public void growStructure(int amount) {
 		if (amount < 0) {
@@ -1296,80 +1295,50 @@ public class StructureDataType extends CompositeDataTypeImpl implements Structur
 		int n = components.size();
 		for (int i = n - 1; i >= 0; i--) {
 			DataTypeComponentImpl dtc = components.get(i);
-			boolean removeBitFieldComponent = false;
 			if (dtc.isBitFieldComponent()) {
+				// Do not allow bitfield to be destroyed
+				// If base type is removed - revert to primitive type
 				BitFieldDataType bitfieldDt = (BitFieldDataType) dtc.getDataType();
-				removeBitFieldComponent = bitfieldDt.getBaseDataType() == dt;
+				if (bitfieldDt.getBaseDataType() == dt &&
+					updateBitFieldDataType(dtc, dt, bitfieldDt.getPrimitiveBaseDataType())) {
+					changed = true;
+				}
 			}
-			if (removeBitFieldComponent || dtc.getDataType() == dt) {
-				dt.removeParent(this);
-// FIXME: Consider replacing with undefined type instead of removing (don't remove bitfield)
-				components.remove(i);
-				shiftOffsets(i, dtc.getLength() - 1, 0);
-				--numComponents; // may be revised by repack
+			else if (dtc.getDataType() == dt) {
+				setComponentDataType(dtc, BadDataType.dataType, i);
 				changed = true;
 			}
 		}
-		if (changed) {
+		// Should be no impact for non-packed
+		if (changed && !isPackingEnabled()) {
 			repack(true);
 		}
 	}
 
 	@Override
-	public void dataTypeReplaced(DataType oldDt, DataType replacementDt)
-			throws IllegalArgumentException {
-		DataType newDt = replacementDt;
+	public void dataTypeReplaced(DataType oldDt, DataType newDt) throws IllegalArgumentException {
+		DataType replacementDt = newDt;
 		try {
-			validateDataType(replacementDt);
+			replacementDt = validateDataType(replacementDt); // blocks DEFAULT use for packed
 			replacementDt = replacementDt.clone(dataMgr);
 			checkAncestry(replacementDt);
 		}
 		catch (Exception e) {
-			// TODO: should we use Undefined1 instead to avoid cases where
-			// DEFAULT datatype can not be used (bitfield, aligned structure, etc.)
-			// TODO: failing silently is rather hidden
-			replacementDt = DataType.DEFAULT;
+			// Handle bad replacement with use of undefined component
+			replacementDt = isPackingEnabled() ? Undefined1DataType.dataType : DataType.DEFAULT;
 		}
 
 		boolean changed = false;
 		for (int i = components.size() - 1; i >= 0; i--) {
-
 			DataTypeComponentImpl comp = components.get(i);
-
-			boolean remove = false;
 			if (comp.isBitFieldComponent()) {
-				try {
-					changed |= updateBitFieldDataType(comp, oldDt, replacementDt);
-				}
-				catch (InvalidDataTypeException e) {
-					Msg.error(this,
-						"Invalid bitfield replacement type " + newDt.getName() +
-							", removing bitfield " + comp.getDataType().getName() + ": " +
-							getPathName());
-					remove = true;
-				}
+				changed |= updateBitFieldDataType(comp, oldDt, replacementDt);
 			}
 			else if (comp.getDataType() == oldDt) {
-				if (replacementDt == DEFAULT && isPackingEnabled()) {
-					Msg.error(this,
-						"Invalid replacement type " + newDt.getName() + ", removing component " +
-							comp.getDataType().getName() + ": " + getPathName());
-					remove = true;
-				}
-				else {
-					setComponentDataType(comp, replacementDt, i);
-					changed = true;
-				}
-			}
-			if (remove) {
-				// error case - remove component
-				oldDt.removeParent(this);
-				components.remove(i);
-				shiftOffsets(i, comp.getLength() - 1, 0); // ordinals only
+				setComponentDataType(comp, replacementDt, i);
 				changed = true;
 			}
 		}
-
 		if (changed) {
 			repack(false);
 			notifySizeChanged(); // also handles alignment change

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

@@ -19,7 +19,6 @@ import java.util.*;
 
 import ghidra.docking.settings.Settings;
 import ghidra.program.model.mem.MemBuffer;
-import ghidra.util.Msg;
 import ghidra.util.UniversalID;
 
 /**
@@ -505,57 +504,26 @@ public class UnionDataType extends CompositeDataTypeImpl implements UnionInterna
 	public void dataTypeReplaced(DataType oldDt, DataType newDt) throws IllegalArgumentException {
 		DataType replacementDt = newDt;
 		try {
-			validateDataType(replacementDt);
-			if (replacementDt.getDataTypeManager() != dataMgr) {
-				replacementDt = replacementDt.clone(dataMgr);
-			}
+			replacementDt = validateDataType(replacementDt); // blocks DEFAULT use
+			replacementDt = replacementDt.clone(dataMgr);
 			checkAncestry(replacementDt);
 		}
 		catch (Exception e) {
-			// TODO: should we use Undefined instead since we do not support
-			// DEFAULT in Unions
-			replacementDt = DataType.DEFAULT;
+			replacementDt = Undefined1DataType.dataType;
 		}
 		boolean changed = false;
 		for (int i = components.size() - 1; i >= 0; i--) {
-
 			DataTypeComponentImpl dtc = components.get(i);
-
-			boolean remove = false;
 			if (dtc.isBitFieldComponent()) {
-				try {
-					changed |= updateBitFieldDataType(dtc, oldDt, replacementDt);
-				}
-				catch (InvalidDataTypeException e) {
-					Msg.error(this,
-						"Invalid bitfield replacement type " + newDt.getName() +
-							", removing bitfield " + dtc.getDataType().getName() + ": " +
-							getPathName());
-					remove = true;
-				}
+				changed |= updateBitFieldDataType(dtc, oldDt, replacementDt);
 			}
 			else if (dtc.getDataType() == oldDt) {
-				if (replacementDt == DEFAULT) {
-					Msg.error(this,
-						"Invalid replacement type " + newDt.getName() + ", removing component " +
-							dtc.getDataType().getName() + ": " + getPathName());
-					remove = true;
-				}
-				else {
-					int len = getPreferredComponentLength(newDt, dtc.getLength());
-					oldDt.removeParent(this);
-					dtc.setLength(len);
-					dtc.setDataType(replacementDt);
-					dtc.invalidateSettings();
-					replacementDt.addParent(this);
-					changed = true;
-				}
-			}
-			if (remove) {
-				// error case - remove component
+				int len = getPreferredComponentLength(newDt, dtc.getLength());
 				oldDt.removeParent(this);
-				components.remove(i);
-				shiftOrdinals(i, -1);
+				dtc.setLength(len);
+				dtc.setDataType(replacementDt);
+				dtc.invalidateSettings();
+				replacementDt.addParent(this);
 				changed = true;
 			}
 		}
@@ -570,19 +538,22 @@ public class UnionDataType extends CompositeDataTypeImpl implements UnionInterna
 		boolean changed = false;
 		for (int i = components.size() - 1; i >= 0; i--) { // reverse order
 			DataTypeComponentImpl dtc = components.get(i);
-			boolean removeBitFieldComponent = false;
 			if (dtc.isBitFieldComponent()) {
+				// Do not allow bitfield to be destroyed
+				// If base type is removed - revert to primitive type
 				BitFieldDataType bitfieldDt = (BitFieldDataType) dtc.getDataType();
-				removeBitFieldComponent = bitfieldDt.getBaseDataType() == dt;
+				if (bitfieldDt.getBaseDataType() == dt &&
+					updateBitFieldDataType(dtc, dt, bitfieldDt.getPrimitiveBaseDataType())) {
+					changed = true;
+				}
 			}
-			if (removeBitFieldComponent || dtc.getDataType() == dt) {
+			else if (dtc.getDataType() == dt) {
 				dt.removeParent(this);
-				components.remove(i);
-				shiftOrdinals(i, -1);
+				dtc.setDataType(BadDataType.dataType); // updates record
 				changed = true;
 			}
 		}
-		if (changed && !repack(true) && isPackingEnabled()) {
+		if (changed && isPackingEnabled() && !repack(true)) {
 			// NOTE: Must assume alignment change since we are unable to determine
 			// without stored alignment
 			notifyAlignmentChanged();

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

@@ -1153,13 +1153,34 @@ public class StructureDBTest extends AbstractGenericTest {
 		assertEquals(8, struct.getLength());
 
 		struct.add(new ArrayDataType(IntegerDataType.dataType, 0, -1), "flex", "FlexComment");
-		assertEquals(5, struct.getNumComponents());
-		assertEquals(8, struct.getLength());
+
+		//@formatter:off
+		CompositeTestUtils.assertExpectedComposite(this, "/Test\n" + 
+			"pack(disabled)\n" + 
+			"Structure Test {\n" + 
+			"   0   byte   1   field1   \"Comment1\"\n" + 
+			"   1   word   2      \"Comment2\"\n" + 
+			"   3   dword   4   field3   \"\"\n" + 
+			"   7   byte   1   field4   \"Comment4\"\n" + 
+			"   8   int[0]   0   flex   \"FlexComment\"\n" + 
+			"}\n" + 
+			"Length: 8 Alignment: 1", struct);
+		//@formatter:on
 
 		dataMgr.remove(dataMgr.resolve(IntegerDataType.dataType, null), TaskMonitor.DUMMY);
 
-		assertEquals(4, struct.getNumComponents());
-		assertEquals(8, struct.getLength());
+		//@formatter:off
+		CompositeTestUtils.assertExpectedComposite(this, "/Test\n" + 
+			"pack(disabled)\n" + 
+			"Structure Test {\n" + 
+			"   0   byte   1   field1   \"Comment1\"\n" + 
+			"   1   word   2      \"Comment2\"\n" + 
+			"   3   dword   4   field3   \"\"\n" + 
+			"   7   byte   1   field4   \"Comment4\"\n" + 
+			"   8   -BAD-   0   flex   \"Type 'int[0]' was deleted; FlexComment\"\n" + 
+			"}\n" + 
+			"Length: 8 Alignment: 1", struct);
+		//@formatter:on
 	}
 
 	@Test
@@ -1197,9 +1218,9 @@ public class StructureDBTest extends AbstractGenericTest {
 			"   1   word   2      \"Comment2\"\n" + 
 			"   3   dword   4   field3   \"\"\n" + 
 			"   7   byte   1   field4   \"Comment4\"\n" + 
-//			"   8   undefined   1      \"\"\n" + 
-//			"   9   undefined   1      \"\"\n" + 
-//			"   10   undefined   1      \"\"\n" + 
+			"   8   int:4(0)   1   MyBit1   \"Type 'Foo' was deleted; bitComment\"\n" + 
+			"   9   int:3(0)   1   MyBit2   \"Type 'Foo' was deleted; bitComment\"\n" + 
+			"   10   int:2(0)   1   MyBit3   \"Type 'Foo' was deleted; bitComment\"\n" + 
 			"}\n" + 
 			"Length: 11 Alignment: 1", struct);
 		//@formatter:on
@@ -1807,14 +1828,33 @@ public class StructureDBTest extends AbstractGenericTest {
 		DataType dt = struct.getDataTypeManager().getDataType(struct.getCategoryPath(), "test1");
 		assertNotNull(dt);
 
-		DataTypeComponent[] dtc = struct.getComponents();
-		assertEquals(5, dtc.length);
+		//@formatter:off
+		CompositeTestUtils.assertExpectedComposite(this, "/Test\n" + 
+			"pack(disabled)\n" + 
+			"Structure Test {\n" + 
+			"   0   byte   1   field1   \"Comment1\"\n" + 
+			"   1   word   2      \"Comment2\"\n" + 
+			"   3   dword   4   field3   \"\"\n" + 
+			"   7   byte   1   field4   \"Comment4\"\n" + 
+			"   8   test1   5      \"\"\n" + 
+			"}\n" + 
+			"Length: 13 Alignment: 1", struct);
+		//@formatter:on
 
 		dt.getDataTypeManager().remove(dt, new TaskMonitorAdapter());
-		dtc = struct.getComponents();
-		assertEquals(9, dtc.length);
 
-		assertEquals(9, struct.getNumComponents());
+		//@formatter:off
+		CompositeTestUtils.assertExpectedComposite(this, "/Test\n" + 
+			"pack(disabled)\n" + 
+			"Structure Test {\n" + 
+			"   0   byte   1   field1   \"Comment1\"\n" + 
+			"   1   word   2      \"Comment2\"\n" + 
+			"   3   dword   4   field3   \"\"\n" + 
+			"   7   byte   1   field4   \"Comment4\"\n" + 
+			"   8   -BAD-   5      \"Type 'test1' was deleted\"\n" + 
+			"}\n" + 
+			"Length: 13 Alignment: 1", struct);
+		//@formatter:on
 	}
 
 	@Test

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

@@ -280,6 +280,8 @@ public class UnionDBTest extends AbstractGenericTest {
 			"Union TestUnion {\n" + 
 			"   0   byte   1   field1   \"Comment1\"\n" + 
 			"   0   word   2      \"Comment2\"\n" + 
+			"   0   int:4(0)   1   bf1   \"Type 'Foo' was deleted; bf1Comment\"\n" + 
+			"   0   int:4(0)   1   bf2   \"Type 'Foo' was deleted; bf2Comment\"\n" + 
 			"   0   dword   4   field3   \"\"\n" + 
 			"   0   byte   1   field4   \"Comment4\"\n" + 
 			"}\n" +