diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CategoryDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CategoryDB.java index 5f96e2b3bc..564b1e3814 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CategoryDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CategoryDB.java @@ -636,20 +636,20 @@ class CategoryDB extends DatabaseObject implements Category { list.add(baseType); } - List relatedNameDataTypes = conflictMap.getDataTypesForBaseName(baseName); + List 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> { + private class ConflictMap extends LazyLoadingCachingMap> { ConflictMap(Lock lock) { super(lock); @@ -657,8 +657,10 @@ class CategoryDB extends DatabaseObject implements Category { /** * 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 @@ class CategoryDB extends DatabaseObject implements Category { if (isConflictName(dataTypeName)) { String baseName = getBaseName(dataTypeName); Map 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,10 +681,12 @@ class CategoryDB extends DatabaseObject implements Category { /** * 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> map = getMap(); if (map == null) { return; @@ -693,17 +694,14 @@ class CategoryDB extends DatabaseObject implements Category { String dataTypeName = dataType.getName(); String baseName = getBaseName(dataTypeName); - Map innerMap = map.get(baseName); - if (innerMap == null) { - innerMap = new HashMap<>(); - put(baseName, innerMap); - } + Map 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 @@ class CategoryDB extends DatabaseObject implements Category { * @return a list of all conflict named data types that would have the given base name if * no conflicts existed */ - List getDataTypesForBaseName(String baseName) { + List 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 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()); } diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeManagerDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeManagerDB.java index e862a76f4d..bccbcb8563 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeManagerDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeManagerDB.java @@ -3206,7 +3206,6 @@ abstract public class DataTypeManagerDB implements DataTypeManager { lock.acquire(); try { long[] ids = parentChildAdapter.getParentIds(childID); - // TODO: consider deduping ids using Set List dts = new ArrayList<>(); for (long id : ids) { DataType dt = getDataType(id); diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/LazyLoadingCachingMap.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/LazyLoadingCachingMap.java index 75758544a4..e491b57e7e 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/LazyLoadingCachingMap.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/LazyLoadingCachingMap.java @@ -44,7 +44,8 @@ public abstract class LazyLoadingCachingMap { } /** - * 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 loadMap(); @@ -113,7 +114,8 @@ public abstract class LazyLoadingCachingMap { // 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. diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Category.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Category.java index 29cb25610c..9e3684b225 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Category.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Category.java @@ -52,11 +52,13 @@ public interface Category extends Comparable { /** * 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 getDataTypesByBaseName(String name);