From e1b9e27670c002c0f5fa4254d17b61982f19c3fc Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Tue, 15 Nov 2022 17:06:32 -0500 Subject: [PATCH] GP-2653: Use PointerTypedef for pointers from register space to ram --- .../register/DebuggerRegistersProvider.java | 33 +++++++++++-- .../gui/watch/DebuggerWatchesProvider.java | 16 +++++-- .../plugin/core/debug/gui/watch/WatchRow.java | 46 ++++++++++++++---- .../workflow/DisassembleAtPcDebuggerBot.java | 16 +++---- .../DebuggerRegistersProviderTest.java | 7 ++- .../watch/DebuggerWatchesProviderTest.java | 2 +- .../trace/database/listing/DBTraceData.java | 10 +--- .../listing/DBTraceDefinedDataView.java | 3 +- .../ghidra/trace/util/TraceRegisterUtils.java | 47 ------------------- 9 files changed, 95 insertions(+), 85 deletions(-) diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java index 557b2906be..ed56e296b7 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProvider.java @@ -61,8 +61,7 @@ import ghidra.framework.options.annotation.*; import ghidra.framework.plugintool.*; import ghidra.framework.plugintool.annotation.AutoServiceConsumed; import ghidra.program.model.address.*; -import ghidra.program.model.data.DataType; -import ghidra.program.model.data.DataTypeEncodeException; +import ghidra.program.model.data.*; import ghidra.program.model.lang.*; import ghidra.program.model.listing.Data; import ghidra.program.model.util.CodeUnitInsertionException; @@ -577,6 +576,9 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter mainPanel.add(regsFilterPanel, BorderLayout.SOUTH); regsTable.getSelectionModel().addListSelectionListener(evt -> { + if (evt.getValueIsAdjusting()) { + return; + } myActionContext = new DebuggerRegisterActionContext(this, regsFilterPanel.getSelectedItem(), regsTable); contextChanged(); @@ -584,7 +586,7 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter regsTable.addMouseListener(new MouseAdapter() { @Override public void mouseClicked(MouseEvent e) { - if (e.getClickCount() == 2) { + if (e.getClickCount() == 2 && e.getButton() == MouseEvent.BUTTON1) { navigateToAddress(); } } @@ -664,7 +666,7 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter if (data == null || data.getValueClass() != Address.class) { return; } - Address address = (Address) TraceRegisterUtils.getValueHackPointer(data); + Address address = (Address) data.getValue(); if (address == null) { return; } @@ -920,6 +922,27 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter void writeRegisterDataType(Register register, DataType dataType) { try (UndoableTransaction tid = UndoableTransaction.start(current.getTrace(), "Edit Register Type")) { + if (dataType instanceof Pointer ptrType && register.getAddress().isRegisterAddress()) { + // Because we're about to use the size, resolve it first + ptrType = (Pointer) current.getTrace() + .getDataTypeManager() + .resolve(dataType, DataTypeConflictHandler.DEFAULT_HANDLER); + /** + * TODO: This should be the current platform instead, but it's not clear how to do + * that. The PointerTypedef uses the program (taken from the MemBuffer) to lookup + * the configured address space by name. Might be better if MemBuffer/CodeUnit had + * getAddressFactory(). Still, I'd need guest-platform data units before I could + * override that meaningfully. + */ + /** + * AddressSpace space = + * current.getPlatform().getAddressFactory().getDefaultAddressSpace(); + */ + AddressSpace space = + current.getTrace().getBaseAddressFactory().getDefaultAddressSpace(); + dataType = new PointerTypedef(null, ptrType.getDataType(), ptrType.getLength(), + ptrType.getDataTypeManager(), space); + } TraceCodeSpace space = getRegisterMemorySpace(true).getCodeSpace(true); long snap = current.getViewSnap(); TracePlatform platform = current.getPlatform(); @@ -986,7 +1009,7 @@ public class DebuggerRegistersProvider extends ComponentProviderAdapter if (data == null) { return null; } - return TraceRegisterUtils.getValueRepresentationHackPointer(data); + return data.getDefaultValueRepresentation(); } /** diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/watch/DebuggerWatchesProvider.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/watch/DebuggerWatchesProvider.java index 7cd0ccce67..75a32475bf 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/watch/DebuggerWatchesProvider.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/watch/DebuggerWatchesProvider.java @@ -16,8 +16,7 @@ package ghidra.app.plugin.core.debug.gui.watch; import java.awt.*; -import java.awt.event.MouseAdapter; -import java.awt.event.MouseEvent; +import java.awt.event.*; import java.util.ArrayList; import java.util.List; import java.util.Objects; @@ -428,10 +427,17 @@ public class DebuggerWatchesProvider extends ComponentProviderAdapter watchTable.addMouseListener(new MouseAdapter() { @Override public void mouseClicked(MouseEvent e) { - if (e.getClickCount() != 2 || e.getButton() != MouseEvent.BUTTON1) { - return; + if (e.getClickCount() == 2 && e.getButton() == MouseEvent.BUTTON1) { + navigateToSelectedWatch(); + } + } + }); + watchTable.addKeyListener(new KeyAdapter() { + @Override + public void keyPressed(KeyEvent e) { + if (e.getKeyCode() == KeyEvent.VK_ENTER) { + navigateToSelectedWatch(); } - navigateToSelectedWatch(); } }); diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/watch/WatchRow.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/watch/WatchRow.java index 5f3f1756e2..9fde899120 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/watch/WatchRow.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/watch/WatchRow.java @@ -32,12 +32,10 @@ import ghidra.pcode.exec.*; import ghidra.pcode.exec.DebuggerPcodeUtils.WatchValue; import ghidra.pcode.utils.Utils; import ghidra.program.model.address.*; -import ghidra.program.model.data.DataType; -import ghidra.program.model.data.DataTypeEncodeException; +import ghidra.program.model.data.*; import ghidra.program.model.listing.Function; import ghidra.program.model.listing.Program; -import ghidra.program.model.mem.ByteMemBufferImpl; -import ghidra.program.model.mem.MemBuffer; +import ghidra.program.model.mem.*; import ghidra.program.model.symbol.*; import ghidra.program.util.ProgramLocation; import ghidra.trace.model.*; @@ -46,6 +44,7 @@ import ghidra.trace.model.memory.TraceMemoryState; import ghidra.trace.model.symbol.TraceLabelSymbol; import ghidra.util.Msg; import ghidra.util.NumericUtilities; +import ghidra.util.database.UndoableTransaction; public class WatchRow { public static final int TRUNCATE_BYTES_LENGTH = 64; @@ -148,11 +147,20 @@ public class WatchRow { }, AsyncUtils.SWING_EXECUTOR); } + private ByteMemBufferImpl createMemBuffer() { + return new ByteMemBufferImpl(address, value, provider.language.isBigEndian()) { + @Override + public Memory getMemory() { + return provider.current.getTrace().getProgramView().getMemory(); + } + }; + } + protected String parseAsDataTypeStr() { if (dataType == null || value == null) { return ""; } - MemBuffer buffer = new ByteMemBufferImpl(address, value, provider.language.isBigEndian()); + MemBuffer buffer = createMemBuffer(); return dataType.getRepresentation(buffer, settings, value.length); } @@ -160,8 +168,8 @@ public class WatchRow { if (dataType == null || value == null) { return null; } - MemBuffer buffer = new ByteMemBufferImpl(address, value, provider.language.isBigEndian()); - return dataType.getValue(buffer, SettingsImpl.NO_SETTINGS, value.length); + MemBuffer buffer = createMemBuffer(); + return dataType.getValue(buffer, settings, value.length); } public void setExpression(String expression) { @@ -210,12 +218,34 @@ public class WatchRow { } public void setDataType(DataType dataType) { + if (dataType instanceof Pointer ptrType && address != null && + address.isRegisterAddress()) { + /** + * NOTE: This will not catch it if the expression cannot be evaluated. When it can later + * be evaluated, no check is performed. + * + * TODO: This should be for the current platform. These don't depend on the trace's code + * storage, so it should be easier to implement. Still, I'll wait to tackle that all at + * once. + */ + AddressSpace space = + provider.current.getTrace().getBaseAddressFactory().getDefaultAddressSpace(); + DataTypeManager dtm = ptrType.getDataTypeManager(); + dataType = + new PointerTypedef(null, ptrType.getDataType(), ptrType.getLength(), dtm, space); + if (dtm != null) { + try (UndoableTransaction tid = + UndoableTransaction.start(dtm, "Resolve data type")) { + dataType = dtm.resolve(dataType, DataTypeConflictHandler.DEFAULT_HANDLER); + } + } + } this.typePath = dataType == null ? null : dataType.getPathName(); this.dataType = dataType; + settings.setDefaultSettings(dataType == null ? null : dataType.getDefaultSettings()); valueString = parseAsDataTypeStr(); valueObj = parseAsDataTypeObj(); provider.contextChanged(); - settings.setDefaultSettings(dataType == null ? null : dataType.getDefaultSettings()); if (dataType != null) { savedSettings.read(dataType.getSettingsDefinitions(), dataType.getDefaultSettings()); } diff --git a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/workflow/DisassembleAtPcDebuggerBot.java b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/workflow/DisassembleAtPcDebuggerBot.java index deb6c338f4..3601cd77e7 100644 --- a/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/workflow/DisassembleAtPcDebuggerBot.java +++ b/Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/workflow/DisassembleAtPcDebuggerBot.java @@ -33,7 +33,7 @@ import ghidra.framework.model.DomainObject; import ghidra.framework.options.annotation.HelpInfo; import ghidra.framework.plugintool.PluginTool; import ghidra.program.model.address.*; -import ghidra.program.model.data.PointerDataType; +import ghidra.program.model.data.*; import ghidra.program.model.lang.Register; import ghidra.program.model.util.CodeUnitInsertionException; import ghidra.trace.model.*; @@ -225,9 +225,12 @@ public class DisassembleAtPcDebuggerBot implements DebuggerBot { try (UndoableTransaction tid = UndoableTransaction.start(trace, "Disassemble: PC is code pointer")) { TraceCodeSpace regCode = codeManager.getCodeRegisterSpace(thread, frameLevel, true); + // TODO: Should be same platform as pc, not necessarily base + AddressSpace space = trace.getBaseAddressFactory().getDefaultAddressSpace(); + PointerTypedef type = new PointerTypedef(null, VoidDataType.dataType, + pc.getMinimumByteSize(), null, space); try { - pcUnit = regCode.definedData() - .create(Lifespan.nowOn(pcSnap), pc, PointerDataType.dataType); + pcUnit = regCode.definedData().create(Lifespan.nowOn(pcSnap), pc, type); } catch (CodeUnitInsertionException e) { // I guess something's already there. Leave it, then! @@ -235,11 +238,8 @@ public class DisassembleAtPcDebuggerBot implements DebuggerBot { pcUnit = regCode.definedData().getForRegister(pcSnap, pc); } } - if (pcUnit != null) { - Address pcVal = (Address) TraceRegisterUtils.getValueHackPointer(pcUnit); - if (pcVal != null) { - disassemble(pcVal, thread, memSnap); - } + if (pcUnit != null && pcUnit.getValue() instanceof Address pcVal) { + disassemble(pcVal, thread, memSnap); } } diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProviderTest.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProviderTest.java index 4edea1619c..7b8d21ce64 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProviderTest.java +++ b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/register/DebuggerRegistersProviderTest.java @@ -38,6 +38,7 @@ import ghidra.app.services.DebuggerStateEditingService.StateEditingMode; import ghidra.app.services.TraceRecorder; import ghidra.docking.settings.FormatSettingsDefinition; import ghidra.docking.settings.Settings; +import ghidra.program.model.address.AddressSpace; import ghidra.program.model.data.*; import ghidra.program.model.lang.Register; import ghidra.program.model.util.CodeUnitInsertionException; @@ -139,8 +140,10 @@ public class DebuggerRegistersProviderTest extends AbstractGhidraHeadedDebuggerG throws CodeUnitInsertionException { TraceCodeSpace regCode = tb.trace.getCodeManager().getCodeRegisterSpace(thread, true); - regCode.definedData().create(Lifespan.nowOn(0), pc, PointerDataType.dataType); - // TODO: Pointer needs to be to ram, not register space + DataTypeManager dtm = tb.trace.getDataTypeManager(); + AddressSpace space = tb.host.getAddressFactory().getDefaultAddressSpace(); + PointerTypedef ramPtr = new PointerTypedef(null, VoidDataType.dataType, -1, dtm, space); + regCode.definedData().create(Lifespan.nowOn(0), pc, ramPtr); regCode.definedData().create(Lifespan.nowOn(0), r0, r0Struct); } diff --git a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/watch/DebuggerWatchesProviderTest.java b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/watch/DebuggerWatchesProviderTest.java index 5c14d4728f..9749da7307 100644 --- a/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/watch/DebuggerWatchesProviderTest.java +++ b/Ghidra/Debug/Debugger/src/test/java/ghidra/app/plugin/core/debug/gui/watch/DebuggerWatchesProviderTest.java @@ -732,9 +732,9 @@ public class DebuggerWatchesProviderTest extends AbstractGhidraHeadedDebuggerGUI rowR0.setDataType(PointerDataType.dataType); registersProvider.setSelectedRow(rowR0); }); - waitForWatches(); performEnabledAction(registersProvider, watchesProvider.actionAddFromRegister, true); + waitForWatches(); WatchRow watch = Unique.assertOne(watchesProvider.watchTableModel.getModelData()); assertEquals("r0", watch.getExpression()); diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/listing/DBTraceData.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/listing/DBTraceData.java index 15d08a1fb4..5f3d7183ad 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/listing/DBTraceData.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/listing/DBTraceData.java @@ -120,9 +120,9 @@ public class DBTraceData extends AbstractDBTraceCodeUnit * @param platform the platform * @param dataType the data type */ - protected void set(InternalTracePlatform platform, DataType dataType) { + protected void set(InternalTracePlatform platform, long dataTypeID) { this.platformKey = platform.getIntKey(); - this.dataTypeID = space.dataTypeManager.getResolvedID(dataType); + this.dataTypeID = dataTypeID; update(PLATFORM_COLUMN, DATATYPE_COLUMN); this.platform = platform; @@ -139,12 +139,6 @@ public class DBTraceData extends AbstractDBTraceCodeUnit * @return the length, or -1 */ protected int getDataTypeLength() { - if (baseDataType instanceof Pointer) { - // TODO: Also need to know where this address maps into the other language's spaces.... - // NOTE: Using default data space for now - // TODO: I may not need this Pointer check, as clone(dtm) should adjust already - return getLanguage().getDefaultDataSpace().getPointerSize(); - } return dataType.getLength(); // -1 is checked elsewhere } diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/listing/DBTraceDefinedDataView.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/listing/DBTraceDefinedDataView.java index 1e54507fba..2c690a30b8 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/listing/DBTraceDefinedDataView.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/listing/DBTraceDefinedDataView.java @@ -157,9 +157,10 @@ public class DBTraceDefinedDataView extends AbstractBaseDBTraceDefinedUnitsView< return space.undefinedData.getAt(startSnap, address); } + long dataTypeID = space.dataTypeManager.getResolvedID(dataType); DBTraceData created = space.dataMapSpace.put(tasr, null); // TODO: data units with a guest platform - created.set(space.trace.getPlatformManager().getHostPlatform(), dataType); + created.set(space.trace.getPlatformManager().getHostPlatform(), dataTypeID); // TODO: Explicitly remove undefined from cache, or let weak refs take care of it? cacheForContaining.notifyNewEntry(lifespan, createdRange, created); diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/util/TraceRegisterUtils.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/util/TraceRegisterUtils.java index 8f291b4b1d..84fc8720b2 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/util/TraceRegisterUtils.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/util/TraceRegisterUtils.java @@ -18,7 +18,6 @@ package ghidra.trace.util; import java.math.BigInteger; import java.nio.ByteBuffer; import java.util.Arrays; -import java.util.function.BiConsumer; import org.apache.commons.lang3.ArrayUtils; @@ -124,28 +123,6 @@ public enum TraceRegisterUtils { return seekComponent(data, rangeForRegister(reg)); } - public static Object getValueHackPointer(TraceData data) { - if (data.getValueClass() != Address.class) { - return data.getValue(); - } - if (!data.getAddress().getAddressSpace().isRegisterSpace()) { - return data.getValue(); - } - return PointerDataType.getAddressValue(data, data.getLength(), - data.getTrace().getBaseAddressFactory().getDefaultAddressSpace()); - } - - public static String getValueRepresentationHackPointer(TraceData data) { - if (data.getValueClass() != Address.class) { - return data.getDefaultValueRepresentation(); - } - Address addr = (Address) getValueHackPointer(data); - if (addr == null) { - return "NaP"; - } - return addr.toString(); - } - public static RegisterValue encodeValueRepresentationHackPointer(Register register, TraceData data, String representation) throws DataTypeEncodeException { DataType dataType = data.getBaseDataType(); @@ -209,30 +186,6 @@ public enum TraceRegisterUtils { return new RegisterValue(register, arr); } - public static RegisterValue getRegisterValue(Register reg, - BiConsumer readAction) { - /* - * The byte array for reg values spans the whole base register, but we'd like to avoid - * over-reading, so we'll zero in on the bytes actually included in the mask. We'll then - * have to handle endianness and such. The regval instance should then apply the actual mask - * for the sub-register, if applicable. - */ - int byteLength = reg.getNumBytes(); - byte[] mask = reg.getBaseMask(); - ByteBuffer buf = ByteBuffer.allocate(mask.length * 2); - buf.put(mask); - int maskOffset = TraceRegisterUtils.computeMaskOffset(reg); - int startVal = buf.position() + maskOffset; - buf.position(startVal); - buf.limit(buf.position() + byteLength); - readAction.accept(reg.getAddress(), buf); - byte[] arr = buf.array(); - if (!reg.isBigEndian() && !reg.isProcessorContext()) { - ArrayUtils.reverse(arr, mask.length, buf.capacity()); - } - return new RegisterValue(reg, arr); - } - public static boolean isByteBound(Register register) { return register.getLeastSignificantBit() % 8 == 0 && register.getBitLength() % 8 == 0; }