From 6ccf256055835497e0c986772091b1da7e297849 Mon Sep 17 00:00:00 2001 From: emteere <47253321+emteere@users.noreply.github.com> Date: Thu, 2 May 2024 14:23:28 -0400 Subject: [PATCH] GP-4535 Corrected storage of default context on disassembly errors and switch table targets. Improves disassembly for programs with no stored context as context table will be empty. --- .../analysis/ConstantPropagationAnalyzer.java | 2 +- .../function/DecompilerSwitchAnalysisCmd.java | 45 +++++--- .../Framework/DB/src/main/java/db/Table.java | 2 +- .../DB/src/main/java/db/TableRecord.java | 2 +- .../database/util/AddressRangeMapDB.java | 109 ++++++++++-------- .../program/disassemble/Disassembler.java | 7 +- .../util/AbstractStoredProgramContext.java | 20 ++-- 7 files changed, 106 insertions(+), 81 deletions(-) diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/analysis/ConstantPropagationAnalyzer.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/analysis/ConstantPropagationAnalyzer.java index 83c1f6dda7..5a42d85d48 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/analysis/ConstantPropagationAnalyzer.java +++ b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/analysis/ConstantPropagationAnalyzer.java @@ -66,7 +66,7 @@ public class ConstantPropagationAnalyzer extends AbstractAnalyzer { protected static final String MAX_THREAD_COUNT_OPTION_NAME = "Max Threads"; protected static final String MAX_THREAD_COUNT_OPTION_DESCRIPTION = "Maximum threads for constant propagation. Too many threads causes thrashing in DB."; - protected static final int MAX_THREAD_COUNT_OPTION_DEFAULT_VALUE = 2; + protected static final int MAX_THREAD_COUNT_OPTION_DEFAULT_VALUE = 1; protected static final String MIN_KNOWN_REFADDRESS_OPTION_NAME = "Min absolute reference"; protected static final String MIN_KNOWN_REFADDRESS_OPTION_DESCRIPTION = diff --git a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerSwitchAnalysisCmd.java b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerSwitchAnalysisCmd.java index 6d7ecd1534..a7b8e702a3 100644 --- a/Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerSwitchAnalysisCmd.java +++ b/Ghidra/Features/Decompiler/src/main/java/ghidra/app/cmd/function/DecompilerSwitchAnalysisCmd.java @@ -184,25 +184,12 @@ public class DecompilerSwitchAnalysisCmd extends BackgroundCommand { if (disSetList.contains(caseStart)) { continue; } - if (switchContext != null) { - try { - // Combine flowed switch context with context register value at case address - RegisterValue curContext = - programContext.getRegisterValue(baseContextRegister, caseStart); - if (curContext != null) { - curContext = curContext.combineValues(switchContext); - - // lay down the new merged context - programContext.setRegisterValue(caseStart, caseStart, curContext); - } - else { - programContext.setRegisterValue(caseStart, caseStart, switchContext); - } - } - catch (ContextChangeException e) { - // This can occur when two or more threads are working on the same function - continue; - } + try { + setSwitchTargetContext(programContext, caseStart, switchContext); + } + catch (ContextChangeException e) { + // This can occur when two or more threads are working on the same function + continue; } disSetList.add(caseStart); } @@ -229,6 +216,26 @@ public class DecompilerSwitchAnalysisCmd extends BackgroundCommand { } } + private void setSwitchTargetContext(ProgramContext programContext, Address targetStart, RegisterValue switchContext) throws ContextChangeException { + if (switchContext == null) { + return; + } + + // Combine flowed switch context with context register value at case address + RegisterValue curContext = + programContext.getNonDefaultValue(switchContext.getRegister(), targetStart); + if (curContext != null) { + switchContext = curContext.combineValues(switchContext); + } + + if (switchContext == null || !switchContext.hasAnyValue()) { + return; + } + + // only store if different than what is already there, which could be a default value + program.getProgramContext().setRegisterValue(targetStart, targetStart, switchContext); + } + private void labelSwitch(JumpTable table, TaskMonitor monitor) throws CancelledException { AddLabelCmd tableNameLabel = new AddLabelCmd(table.getSwitchAddress(), "switchD", SourceType.ANALYSIS); diff --git a/Ghidra/Framework/DB/src/main/java/db/Table.java b/Ghidra/Framework/DB/src/main/java/db/Table.java index 731ddf3f8a..c1b7e9739d 100644 --- a/Ghidra/Framework/DB/src/main/java/db/Table.java +++ b/Ghidra/Framework/DB/src/main/java/db/Table.java @@ -583,7 +583,7 @@ public class Table { * @return record count */ public int getRecordCount() { - return tableRecord.getRecordCount(); + return (tableRecord == null ? 0 : tableRecord.getRecordCount()); } /** diff --git a/Ghidra/Framework/DB/src/main/java/db/TableRecord.java b/Ghidra/Framework/DB/src/main/java/db/TableRecord.java index 5c08c75d62..3460d8751e 100644 --- a/Ghidra/Framework/DB/src/main/java/db/TableRecord.java +++ b/Ghidra/Framework/DB/src/main/java/db/TableRecord.java @@ -250,7 +250,7 @@ class TableRecord implements Comparable { * @return table's record count */ int getRecordCount() { - return record.getIntValue(RECORD_COUNT_COLUMN); + return (record == null ? 0 : record.getIntValue(RECORD_COUNT_COLUMN)); } /** diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/util/AddressRangeMapDB.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/util/AddressRangeMapDB.java index 069a677e6c..7c76443f37 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/util/AddressRangeMapDB.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/util/AddressRangeMapDB.java @@ -21,6 +21,7 @@ import java.util.List; import db.*; import db.util.ErrorHandler; +import generic.stl.Pair; import ghidra.program.database.map.AddressKeyRecordIterator; import ghidra.program.database.map.AddressMap; import ghidra.program.model.address.*; @@ -85,9 +86,8 @@ public class AddressRangeMapDB implements DBListener { private Schema rangeMapSchema; private Table rangeMapTable; - // caching - private Field lastValue; - private AddressRange lastRange; + // caching, single value, so safer to check + private Pair lastValue; private int modCount; @@ -136,10 +136,16 @@ public class AddressRangeMapDB implements DBListener { * @throws DuplicateNameException if there is already range map with that name */ public boolean setName(String newName) throws DuplicateNameException { - String newTableName = RANGE_MAP_TABLE_PREFIX + newName; - if (rangeMapTable == null || rangeMapTable.setName(newTableName)) { - tableName = newTableName; - return true; + lock.acquire(); + try { + String newTableName = RANGE_MAP_TABLE_PREFIX + newName; + if (rangeMapTable == null || rangeMapTable.setName(newTableName)) { + tableName = newTableName; + return true; + } + } + finally { + lock.release(); } return false; } @@ -149,13 +155,8 @@ public class AddressRangeMapDB implements DBListener { * @return true if this map is empty */ public boolean isEmpty() { - lock.acquire(); - try { - return rangeMapTable == null || rangeMapTable.getRecordCount() == 0; - } - finally { - lock.release(); - } + Table localTable = rangeMapTable; + return localTable == null || localTable.getRecordCount() == 0; } /** @@ -165,7 +166,8 @@ public class AddressRangeMapDB implements DBListener { * @return record count */ public int getRecordCount() { - return rangeMapTable != null ? rangeMapTable.getRecordCount() : 0; + Table localTable = rangeMapTable; + return localTable == null ? 0 : localTable.getRecordCount(); } /** @@ -174,23 +176,26 @@ public class AddressRangeMapDB implements DBListener { * @return value or null no value exists */ public Field getValue(Address address) { + if (isEmpty()) { + return null; + } + // check last cached range + Pair localValue = lastValue; + if (localValue != null && localValue.first.contains(address)) { + return localValue.second; + } lock.acquire(); try { if (rangeMapTable == null) { return null; } - // check last cached range - if (lastRange != null && lastRange.contains(address)) { - return lastValue; - } - DBRecord record = findRecordContaining(address); List ranges = getRangesForRecord(record); for (AddressRange range : ranges) { if (range.contains(address)) { - lastRange = range; - lastValue = record.getFieldValue(VALUE_COL); - return lastValue; + Field fieldValue = record.getFieldValue(VALUE_COL); + lastValue = new Pair(range, fieldValue); + return fieldValue; } } } @@ -214,16 +219,19 @@ public class AddressRangeMapDB implements DBListener { * @throws IllegalArgumentException if the end address is greater then the start address */ public void paintRange(Address startAddress, Address endAddress, Field value) { + if (value == null && isEmpty()) { + return; + } AddressRange.checkValidRange(startAddress, endAddress); lock.acquire(); try { + if (value == null && rangeMapTable == null) { + return; + } clearCache(); ++modCount; if (rangeMapTable == null) { - if (value == null) { - return; - } createTable(); } doPaintRange(startAddress, endAddress, value); @@ -246,7 +254,7 @@ public class AddressRangeMapDB implements DBListener { */ public void moveAddressRange(Address fromAddr, Address toAddr, long length, TaskMonitor monitor) throws CancelledException { - if (length <= 0 || rangeMapTable == null) { + if (length <= 0 || isEmpty()) { return; } @@ -254,6 +262,9 @@ public class AddressRangeMapDB implements DBListener { AddressRangeMapDB tmpMap = null; lock.acquire(); try { + if (rangeMapTable == null) { + return; + } tmpDb = dbHandle.getScratchPad(); tmpMap = new AddressRangeMapDB(tmpDb, addressMap, lock, "TEMP", errHandler, valueField, indexed); @@ -310,9 +321,15 @@ public class AddressRangeMapDB implements DBListener { * @return set of addresses where a values has been set */ public AddressSet getAddressSet() { + AddressSet set = new AddressSet(); + if (isEmpty()) { + return set; + } lock.acquire(); try { - AddressSet set = new AddressSet(); + if (rangeMapTable == null) { + return set; + } AddressRangeIterator addressRanges = getAddressRanges(); for (AddressRange addressRange : addressRanges) { set.add(addressRange); @@ -331,11 +348,15 @@ public class AddressRangeMapDB implements DBListener { */ public AddressSet getAddressSet(Field value) { AddressSet set = new AddressSet(); - if (rangeMapTable == null) { + if (isEmpty()) { return set; } lock.acquire(); try { + if (rangeMapTable == null) { + return set; + } + RecordIterator it = rangeMapTable.indexIterator(VALUE_COL, value, value, true); while (it.hasNext()) { DBRecord record = it.next(); @@ -357,7 +378,7 @@ public class AddressRangeMapDB implements DBListener { * @return AddressRangeIterator that iterates over all occupied ranges in the map */ public AddressRangeIterator getAddressRanges() { - if (rangeMapTable == null) { + if (isEmpty()) { return new EmptyAddressRangeIterator(); } try { @@ -377,7 +398,7 @@ public class AddressRangeMapDB implements DBListener { * given start address */ public AddressRangeIterator getAddressRanges(Address startAddress) { - if (rangeMapTable == null) { + if (isEmpty()) { return new EmptyAddressRangeIterator(); } try { @@ -398,7 +419,7 @@ public class AddressRangeMapDB implements DBListener { * given start address */ public AddressRangeIterator getAddressRanges(Address startAddress, Address endAddr) { - if (rangeMapTable == null) { + if (isEmpty()) { return new EmptyAddressRangeIterator(); } try { @@ -492,14 +513,13 @@ public class AddressRangeMapDB implements DBListener { * @return an address range that contains the given address and has all the same value */ public AddressRange getAddressRangeContaining(Address address) { - + // check cache + Pair localValue = lastValue; + if (localValue != null && localValue.first.contains(address)) { + return localValue.first; + } lock.acquire(); try { - // check cache - if (lastRange != null && lastRange.contains(address)) { - return lastRange; - } - // look for a stored value range that contains that address AddressRange range = findValueRangeContainingAddress(address); if (range == null) { @@ -522,8 +542,8 @@ public class AddressRangeMapDB implements DBListener { List rangesForRecord = getRangesForRecord(record); for (AddressRange range : rangesForRecord) { if (range.contains(address)) { - lastRange = range; - lastValue = record.getFieldValue(VALUE_COL); + Field fieldValue = record.getFieldValue(VALUE_COL); + lastValue = new Pair(range, fieldValue); return range; } } @@ -786,14 +806,7 @@ public class AddressRangeMapDB implements DBListener { * Clears the "last range" cache */ private void clearCache() { - lock.acquire(); - try { - lastRange = null; - lastValue = null; - } - finally { - lock.release(); - } + lastValue = null; } DBRecord getAddressWrappingRecord() throws IOException { diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/disassemble/Disassembler.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/disassemble/Disassembler.java index 1992482da4..0428831e99 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/disassemble/Disassembler.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/disassemble/Disassembler.java @@ -1389,7 +1389,12 @@ public class Disassembler implements DisassemblerConflictHandler { RegisterValue contextValue = conflict.getParseContextValue(); if (contextValue != null) { try { - program.getProgramContext().setRegisterValue(address, address, contextValue); + RegisterValue curContextValue = program.getProgramContext().getRegisterValue(contextValue.getRegister(), address); + + // only store if different than what is already there, which could be a default value + if (!contextValue.equals(curContextValue)) { + program.getProgramContext().setRegisterValue(address, address, contextValue); + } } catch (ContextChangeException e) { // ignore - existing instruction likely blocked context modification diff --git a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/util/AbstractStoredProgramContext.java b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/util/AbstractStoredProgramContext.java index aa12eae564..5b06b565cf 100644 --- a/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/util/AbstractStoredProgramContext.java +++ b/Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/util/AbstractStoredProgramContext.java @@ -143,7 +143,7 @@ abstract public class AbstractStoredProgramContext extends AbstractProgramContex } RegisterValueStore store = map.get(register.getBaseRegister()); - if (store == null) { + if (store == null || store.isEmpty()) { return null; } @@ -153,8 +153,8 @@ abstract public class AbstractStoredProgramContext extends AbstractProgramContex @Override public AddressRangeIterator getRegisterValueAddressRanges(Register register) { RegisterValueStore store = registerValueMap.get(register.getBaseRegister()); - if (store == null) { - return new AddressSet().getAddressRanges(); + if (store == null || store.isEmpty()) { + return new EmptyAddressRangeIterator(); } return new RegisterAddressRangeIterator(register, store.getAddressRangeIterator(), registerValueMap); @@ -163,7 +163,7 @@ abstract public class AbstractStoredProgramContext extends AbstractProgramContex @Override public AddressRange getRegisterValueRangeContaining(Register register, Address addr) { RegisterValueStore store = registerValueMap.get(register.getBaseRegister()); - if (store == null) { + if (store == null || store.isEmpty()) { return new AddressRangeImpl(addr, addr); } return store.getValueRangeContaining(addr); @@ -173,8 +173,8 @@ abstract public class AbstractStoredProgramContext extends AbstractProgramContex public AddressRangeIterator getRegisterValueAddressRanges(Register register, Address start, Address end) { RegisterValueStore store = registerValueMap.get(register.getBaseRegister()); - if (store == null) { - return new AddressSet().getAddressRanges(); + if (store == null || store.isEmpty()) { + return new EmptyAddressRangeIterator(); } return new RegisterAddressRangeIterator(register, store.getAddressRangeIterator(start, end), registerValueMap); @@ -183,8 +183,8 @@ abstract public class AbstractStoredProgramContext extends AbstractProgramContex @Override public AddressRangeIterator getDefaultRegisterValueAddressRanges(Register register) { RegisterValueStore store = defaultRegisterValueMap.get(register.getBaseRegister()); - if (store == null) { - return new AddressSet().getAddressRanges(); + if (store == null || store.isEmpty()) { + return new EmptyAddressRangeIterator(); } return new RegisterAddressRangeIterator(register, store.getAddressRangeIterator(), defaultRegisterValueMap); @@ -194,8 +194,8 @@ abstract public class AbstractStoredProgramContext extends AbstractProgramContex public AddressRangeIterator getDefaultRegisterValueAddressRanges(Register register, Address start, Address end) { RegisterValueStore store = defaultRegisterValueMap.get(register.getBaseRegister()); - if (store == null) { - return new AddressSet().getAddressRanges(); + if (store == null || store.isEmpty()) { + return new EmptyAddressRangeIterator(); } return new RegisterAddressRangeIterator(register, store.getAddressRangeIterator(start, end), defaultRegisterValueMap);