Merge remote-tracking branch 'origin/GP-5274_ghizard_DefaultCompositeMember_improve_align_add_nopack_option--SQUASHED'

Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/ApplyDataTypes.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.
@@ -121,9 +121,9 @@ public class ApplyDataTypes {
 			Composite composite = (Composite) cachedDataType;
 			PdbUtil.clearComponents(composite);
 
-			if (!DefaultCompositeMember.applyDataTypeMembers(composite, compositeDefinition.isClass,
-				compositeDefinition.length, getNormalMembersOnly(compositeDefinition),
-				msg -> Msg.warn(this, msg), monitor)) {
+			if (!DefaultCompositeMember.applyDataTypeMembers(composite, false,
+				compositeDefinition.isClass, compositeDefinition.length,
+				getNormalMembersOnly(compositeDefinition), msg -> Msg.warn(this, msg), monitor)) {
 				PdbUtil.clearComponents(composite);
 			}
 

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

@@ -22,9 +22,9 @@ import ghidra.program.model.data.BitFieldDataType;
 import ghidra.program.model.data.DataType;
 
 /**
- * <code>BitFieldGroupCompositeMember</code> provides the ability to collect related 
+ * <code>BitFieldGroupCompositeMember</code> provides the ability to collect related
  * {@link DefaultCompositeMember} members within a group during the composite reconstruction
- * process.  
+ * process.
  */
 public class BitFieldGroupCompositeMember extends CompositeMember {
 
@@ -128,7 +128,7 @@ public class BitFieldGroupCompositeMember extends CompositeMember {
 	}
 
 	@Override
-	void finalizeDataType(int preferredSize) {
+	void finalizeDataType(int preferredSize, boolean packingDisabled) {
 		return; // nothing to do
 	}
 
@@ -149,8 +149,8 @@ public class BitFieldGroupCompositeMember extends CompositeMember {
 
 	/**
 	 * Add a new member to the end of this bit-field group.  The caller should ensure that the
-	 * specified member is a suitable addition to this group (must be single bit field whose 
-	 * member offset and length match this group's). 
+	 * specified member is a suitable addition to this group (must be single bit field whose
+	 * member offset and length match this group's).
 	 * @param member bit-field member (must have data type of BitFieldDataType).
 	 * @throws IllegalArgumentException if specified member is not suitable for this group.
 	 */

Ghidra/Features/PDB/src/main/java/ghidra/app/util/bin/format/pdb/CompositeMember.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.
@@ -25,12 +25,14 @@ abstract class CompositeMember {
 
 	/**
 	 * Due to the dynamic restructuring of data type containers, this method should be invoked
-	 * on the root container prior to adding or applying the associated data type to the program.  
-	 * This method will appropriately rename and categorize associated anonymous structures and 
+	 * on the root container prior to adding or applying the associated data type to the program.
+	 * This method will appropriately rename and categorize associated anonymous structures and
 	 * unions to reflect the final organization and check if internal alignment should be enabled.
 	 * @param preferredSize preferred size of composite if known, else <= 0 if unknown
+	 * @param packingDisabled {@code true} to disable any attempted use of packing on this
+	 * composite or any of its generated composite dependencies
 	 */
-	abstract void finalizeDataType(int preferredSize);
+	abstract void finalizeDataType(int preferredSize, boolean packingDisabled);
 
 	/**
 	 * Determine if this member is a container
@@ -58,7 +60,7 @@ abstract class CompositeMember {
 
 	/**
 	 * Determine if this member is a bit-field member not yet contained within a group.
-	 * If true is returned this instance is ensured to be a {@link DefaultCompositeMember} instance 
+	 * If true is returned this instance is ensured to be a {@link DefaultCompositeMember} instance
 	 * whose data type is {@link BitFieldDataType}.
 	 * @return true if bit-field member not yet contained within a group
 	 */
@@ -96,16 +98,17 @@ abstract class CompositeMember {
 	abstract void setParent(DefaultCompositeMember parent);
 
 	/**
-	 * Add specified member to this member.  If this member is not a composite 
-	 * it will trigger the creation 
-	 * @param member
-	 * @return
+	 * Add specified member to this member.  If this member is not a composite
+	 * it will trigger the creation
+	 * @param member the member to be added
+	 * @return {@code true} if the specified member was successfully added to this member
 	 */
 	abstract boolean addMember(DefaultCompositeMember member);
 
 	/**
 	 * Instructs this member to add itself to the specified structure
 	 * @param structure composite structure
+	 * @return {@code true} if this member was successfully added to the specified structure
 	 */
 	abstract boolean addToStructure(DefaultCompositeMember structure);
 }

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

@@ -64,7 +64,7 @@ public class DefaultCompositeMember extends CompositeMember {
 	private TreeMap<Integer, CompositeMember> structureMemberOffsetMap;
 	private RangeMap structureMemberRangeMap;
 	private int largestPrimitiveSize;
-	private boolean hasPadding = false;
+	private boolean hasStructurePadding = false;
 
 	// Union container data
 	private List<CompositeMember> unionMemberList;
@@ -250,7 +250,7 @@ public class DefaultCompositeMember extends CompositeMember {
 	}
 
 	@Override
-	void finalizeDataType(int preferredSize) {
+	void finalizeDataType(int preferredSize, boolean packingDisabled) {
 		if (!isContainer()) {
 			return;
 		}
@@ -258,7 +258,7 @@ public class DefaultCompositeMember extends CompositeMember {
 			updateContainerNameAndCategoryPath("s");
 			CompositeMember lastMember = null;
 			for (CompositeMember member : structureMemberOffsetMap.values()) {
-				member.finalizeDataType(0);
+				member.finalizeDataType(0, packingDisabled);
 				lastMember = member;
 			}
 			transformLastMemberIntoFlexArray(lastMember);
@@ -269,10 +269,10 @@ public class DefaultCompositeMember extends CompositeMember {
 		else if (isUnionContainer()) {
 			updateContainerNameAndCategoryPath("u");
 			for (CompositeMember member : unionMemberList) {
-				member.finalizeDataType(0);
+				member.finalizeDataType(0, packingDisabled);
 			}
 		}
-		alignComposite(preferredSize);
+		alignComposite(preferredSize, packingDisabled);
 	}
 
 	/**
@@ -318,8 +318,10 @@ public class DefaultCompositeMember extends CompositeMember {
 	/**
 	 * Align container composite data type if possible.
 	 * @param preferredSize preferred size of composite if known, else <= 0 if unknown
+	 * @param packingDisabled {@code true} to disable any attempted use of packing on this
+	 * composite or any of its generated composite dependencies
 	 */
-	private void alignComposite(int preferredSize) {
+	private void alignComposite(int preferredSize, boolean packingDisabled) {
 
 		Composite composite = (Composite) memberDataType;
 
@@ -333,6 +335,20 @@ public class DefaultCompositeMember extends CompositeMember {
 			return;
 		}
 
+		// Try to get unions to get to the preferred size by adding a padding member
+		if (composite instanceof Union && preferredSize > composite.getAlignedLength()) {
+			ArrayDataType padding = new ArrayDataType(CharDataType.dataType, preferredSize);
+			composite.add(padding, PADDING_COMPONENT_NAME, "");
+		}
+
+		if (packingDisabled) {
+			if (composite instanceof Structure) {
+				removeAllPadding(composite); // includes bit-field padding
+			}
+			setComputedAlignment(composite);
+			return;
+		}
+
 		Composite copy = (Composite) composite.copy(dataTypeManager);
 
 		int pack = 0;
@@ -341,18 +357,18 @@ public class DefaultCompositeMember extends CompositeMember {
 		boolean alignOK = isGoodAlignment(copy, preferredSize);
 		if (alignOK) {
 			composite.setToDefaultPacking();
-			if (hasPadding) {
+			if (hasStructurePadding) {
 				removeUnnecessaryPadding(composite);
 			}
 		}
-		else {
+		else if (composite instanceof Structure) {
 			if (preferredSize > 0 && copy.getLength() != preferredSize) {
 				copy.setToMachineAligned(); // will only impact structure length
 				alignOK = isGoodAlignment(copy, preferredSize);
 				if (alignOK) {
 					composite.setToDefaultPacking();
 					composite.setToMachineAligned();
-					if (hasPadding) {
+					if (hasStructurePadding) {
 						removeUnnecessaryPadding(composite);
 					}
 				}
@@ -362,7 +378,7 @@ public class DefaultCompositeMember extends CompositeMember {
 			}
 			if (!alignOK) {
 				removeAllPadding(composite); // includes bit-field padding
-				if (!hasPadding) {
+				if (!hasStructurePadding) {
 					pack = 1;
 					copy.setExplicitPackingValue(pack);
 					alignOK = isGoodAlignment(copy, preferredSize);
@@ -372,13 +388,46 @@ public class DefaultCompositeMember extends CompositeMember {
 				}
 			}
 		}
-		if (!alignOK && errorConsumer != null && !isClass) { // don't complain about Class structs which always fail
+		// Unions fall through to here
+		if (!alignOK) {
+			setComputedAlignment(composite);
+		}
+		// Don't complain about Class structs which always fail... this might always be true
+		//  for the MSDIA analyzer, but we hope classes will align better with PDB Universal in
+		//  the future
+		if (!alignOK && errorConsumer != null && !isClass) {
 			String anonymousStr = parent != null ? " anonymous " : "";
 			errorConsumer.accept("PDB " + anonymousStr + memberType +
 				" reconstruction failed to align " + composite.getPathName());
 		}
 	}
 
+	/**
+	 * Computes and sets the alignment of the composite.  Will not set an alignment if the
+	 * calculation does not make sense
+	 * @param composite the composite to affect
+	 */
+	private void setComputedAlignment(Composite composite) {
+		// Only need to compute alignment from immediate child components, as those components
+		//  would have already seen this method in their calculations.
+		// If any component of the composite is not at an offset that is a multiple of that
+		//  comonent's alignment, then abort setting the composite alignment
+		int compositeAlignment = 1;
+		for (DataTypeComponent dtc : composite.getDefinedComponents()) {
+			DataType dt = dtc.getDataType();
+			int offset = dtc.getOffset();
+			int alignment = dt.getAlignment();
+			if (offset % alignment != 0) {
+				return;
+			}
+			compositeAlignment = Integer.max(compositeAlignment, alignment);
+		}
+		if (composite.getLength() % compositeAlignment != 0) {
+			return;
+		}
+		composite.align(compositeAlignment);
+	}
+
 	private void removeUnnecessaryPadding(Composite packedComposite) {
 		if (!packedComposite.isPackingEnabled()) {
 			throw new IllegalArgumentException("composite must have packing enabled");
@@ -507,7 +556,7 @@ public class DefaultCompositeMember extends CompositeMember {
 			structureMemberRangeMap = new RangeMap(-1);
 			// allow padding size to use pointer-size and smaller by default
 			largestPrimitiveSize = memberDataType.getDataOrganization().getPointerSize();
-			hasPadding = false;
+			hasStructurePadding = false;
 			unionMemberList = null;
 		}
 		else {
@@ -750,8 +799,8 @@ public class DefaultCompositeMember extends CompositeMember {
 	/**
 	 * Insert minimal padding into structure prior to the addition of a component such that packing
 	 * will allow component to be placed at intended offset.
-	 * @param nextComponentOffset 
-	 * @param dt 
+	 * @param nextComponentOffset
+	 * @param dt
 	 */
 	private void insertMinimalStructurePadding(int nextComponentOffset, DataType dt) {
 
@@ -786,7 +835,7 @@ public class DefaultCompositeMember extends CompositeMember {
 			int paddingOffset =
 				DataOrganizationImpl.getAlignedOffset(paddingDt.getAlignment(), structLen);
 			struct.insertAtOffset(paddingOffset, paddingDt, -1, PADDING_COMPONENT_NAME, null);
-			hasPadding = true;
+			hasStructurePadding = true;
 
 			structLen = struct.getLength();
 			fillSpace = nextComponentOffset - structLen;
@@ -1296,6 +1345,8 @@ public class DefaultCompositeMember extends CompositeMember {
 	 * Buildup an empty composite by applying datatype composite members.
 	 * Only those children with a kind of "Member" will be processed.
 	 * @param composite empty composite to which members will be added
+	 * @param packingDisabled {@code true} to disable any attempted use of packing on this
+	 * composite or any of its generated composite dependencies
 	 * @param isClass true if composite corresponds to a Class structure, else false
 	 * @param preferredCompositeSize preferred size of composite, <= 0 indicates unknown
 	 * @param members list of composite members
@@ -1304,9 +1355,10 @@ public class DefaultCompositeMember extends CompositeMember {
 	 * @return true if members successfully added to composite
 	 * @throws CancelledException if monitor is cancelled
 	 */
-	public static boolean applyDataTypeMembers(Composite composite, boolean isClass,
-			int preferredCompositeSize, List<? extends PdbMember> members,
-			Consumer<String> errorConsumer, TaskMonitor monitor) throws CancelledException {
+	public static boolean applyDataTypeMembers(Composite composite, boolean packingDisabled,
+			boolean isClass, int preferredCompositeSize,
+			List<? extends PdbMember> members, Consumer<String> errorConsumer, TaskMonitor monitor)
+			throws CancelledException {
 
 		Composite editComposite = composite;
 
@@ -1332,7 +1384,7 @@ public class DefaultCompositeMember extends CompositeMember {
 			}
 		}
 
-		rootMember.finalizeDataType(preferredCompositeSize);
+		rootMember.finalizeDataType(preferredCompositeSize, packingDisabled);
 		return true;
 	}
 

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

@@ -163,7 +163,7 @@ public class CompositeTypeApplier extends AbstractComplexTypeApplier {
 		addVftPtrs(composite, classType, lists.vftPtrs(), type, myMembers);
 		addMembers(composite, classType, lists.nonstaticMembers(), type, myMembers);
 
-		if (!DefaultCompositeMember.applyDataTypeMembers(composite, isClass, size, myMembers,
+		if (!DefaultCompositeMember.applyDataTypeMembers(composite, false, isClass, size, myMembers,
 			msg -> reconstructionWarn(msg, hasHiddenComponents(lists)),
 			applicator.getCancelOnlyWrappingMonitor())) {
 			clearComponents(composite);

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

@@ -686,7 +686,7 @@ public class CppCompositeType {
 				addLayoutPdbMembers(directClassPdbMembers, layoutMembers);
 				insertVirtualFunctionTablePointers(directClassPdbMembers);
 
-				if (!DefaultCompositeMember.applyDataTypeMembers(directDataType, false, 0,
+				if (!DefaultCompositeMember.applyDataTypeMembers(directDataType, false, false, 0,
 					directClassPdbMembers, msg -> Msg.warn(this, msg), monitor)) {
 					clearComponents(directDataType);
 				}
@@ -706,7 +706,8 @@ public class CppCompositeType {
 						directDataType = new StructureDataType(cn.getParent(), cn.getName(), 0,
 							composite.getDataTypeManager());
 						if (!DefaultCompositeMember.applyDataTypeMembers(directDataType, false,
-							size, directClassPdbMembers, msg -> Msg.warn(this, msg), monitor)) {
+							false, size, directClassPdbMembers, msg -> Msg.warn(this, msg),
+							monitor)) {
 							clearComponents(directDataType);
 						}
 						directClassLength = getCompositeLength(directDataType);
@@ -735,7 +736,7 @@ public class CppCompositeType {
 				throw new PdbException("Unhandled layout mode");
 		}
 
-		if (!DefaultCompositeMember.applyDataTypeMembers(composite, false, size, memberData,
+		if (!DefaultCompositeMember.applyDataTypeMembers(composite, false, false, size, memberData,
 			msg -> Msg.warn(this, msg), monitor)) {
 			clearComponents(composite);
 		}
@@ -826,7 +827,7 @@ public class CppCompositeType {
 				addLayoutPdbMembers(directClassPdbMembers, layoutMembers);
 				insertVirtualFunctionTablePointers(directClassPdbMembers);
 
-				if (!DefaultCompositeMember.applyDataTypeMembers(directDataType, false, 0,
+				if (!DefaultCompositeMember.applyDataTypeMembers(directDataType, false, false, 0,
 					directClassPdbMembers, msg -> Msg.warn(this, msg), monitor)) {
 					clearComponents(directDataType);
 				}
@@ -846,7 +847,8 @@ public class CppCompositeType {
 						directDataType = new StructureDataType(cn.getParent(), cn.getName(), 0,
 							composite.getDataTypeManager());
 						if (!DefaultCompositeMember.applyDataTypeMembers(directDataType, false,
-							size, directClassPdbMembers, msg -> Msg.warn(this, msg), monitor)) {
+							false, size, directClassPdbMembers, msg -> Msg.warn(this, msg),
+							monitor)) {
 							clearComponents(directDataType);
 						}
 						directClassLength = getCompositeLength(directDataType);
@@ -875,7 +877,7 @@ public class CppCompositeType {
 				throw new PdbException("Unhandled layout mode");
 		}
 
-		if (!DefaultCompositeMember.applyDataTypeMembers(composite, false, size, memberData,
+		if (!DefaultCompositeMember.applyDataTypeMembers(composite, false, false, size, memberData,
 			msg -> Msg.warn(this, msg), monitor)) {
 			clearComponents(composite);
 		}

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

@@ -545,7 +545,7 @@ public class PdbResearch {
 			members.add(member);
 			size += extra.getLength();
 		}
-		if (!DefaultCompositeMember.applyDataTypeMembers(composite, false, size, members,
+		if (!DefaultCompositeMember.applyDataTypeMembers(composite, false, false, size, members,
 			msg -> reconstructionWarn(msg), monitor)) {
 			((Structure) composite).deleteAll();
 		}

Ghidra/Features/PDB/src/main/java/ghidra/app/util/pdb/pdbapplicator/VtShapeTypeApplier.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.
@@ -127,7 +127,7 @@ public class VtShapeTypeApplier extends MsDataTypeApplier {
 			members.add(member);
 		}
 		// offset has the total size at this point
-		if (!DefaultCompositeMember.applyDataTypeMembers(shape, false, offset, members,
+		if (!DefaultCompositeMember.applyDataTypeMembers(shape, false, false, offset, members,
 			msg -> Msg.warn(this, msg), applicator.getCancelOnlyWrappingMonitor())) {
 			CompositeTypeApplier.clearComponents(shape);
 		}

Ghidra/Features/PDB/src/test/java/ghidra/app/util/pdb/pdbapplicator/ConflictHandler2Test.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.
@@ -33,8 +33,8 @@ import ghidra.util.task.TaskMonitor;
 
 /**
  * Tests for the {@link DataTypeConflictHandler conflict handler} stuff.
- *  
- * 
+ *
+ *
  */
 public class ConflictHandler2Test extends AbstractGhidraHeadedIntegrationTest {
 	private ProgramDB program;
@@ -136,7 +136,7 @@ public class ConflictHandler2Test extends AbstractGhidraHeadedIntegrationTest {
 			size += extra.getLength();
 		}
 		try {
-			if (!DefaultCompositeMember.applyDataTypeMembers(composite, false, size, members,
+			if (DefaultCompositeMember.applyDataTypeMembers(composite, false, false, size, members,
 				msg -> Msg.warn(ConflictHandler2Test.class, msg), monitor)) {
 				((Structure) composite).deleteAll();
 			}