diff --git a/Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/data/CategoryTest.java b/Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/data/CategoryTest.java index b846c40931..c8128b93a5 100644 --- a/Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/data/CategoryTest.java +++ b/Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/data/CategoryTest.java @@ -575,6 +575,37 @@ public class CategoryTest extends AbstractGhidraHeadedIntegrationTest { assertEquals(comps.length - 1, newDt.getDefinedComponents().length); } + @Test + public void testDataTypeConflictHandling() throws Exception { + Category sub1 = root.createCategory("Cat1"); + DataType dt1 = new StructureDataType("DT", 1); + DataType dt2 = new StructureDataType("DT", 2); + DataType added1 = sub1.addDataType(dt1, null); + DataType added2 = sub1.addDataType(dt2, null); + assertEquals("DT", added1.getName()); + assertEquals("DT.conflict", added2.getName()); + + List list = sub1.getDataTypesByBaseName("DT"); + assertEquals(2, list.size()); + assertEquals(added1, list.get(0)); + assertEquals(added2, list.get(1)); + + list = sub1.getDataTypesByBaseName("DT.conflict"); + assertEquals(2, list.size()); + assertEquals(added1, list.get(0)); + assertEquals(added2, list.get(1)); + + sub1.remove(added2, TaskMonitor.DUMMY); + list = sub1.getDataTypesByBaseName("DT"); + assertEquals(1, list.size()); + assertEquals(added1, list.get(0)); + + list = sub1.getDataTypesByBaseName("DT.conflict"); + assertEquals(1, list.size()); + assertEquals(added1, list.get(0)); + + } + @Test public void testGetDataTypeManager() throws Exception { Category sub1 = root.createCategory("SubCat-A"); @@ -772,10 +803,7 @@ public class CategoryTest extends AbstractGhidraHeadedIntegrationTest { clearEvents(); struct2 = (Structure) newDt.insert(3, struct2).getDataType(); - int eventCount = getEventCount(); - if (4 != eventCount) { - System.err.println("halt!"); - } + assertEquals(5, getEventCount()); Event ev = getEvent(4); assertEquals("DT Changed", ev.evName); 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 348b6be6c2..7d277d2a7f 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 @@ -16,8 +16,7 @@ package ghidra.program.database.data; import java.io.IOException; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.concurrent.ConcurrentHashMap; import db.Record; @@ -26,6 +25,7 @@ import ghidra.program.database.DatabaseObject; import ghidra.program.model.data.*; import ghidra.program.model.data.DataTypeConflictHandler.ConflictResult; import ghidra.util.InvalidNameException; +import ghidra.util.Lock; import ghidra.util.exception.AssertException; import ghidra.util.exception.DuplicateNameException; import ghidra.util.task.TaskMonitor; @@ -41,6 +41,7 @@ class CategoryDB extends DatabaseObject implements Category { private LazyLoadingCachingMap subcategoryMap; private LazyLoadingCachingMap dataTypeMap; + private ConflictMap conflictMap; /** * Category Constructor @@ -57,19 +58,19 @@ class CategoryDB extends DatabaseObject implements Category { this.name = name; this.parent = parent; - subcategoryMap = new LazyLoadingCachingMap<>(mgr.lock, CategoryDB.class) { + subcategoryMap = new LazyLoadingCachingMap<>(mgr.lock) { @Override public Map loadMap() { return buildSubcategoryMap(); } }; - dataTypeMap = new LazyLoadingCachingMap<>(mgr.lock, DataType.class) { + dataTypeMap = new LazyLoadingCachingMap<>(mgr.lock) { @Override public Map loadMap() { return createDataTypeMap(); } }; - + conflictMap = new ConflictMap(mgr.lock); } /** @@ -102,6 +103,7 @@ class CategoryDB extends DatabaseObject implements Category { protected boolean refresh(Record rec) { subcategoryMap.clear(); dataTypeMap.clear(); + conflictMap.clear(); if (isRoot()) { return true; @@ -210,13 +212,26 @@ class CategoryDB extends DatabaseObject implements Category { return map; } + private String getBaseName(String dataTypeName) { + int indexOf = dataTypeName.indexOf(DataType.CONFLICT_SUFFIX); + if (indexOf <= 0) { + return dataTypeName; + } + return dataTypeName.substring(0, indexOf); + } + + private boolean isConflictName(String dataTypeName) { + return dataTypeName.contains(DataType.CONFLICT_SUFFIX); + } + /** * @see ghidra.program.model.data.Category#getCategories() */ @Override public Category[] getCategories() { validate(mgr.lock); - return subcategoryMap.valuesToArray(); + Collection categories = subcategoryMap.values(); + return categories.toArray(new Category[categories.size()]); } /** @@ -225,7 +240,8 @@ class CategoryDB extends DatabaseObject implements Category { @Override public DataType[] getDataTypes() { validate(mgr.lock); - return dataTypeMap.valuesToArray(); + Collection dataTypes = dataTypeMap.values(); + return dataTypes.toArray(new DataType[dataTypes.size()]); } /** @@ -587,19 +603,144 @@ class CategoryDB extends DatabaseObject implements Category { } void dataTypeRenamed(DataType childDataType, String oldName) { - dataTypeMap.remove(oldName); - dataTypeMap.put(childDataType.getName(), childDataType); + dataTypeRemoved(oldName); + dataTypeAdded(childDataType); } - void dataTypeAdded(DataType childDataType) { - dataTypeMap.put(childDataType.getName(), childDataType); + void dataTypeAdded(DataType dataType) { + String dtName = dataType.getName(); + dataTypeMap.put(dtName, dataType); + if (isConflictName(dtName)) { + conflictMap.addDataType(dataType); + } } void dataTypeRemoved(String dataTypeName) { dataTypeMap.remove(dataTypeName); + if (isConflictName(dataTypeName)) { + conflictMap.removeDataTypeName(dataTypeName); + } } void categoryAdded(CategoryDB cat) { subcategoryMap.put(cat.getName(), cat); } + + @Override + public List getDataTypesByBaseName(String dataTypeName) { + List list = new ArrayList<>(); + String baseName = getBaseName(dataTypeName); + + DataType baseType = dataTypeMap.get(baseName); + if (baseType != null) { + list.add(baseType); + } + + List relatedNameDataTypes = conflictMap.getDataTypesByBaseName(baseName); + list.addAll(relatedNameDataTypes); + return list; + } + + /** + * 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. + */ + private class ConflictMap extends LazyLoadingCachingMap> { + + 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 {@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 + protected Map> loadMap() { + Map> map = new HashMap<>(); + Collection values = dataTypeMap.values(); + for (DataType dataType : values) { + String dataTypeName = dataType.getName(); + if (isConflictName(dataTypeName)) { + String baseName = getBaseName(dataTypeName); + Map innerMap = + map.computeIfAbsent(baseName, b -> new HashMap<>()); + innerMap.put(dataTypeName, dataType); + } + } + return map; + } + + /** + * Adds the data type to the conflict mapping structure. If the mapping is currently not + * 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; + } + + String dataTypeName = dataType.getName(); + String baseName = getBaseName(dataTypeName); + 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. 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) { + Map> map = getMap(); + if (map == null) { + return; + } + String baseName = getBaseName(dataTypeName); + Map innerMap = map.get(baseName); + if (innerMap == null) { + return; + } + innerMap.remove(dataTypeName); + } + + /** + * Returns a list of all data types that have conflict names for the given base name + * @param baseName the data type base name to search for (i.e. the .conflict suffix removed) + * @return a list of all conflict named data types that would have the given base name if + * no conflicts existed + */ + 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 24d17bf75f..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 @@ -934,14 +934,12 @@ abstract public class DataTypeManagerDB implements DataTypeManager { if (category == null) { return null; } - String namePrefix = dtName + DataType.CONFLICT_SUFFIX; - DataType[] dataTypes = category.getDataTypes(); - for (DataType candidate : dataTypes) { + List relatedByName = category.getDataTypesByBaseName(dtName); + + for (DataType candidate : relatedByName) { String candidateName = candidate.getName(); - if (candidateName.startsWith(namePrefix)) { - if (!candidateName.equals(excludedName) && candidate.isEquivalent(dataType)) { - return candidate; - } + if (!candidateName.equals(excludedName) && candidate.isEquivalent(dataType)) { + return candidate; } } return null; @@ -3208,13 +3206,12 @@ 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 (int i = 0; i < ids.length; i++) { - DataType dt = getDataType(ids[i]); + for (long id : ids) { + DataType dt = getDataType(id); if (dt == null) { // cleanup invalid records for missing parent - attemptRecordRemovalForParent(ids[i]); + attemptRecordRemovalForParent(id); } else { dts.add(dt); 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 5f3ef6cb9c..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 @@ -16,8 +16,7 @@ package ghidra.program.database.data; import java.lang.ref.SoftReference; -import java.lang.reflect.Array; -import java.util.Map; +import java.util.*; import ghidra.util.Lock; @@ -39,15 +38,14 @@ public abstract class LazyLoadingCachingMap { private Lock lock; private SoftReference> softRef; - private Class valueClass; - protected LazyLoadingCachingMap(Lock lock, Class valueClass) { + protected LazyLoadingCachingMap(Lock lock) { this.lock = lock; - this.valueClass = valueClass; } /** - * 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(); @@ -96,13 +94,13 @@ public abstract class LazyLoadingCachingMap { } } - public V[] valuesToArray() { + /** + * Returns an unmodifiable view of the values in this map. + * @return an unmodifiable view of the values in this map. + */ + public Collection values() { Map map = getOrLoadMap(); - synchronized (this) { - @SuppressWarnings("unchecked") - V[] array = (V[]) Array.newInstance(valueClass, map.size()); - return map.values().toArray(array); - } + return Collections.unmodifiableCollection(map.values()); } private Map getOrLoadMap() { @@ -113,6 +111,15 @@ public abstract class LazyLoadingCachingMap { return map; } } + + // 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, 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. + lock.acquire(); try { map = getMap(); @@ -132,11 +139,12 @@ public abstract class LazyLoadingCachingMap { * "lock". * @return the underlying map of key,value pairs or null if it is currently not loaded. */ - private Map getMap() { + protected Map getMap() { if (softRef == null) { return null; } return softRef.get(); } + } 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 fd052237c8..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 @@ -15,6 +15,8 @@ */ package ghidra.program.model.data; +import java.util.List; + import ghidra.util.InvalidNameException; import ghidra.util.exception.DuplicateNameException; import ghidra.util.task.TaskMonitor; @@ -48,6 +50,19 @@ public interface Category extends Comparable { */ public abstract DataType[] getDataTypes(); + /** + * Get all data types in this category whose base name matches the base name of the given name. + * 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. 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); + /** * Adds the given datatype to this category. * @param dt the datatype to add to this category.