GP-3279 - PDB conflicts on composite member datatypes of same name for older unnamed members

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

@@ -26,14 +26,14 @@ import ghidra.util.exception.*;
 import ghidra.util.task.TaskMonitor;
 
 /**
- * <code>CompositeMember</code> provides the ability to process PDB data-type records and 
+ * <code>CompositeMember</code> provides the ability to process PDB data-type records and
- * incrementally build-up composite structure and union data-types from a flattened offset-based 
+ * incrementally build-up composite structure and union data-types from a flattened offset-based
- * list of members which may include embedded anonymous composite members.  Composite members 
+ * list of members which may include embedded anonymous composite members.  Composite members
  * correspond to either hard predefined data-types, or structure/union containers whose members
- * are added and refined incrementally.  
+ * are added and refined incrementally.
  * <p>
- * Container members are characterized by a null data-type name, zero length, and will be 
+ * Container members are characterized by a null data-type name, zero length, and will be
- * identified as either a structure or union. 
+ * identified as either a structure or union.
  */
 public class DefaultCompositeMember extends CompositeMember {
 
@@ -143,7 +143,7 @@ public class DefaultCompositeMember extends CompositeMember {
 	 * @param componentOffset member offset within parent
 	 * @param baseDataType bitfield base datatype
 	 * @param bitSize bitfield size in bits
-	 * @param bitOffsetWithinBaseType offset of bitfield within base type 
+	 * @param bitOffsetWithinBaseType offset of bitfield within base type
 	 * @throws InvalidDataTypeException invalid baseDataType for bitfield
 	 */
 	private DefaultCompositeMember(int componentOffset, DataType baseDataType, int bitSize,
@@ -220,7 +220,7 @@ public class DefaultCompositeMember extends CompositeMember {
 			parent.memberNameChanged(oldMemberName, memberName);
 		}
 		catch (InvalidNameException | DuplicateNameException e) {
-			// exceptions are unexpected 
+			// exceptions are unexpected
 			throw new AssertException(e);
 		}
 	}
@@ -309,7 +309,7 @@ public class DefaultCompositeMember extends CompositeMember {
 	}
 
 	/**
-	 * Align container composite data type if possible.  
+	 * Align container composite data type if possible.
 	 * @param preferredSize preferred size of composite if known, else <= 0 if unknown
 	 */
 	private void alignComposite(int preferredSize) {
@@ -335,11 +335,19 @@ public class DefaultCompositeMember extends CompositeMember {
 			composite.setToDefaultPacking();
 		}
 		else {
-			pack = 1;
+			copy.setToMachineAligned();
-			copy.setExplicitPackingValue(pack);
 			alignOK = isGoodAlignment(copy, preferredSize);
 			if (alignOK) {
-				composite.setExplicitPackingValue(pack);
+				composite.setToDefaultPacking();
+				composite.setToMachineAligned();
+			}
+			else {
+				pack = 1;
+				copy.setExplicitPackingValue(pack);
+				alignOK = isGoodAlignment(copy, preferredSize);
+				if (alignOK) {
+					composite.setExplicitPackingValue(pack);
+				}
 			}
 		}
 		if (!alignOK && errorConsumer != null && !isClass) { // don't complain about Class structs which always fail
@@ -503,7 +511,7 @@ public class DefaultCompositeMember extends CompositeMember {
 	 * false if unable to resolve member's data-type or other error occurred.
 	 * NOTE: there may be complex hierarchies not yet handled.
 	 * @throws CancelledException if operation cancelled
-	 * @throws DataTypeDependencyException if child's datatype can not be resolved.  
+	 * @throws DataTypeDependencyException if child's datatype can not be resolved.
 	 * It may be possible to skip and continue with next child.
 	 */
 	private boolean addMember(PdbMember child, TaskMonitor monitor)
@@ -813,7 +821,7 @@ public class DefaultCompositeMember extends CompositeMember {
 
 	private boolean addStructureMember(DefaultCompositeMember member) {
 		try {
-			// check for conflict within structure container deferred  
+			// check for conflict within structure container deferred
 			int conflictOffset = structureMemberRangeMap.getValue(member.memberOffset);
 			if (conflictOffset < 0) {
 
@@ -1086,10 +1094,10 @@ public class DefaultCompositeMember extends CompositeMember {
 
 	/**
 	 * This method facilitates the removal and collection of all siblings of this
-	 * member from its parent container.  Only those siblings whose offset is greater 
+	 * member from its parent container.  Only those siblings whose offset is greater
-	 * than this member's offset will be included.  The use of this method is necessary when 
+	 * than this member's offset will be included.  The use of this method is necessary when
-	 * a member sequence has been added to a structure container and it is later decided to 
+	 * a member sequence has been added to a structure container and it is later decided to
-	 * push this member and its siblings into a new sub-composite.  Before they can be 
+	 * push this member and its siblings into a new sub-composite.  Before they can be
 	 * added to the new container they must be removed from their current container
 	 * using this method.
 	 * @return list of sibling structure members removed from parent
@@ -1145,8 +1153,8 @@ public class DefaultCompositeMember extends CompositeMember {
 	}
 
 	/**
-	 * Buildup an empty composite by applying datatype composite members.  
+	 * Buildup an empty composite by applying datatype composite members.
-	 * Only those children with a kind of "Member" will be processed. 
+	 * Only those children with a kind of "Member" will be processed.
 	 * @param composite empty composite to which members will be added
 	 * @param isClass true if composite corresponds to a Class structure, else false
 	 * @param preferredCompositeSize preferred size of composite, <= 0 indicates unknown
@@ -1195,8 +1203,8 @@ public class DefaultCompositeMember extends CompositeMember {
 	private static enum MemberType {
 
 		//@formatter:off
-		STRUCTURE, 
+		STRUCTURE,
-		UNION, 
+		UNION,
 		MEMBER;
 		//@formatter:on
 

Ghidra/Features/PDB/src/main/java/ghidra/app/util/pdb/pdbapplicator/CompositeTypeApplier.java

@@ -97,22 +97,59 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 			return;
 		}
 
-		dataType = createEmptyComposite((AbstractCompositeMsType) msType);
+		SymbolPath sp = getFixedSymbolPath();
-		String mangledName = ((AbstractCompositeMsType) msType).getMangledName();
+		CategoryPath categoryPath = applicator.getCategory(sp.getParent());
-		classType = new CppCompositeType((Composite) dataType, mangledName);
+		ComboType c = getOrCreateComposite(applicator, this, (AbstractCompositeMsType) msType,
-		classType.setName(getName());
+			categoryPath, sp);
-		classType.setSize(DefaultPdbApplicator.bigIntegerToInt(applicator, getSize()));
+		dataType = c.dt();
-		if (msType instanceof AbstractClassMsType) {
+		classType = c.ct();
-			classType.setClass();
+	}
-		}
-		else if (msType instanceof AbstractStructureMsType) {
-			classType.setStruct();
-		}
-		else if (msType instanceof AbstractUnionMsType) {
-			classType.setUnion();
-		}
-		classType.setFinal(isFinal());
 
+	private record ComboType(DataType dt, CppCompositeType ct) {
+	}
+
+	// DefaultPdbApplicator is passed in for bigIntegerToInt...
+	//  TODO: find a better way... maybe eventually eliminate PdbMsgLog
+	private static ComboType getOrCreateComposite(DefaultPdbApplicator myApplicator,
+			AbstractComplexTypeApplier myCompositeApplier, AbstractCompositeMsType compositeMsType,
+			CategoryPath categoryPath, SymbolPath fixedSymbolPath) {
+
+		Composite myComposite;
+		CppCompositeType myClassType;
+
+		String mangledName = compositeMsType.getMangledName();
+
+		if (compositeMsType instanceof AbstractClassMsType) {
+			myApplicator.predefineClass(fixedSymbolPath);
+			myComposite = new StructureDataType(categoryPath, fixedSymbolPath.getName(), 0,
+				myApplicator.getDataTypeManager());
+			myClassType = new CppCompositeType(myComposite, mangledName);
+			myClassType.setClass();
+		}
+		else if (compositeMsType instanceof AbstractStructureMsType) {
+			myComposite = new StructureDataType(categoryPath, fixedSymbolPath.getName(), 0,
+				myApplicator.getDataTypeManager());
+			myClassType = new CppCompositeType(myComposite, mangledName);
+			myClassType.setStruct();
+		}
+		else if (compositeMsType instanceof AbstractUnionMsType) {
+			myComposite = new UnionDataType(categoryPath, fixedSymbolPath.getName(),
+				myApplicator.getDataTypeManager());
+			myClassType = new CppCompositeType(myComposite, mangledName);
+			myClassType.setUnion();
+		}
+		else { // InterfaceMsType
+			String message = "Unsupported datatype (" + compositeMsType.getClass().getSimpleName() +
+				"): " + fixedSymbolPath.getPath();
+			myApplicator.appendLogMsg(message);
+			return null;
+		}
+		myClassType.setName(myCompositeApplier.getMsType().getName());
+		myClassType.setSize(
+			DefaultPdbApplicator.bigIntegerToInt(myApplicator, myCompositeApplier.getSize()));
+		myClassType.setFinal(compositeMsType.getMsProperty().isSealed());
+
+		return new ComboType(myComposite, myClassType);
 	}
 
 	@Override
@@ -146,11 +183,18 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 		// TODO: Dome some output comparisons with and without the !isNested()
 		// test which is intended to ignore nested anonymous composites.
 
-		if (!isForwardReference() && !isNested()) {
+		if (!isForwardReference() && !isNested() && !isUnnamed()) {
 			super.resolve();
 		}
 	}
 
+	private boolean isUnnamed() {
+		if (msType instanceof AbstractClassMsType) {
+			return false;
+		}
+		return "__unnamed".equals(((AbstractComplexMsType) msType).getName());
+	}
+
 	private void applyOrDeferForDependencies() throws PdbException, CancelledException {
 		AbstractCompositeMsType type = (AbstractCompositeMsType) msType;
 		MsProperty property = type.getMsProperty();
@@ -202,7 +246,7 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 				applicator.addApplierDependency(this, memberTypeApplierIterated);
 			}
 		}
-		if (!isDeferred()) {
+		if (!isDeferred() || isUnnamed()) {
 			applyInternal();
 		}
 	}
@@ -260,7 +304,11 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 		if (isApplied()) {
 			return;
 		}
-		Composite composite = (Composite) dataType;
+		applyInternal((Composite) dataType);
+		setApplied();
+	}
+
+	private void applyInternal(Composite composite) throws CancelledException, PdbException {
 
 		AbstractCompositeMsType type = (AbstractCompositeMsType) msType;
 
@@ -272,13 +320,16 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 					"Unexpected base classes for union type: " + type.getName());
 			}
 		}
+		if (isUnnamed()) {
+			applyCpp = false;
+		}
+
 		if (applyCpp) {
 			applyCpp(composite, type);
 		}
 		else {
 			applyBasic(composite, type);
 		}
-		setApplied();
 	}
 
 	//==============================================================================================
@@ -384,6 +435,35 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 		return dataType;
 	}
 
+	/**
+	 * Returns a datatype or a newly minted datatype if it is an unnamed datatype.  In the latter
+	 * case, the type is put into an internal category of the parent and the name is given
+	 * a suffix based on the ordinal within the parent
+	 * @param categoryPath the CategoryPath
+	 * @param ordinal the ordinal
+	 * @return the Datatype
+	 * @throws CancelledException upon user cancellation
+	 * @throws PdbException upon error
+	 */
+	DataType getDataType(CategoryPath categoryPath, int ordinal)
+			throws CancelledException, PdbException {
+		if (!isUnnamed()) {
+			return getDataType();
+		}
+		return mintNestedUnnamedDataType(categoryPath, ordinal);
+	}
+
+	private DataType mintNestedUnnamedDataType(CategoryPath categoryPath, int ordinal)
+			throws CancelledException, PdbException {
+		SymbolPath sp = getFixedSymbolPath();
+		SymbolPath modifiedSymbolPath =
+			new SymbolPath(sp.getParent(), sp.getName() + "_" + ordinal);
+		ComboType c = getOrCreateComposite(applicator, this, (AbstractCompositeMsType) msType,
+			categoryPath, modifiedSymbolPath);
+		applyInternal((Composite) c.dt());
+		return c.dt();
+	}
+
 	@Override
 	DataType getCycleBreakType() {
 		if (isForwardReference() && definitionApplier != null && definitionApplier.isApplied()) {
@@ -416,34 +496,6 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 		}
 	}
 
-	private Composite createEmptyComposite(AbstractCompositeMsType type) {
-
-		SymbolPath fixedPath = getFixedSymbolPath();
-		CategoryPath categoryPath = applicator.getCategory(fixedPath.getParent());
-
-		Composite composite;
-		if (type instanceof AbstractClassMsType) {
-			applicator.predefineClass(fixedPath);
-			composite = new StructureDataType(categoryPath, fixedPath.getName(), 0,
-				applicator.getDataTypeManager());
-		}
-		else if (type instanceof AbstractStructureMsType) {
-			composite = new StructureDataType(categoryPath, fixedPath.getName(), 0,
-				applicator.getDataTypeManager());
-		}
-		else if (type instanceof AbstractUnionMsType) {
-			composite = new UnionDataType(categoryPath, fixedPath.getName(),
-				applicator.getDataTypeManager());
-		}
-		else { // InterfaceMsType
-			String message = "Unsupported datatype (" + type.getClass().getSimpleName() + "): " +
-				fixedPath.getPath();
-			applicator.appendLogMsg(message);
-			return null;
-		}
-		return composite;
-	}
-
 	private boolean hasBaseClasses() {
 		AbstractCompositeMsType defType;
 		if (definitionApplier == null) {
@@ -644,7 +696,8 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 		return new CppCompositeType.ClassFieldAttributes(myAccess, myProperty);
 	}
 
-	private void addMembers(Composite composite, FieldListTypeApplier fieldListApplier) {
+	private void addMembers(Composite composite, FieldListTypeApplier fieldListApplier)
+			throws CancelledException, PdbException {
 
 		AbstractCompositeMsType type = (AbstractCompositeMsType) msType;
 
@@ -660,6 +713,7 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 				memberAttributes.getAccess(); // TODO: do something with this and other attributes
 				MsTypeApplier fieldApplier = memberTypeApplier.getFieldTypeApplier();
 
+				DataType fieldDataType = null;
 				if (fieldApplier instanceof CompositeTypeApplier) {
 					CompositeTypeApplier defApplier =
 						((CompositeTypeApplier) fieldApplier).getDefinitionApplier(
@@ -667,8 +721,12 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 					if (defApplier != null) {
 						fieldApplier = defApplier;
 					}
+					fieldDataType = ((CompositeTypeApplier) fieldApplier).getDataType(
+						ClassTypeUtils.getInternalsCategoryPath(this), members.size());
+				}
+				if (fieldDataType == null) {
+					fieldDataType = fieldApplier.getDataType();
 				}
-				DataType fieldDataType = fieldApplier.getDataType();
 				boolean isFlexibleArray;
 				if (fieldApplier instanceof ArrayTypeApplier) {
 					isFlexibleArray = ((ArrayTypeApplier) fieldApplier).isFlexibleArray();
@@ -676,7 +734,13 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 				else {
 					isFlexibleArray = false;
 				}
-				if (fieldDataType == null) {
+				if (fieldApplier instanceof CompositeTypeApplier &&
+					((CompositeTypeApplier) fieldApplier).isUnnamed()) {
+					DefaultPdbUniversalMember member = new DefaultPdbUniversalMember(applicator,
+						memberName, fieldDataType, offset);
+					members.add(member);
+				}
+				else if (fieldDataType == null) {
 					if (fieldApplier instanceof PrimitiveTypeApplier &&
 						((PrimitiveTypeApplier) fieldApplier).isNoType()) {
 						DefaultPdbUniversalMember member = new DefaultPdbUniversalMember(applicator,

Ghidra/Features/PDB/src/main/java/ghidra/app/util/pdb/pdbapplicator/DefaultPdbUniversalMember.java

@@ -101,7 +101,7 @@ public class DefaultPdbUniversalMember extends PdbMember {
 
 		DataType dt = getDataTypeInternal();
 		if (dt instanceof PdbBitField) {
-			PdbBitField bfDt = (PdbBitField) dataType;
+			PdbBitField bfDt = (PdbBitField) dt;
 			builder.append(", type=");
 			builder.append(bfDt.getBaseDataType().getName());
 			builder.append(", offset=");
@@ -113,7 +113,7 @@ public class DefaultPdbUniversalMember extends PdbMember {
 		}
 		else {
 			builder.append(", type=");
-			builder.append(dataType.getName());
+			builder.append(dt.getName());
 			builder.append(", offset=");
 			builder.append(getOffset());
 		}