From 7743993a08c2c3394a24f2fe2795fadeb0843d4b Mon Sep 17 00:00:00 2001 From: Jonah Graham Date: Tue, 18 May 2021 13:36:12 -0400 Subject: [PATCH] Bug 572880: Keep internal representation of opcode as a String We receive a string from GDB and then display the same string to the user. So instead of converting it internally into a Byte[] to then convert it back into a String, keep it as a String. This fixes a regression where some GDBs' output format was not as space separated 8-bit-bytes, but as words or similar. Change-Id: I4ea241ff9ea45165489604fee26a3593ec3f6756 --- .../cdt/debug/dap/DapDisassemblyBackend.java | 7 +-- .../disassembly/dsf/DisassemblyPosition.java | 11 ++-- .../ui/disassembly/dsf/DisassemblyUtils.java | 39 -------------- .../disassembly/dsf/IDisassemblyDocument.java | 17 +++++-- .../ui/disassembly/DisassemblyBackendDsf.java | 9 ++-- .../ui/disassembly/OpcodeRulerColumn.java | 21 ++------ .../model/DisassemblyDocument.java | 51 ++++++++++--------- .../model/DisassemblyWithSourcePosition.java | 6 +-- 8 files changed, 58 insertions(+), 103 deletions(-) diff --git a/debug/org.eclipse.cdt.debug.dap/src/org/eclipse/cdt/debug/dap/DapDisassemblyBackend.java b/debug/org.eclipse.cdt.debug.dap/src/org/eclipse/cdt/debug/dap/DapDisassemblyBackend.java index 5a626e710bb..a05f1f560a1 100644 --- a/debug/org.eclipse.cdt.debug.dap/src/org/eclipse/cdt/debug/dap/DapDisassemblyBackend.java +++ b/debug/org.eclipse.cdt.debug.dap/src/org/eclipse/cdt/debug/dap/DapDisassemblyBackend.java @@ -258,13 +258,8 @@ public class DapDisassemblyBackend extends AbstractDisassemblyBackend { funcOffset = ""; //$NON-NLS-1$ } - Byte[] opcode = null; - if (instruction.getInstructionBytes() != null) { - opcode = DisassemblyUtils.decodeOpcode(instruction.getInstructionBytes()); - } - p = fCallback.getDocument().insertDisassemblyLine(p, address, instrLength.intValue(), funcOffset, - opcode, instruction.getInstruction(), file, lineNumber); + instruction.getInstructionBytes(), instruction.getInstruction(), file, lineNumber); if (p == null) { break; } diff --git a/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/DisassemblyPosition.java b/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/DisassemblyPosition.java index b606657dcc2..369bede7956 100644 --- a/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/DisassemblyPosition.java +++ b/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/DisassemblyPosition.java @@ -21,7 +21,10 @@ import java.math.BigInteger; public class DisassemblyPosition extends AddressRangePosition { public char[] fFunction; - public Byte[] fOpcode; + /** + * String of opcodes as it will be displayed to users. Can be null which is handled the same as empty string. + */ + public String fRawOpcode; /** * @param offset @@ -29,12 +32,12 @@ public class DisassemblyPosition extends AddressRangePosition { * @param addressOffset * @param addressLength * @param functionOffset - * @param opcode + * @param rawOpcode String of opcodes as it will be displayed to users. Can be null which is handled the same as empty string. */ public DisassemblyPosition(int offset, int length, BigInteger addressOffset, BigInteger addressLength, - String functionOffset, Byte[] opcode) { + String functionOffset, String rawOpcode) { super(offset, length, addressOffset, addressLength); - fOpcode = opcode; + fRawOpcode = rawOpcode; fFunction = functionOffset.toCharArray(); } diff --git a/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/DisassemblyUtils.java b/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/DisassemblyUtils.java index 9faf64a51c1..bf1f3dd53db 100644 --- a/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/DisassemblyUtils.java +++ b/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/DisassemblyUtils.java @@ -15,8 +15,6 @@ package org.eclipse.cdt.debug.internal.ui.disassembly.dsf; import java.math.BigInteger; -import java.util.ArrayList; -import java.util.List; import org.eclipse.cdt.debug.ui.CDebugUIPlugin; import org.eclipse.core.runtime.Platform; @@ -70,41 +68,4 @@ public class DisassemblyUtils { } return new BigInteger(string); } - - /** - * Decode given string representation of a space separated hex encoded byte - * array - * - * @param value - * space separated hexadecimal byte array - * @return opcode bytes as Byte array, an empty array if the decoding failed - */ - public static Byte[] decodeOpcode(String value) { - List opcodeBytesList = new ArrayList<>(); - if (value == null || value.isBlank()) { - return new Byte[0]; - } - // Removing space separation and parse as bytes - for (String opcodeStringValue : value.split("\\s+")) { //$NON-NLS-1$ - if (opcodeStringValue.length() > 0) { - // Check that the opcode does not contain some invalid byte sequence - if (opcodeStringValue.length() > 2) { - return new Byte[0]; - } - byte byteValue = 0; - char charAtIndexZero = opcodeStringValue.charAt(0); - char charAtIndexOne = opcodeStringValue.length() > 1 ? opcodeStringValue.charAt(1) : 0; - - int digitZero = Character.digit(charAtIndexZero, 16); - int digitOne = Character.digit(charAtIndexOne, 16); - // Check if characters are valid hex numbers - if (digitZero == -1 || digitOne == -1) { - return new Byte[0]; - } - byteValue = (byte) ((digitZero << 4) + digitOne); - opcodeBytesList.add(Byte.valueOf(byteValue)); - } - } - return opcodeBytesList.toArray(new Byte[0]); - } } diff --git a/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/IDisassemblyDocument.java b/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/IDisassemblyDocument.java index c46791653d2..0e2488d5700 100644 --- a/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/IDisassemblyDocument.java +++ b/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/disassembly/dsf/IDisassemblyDocument.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2010, 2012 Wind River Systems, Inc. and others. + * Copyright (c) 2010, 2021 Wind River Systems, Inc. and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -20,8 +20,9 @@ import java.math.BigInteger; import org.eclipse.jface.text.BadLocationException; /** - * DSF Disassembly view backends (CDI and DSF) need this limited access to the - * editor/view Document. + * Disassembly view backends need this limited access to the + * editor/view Document. The known backends are DSF, TCF and Dap. Formerly + * the CDI backend used it before it was removed from the CDT soure tree. */ public interface IDisassemblyDocument { @@ -34,10 +35,20 @@ public interface IDisassemblyDocument { String functionOffset, String instruction, String compilationPath, int lineNumber) throws BadLocationException; + /** + * This method. that takes opcode as a Byte[] exists solely for TCF integration. + */ AddressRangePosition insertDisassemblyLine(AddressRangePosition p, BigInteger address, int length, String functionOffset, Byte[] opcode, String instruction, String compilationPath, int lineNumber) throws BadLocationException; + /** + * @param rawOpcode String of opcodes as it will be displayed to users. Can be null which is handled the same as empty string. + */ + AddressRangePosition insertDisassemblyLine(AddressRangePosition p, BigInteger address, int length, + String functionOffset, String rawOpcode, String instruction, String compilationPath, int lineNumber) + throws BadLocationException; + AddressRangePosition getDisassemblyPosition(BigInteger address); BigInteger getAddressOfLine(int line); diff --git a/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyBackendDsf.java b/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyBackendDsf.java index 58c6dad413e..fd877d30db0 100644 --- a/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyBackendDsf.java +++ b/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyBackendDsf.java @@ -739,10 +739,10 @@ public class DisassemblyBackendDsf extends AbstractDisassemblyBackend implements functionOffset = ""; //$NON-NLS-1$ } - Byte[] opcode = {}; + String opcode = null; // Get raw Opcodes if available if (instruction instanceof IInstructionWithRawOpcode) { - opcode = DisassemblyUtils.decodeOpcode(((IInstructionWithRawOpcode) instruction).getRawOpcode()); + opcode = ((IInstructionWithRawOpcode) instruction).getRawOpcode(); } p = fCallback.getDocument().insertDisassemblyLine(p, address, instrLength.intValue(), functionOffset, @@ -895,10 +895,9 @@ public class DisassemblyBackendDsf extends AbstractDisassemblyBackend implements funcOffset = ""; //$NON-NLS-1$ } - Byte[] opcode = {}; + String opcode = null; if (instruction instanceof IInstructionWithRawOpcode) { - opcode = DisassemblyUtils - .decodeOpcode(((IInstructionWithRawOpcode) instruction).getRawOpcode()); + opcode = ((IInstructionWithRawOpcode) instruction).getRawOpcode(); } p = fCallback.getDocument().insertDisassemblyLine(p, address, instrLength.intValue(), funcOffset, diff --git a/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/OpcodeRulerColumn.java b/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/OpcodeRulerColumn.java index fcf1025c895..8652900b26a 100644 --- a/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/OpcodeRulerColumn.java +++ b/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/OpcodeRulerColumn.java @@ -13,8 +13,6 @@ *******************************************************************************/ package org.eclipse.cdt.dsf.debug.internal.ui.disassembly; -import java.util.StringJoiner; - import org.eclipse.cdt.debug.internal.ui.disassembly.dsf.AddressRangePosition; import org.eclipse.cdt.debug.internal.ui.disassembly.dsf.DisassemblyPosition; import org.eclipse.cdt.dsf.debug.internal.ui.disassembly.model.DisassemblyDocument; @@ -31,7 +29,7 @@ public class OpcodeRulerColumn extends DisassemblyRulerColumn { public static final String ID = "org.eclipse.cdt.dsf.ui.disassemblyColumn.opcode"; //$NON-NLS-1$ /** Maximum width of column (in characters) */ - /** 15 bytes plus separator */ + /** 15 bytes plus separator for typical 8-bit instruction words */ private static final int MAXWIDTH = 44; /** @@ -56,9 +54,8 @@ public class OpcodeRulerColumn extends DisassemblyRulerColumn { AddressRangePosition pos = doc.getDisassemblyPosition(offset); if (pos instanceof DisassemblyPosition && pos.length > 0 && pos.offset == offset && pos.fValid) { DisassemblyPosition disassPos = (DisassemblyPosition) pos; - if (disassPos.fOpcode != null) { - // Format the output. - return getOpcodeString(disassPos.fOpcode); + if (disassPos.fRawOpcode != null) { + return disassPos.fRawOpcode; } } else if (pos != null && !pos.fValid) { return DOTS.substring(0, nChars); @@ -70,18 +67,6 @@ public class OpcodeRulerColumn extends DisassemblyRulerColumn { return ""; //$NON-NLS-1$ } - protected String getOpcodeString(Byte[] opcode) { - if (opcode.length == 0) { - return "??"; //$NON-NLS-1$ - } - StringJoiner opcodeStringJoiner = new StringJoiner(" "); //$NON-NLS-1$ - for (int i = 0; i < opcode.length; i++) { - opcodeStringJoiner.add(String.format("%02x", //$NON-NLS-1$ - opcode[i].intValue() & 0xff)); - } - return opcodeStringJoiner.toString(); - } - @Override protected int computeNumberOfCharacters() { DisassemblyDocument doc = (DisassemblyDocument) getParentRuler().getTextViewer().getDocument(); diff --git a/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/model/DisassemblyDocument.java b/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/model/DisassemblyDocument.java index 5614ab7e27e..157fa4666e4 100644 --- a/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/model/DisassemblyDocument.java +++ b/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/model/DisassemblyDocument.java @@ -24,6 +24,7 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.StringJoiner; import org.eclipse.cdt.debug.internal.ui.disassembly.dsf.AddressRangePosition; import org.eclipse.cdt.debug.internal.ui.disassembly.dsf.DisassemblyPosition; @@ -73,7 +74,7 @@ public class DisassemblyDocument extends REDDocument implements IDisassemblyDocu private final Map fFileInfoMap = new HashMap<>(); private int fMaxFunctionLength = 0; - /** Max opcode length is equal to the single bytes shown */ + /** Max opcode length is the maximum string length of the opcode strings */ private int fMaxOpcodeLength = 0; private boolean fShowAddresses = false; @@ -156,19 +157,7 @@ public class DisassemblyDocument extends REDDocument implements IDisassemblyDocu } public int getMaxOpcodeLength() { - // Check if an error on opcode decode occurred, in - // this case we show ?? - if (fMaxOpcodeLength == 0) { - return 2; - } - int retVal = fMaxOpcodeLength; - // We show the bytes in hex, therefore it needs to be multiplied by 2 - retVal *= 2; - // add bytes separator length - if (retVal > 0) { - retVal += fMaxOpcodeLength - 1; - } - return retVal; + return fMaxOpcodeLength; } public int getAddressLength() { @@ -704,8 +693,8 @@ public class DisassemblyDocument extends REDDocument implements IDisassemblyDocu if (functionLength > fMaxFunctionLength) { fMaxFunctionLength = functionLength; } - if (disassPos.fOpcode != null) { - int opcodeLength = disassPos.fOpcode.length; + if (disassPos.fRawOpcode != null) { + int opcodeLength = disassPos.fRawOpcode.length(); if (opcodeLength > fMaxOpcodeLength) { fMaxOpcodeLength = opcodeLength; } @@ -1008,22 +997,33 @@ public class DisassemblyDocument extends REDDocument implements IDisassemblyDocu return pos; } - /* (non-Javadoc) - * @see org.eclipse.cdt.debug.internal.ui.disassembly.dsf.IDisassemblyDocument#insertDisassemblyLine(org.eclipse.cdt.debug.internal.ui.disassembly.dsf.AddressRangePosition, java.math.BigInteger, int, java.lang.String, java.lang.String, java.lang.String, int) - */ @Override public AddressRangePosition insertDisassemblyLine(AddressRangePosition pos, BigInteger address, int length, String functionOffset, String instruction, String file, int lineNr) throws BadLocationException { - return insertDisassemblyLine(pos, address, length, functionOffset, null, instruction, file, lineNr); + return insertDisassemblyLine(pos, address, length, functionOffset, (String) null, instruction, file, lineNr); } - /* (non-Javadoc) - * @see org.eclipse.cdt.debug.internal.ui.disassembly.dsf.IDisassemblyDocument#insertDisassemblyLine(org.eclipse.cdt.debug.internal.ui.disassembly.dsf.AddressRangePosition, java.math.BigInteger, int, java.lang.String, java.lang.String, java.lang.String, int) - */ @Override public AddressRangePosition insertDisassemblyLine(AddressRangePosition pos, BigInteger address, int length, String functionOffset, Byte[] opcode, String instruction, String file, int lineNr) throws BadLocationException { + + String opcodeString = "??"; //$NON-NLS-1$ + if (opcode != null && opcode.length > 0) { + StringJoiner opcodeStringJoiner = new StringJoiner(" "); //$NON-NLS-1$ + for (int i = 0; i < opcode.length; i++) { + opcodeStringJoiner.add(String.format("%02x", //$NON-NLS-1$ + opcode[i].intValue() & 0xff)); + } + opcodeString = opcodeStringJoiner.toString(); + } + return insertDisassemblyLine(pos, address, length, functionOffset, opcodeString, instruction, file, lineNr); + } + + @Override + public AddressRangePosition insertDisassemblyLine(AddressRangePosition pos, BigInteger address, int length, + String functionOffset, String rawOpcode, String instruction, String file, int lineNr) + throws BadLocationException { assert isGuiThread(); String disassLine = null; if (instruction == null || instruction.length() == 0) { @@ -1034,14 +1034,15 @@ public class DisassemblyDocument extends REDDocument implements IDisassemblyDocu AddressRangePosition disassPos; if (lineNr < 0) { disassPos = new DisassemblyPosition(0, disassLine.length(), address, BigInteger.valueOf(length), - functionOffset, opcode); + functionOffset, rawOpcode); } else { disassPos = new DisassemblyWithSourcePosition(0, disassLine.length(), address, BigInteger.valueOf(length), - functionOffset, opcode, file, lineNr); + functionOffset, rawOpcode, file, lineNr); } pos = insertAddressRange(pos, disassPos, disassLine, true); addDisassemblyPosition(disassPos); return pos; + } /** diff --git a/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/model/DisassemblyWithSourcePosition.java b/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/model/DisassemblyWithSourcePosition.java index 8c9cc39b0ed..4423831e23d 100644 --- a/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/model/DisassemblyWithSourcePosition.java +++ b/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/model/DisassemblyWithSourcePosition.java @@ -31,11 +31,11 @@ public class DisassemblyWithSourcePosition extends DisassemblyPosition { * @param addressOffset * @param addressLength * @param functionOffset - * @param opcode + * @param rawOpcode String of opcodes as it will be displayed to users. Can be null which is handled the same as empty string. */ public DisassemblyWithSourcePosition(int offset, int length, BigInteger addressOffset, BigInteger addressLength, - String functionOffset, Byte[] opcode, String file, int lineNr) { - super(offset, length, addressOffset, addressLength, functionOffset, opcode); + String functionOffset, String rawOpcode, String file, int lineNr) { + super(offset, length, addressOffset, addressLength, functionOffset, rawOpcode); fFile = file; fLine = lineNr; }