GP-929 Fix DWARF data type import regression

Fix regression when structure data types defined in DWARF are
encountered in a specific order, an earlier empty definition might be
retained in favor of a later more correct definition.

Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/dwarf4/next/DWARFDataTypeConflictHandler.java

@@ -172,21 +172,48 @@ class DWARFDataTypeConflictHandler extends DataTypeConflictHandler {
 				!SystemUtilities.isEqual(fullDTCAt.getFieldName(), partDTC.getFieldName())) {
 				return false;
 			}
-			DataType partDT = partDTC.getDataType();
-			DataType fullDT = fullDTCAt.getDataType();
-			if (doRelaxedCompare(partDT, fullDT, visitedDataTypes) == RENAME_AND_ADD) {
+			if (!isMemberFieldPartiallyCompatible(fullDTCAt, partDTC, visitedDataTypes)) {
 				return false;
 			}
 		}
 		if ( part.getFlexibleArrayComponent() != null ) {
-			return full.getFlexibleArrayComponent() != null
-					&& doRelaxedCompare(part.getFlexibleArrayComponent().getDataType(),
-							full.getFlexibleArrayComponent().getDataType(), visitedDataTypes) != RENAME_AND_ADD;
+			return full.getFlexibleArrayComponent() != null &&
+				isMemberFieldPartiallyCompatible(full.getFlexibleArrayComponent(),
+					part.getFlexibleArrayComponent(), visitedDataTypes);
 		}
 
 		return true;
 	}
 
+	boolean isMemberFieldPartiallyCompatible(DataTypeComponent fullDTC, DataTypeComponent partDTC,
+			Set<Long> visitedDataTypes) {
+		DataType partDT = partDTC.getDataType();
+		DataType fullDT = fullDTC.getDataType();
+		ConflictResult dtCompResult = doRelaxedCompare(partDT, fullDT, visitedDataTypes);
+		switch (dtCompResult) {
+			case RENAME_AND_ADD:
+				// The data type of the field in the 'full' structure is completely
+				// different than the field in the 'part' structure, therefore
+				// the candidate 'part' structure is not a partial definition of the full struct
+				return false;
+			case REPLACE_EXISTING:
+				// Return true (meaning the field from the 'full' struct is the same or better
+				// than the field from the 'part' structure) if the components are size compatible.
+				// This is an intentionally fuzzy match to allow structures with fields
+				// that are generally the same at a binary level to match.
+				// For example, the same structure defined in 2 separate compile units with
+				// slightly different types for the field (due to different compiler options
+				// or versions or languages)
+				return fullDTC.getLength() >= partDTC.getLength();
+			case USE_EXISTING:
+			default:
+				// the data type of the field in the 'full' structure is the same as
+				// or a better version of the field in the 'part' structure.
+				return true;
+		}
+
+	}
+
 	private DataTypeComponent getBitfieldByOffsets(Structure full, DataTypeComponent partDTC) {
 		BitFieldDataType partBF = (BitFieldDataType) partDTC.getDataType();
 

Ghidra/Features/Base/src/main/java/ghidra/app/util/bin/format/dwarf4/next/DWARFDataTypeImporter.java

@@ -140,14 +140,11 @@ public class DWARFDataTypeImporter {
 		if (result != null) {
 			return result;
 		}
+
+		// Query the dwarfDTM for plain Ghidra dataTypes that were previously
+		// registered in a different import session.
 		DataType alreadyImportedDT = dwarfDTM.getDataType(diea.getOffset(), null);
-		if (alreadyImportedDT != null &&
-			!(alreadyImportedDT instanceof Array &&
-				((Array) alreadyImportedDT).getNumElements() == 1)) {
-			// HACK: don't re-use previously imported single-element 
-			// Ghidra array datatype because they may have actually been an empty array
-			// definition we need the special meta-data flag DWARFDataType.isEmptyArrayType
-			// which is only available in a freshly created DWARFDataType.
+		if (shouldReuseAlreadyImportedDT(alreadyImportedDT)) {
 			return new DWARFDataType(alreadyImportedDT, null, diea.getOffset());
 		}
 
@@ -223,6 +220,28 @@ public class DWARFDataTypeImporter {
 		return result;
 	}
 
+	/**
+	 * Returns true if the previously imported data type should be reused.
+	 * <p>
+	 * Don't re-use previously imported single-element
+	 * Ghidra array datatypes because they may have actually been an empty array
+	 * definition and we need the special meta-data flag DWARFDataType.isEmptyArrayType
+	 * which is only available in a freshly created DWARFDataType.
+	 * <p>
+	 * Don't re-use empty structs (isNotYetDefined) to ensure that newer
+	 * definitions of the same struct are given a chance to be resolved() 
+	 * into the DTM. 
+	 * 
+	 * @param alreadyImportedDT dataType to check
+	 * @return boolean true if its okay to reuse the data type
+	 */
+	private boolean shouldReuseAlreadyImportedDT(DataType alreadyImportedDT) {
+		return alreadyImportedDT != null &&
+			!alreadyImportedDT.isNotYetDefined() &&
+			!(alreadyImportedDT instanceof Array &&
+				((Array) alreadyImportedDT).getNumElements() == 1);
+	}
+
 	/*
 	 * when a clone()'d datatype is created, update the current mappings to
 	 * point to the new instance instead of the old instance.

Ghidra/Features/Base/src/test.slow/java/ghidra/app/util/bin/format/dwarf4/next/DWARFDataTypeImporterTest.java

@@ -165,11 +165,10 @@ public class DWARFDataTypeImporterTest extends DWARFTestBase {
 	 * @throws DWARFException
 	 */
 	@Test
-	@SuppressWarnings("unused")
 	public void testStructDanglingDecl() throws CancelledException, IOException, DWARFException {
 
 		// CU1
-		DebugInfoEntry declDIE = newDeclStruct("mystruct").create(cu);
+		newDeclStruct("mystruct").create(cu);
 
 		// CU2
 		DebugInfoEntry intDIE = addInt(cu2);
@@ -188,11 +187,85 @@ public class DWARFDataTypeImporterTest extends DWARFTestBase {
 		assertNull(structdt2);
 	}
 
-	/**
+	/*
+	 * This test is more about the StructureDB implementation updating ordinals of
+	 * components correctly when an embedded datatype changes size.
+	 * <p>
+	 * The initial version of struct2.guardfield will have an ordinal of 15 to account for
+	 * all the invisible undefines that are between the first field and the guardfield at offset
+	 * 16.  When the data type for struct1 is overwritten with new information that
+	 * changes it size, the undefines are no longer needed and the ordinal of guardfield should
+	 * just be 1.
+	 */
+	@Test
+	public void testStructDeclThenGrow() throws CancelledException, IOException, DWARFException {
+
+		// CU1
+		DebugInfoEntry intDIE = addInt(cu);
+		DebugInfoEntry struct1Decl = newDeclStruct("struct1").create(cu);
+		DebugInfoEntry struct2 = newStruct("struct2", 20).create(cu);
+		newMember(struct2, "struct1field", struct1Decl, 0).create(cu);
+		newMember(struct2, "guardfield", intDIE, 16).create(cu);
+
+		// CU2
+		DebugInfoEntry int2DIE = addInt(cu2);
+
+		DebugInfoEntry struct1Impl = newStruct("struct1", 16).create(cu2);
+		newMember(struct1Impl, "f1", int2DIE, 0).create(cu2);
+		newMember(struct1Impl, "f2", int2DIE, 4).create(cu2);
+		newMember(struct1Impl, "f3", int2DIE, 8).create(cu2);
+		newMember(struct1Impl, "f4", int2DIE, 12).create(cu2);
+
+		importAllDataTypes();
+
+		Structure struct2dt = (Structure)dataMgr.getDataType(rootCP, "struct2");
+
+		assertEquals(2, struct2dt.getNumComponents());
+		assertEquals(2, struct2dt.getNumDefinedComponents());
+	}
+
+	@Test
+	public void testStructDeclThatIsLaterDefined()
+			throws CancelledException, IOException, DWARFException {
+
+		// CU1
+		// struct structA; // fwd decl
+		// struct structB { structA struct1field; int guardfield; }
+		DebugInfoEntry intDIE = addInt(cu);
+		DebugInfoEntry structADecl = newDeclStruct("structA").create(cu);
+		DebugInfoEntry structB = newStruct("structB", 20).create(cu);
+		newMember(structB, "structAfield", structADecl, 0).create(cu);
+		newMember(structB, "guardfield", intDIE, 16).create(cu);
+
+		// CU2
+		// struct structB { structA struct1field; int guardfield; }
+		// struct structA { int f1, f2, f3, f4; }
+		// Redefine structB with the same info, but to a fully
+		// specified structA instance instead of missing fwd decl.
+		// The order of the DIE records is important.  The structure (structB) 
+		// containing the problematic structA needs to be hit first so we can
+		// test that cached types are handled correctly.
+		DebugInfoEntry int2DIE = addInt(cu2);
+		DebugInfoEntry structB_cu2 = newStruct("structB", 20).create(cu2);
+		newMember(structB_cu2, "structAfield", getForwardOffset(cu2, 2), 0).create(cu2);
+		newMember(structB_cu2, "guardfield", int2DIE, 16).create(cu2);
+
+		DebugInfoEntry structA_cu2 = newStruct("structA", 16).create(cu2);
+		newMember(structA_cu2, "f1", int2DIE, 0).create(cu2);
+		newMember(structA_cu2, "f2", int2DIE, 4).create(cu2);
+		newMember(structA_cu2, "f3", int2DIE, 8).create(cu2);
+		newMember(structA_cu2, "f4", int2DIE, 12).create(cu2);
+
+		importAllDataTypes();
+
+		Structure structAdt = (Structure) dataMgr.getDataType(rootCP, "structA");
+		Structure structBdt = (Structure) dataMgr.getDataType(rootCP, "structB");
+		assertEquals(2, structBdt.getNumComponents());
+		assertEquals(4, structAdt.getNumComponents());
+	}
+
+	/*
 	 * Test structure definition when the same structure is defined in two different CUs.
-	 * @throws CancelledException
-	 * @throws IOException
-	 * @throws DWARFException
 	 */
 	@Test
 	public void testStructDup() throws CancelledException, IOException, DWARFException {
@@ -648,6 +721,49 @@ public class DWARFDataTypeImporterTest extends DWARFTestBase {
 
 	}
 
+	/*
+	 * Test flex array where dims were specified with no value, relying on
+	 * default.
+	 */
+	@Test
+	public void testStructFlexarray_noValue()
+			throws CancelledException, IOException, DWARFException {
+
+		DebugInfoEntry intDIE = addInt(cu);
+		DebugInfoEntry arrayDIE = newArray(cu, intDIE, true, -1);
+
+		DebugInfoEntry structDIE = newStruct("mystruct", 100).create(cu);
+		newMember(structDIE, "f1", intDIE, 0).create(cu);
+		newMember(structDIE, "flexarray", arrayDIE, 100).create(cu);
+
+		importAllDataTypes();
+
+		Structure structdt = (Structure) dataMgr.getDataType(rootCP, "mystruct");
+		assertNotNull(structdt.getFlexibleArrayComponent());
+
+	}
+
+	/*
+	 * Test flex array where dims were specified using a count=0 value instead of a
+	 * upperbound=-1.
+	 */
+	@Test
+	public void testStructFlexarray_0count()
+			throws CancelledException, IOException, DWARFException {
+
+		DebugInfoEntry intDIE = addInt(cu);
+		DebugInfoEntry arrayDIE = newArrayUsingCount(cu, intDIE, 0);
+
+		DebugInfoEntry structDIE = newStruct("mystruct", 100).create(cu);
+		newMember(structDIE, "f1", intDIE, 0).create(cu);
+		newMember(structDIE, "flexarray", arrayDIE, 100).create(cu);
+
+		importAllDataTypes();
+
+		Structure structdt = (Structure) dataMgr.getDataType(rootCP, "mystruct");
+		assertNotNull(structdt.getFlexibleArrayComponent());
+	}
+
 	@Test
 	public void testStructInteriorFlexarray()
 			throws CancelledException, IOException, DWARFException {
@@ -691,7 +807,7 @@ public class DWARFDataTypeImporterTest extends DWARFTestBase {
 
 		Structure structdt = (Structure) dataMgr.getDataType(rootCP, "mystruct");
 		List<DataTypeComponent> bitfields = getBitFieldComponents(structdt);
-		Set<Integer> expectedBitfieldSizes = new HashSet<Integer>(Set.of(2, 3, 9));
+		Set<Integer> expectedBitfieldSizes = new HashSet<>(Set.of(2, 3, 9));
 		for (DataTypeComponent dtc : bitfields) {
 			BitFieldDataType bfdt = (BitFieldDataType) dtc.getDataType();
 			expectedBitfieldSizes.remove(bfdt.getBitSize());
@@ -824,6 +940,9 @@ public class DWARFDataTypeImporterTest extends DWARFTestBase {
 		// anon struct
 	}
 
+	/*
+	 * Test array defintions that use upper_bound attribute
+	 */
 	@Test
 	public void testArray() throws CancelledException, IOException, DWARFException {
 		DebugInfoEntry intDIE = addInt(cu);
@@ -833,8 +952,24 @@ public class DWARFDataTypeImporterTest extends DWARFTestBase {
 
 		Array arr = (Array) dwarfDTM.getDataType(arrayDIE.getOffset(), null);
 		assertNotNull(arr);
+		assertEquals(11, arr.getNumElements());
 	}
 
+	/*
+	 * Tests array definitions that use count attribute instead of upper_bounds
+	 */
+	@Test
+	public void testArrayWithCountAttr() throws CancelledException, IOException, DWARFException {
+		DebugInfoEntry intDIE = addInt(cu);
+		DebugInfoEntry arrayDIE = newArrayUsingCount(cu, intDIE, 10);
+
+		importAllDataTypes();
+
+		Array arr = (Array) dwarfDTM.getDataType(arrayDIE.getOffset(), null);
+		assertNotNull(arr);
+		assertEquals(10, arr.getNumElements());
+	}
+	
 	// not implemented yet
 	public void testSubr() {
 		// func ptrs

Ghidra/Features/Base/src/test/java/ghidra/app/util/bin/format/dwarf4/DWARFTestBase.java

@@ -16,7 +16,7 @@
 package ghidra.app.util.bin.format.dwarf4;
 
 import static ghidra.app.util.bin.format.dwarf4.encoding.DWARFAttribute.*;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
 
 import java.io.IOException;
 
@@ -264,9 +264,16 @@ public class DWARFTestBase extends AbstractGhidraHeadedIntegrationTest {
 			DebugInfoEntry dataType, int offset) {
 		assertTrue(
 			dataType == null || dataType.getCompilationUnit() == parentStruct.getCompilationUnit());
+		return newMember(parentStruct, fieldName, dataType.getOffset(), offset);
+	}
+
+	protected DIECreator newMember(DebugInfoEntry parentStruct, String fieldName,
+			long memberDIEOffset, int offset) {
 		DIECreator field =
-			new DIECreator(DWARFTag.DW_TAG_member).addString(DW_AT_name, fieldName).addRef(
-				DW_AT_type, dataType).setParent(parentStruct);
+			new DIECreator(DWARFTag.DW_TAG_member).addString(DW_AT_name, fieldName)
+					.addRef(
+						DW_AT_type, memberDIEOffset)
+					.setParent(parentStruct);
 		if (offset != -1) {
 			field.addInt(DW_AT_data_member_location, offset);
 		}
@@ -285,11 +292,11 @@ public class DWARFTestBase extends AbstractGhidraHeadedIntegrationTest {
 
 	protected DebugInfoEntry newArray(MockDWARFCompilationUnit dcu, DebugInfoEntry baseTypeDIE,
 			boolean elideEmptyDimRangeValue, int... dimensions) {
-		DebugInfoEntry arrayType = new DIECreator(DWARFTag.DW_TAG_array_type) //
+		DebugInfoEntry arrayType = new DIECreator(DWARFTag.DW_TAG_array_type)
 			.addRef(DW_AT_type, baseTypeDIE).create(dcu);
 		for (int dimIndex = 0; dimIndex < dimensions.length; dimIndex++) {
 			int dim = dimensions[dimIndex];
-			DIECreator dimDIE = new DIECreator(DWARFTag.DW_TAG_subrange_type) //
+			DIECreator dimDIE = new DIECreator(DWARFTag.DW_TAG_subrange_type)
 				.setParent(arrayType);
 			if (dim != -1 || !elideEmptyDimRangeValue) {
 				dimDIE.addInt(DW_AT_upper_bound, dimensions[dimIndex]);
@@ -299,6 +306,18 @@ public class DWARFTestBase extends AbstractGhidraHeadedIntegrationTest {
 		return arrayType;
 	}
 
+	protected DebugInfoEntry newArrayUsingCount(MockDWARFCompilationUnit dcu,
+			DebugInfoEntry baseTypeDIE, int count) {
+		DebugInfoEntry arrayType = new DIECreator(DWARFTag.DW_TAG_array_type)
+				.addRef(DW_AT_type, baseTypeDIE)
+				.create(dcu);
+		DIECreator dimDIE = new DIECreator(DWARFTag.DW_TAG_subrange_type)
+				.setParent(arrayType);
+		dimDIE.addInt(DW_AT_count, count);
+		dimDIE.create(dcu);
+		return arrayType;
+	}
+
 	protected Address addr(long l) {
 		return space.getAddress(l);
 	}