diff --git a/Ghidra/Features/VersionTracking/src/main/help/help/topics/VersionTrackingPlugin/images/VTOptions_ApplyMarkupDialog.png b/Ghidra/Features/VersionTracking/src/main/help/help/topics/VersionTrackingPlugin/images/VTOptions_ApplyMarkupDialog.png index 72d418cb18..01496cb332 100644 Binary files a/Ghidra/Features/VersionTracking/src/main/help/help/topics/VersionTrackingPlugin/images/VTOptions_ApplyMarkupDialog.png and b/Ghidra/Features/VersionTracking/src/main/help/help/topics/VersionTrackingPlugin/images/VTOptions_ApplyMarkupDialog.png differ diff --git a/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/api/markuptype/DataTypeMarkupType.java b/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/api/markuptype/DataTypeMarkupType.java index a83dfcd21a..b649c81698 100644 --- a/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/api/markuptype/DataTypeMarkupType.java +++ b/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/api/markuptype/DataTypeMarkupType.java @@ -266,8 +266,17 @@ public class DataTypeMarkupType extends VTMarkupType { // data that would be overwritten following the first data in the destination. return false; } - - DataType resolvedDataType = program.getDataTypeManager().resolve(dataType, conflictHandler); + + /* + Note: the resolve may add a .conflict type. That will not be removed if an exception is + thrown. Also if an unapply is executed, .conflict types will not be removed. For now, + we leave this up to the user to fix, should they care. Trying to remove .conflict types + during an unapply may have unintended side-effects if the user added new uses of that + conflict type. + */ + + ProgramBasedDataTypeManager dtm = program.getDataTypeManager(); + DataType resolvedDataType = dtm.resolve(dataType, conflictHandler); listing.clearCodeUnits(startAddress, endAddress, false); diff --git a/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/gui/util/VTMatchApplyChoices.java b/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/gui/util/VTMatchApplyChoices.java index 3ec5905cca..cdbb05156c 100644 --- a/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/gui/util/VTMatchApplyChoices.java +++ b/Ghidra/Features/VersionTracking/src/main/java/ghidra/feature/vt/gui/util/VTMatchApplyChoices.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. diff --git a/Ghidra/Features/VersionTracking/src/test.slow/java/ghidra/feature/vt/api/markupitem/DataTypeMarkupItemTest.java b/Ghidra/Features/VersionTracking/src/test.slow/java/ghidra/feature/vt/api/markupitem/DataTypeMarkupItemTest.java index 676624d862..eb8327870d 100644 --- a/Ghidra/Features/VersionTracking/src/test.slow/java/ghidra/feature/vt/api/markupitem/DataTypeMarkupItemTest.java +++ b/Ghidra/Features/VersionTracking/src/test.slow/java/ghidra/feature/vt/api/markupitem/DataTypeMarkupItemTest.java @@ -29,6 +29,7 @@ import ghidra.feature.vt.gui.util.VTMatchApplyChoices.DataTypeConflictChoices; import ghidra.feature.vt.gui.util.VTMatchApplyChoices.ReplaceDataChoices; import ghidra.feature.vt.gui.util.VTOptionDefines; import ghidra.framework.options.ToolOptions; +import ghidra.program.database.data.ProgramDataTypeManager; import ghidra.program.model.address.Address; import ghidra.program.model.data.*; import ghidra.program.model.lang.*; @@ -86,6 +87,57 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { doTestFindAndApplyMarkupItem(validator); } + @Test + public void testReplace_ConflictInDtm_ChooseExistingType() throws Exception { + + Address srcAddr = addr("0x010074e6", sourceProgram); + StructureDataType coolStruct1 = createCoolStruct1(); + Data sourceData = setDataType(sourceProgram, srcAddr, coolStruct1, coolStruct1.getLength()); + + // Add a type to the destination that has the same data type path, but is not equivalent + StructureDataType coolStruct2 = createCoolStruct2(); + addToDestinationDtm(coolStruct2); + + // make room for the type to be applied + Address destAddr = addr("0x010074e6", destinationProgram); + clear(destinationProgram, destAddr, coolStruct2.getLength()); + + StringDataType destDt = new StringDataType(); + Data destData = setDataType(destinationProgram, destAddr, destDt, 4); + + DataTypeValidator validator = new DataTypeValidator(sourceData, destData, + ReplaceDataChoices.REPLACE_FIRST_DATA_ONLY); + validator.setConflictChoice(DataTypeConflictChoices.USE_EXISTING); + validator.setKeepExistingType(true); + doTestFindAndApplyMarkupItem(validator); + + assertConflictTypeInDestinationDtm(coolStruct1, false); + } + + @Test + public void testReplace_ConflictInDtm_ChooseRenameAndAdd() throws Exception { + + Address srcAddr = addr("0x010074e6", sourceProgram); + StructureDataType coolStruct1 = createCoolStruct1(); + Data sourceData = setDataType(sourceProgram, srcAddr, coolStruct1, coolStruct1.getLength()); + + // Add a type to the destination that has the same data type path, but is not equivalent + StructureDataType coolStruct2 = createCoolStruct2(); + addToDestinationDtm(coolStruct2); + + Address destAddr = addr("0x010074e6", destinationProgram); + StringDataType destDt = new StringDataType(); + Data destData = setDataType(destinationProgram, destAddr, destDt, 4); + setDataType(destinationProgram, destAddr.add(4), destDt, 6); + + DataTypeValidator validator = new DataTypeValidator(sourceData, destData, + ReplaceDataChoices.REPLACE_FIRST_DATA_ONLY); + validator.setConflictChoice(DataTypeConflictChoices.RENAME_AND_ADD); + doTestFindAndApplyMarkupItem(validator); + + assertConflictTypeInDestinationDtm(coolStruct1, true); + } + @Test public void testReplaceLargerWithSmaller() throws Exception { @@ -308,7 +360,7 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { @Test public void testRejectedApplyDoesNotMutateDestinationDataTypeManager() throws Exception { - + Address sourceAddress = addr("0x010074e6", sourceProgram); // LoadCursorW StructureDataType sourceDataType = new StructureDataType("RejectedApplyStruct", 0); sourceDataType.add(new DWordDataType()); @@ -446,6 +498,48 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { // Private Methods //================================================================================================== + private void addToDestinationDtm(StructureDataType struct) { + ProgramDataTypeManager destDtm = destinationProgram.getDataTypeManager(); + tx(destDtm, () -> { + destDtm.resolve(struct, null); + }); + + DataTypePath dtp = struct.getDataTypePath(); + DataType resolvedDtm = destDtm.getDataType(dtp); + assertNotNull(resolvedDtm); + } + + private StructureDataType createCoolStruct1() { + StructureDataType struct = new StructureDataType("CoolStructure", 0); + struct.add(new DWordDataType()); + return struct; + } + + // 'CoolStructure' that is slightly different than that made in createCoolStruct1() + private StructureDataType createCoolStruct2() { + StructureDataType struct = new StructureDataType("CoolStructure", 0); + struct.add(new DWordDataType()); + struct.add(new DWordDataType()); + return struct; + } + + private void assertConflictTypeInDestinationDtm(DataType dt, boolean expectConflict) { + + DataTypePath dtp = dt.getDataTypePath(); + String name = dt.getName() + ".conflict"; + CategoryPath cp = dtp.getCategoryPath(); + DataTypePath conflictPath = new DataTypePath(cp, name); + + ProgramDataTypeManager destDtm = destinationProgram.getDataTypeManager(); + DataType conflictType = destDtm.getDataType(conflictPath); + if (expectConflict) { + assertNotNull(conflictType); + } + else { + assertNull(conflictType); + } + } + private Structure createGadgetStruct() { Structure gadgetStruct = new StructureDataType("Gadget", 0); @@ -461,14 +555,13 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { private Data setDataType(Program program, Address address, DataType dataType, int length) { - int txID = program.startTransaction("Change Data Type"); - boolean commit = false; - try { + return tx(program, () -> { Listing listing = program.getListing(); Data sourceData = listing.getDataAt(address); if (sourceData == null) { return null; } + listing.clearCodeUnits(address, sourceData.getMaxAddress(), false); Data data; if (length > 0) { @@ -477,23 +570,20 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { else { data = listing.createData(address, dataType); } - commit = true; return data; - } - catch (Exception e) { - // Commit is false by default so nothing else to do. - return null; - } - finally { - program.endTransaction(txID, commit); - } + }); + } + + private void clear(Program p, Address a, int length) { + tx(p, () -> { + Listing listing = p.getListing(); + listing.clearCodeUnits(a, a.add(length), false); + }); } private Instruction createInstruction(Program program, Address atAddress) { - int txID = program.startTransaction("Create Instruction"); - boolean commit = false; - try { + return tx(program, () -> { Listing listing = program.getListing(); Memory memory = program.getMemory(); MemBuffer buf = new DumbMemBufferImpl(memory, atAddress); @@ -502,16 +592,8 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { InstructionPrototype proto = program.getLanguage().parse(buf, context, false); Instruction createdInstruction = listing.createInstruction(atAddress, proto, buf, context, 0); - commit = true; return createdInstruction; - } - catch (Exception e) { - // Commit is false by default so nothing else to do. - return null; - } - finally { - program.endTransaction(txID, commit); - } + }); } //================================================================================================== @@ -528,6 +610,7 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { private int originalDestinationLength; private ReplaceDataChoices dataTypeChoice; private DataTypeConflictChoices conflictChoice; + private boolean keepExistingType; DataTypeValidator(Data sourceData, Data destinationData, ReplaceDataChoices dataTypeChoice) { @@ -548,6 +631,10 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { this.conflictChoice = conflictChoice; } + void setKeepExistingType(boolean keep) { + this.keepExistingType = keep; + } + @Override protected Address getDestinationApplyAddress() { return getDestinationMatchAddress(); @@ -587,10 +674,19 @@ public class DataTypeMarkupItemTest extends AbstractVTMarkupItemTest { Data currentDestinationData = listing.getDataAt(getDestinationApplyAddress()); DataType currentDestinationDataType = currentDestinationData.getDataType(); int currentDestinationLength = currentDestinationData.getLength(); - assertTrue("Data type was not applied", - sourceDataType.isEquivalent(currentDestinationDataType)); - assertTrue("Data type was not set to the source data type's size", - sourceLength == currentDestinationLength); + + if (keepExistingType) { + // guilty knowledge: keeping the existing type is used when the types have the same + // data type path, but are not equivalent + assertEquals(sourceDataType.getDataTypePath(), + currentDestinationDataType.getDataTypePath()); + } + else { + assertTrue("Data type was not applied", + sourceDataType.isEquivalent(currentDestinationDataType)); + assertTrue("Data type was not set to the source data type's size", + sourceLength == currentDestinationLength); + } } @Override