Support disassembling RISC-V proprietary instructions #145793
Conversation
RISC-V supports proprietary extensions, where the TD files don't know about certain instructions, and the disassembler can't disassemble them. Internal users want to be able to disassemble these instructions. With llvm-objdump, the solution is to pipe the output of the disassembly through a filter program. This patch modifies LLDB's disassembly to look more like llvm-objdump's, and includes an example python script that adds a command "fdis" that will disassemble, then pipe the output through a specified filter program. This has been tested with crustfilt, a sample filter located at https://github.com/quic/crustfilt . Changes in this PR: - Decouple "can't disassemble" with "instruction size". DisassemblerLLVMC::MCDisasmInstance::GetMCInst now returns a bool for valid disassembly, and has the size as an out paramter. Use the size even if the disassembly is invalid. Disassemble if disassemby is valid. - Always print out the opcode when -b is specified. Previously it wouldn't print out the opcode if it couldn't disassemble. - Print out RISC-V opcodes the way llvm-objdump does. Add DumpRISCV method based on RISC-V pretty printer in llvm-objdump.cpp. - Print <unknown> for instructions that can't be disassembled, matching llvm-objdump, instead of printing nothing. - Update max riscv32 and riscv64 instruction size to 8. - Add example "fdis" command script. Change-Id: Ie5a359d9e87a12dde79a8b5c9c7a146440a550c5
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: None (tedwoodward) ChangesRISC-V supports proprietary extensions, where the TD files don't know about certain instructions, and the disassembler can't disassemble them. Internal users want to be able to disassemble these instructions in LLDB. With llvm-objdump, the solution is to pipe the output of the disassembly through a filter program. This patch modifies LLDB's disassembly to look more like llvm-objdump's, and includes an example python script that adds a command "fdis" that will disassemble, then pipe the output through a specified filter program. This has been tested with crustfilt, a sample filter located at https://github.com/quic/crustfilt . Changes in this PR:
Change-Id: Ie5a359d9e87a12dde79a8b5c9c7a146440a550c5 Full diff: https://github.com/llvm/llvm-project/pull/145793.diff 6 Files Affected:
diff --git a/lldb/examples/python/filter_disasm.py b/lldb/examples/python/filter_disasm.py
new file mode 100644
index 0000000000000..adb3455209055
--- /dev/null
+++ b/lldb/examples/python/filter_disasm.py
@@ -0,0 +1,87 @@
+"""
+Defines a command, fdis, that does filtered disassembly. The command does the
+lldb disassemble command with -b and any other arguments passed in, and
+pipes that through a provided filter program.
+
+The intention is to support disassembly of RISC-V proprietary instructions.
+This is handled with llvm-objdump by piping the output of llvm-objdump through
+a filter program. This script is intended to mimic that workflow.
+"""
+
+import lldb
+import subprocess
+
+filter_program = "crustfilt"
+
+def __lldb_init_module(debugger, dict):
+ debugger.HandleCommand(
+ 'command script add -f filter_disasm.fdis fdis')
+ print("Disassembly filter command (fdis) loaded")
+ print("Filter program set to %s" % filter_program)
+
+
+def fdis(debugger, args, result, dict):
+ """
+ Call the built in disassembler, then pass its output to a filter program
+ to add in disassembly for hidden opcodes.
+ Except for get and set, use the fdis command like the disassemble command.
+ By default, the filter program is crustfilt, from
+ https://github.com/quic/crustfilt . This can be changed by changing
+ the global variable filter_program.
+
+ Usage:
+ fdis [[get] [set <program>] [<disassembly options>]]
+
+ Choose one of the following:
+ get
+ Gets the current filter program
+
+ set <program>
+ Sets the current filter program. This can be an executable, which
+ will be found on PATH, or an absolute path.
+
+ <disassembly options>
+ If the first argument is not get or set, the args will be passed
+ to the disassemble command as is.
+
+ """
+
+ global filter_program
+ args_list = args.split(' ')
+ result.Clear()
+
+ if len(args_list) == 1 and args_list[0] == 'get':
+ result.PutCString(filter_program)
+ result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
+ return
+
+ if len(args_list) == 2 and args_list[0] == 'set':
+ filter_program = args_list[1]
+ result.PutCString("Filter program set to %s" % filter_program)
+ result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
+ return
+
+ res = lldb.SBCommandReturnObject()
+ debugger.GetCommandInterpreter().HandleCommand('disassemble -b ' + args, res)
+ if (len(res.GetError()) > 0):
+ result.SetError(res.GetError())
+ result.SetStatus(lldb.eReturnStatusFailed)
+ return
+ output = res.GetOutput()
+
+ try:
+ proc = subprocess.run([filter_program], capture_output=True, text=True, input=output)
+ except (subprocess.SubprocessError, OSError) as e:
+ result.PutCString("Error occurred. Original disassembly:\n\n" + output)
+ result.SetError(str(e))
+ result.SetStatus(lldb.eReturnStatusFailed)
+ return
+
+ print(proc.stderr)
+ if proc.stderr:
+ pass
+ #result.SetError(proc.stderr)
+ #result.SetStatus(lldb.eReturnStatusFailed)
+ else:
+ result.PutCString(proc.stdout)
+ result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
diff --git a/lldb/include/lldb/Core/Opcode.h b/lldb/include/lldb/Core/Opcode.h
index f72f2687b54fe..88ef17093d3f3 100644
--- a/lldb/include/lldb/Core/Opcode.h
+++ b/lldb/include/lldb/Core/Opcode.h
@@ -200,6 +200,7 @@ class Opcode {
}
int Dump(Stream *s, uint32_t min_byte_width);
+ int DumpRISCV(Stream *s, uint32_t min_byte_width);
const void *GetOpcodeBytes() const {
return ((m_type == Opcode::eTypeBytes) ? m_data.inst.bytes : nullptr);
diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp
index 833e327579a29..f95e446448036 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -658,8 +658,13 @@ void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
// the byte dump to be able to always show 15 bytes (3 chars each) plus a
// space
if (max_opcode_byte_size > 0)
- m_opcode.Dump(&ss, max_opcode_byte_size * 3 + 1);
- else
+ // make RISC-V opcode dump look like llvm-objdump
+ if (exe_ctx &&
+ exe_ctx->GetTargetSP()->GetArchitecture().GetTriple().isRISCV())
+ m_opcode.DumpRISCV(&ss, max_opcode_byte_size * 3 + 1);
+ else
+ m_opcode.Dump(&ss, max_opcode_byte_size * 3 + 1);
+ else
m_opcode.Dump(&ss, 15 * 3 + 1);
} else {
// Else, we have ARM or MIPS which can show up to a uint32_t 0x00000000
@@ -685,10 +690,13 @@ void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
}
}
const size_t opcode_pos = ss.GetSizeOfLastLine();
- const std::string &opcode_name =
+ std::string &opcode_name =
show_color ? m_markup_opcode_name : m_opcode_name;
const std::string &mnemonics = show_color ? m_markup_mnemonics : m_mnemonics;
+ if (opcode_name.empty())
+ opcode_name = "<unknown>";
+
// The default opcode size of 7 characters is plenty for most architectures
// but some like arm can pull out the occasional vqrshrun.s16. We won't get
// consistent column spacing in these cases, unfortunately. Also note that we
diff --git a/lldb/source/Core/Opcode.cpp b/lldb/source/Core/Opcode.cpp
index 3e30d98975d8a..dbcd18cc0d8d2 100644
--- a/lldb/source/Core/Opcode.cpp
+++ b/lldb/source/Core/Opcode.cpp
@@ -78,6 +78,44 @@ lldb::ByteOrder Opcode::GetDataByteOrder() const {
return eByteOrderInvalid;
}
+// make RISC-V byte dumps look like llvm-objdump, instead of just dumping bytes
+int Opcode::DumpRISCV(Stream *s, uint32_t min_byte_width) {
+ const uint32_t previous_bytes = s->GetWrittenBytes();
+ // if m_type is not bytes, call Dump
+ if (m_type != Opcode::eTypeBytes)
+ return Dump(s, min_byte_width);
+
+ // from RISCVPrettyPrinter in llvm-objdump.cpp
+ // if size % 4 == 0, print as 1 or 2 32 bit values (32 or 64 bit inst)
+ // else if size % 2 == 0, print as 1 or 3 16 bit values (16 or 48 bit inst)
+ // else fall back and print bytes
+ for (uint32_t i = 0; i < m_data.inst.length;) {
+ if (i > 0)
+ s->PutChar(' ');
+ if (!(m_data.inst.length % 4)) {
+ s->Printf("%2.2x%2.2x%2.2x%2.2x", m_data.inst.bytes[i + 3],
+ m_data.inst.bytes[i + 2],
+ m_data.inst.bytes[i + 1],
+ m_data.inst.bytes[i + 0]);
+ i += 4;
+ } else if (!(m_data.inst.length % 2)) {
+ s->Printf("%2.2x%2.2x", m_data.inst.bytes[i + 1],
+ m_data.inst.bytes[i + 0]);
+ i += 2;
+ } else {
+ s->Printf("%2.2x", m_data.inst.bytes[i]);
+ ++i;
+ }
+ }
+
+ uint32_t bytes_written_so_far = s->GetWrittenBytes() - previous_bytes;
+ // Add spaces to make sure bytes display comes out even in case opcodes aren't
+ // all the same size.
+ if (bytes_written_so_far < min_byte_width)
+ s->Printf("%*s", min_byte_width - bytes_written_so_far, "");
+ return s->GetWrittenBytes() - previous_bytes;
+}
+
uint32_t Opcode::GetData(DataExtractor &data) const {
uint32_t byte_size = GetByteSize();
uint8_t swap_buf[8];
diff --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
index ed6047f8f4ef3..eeb6020abd73a 100644
--- a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -61,6 +61,8 @@ class DisassemblerLLVMC::MCDisasmInstance {
uint64_t GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
lldb::addr_t pc, llvm::MCInst &mc_inst) const;
+ bool GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
+ lldb::addr_t pc, llvm::MCInst &mc_inst, size_t &size) const;
void PrintMCInst(llvm::MCInst &mc_inst, lldb::addr_t pc,
std::string &inst_string, std::string &comments_string);
void SetStyle(bool use_hex_immed, HexImmediateStyle hex_style);
@@ -524,11 +526,11 @@ class InstructionLLVMC : public lldb_private::Instruction {
const addr_t pc = m_address.GetFileAddress();
llvm::MCInst inst;
- const size_t inst_size =
- mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
- if (inst_size == 0)
- m_opcode.Clear();
- else {
+ size_t inst_size = 0;
+ m_is_valid = mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len,
+ pc, inst, inst_size);
+ m_opcode.Clear();
+ if (inst_size != 0) {
m_opcode.SetOpcodeBytes(opcode_data, inst_size);
m_is_valid = true;
}
@@ -604,10 +606,11 @@ class InstructionLLVMC : public lldb_private::Instruction {
const uint8_t *opcode_data = data.GetDataStart();
const size_t opcode_data_len = data.GetByteSize();
llvm::MCInst inst;
- size_t inst_size =
- mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
-
- if (inst_size > 0) {
+ size_t inst_size = 0;
+ bool valid = mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc,
+ inst, inst_size);
+
+ if (valid && inst_size > 0) {
mc_disasm_ptr->SetStyle(use_hex_immediates, hex_style);
const bool saved_use_color = mc_disasm_ptr->GetUseColor();
@@ -1206,9 +1209,10 @@ class InstructionLLVMC : public lldb_private::Instruction {
const uint8_t *opcode_data = data.GetDataStart();
const size_t opcode_data_len = data.GetByteSize();
llvm::MCInst inst;
- const size_t inst_size =
- mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc, inst);
- if (inst_size == 0)
+ size_t inst_size = 0;
+ const bool valid = mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len,
+ pc, inst, inst_size);
+ if (!valid)
return;
m_has_visited_instruction = true;
@@ -1337,19 +1341,18 @@ DisassemblerLLVMC::MCDisasmInstance::MCDisasmInstance(
m_asm_info_up && m_context_up && m_disasm_up && m_instr_printer_up);
}
-uint64_t DisassemblerLLVMC::MCDisasmInstance::GetMCInst(
+bool DisassemblerLLVMC::MCDisasmInstance::GetMCInst(
const uint8_t *opcode_data, size_t opcode_data_len, lldb::addr_t pc,
- llvm::MCInst &mc_inst) const {
+ llvm::MCInst &mc_inst, size_t &size) const {
llvm::ArrayRef<uint8_t> data(opcode_data, opcode_data_len);
llvm::MCDisassembler::DecodeStatus status;
- uint64_t new_inst_size;
- status = m_disasm_up->getInstruction(mc_inst, new_inst_size, data, pc,
+ status = m_disasm_up->getInstruction(mc_inst, size, data, pc,
llvm::nulls());
if (status == llvm::MCDisassembler::Success)
- return new_inst_size;
+ return true;
else
- return 0;
+ return false;
}
void DisassemblerLLVMC::MCDisasmInstance::PrintMCInst(
diff --git a/lldb/source/Utility/ArchSpec.cpp b/lldb/source/Utility/ArchSpec.cpp
index 70b9800f4dade..7c71aaae6bcf2 100644
--- a/lldb/source/Utility/ArchSpec.cpp
+++ b/lldb/source/Utility/ArchSpec.cpp
@@ -228,9 +228,9 @@ static const CoreDefinition g_core_definitions[] = {
{eByteOrderLittle, 4, 4, 4, llvm::Triple::hexagon,
ArchSpec::eCore_hexagon_hexagonv5, "hexagonv5"},
- {eByteOrderLittle, 4, 2, 4, llvm::Triple::riscv32, ArchSpec::eCore_riscv32,
+ {eByteOrderLittle, 4, 2, 8, llvm::Triple::riscv32, ArchSpec::eCore_riscv32,
"riscv32"},
- {eByteOrderLittle, 8, 2, 4, llvm::Triple::riscv64, ArchSpec::eCore_riscv64,
+ {eByteOrderLittle, 8, 2, 8, llvm::Triple::riscv64, ArchSpec::eCore_riscv64,
"riscv64"},
{eByteOrderLittle, 4, 4, 4, llvm::Triple::loongarch32,
|
You can test this locally with the following command:darker --check --diff -r HEAD~1...HEAD lldb/examples/python/filter_disasm.pyView the diff from darker here.--- filter_disasm.py 2025-06-25 21:39:02.000000 +0000
+++ filter_disasm.py 2025-06-25 21:54:28.640611 +0000
@@ -11,77 +11,79 @@
import lldb
import subprocess
filter_program = "crustfilt"
+
def __lldb_init_module(debugger, dict):
- debugger.HandleCommand(
- 'command script add -f filter_disasm.fdis fdis')
+ debugger.HandleCommand("command script add -f filter_disasm.fdis fdis")
print("Disassembly filter command (fdis) loaded")
print("Filter program set to %s" % filter_program)
def fdis(debugger, args, result, dict):
"""
- Call the built in disassembler, then pass its output to a filter program
- to add in disassembly for hidden opcodes.
- Except for get and set, use the fdis command like the disassemble command.
- By default, the filter program is crustfilt, from
- https://github.com/quic/crustfilt . This can be changed by changing
- the global variable filter_program.
+ Call the built in disassembler, then pass its output to a filter program
+ to add in disassembly for hidden opcodes.
+ Except for get and set, use the fdis command like the disassemble command.
+ By default, the filter program is crustfilt, from
+ https://github.com/quic/crustfilt . This can be changed by changing
+ the global variable filter_program.
- Usage:
- fdis [[get] [set <program>] [<disassembly options>]]
+ Usage:
+ fdis [[get] [set <program>] [<disassembly options>]]
- Choose one of the following:
- get
- Gets the current filter program
+ Choose one of the following:
+ get
+ Gets the current filter program
- set <program>
- Sets the current filter program. This can be an executable, which
- will be found on PATH, or an absolute path.
+ set <program>
+ Sets the current filter program. This can be an executable, which
+ will be found on PATH, or an absolute path.
- <disassembly options>
- If the first argument is not get or set, the args will be passed
- to the disassemble command as is.
+ <disassembly options>
+ If the first argument is not get or set, the args will be passed
+ to the disassemble command as is.
"""
global filter_program
- args_list = args.split(' ')
+ args_list = args.split(" ")
result.Clear()
- if len(args_list) == 1 and args_list[0] == 'get':
+ if len(args_list) == 1 and args_list[0] == "get":
result.PutCString(filter_program)
result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
return
- if len(args_list) == 2 and args_list[0] == 'set':
+ if len(args_list) == 2 and args_list[0] == "set":
filter_program = args_list[1]
result.PutCString("Filter program set to %s" % filter_program)
result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
return
res = lldb.SBCommandReturnObject()
- debugger.GetCommandInterpreter().HandleCommand('disassemble -b ' + args, res)
- if (len(res.GetError()) > 0):
+ debugger.GetCommandInterpreter().HandleCommand("disassemble -b " + args, res)
+ if len(res.GetError()) > 0:
result.SetError(res.GetError())
result.SetStatus(lldb.eReturnStatusFailed)
return
output = res.GetOutput()
try:
- proc = subprocess.run([filter_program], capture_output=True, text=True, input=output)
+ proc = subprocess.run(
+ [filter_program], capture_output=True, text=True, input=output
+ )
except (subprocess.SubprocessError, OSError) as e:
result.PutCString("Error occurred. Original disassembly:\n\n" + output)
result.SetError(str(e))
result.SetStatus(lldb.eReturnStatusFailed)
return
print(proc.stderr)
if proc.stderr:
pass
- #result.SetError(proc.stderr)
- #result.SetStatus(lldb.eReturnStatusFailed)
+ # result.SetError(proc.stderr)
+ # result.SetStatus(lldb.eReturnStatusFailed)
else:
result.PutCString(proc.stdout)
result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- lldb/include/lldb/Core/Opcode.h lldb/source/Core/Disassembler.cpp lldb/source/Core/Opcode.cpp lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp lldb/source/Utility/ArchSpec.cppView the diff from clang-format here.diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp
index f95e44644..e1ba74d22 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -664,7 +664,7 @@ void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
m_opcode.DumpRISCV(&ss, max_opcode_byte_size * 3 + 1);
else
m_opcode.Dump(&ss, max_opcode_byte_size * 3 + 1);
- else
+ else
m_opcode.Dump(&ss, 15 * 3 + 1);
} else {
// Else, we have ARM or MIPS which can show up to a uint32_t 0x00000000
@@ -690,8 +690,7 @@ void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
}
}
const size_t opcode_pos = ss.GetSizeOfLastLine();
- std::string &opcode_name =
- show_color ? m_markup_opcode_name : m_opcode_name;
+ std::string &opcode_name = show_color ? m_markup_opcode_name : m_opcode_name;
const std::string &mnemonics = show_color ? m_markup_mnemonics : m_mnemonics;
if (opcode_name.empty())
diff --git a/lldb/source/Core/Opcode.cpp b/lldb/source/Core/Opcode.cpp
index dbcd18cc0..a09db58bb 100644
--- a/lldb/source/Core/Opcode.cpp
+++ b/lldb/source/Core/Opcode.cpp
@@ -94,13 +94,12 @@ int Opcode::DumpRISCV(Stream *s, uint32_t min_byte_width) {
s->PutChar(' ');
if (!(m_data.inst.length % 4)) {
s->Printf("%2.2x%2.2x%2.2x%2.2x", m_data.inst.bytes[i + 3],
- m_data.inst.bytes[i + 2],
- m_data.inst.bytes[i + 1],
- m_data.inst.bytes[i + 0]);
+ m_data.inst.bytes[i + 2], m_data.inst.bytes[i + 1],
+ m_data.inst.bytes[i + 0]);
i += 4;
} else if (!(m_data.inst.length % 2)) {
s->Printf("%2.2x%2.2x", m_data.inst.bytes[i + 1],
- m_data.inst.bytes[i + 0]);
+ m_data.inst.bytes[i + 0]);
i += 2;
} else {
s->Printf("%2.2x", m_data.inst.bytes[i]);
diff --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
index eeb6020ab..ae780a0a5 100644
--- a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -608,8 +608,8 @@ public:
llvm::MCInst inst;
size_t inst_size = 0;
bool valid = mc_disasm_ptr->GetMCInst(opcode_data, opcode_data_len, pc,
- inst, inst_size);
-
+ inst, inst_size);
+
if (valid && inst_size > 0) {
mc_disasm_ptr->SetStyle(use_hex_immediates, hex_style);
@@ -1341,14 +1341,15 @@ DisassemblerLLVMC::MCDisasmInstance::MCDisasmInstance(
m_asm_info_up && m_context_up && m_disasm_up && m_instr_printer_up);
}
-bool DisassemblerLLVMC::MCDisasmInstance::GetMCInst(
- const uint8_t *opcode_data, size_t opcode_data_len, lldb::addr_t pc,
- llvm::MCInst &mc_inst, size_t &size) const {
+bool DisassemblerLLVMC::MCDisasmInstance::GetMCInst(const uint8_t *opcode_data,
+ size_t opcode_data_len,
+ lldb::addr_t pc,
+ llvm::MCInst &mc_inst,
+ size_t &size) const {
llvm::ArrayRef<uint8_t> data(opcode_data, opcode_data_len);
llvm::MCDisassembler::DecodeStatus status;
- status = m_disasm_up->getInstruction(mc_inst, size, data, pc,
- llvm::nulls());
+ status = m_disasm_up->getInstruction(mc_inst, size, data, pc, llvm::nulls());
if (status == llvm::MCDisassembler::Success)
return true;
else
|
|
Before this change, disassembly of the crustfilt test program looks like this: Note that the instruction at 0x3aa is an 8 byte instruction, but lldb's disassembler is incorrectly treating it as a 2 byte instruction, then incorrectly disassembling the following addresses as 2 byte instructions. After this change, disassembly looks like this: The instruction at 0x3aa is identified as a 64 bit instruction, the opcode is shown, and the instruction is marked as The output from fdis looks like this: The filter replaces the instructions with instructions that it can disassemble. |
|
Thanks @tedwoodward! |
There was a problem hiding this comment.
I see the idea but am a bit wary of making the output look like objdump as a general goal just because they are not used for the same thing, debuggers do sometimes have different goals. In this case I think it's OK because you're improving a part of the disassembly that wouldn't be useful anyway.
Is there a reason we couldn't handle this the same way for all targets?
(lldb) dis
test.o`main:
-> 0xaaaaaaaaa714 <+0>: adrp x0, 17
0xaaaaaaaaa718 <+4>: add x0, x0, #0x10 ; a
0xaaaaaaaaa71c <+8>: mov w1, #0x2 ; =2
0xaaaaaaaaa720 <+12>: str w1, [x0]
0xaaaaaaaaa724 <+16>: adrp x0, 17
0xaaaaaaaaa728 <+20>: add x0, x0, #0x10 ; a
0xaaaaaaaaa72c <+24>: ldr w0, [x0]
0xaaaaaaaaa730 <+28>: ret
(lldb) dis -b
test.o`main:
-> 0xaaaaaaaaa714 <+0>: 0xb0000080 adrp x0, 17
0xaaaaaaaaa718 <+4>: 0x91004000 add x0, x0, #0x10 ; a
0xaaaaaaaaa71c <+8>: 0x52800041 mov w1, #0x2 ; =2
0xaaaaaaaaa720 <+12>: 0xb9000001 str w1, [x0]
0xaaaaaaaaa724 <+16>: 0xb0000080 adrp x0, 17
0xaaaaaaaaa728 <+20>: 0x91004000 add x0, x0, #0x10 ; a
0xaaaaaaaaa72c <+24>: 0xb9400000 ldr w0, [x0]
0xaaaaaaaaa730 <+28>: 0xd65f03c0 ret
The missing part is knowing how to split up that encoding value isn't it. For AArch64 you'd just print it because we only have 32-bit, Intel you would roll dice to randomly decide what to do and RISC-V we have these 2/3 formats.
Arm M profile might benefit from it but I have never been asked to support such a thing. With the thumb 16 and 32-bit encodings plus the sort of custom instruction extension it has.
Because as usual, anything if certain architecture in generic code isn't great, but hiding that behind a virtual method is just the same thing with more steps.
So I'm not against this, and I agree that RISC-V is a special case in that it leans harder into custom instructions than other architecures. Arm M Profile has a sort of custom instruction thing, and you've been able to use coprocessor encodings for years on many architectures but no one bothered to add this to tools (which is not on you of course, that's their loss).
So this being RISC-V specific, kinda has to be. But it's one generic method away from being a generic "format in a way that's more useful", if we needed it to be in future.
I think you need to expand the comments in this formatting code to say both:
- This is mimicking the objdump format and -
- It is doing so because this is easier to read / substitute / whatever
Then I don't have to go look at what objdump does to figure out the goals of this code.
How standard are these ways of printing the byte encodings? Is it recommended / used by the spec, gnu objdump and llvm objdump? That's the ideal.
If it's non-standard my worry would be you doing this to fit one filtering application and another coming along and wanting it in a different format.
Also this needs tests.
- See if we have tests for
-b, if not, write some for something that is not RISC-V. Easy way to check is to mess up the formatting and see what fails. - Add some
-bfor RISC-V specifically, with all the different formats - Add tests for the filtering script that use another script that pretends to be a filtering program. As this script is less of a demo, more of a real feature for your customers at least.
| print("Filter program set to %s" % filter_program) | ||
|
|
||
|
|
||
| def fdis(debugger, args, result, dict): |
There was a problem hiding this comment.
Istr there is a way to add options for custom commands that show up in help like built in ones do.
There was a problem hiding this comment.
The triple-quote text at the start of the function definition does that.
(lldb) command script import /local/mnt/workspace/ted/my_upstream/llvm-project/l
ldb/examples/python/filter_disasm.py
Disassembly filter command (fdis) loaded
Filter program set to crustfilt
(lldb) help fdis
For more information run 'help fdis' Expects 'raw' input (see 'help
raw-input'.)
Syntax: fdis
Call the built in disassembler, then pass its output to a filter program
to add in disassembly for hidden opcodes.
Except for get and set, use the fdis command like the disassemble command.
By default, the filter program is crustfilt, from
https://github.com/quic/crustfilt . This can be changed by changing
the global variable filter_program.
Usage:
fdis [[get] [set <program>] [<disassembly options>]]
Choose one of the following:
get
Gets the current filter program
set <program>
Sets the current filter program. This can be an executable, which
will be found on PATH, or an absolute path.
<disassembly options>
If the first argument is not get or set, the args will be passed
to the disassemble command as is.
There was a problem hiding this comment.
Oh that's cool.
What I was thinking of means you don't have to format the usage though, it's generated by lldb. I'll have a look for it tomorrow.
| print(proc.stderr) | ||
| if proc.stderr: | ||
| pass | ||
| #result.SetError(proc.stderr) | ||
| #result.SetStatus(lldb.eReturnStatusFailed) |
There was a problem hiding this comment.
I presume this needs to be swapped. Put stderr in the error so that the user will see it.
There was a problem hiding this comment.
I'm not sure the presence of data on stderr means a failure. I'll look into error handling more.
|
Few more things:
|
|
@asb Just pinging you on the chance you have heard discussions about doing this with other tools. What approaches, if any, did they take? |
The llvm disassembler returns the size, and the lldb disassembler sets the Opcode (class instantiation) to a certain type - 8 bit, 16 bit, 16 bit thumb, 32 bit, 64 bit, bytes. For RISC-V, it's set to bytes, which means it prints out 1 byte at a time. I didn't want to add a type, because "bytes" works well for RISC-V, except when it comes to print them out. That is an option, though, if we didn't want to have special pretty printers for the "this is a weird thing" cases.
Sure, I'll do that.
For RISC-V, llvm-objdump mimics gnu objdump, so this change makes the RISC-V byte encodings match both.
That's one thing I'm trying to avoid - we don't want a filter app for objdump and another for the debugger. That's why I changed how the bytes are displayed, and changed "can't disassemble" from blank to
Will do. |
I think that's a good idea. Do you know how to add something to release notes? I've never done it upstream.
I don't know what people are doing with GDB. Possibly nothing yet, since this is pretty new. |
Great, that's standard enough.
Yeah that makes sense, the glue between the tool and the filter may change a bit but a portable filter you can take between tools and toolchains is nice. Which reminds me I didn't ask about the alternatives. I can think of a few, with their own positives and negatives:
Looking at it that way, I think another way to describe this proposal is: And that I definitely agree with. Then you can multiply those pros and cons a bunch if you are a vendor, to public or private customers. You have the effort of telling them no you have to use But now I'm making your argument for you I think. You get the idea. The only real "gotcha" question I can come up with is, wouldn't a user who is interested in custom instructions already have access to a compiler that produces them and that compiler is likely to be llvm, so is lldb from that llvm not within reach already? Educate me on that, I don't know how folks approach developing these extensions. Maybe code generation is the last thing they do, after they know it's worth pursuing. So you've got a lot of hand written assembler and
LLDB's release notes live in a section of LLVM's - https://github.com/llvm/llvm-project/blob/main/llvm/docs/ReleaseNotes.md#changes-to-lldb. |
|
@JDevlieghere / @jasonmolenda you have been in the RISC-V world lately, what do you think to this approach? |
|
Just doing some googling, mostly people talk about modifying the binutils in the gnu toolchain for custom instructions and rebuilding it. This refers to some XML file, but I can't tell if they're talking about an upstream patch, or their own product based on GDB:
Might be my search skills, but I only see references to XML for target descriptions in upstream GDB. Anyway, that's another approach that I guess would require each tool to add an option to load that file. Rather than your proposal which wraps the filter around the tool (kind of, the custom command is still inside of lldb from the user's perspective). You could build the filter options into the disassemble command, but that's more complicated and you can argue that this PR is simply making the output more useful. Which has the side effect of being suitable for filtering. In future someone could come along and propose changing the builtin command. |
This particular PR has to do with Qualcomm's push to be more open source. We don't want to provide custom toolchains for things with good upstream support. That's why we're pushing a bunch of our former downstream code upstream; Polly vectorizer changes, LLVM vectorizer changes, etc. It's made upstream clang performance much closer to downstream clang performance. We got the Xqci guys to open source their extension definition so we could upstream the TD files for it. But...not everyone wants to upstream their extensions, and some people don't want to build a custom toolchain for every little extension. So they decided to use the .insn assembler directive (supported for at least 5 years, according to a quick google search), and a filter program to pipe objdump output through.
Thank you! |
I built that crustfilt precisely because I did not have the ability to add to the compiler's disassembler especially on mass. The sideband decoder approach was much faster to support and was the only reason we were able to move most of our custom architecture to RISCV. It also still covers some of the custom DSP stuff we cannot share / wouldn't be generally useful for people. I'd also argue a lot of RTL development happens in walled gardens now which increases the difficult of building fundamental tools. A python script has very few dependencies that the RTL team likely already has. |
RISC-V supports proprietary extensions, where the TD files don't know about certain instructions, and the disassembler can't disassemble them. Internal users want to be able to disassemble these instructions in LLDB.
With llvm-objdump, the solution is to pipe the output of the disassembly through a filter program. This patch modifies LLDB's disassembly to look more like llvm-objdump's, and includes an example python script that adds a command "fdis" that will disassemble, then pipe the output through a specified filter program. This has been tested with crustfilt, a sample filter located at https://github.com/quic/crustfilt .
Changes in this PR:
Decouple "can't disassemble" with "instruction size". DisassemblerLLVMC::MCDisasmInstance::GetMCInst now returns a bool for valid disassembly, and has the size as an out paramter. Use the size even if the disassembly is invalid. Disassemble if disassemby is valid.
Always print out the opcode when -b is specified. Previously it wouldn't print out the opcode if it couldn't disassemble.
Print out RISC-V opcodes the way llvm-objdump does. Add DumpRISCV method based on RISC-V pretty printer in llvm-objdump.cpp.
Print for instructions that can't be disassembled, matching llvm-objdump, instead of printing nothing.
Update max riscv32 and riscv64 instruction size to 8.
Add example "fdis" command script.