GT-3535 changes from review

Author: ghidravore

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

@@ -636,29 +636,31 @@ public List<DataType> getDataTypesByBaseName(String dataTypeName) {
 			list.add(baseType);
 		}
 
-		List<DataType> relatedNameDataTypes = conflictMap.getDataTypesForBaseName(baseName);
+		List<DataType> relatedNameDataTypes = conflictMap.getDataTypesByBaseName(baseName);
 		list.addAll(relatedNameDataTypes);
 		return list;
 	}
 
 	/**
-	 * Class to handle complexities of having a map as the value in a LazyLoadingCachingMap
-	 * This map uses data type's base name as the key (i.e. all .conflict suffixex stripped off.)
-	 * The value is another map that maps the actual data type's name to the datatype. This map
+	 * Class to handle the complexities of having a map as the value in a LazyLoadingCachingMap
+	 * This map uses the data type's base name as the key (i.e. all .conflict suffixes stripped off.)
+	 * The value is another map that maps the actual data type's name to the data type. This map
 	 * effectively provides an efficient way to get all data types in a category that have the
 	 * same name, but possibly have had their name modified (by appending .conflict) to get around
 	 * the requirement that names have to be unique in the same category.
 	 */
-	class ConflictMap extends LazyLoadingCachingMap<String, Map<String, DataType>> {
+	private class ConflictMap extends LazyLoadingCachingMap<String, Map<String, DataType>> {
 
 		ConflictMap(Lock lock) {
 			super(lock);
 		}
 
 		/**
 		 * Creates a map of all data types whose name has a .conflict suffix where the key
-		 * is the base name and the value is a map of actual name to data type. This mapping is
-		 * maintained as a lazy cache map.
+		 * is the base name and {@link LazyLoadingCachingMap} the value is a map of actual name to data type. This mapping is
+		 * maintained as a lazy cache map. This is only called by the super class when the
+		 * cached needs to be populated and we are depending on it to acquire the necessary
+		 * database lock. (See {@link LazyLoadingCachingMap#loadMap()}
 		 * @return the loaded map
 		 */
 		@Override
@@ -670,10 +672,7 @@ protected Map<String, Map<String, DataType>> loadMap() {
 				if (isConflictName(dataTypeName)) {
 					String baseName = getBaseName(dataTypeName);
 					Map<String, DataType> innerMap = map.get(baseName);
-					if (innerMap == null) {
-						innerMap = new HashMap<>();
-						map.put(baseName, innerMap);
-					}
+					map.computeIfAbsent(baseName, b -> new HashMap<>());
 					innerMap.put(dataTypeName, dataType);
 				}
 			}
@@ -682,28 +681,27 @@ protected Map<String, Map<String, DataType>> loadMap() {
 
 		/**
 		 * Adds the data type to the conflict mapping structure. If the mapping is currently not
-		 * loaded then this method can safely do nothing.
+		 * loaded then this method can safely do nothing. This method is synchronized to provide
+		 * thread safe access/manipulation of the map.
 		 * @param dataType the data type to add to the mapping if the mapping is already loaded
 		 */
 		synchronized void addDataType(DataType dataType) {
+			// if the cache is not currently populated, don't need to do anything
 			Map<String, Map<String, DataType>> map = getMap();
 			if (map == null) {
 				return;
 			}
 
 			String dataTypeName = dataType.getName();
 			String baseName = getBaseName(dataTypeName);
-			Map<String, DataType> innerMap = map.get(baseName);
-			if (innerMap == null) {
-				innerMap = new HashMap<>();
-				put(baseName, innerMap);
-			}
+			Map<String, DataType> innerMap = map.computeIfAbsent(baseName, b -> new HashMap<>());
 			innerMap.put(dataTypeName, dataType);
 		}
 
 		/**
 		 * Removes the data type with the given name from the conflict mapping structure. If the 
-		 * mapping is currently not loaded then this method can safely do nothing.
+		 * mapping is currently not loaded then this method can safely do nothing. This method is
+		 * synchronized to provide thread safe access/manipulate of the map.
 		 * @param dataTypeName the name of the data type  to remove from this mapping
 		 */
 		synchronized void removeDataTypeName(String dataTypeName) {
@@ -725,11 +723,19 @@ synchronized void removeDataTypeName(String dataTypeName) {
 		 * @return a list of all conflict named data types that would have the given base name if
 		 * no conflicts existed
 		 */
-		List<DataType> getDataTypesForBaseName(String baseName) {
+		List<DataType> getDataTypesByBaseName(String baseName) {
+
+			// Note that the following call to get MUST NOT be in a synchronized block because
+			// it may trigger a loading of the cache which requires a database lock and you
+			// can't be synchronized on this class when acquiring a database lock or else a
+			// deadlock will occur.
 			Map<String, DataType> map = get(baseName);
 			if (map == null) {
 				return Collections.emptyList();
 			}
+
+			// the following must be synchronized so that the implied iterator can complete without
+			// another thread changing the map's values.
 			synchronized (this) {
 				return new ArrayList<>(map.values());
 			}

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

@@ -3206,7 +3206,6 @@ List<DataType> getParentDataTypes(long childID) {
 		lock.acquire();
 		try {
 			long[] ids = parentChildAdapter.getParentIds(childID);
-			// TODO: consider deduping ids using Set
 			List<DataType> dts = new ArrayList<>();
 			for (long id : ids) {
 				DataType dt = getDataType(id);

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

@@ -44,7 +44,8 @@ protected LazyLoadingCachingMap(Lock lock) {
 	}
 
 	/**
-	 * This method will reload the map data from scratch.
+	 * This method will reload the map data from scratch. Subclass may assume that the database
+	 * lock has been acquired.
 	 * @return a map containing all current key, value pairs.
 	 */
 	protected abstract Map<K, V> loadMap();
@@ -113,7 +114,8 @@ private Map<K, V> getOrLoadMap() {
 
 		// We must get the database lock before calling loadMap().  Also, we can't get the
 		// database lock while having the synchronization lock for this class or a deadlock can
-		// occur.  Note: all other places where the map is being used or manipulated, it must be done
+		// occur, since the other methods may be called while the client has the db lock.
+		// Note: all other places where the map is being used or manipulated, it must be done
 		// while having the class's synchronization lock since the map itself is not thread safe.
 		// It should be safe here since it creates a new map and then in one operation it sets it
 		// as the map to be used elsewhere.

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

@@ -52,11 +52,13 @@ public interface Category extends Comparable<Category> {
 
 	/**
 	 * Get all data types in this category whose base name matches the base name of the given name.
-	 * The base name of a name if the first part of the string up to where the first ".conflict"
-	 * occurs. In other words find all data types whose name matches the given name once
-	 * any conflict suffixes have been removed from both both the given name and the data types
+	 * The base name of a name is the first part of the string up to where the first ".conflict"
+	 * occurs. In other words, finds all data types whose name matches the given name once
+	 * any conflict suffixes have been removed from both the given name and the data types
 	 * that are being scanned. 
-	 * @param name the name for which to get conflict related data types in this category
+	 * @param name the name for which to get conflict related data types in this category. Note: the
+	 * name that is passed in will be normalized to its base name, so you may pass in names with .conflict
+	 * appended as a convenience.
 	 * @return a list of data types that have the same base name as the base name of the given name
 	 */
 	public abstract List<DataType> getDataTypesByBaseName(String name);