[lldb] 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-07-03 20:26:29.000000 +0000
+++ filter_disasm.py 2025-07-03 20:29:44.925067 +0000
@@ -11,74 +11,82 @@
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, exe_ctx, 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, exe_ctx, res)
- if (len(res.GetError()) > 0):
+ debugger.GetCommandInterpreter().HandleCommand(
+ "disassemble -b " + args, exe_ctx, 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
if proc.returncode:
- result.PutCString("warning: {} returned non-zero value {}".format(filter_program, proc.returncode))
+ result.PutCString(
+ "warning: {} returned non-zero value {}".format(
+ filter_program, proc.returncode
+ )
+ )
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 5ee3fc628..7ca399199 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -665,7 +665,7 @@ void Instruction::Dump(lldb_private::Stream *s, uint32_t max_opcode_byte_size,
m_opcode.DumpRISCV(&ss, max_byte_width);
else
m_opcode.Dump(&ss, max_byte_width);
- 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
@@ -691,8 +691,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 17b4f2d30..c4c6f9f82 100644
--- a/lldb/source/Core/Opcode.cpp
+++ b/lldb/source/Core/Opcode.cpp
@@ -92,16 +92,16 @@ int Opcode::DumpRISCV(Stream *s, uint32_t min_byte_width) {
// if size % 4 == 0, print as 1 or 2 32 bit values (32 or 64 bit inst)
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 size % 2 == 0, print as 1 or 3 16 bit values (16 or 48 bit inst)
+ // else if size % 2 == 0, print as 1 or 3 16 bit values (16 or 48 bit
+ // inst)
} 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 fall back and print bytes
+ // else fall back and print bytes
} else {
s->Printf("%2.2x", m_data.inst.bytes[i]);
++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(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.
There was a problem hiding this comment.
It looks like python's subprocess.run returns a subprocess.CompletedProcess object which has a returncode ivar, and the usual convention in Unix is 0 means success. I wouldn't treat the presence of stderr as indicative of an error by itself.
|
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. |
|
To chime in as one of the devisors and proponents on the toolchain side of this approach:
Precisely we do have a lot of Qualcomm's approach here is twofold:
One approach is to have a downstream-only toolchain, which we control very carefully and which diverges slowly but inevitably from LLVM upstream. Upon realising that almost all of our custom extensions are only used in assembly, and we had no plans to implement code generation for most of the extensions we can't publicise, I realised that there was the ability for us to take a different approach. This approach keeps assembly and disassembly support for their proprietary custom extensions outside the toolchain itself. I describe exactly how to follow the approach here: https://github.com/quic/crustfilt/blob/main/docs/supporting-external-instructions-on-llvm.md but broadly the idea (stolen from Luke Wren's Hazard3 docs (Chapter 4)) is to use assembly macros to hide the On the disassembly side, I agree there are a few different ways this could be supported, a real plugin "I failed to disassemble, can you manage?" interface is one approach, but has a lot of drawbacks compared to just passing the output of objdump through another text-based tool, which is where This then gives us a few different advantages:
And that's how we get here: filtering the output of |
|
To also respond to something earlier in the thread, where there is a little complexity:
One "weird" bit of the approach is that we actually still rely on LLVM's MC-layer to understand the length of the instruction. RISC-V currently has only 2 ratified lengths (16 and 32-bit), but describes an encoding scheme for longer instructions which both GNU objdump and LLVM's MC-layer understand when disassembling. RISC-V does not, at the moment, have a maximum length of instruction, but our callback only implements the scheme up to 176-bit long instructions. On the assembler side, we can only assemble up to 64-bit instructions, so we ensure our teams keep to this lower limit. There are two relevant callbacks on MC's
|
Which is surprising to me, but if I looked at all the assembly only extensions for Arm I'd find the the same thing. I just rarely have to deal with those.
Yes I see how you'd get there. Even if we want to be super reductive, printing the bytes out at least means you can decode by hand. Which you can't do today. So I like the direction.
I should check how this ends up displaying in lldb. AArch64 unlikely to be a problem but Thumb is variable so I hope we at least show the bytes. @tedwoodward I think this change might be more clearly pitched as:
Which I think is a net positive. Still unsure about Arm M profile has https://developer.arm.com/Architectures/Arm%20Custom%20Instructions, I'll look into that and see if it would benefit from a change of output format too. Just in theory though, I don't think we shipped support for open source debuggers and I'm not about to implement it myself. |
| @@ -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 | |||
There was a problem hiding this comment.
If you say instead "Pretty print RISC-V opcodes in the same format that llvm-objdump uses." this hints at the motivation and what keywords to look for in llvm-objdump's source.
lldb/source/Core/Disassembler.cpp
Outdated
| // 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); |
There was a problem hiding this comment.
Pull the minimum size out into:
auto min_byte_width = max_opcode_byte_size * 3 + 1;
Then use it in both calls, so it's clear they use the same minimum.
CDE works by assigning one of the co-processors to be the encodings for the custom instructions and it has a few fixed formats you can use. It's not like RISC-V where the layout can be anything you want. Which means that the disassembler would probably print some form, with encoding, and that can already be used to filter if you wanted to. I also checked how AArch64 works: objdump shows the bytes: As does LLDB: So for AArch64 we happen to mostly match objdump's output, and it's useful enough for humans and scripts because it can only ever be 32-bit encodings. Also having looked at the pretty printer system in llvm-objdump, I think |
lldb/source/Core/Opcode.cpp
Outdated
| // 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 |
There was a problem hiding this comment.
I would put these comments next to the ifs they refer to, I have to read the code anyway so it makes more sense to me to have the commentary interleaved.
DavidSpickett
changed the title
|
Sorry for the late comment on this PR. I understand why the llvm lldb is built against may not know the encodings for proprietary instructions set extensions that are only implemented downstream. I didn't realize that the riscv instructions had a scheme for indicating their lengths, very convenient. I was disassembling a riscv binary that had a bexti instruction from the B extensions for bit manipulation, and until I enabled that feature in the disassembler, it simply stopped at that instruction. At the very least, using the encoded instruction size to move past an instruction like this would be a nice refinement when the cpu/features aren't exactly specified right. The main question I have about this approach is -- the goal here is to have lldb's disassembler output resemble llvm-objdump. Should this simply be an argument to the To be clear, what I'm asking is, should |
|
For that matter, given that you're already requiring the use of a python command (and if I'm honest, our disassembler's output is not the most helpful to people, I know many of us would like to dig in to this. Should it identify basic blocks? Should it have indications of the control flow graph for a function? Should it annotate registers with the types or variable names when we have debug info? All of this becomes more complicated if we're also outputting exactly what llvm-objdump outputs.) |
|
Note, mostly for me, who was curious how lldb prints bytes in -b mode: I know AArch64 -b disassembly will print 0xUInt32, Intel disassembly prints the instruction's array of bytes UInt8 (1-15 of them iirc), like but probably the best analogy today is arm+thumb code which prints as a mix of UInt16 and UInt32, and today disassemble -b prints riscv instructions in the intel style, as an array of bytes. Doesn't seem the ideal format given that we have a known size, today most often 16/32/64, and I guess 48 for funsies. |
Standard instructions are right now only 16/32, but custom instructions can be any multiple of 16. This was the change to llvm-objdump to group bytes like gnu objdump does: b27f86b which was only a bit over a year ago. |
It "doesn't". LLVM objdump implements the scheme described in the spec, but for >32-bit instructions, that scheme is not ratified so it could change in the future (the note about this is there but easy to miss). That said, Qualcomm's custom instructions have adopted the unratified scheme for 48-bit instructions.
IIRC, there's nothing to that provides the raw encoding bytes to pass off to another disassembler. Maybe I missed it, or it's not documented? That said, it would be great not to have to re-implement all of the disassemble command's argument handling, which looks fairly complex to me. |
Thanks again for the helpful pointers and explanations. I'm surprised they gave up so many bits to help us distinguish opcodes! But we really should be using this in lldb as long as they're there. I don't think there's any good reason to print riscv instructions like we do with the intel variable length instructions in lldb's disassembler. Printing as a UInt16, UInt32, UInt16 + UInt16 + UInt16, or UInt32 + UInt32 etc seems like a good change to make to lldb's disassembler style for riscv + I'm only skimming the patch to Opcode::Dump, but it looks like the existing code is relying on an enum when the Opcode object is constructed, and the riscv instructions are constructed with the I think changing the way we print the bytes for the riscv disassembler -b is a very reasonable change, instead of printing an array of UInt8's, and it doesn't seem like we should need to break this apart into a separate Opcode::DumpRISCV method? Adding |
|
So just to recap again as I think about this, the output style differences are:
The Opcode objects are created by None of the Opcode formats (eType8, eType16, eType16_2, eType32, eType64, eTypeBytes) format the opcode in the proposed UInt16/UInt32 combination being proposed for riscv's dump format today. Is the no-0x prefix a requirement for the augmentation tool? Or will it eat that and ignore it. To be honest, I don't think we should print the prefix for any of the targets - it's only helpful for people doing copy and paste into some tool. I've seen other tools which display the bytes of assembly in a function and I can't think of any cases where I've seen one include the prefix like this. It is a clear signal that these are the bytes in big endian order, not memory order on an LE target. Should we have an eType1632Tuples (just throwing a name out there) which will format the bytes in the UInt16/UInt32 combined format shown in this PR? Could print "xxxx", "xxxxyyyy", "xxxxyyyy zzzz", "xxxxxyyyy zzzzaaaa" for 16/32/48/64. |
I don't know if crustfilt requires no 0x, or if it will work with 0x. It's open source, and really an example, so it could be changed easily. The idea (as explained to me) is vendors would write their own filter program, and could use crustfilt as a guide.
We could do that. I didn't want to change the Opcode::Type from eTypeBytes in case it was used for something besides disassembly. |
LLDB uses the LLVM disassembler to determine the size of instructions and to do the actual disassembly. Currently, if the LLVM disassembler can't disassemble an instruction, LLDB will ignore the instruction size, assume the instruction size is the minimum size for that device, print no useful opcode, and print nothing for the instruction. This patch changes this behavior to separate the instruction size and "can't disassemble". If the LLVM disassembler knows the size, but can't dissasemble the instruction, LLDB will use that size. It will print out the opcode, and will print "<unknown>" for the instruction. This is much more useful to both a user and a script. The impetus behind this change is to clean up RISC-V disassembly when the LLVM disassembler doesn't understand all of the instructions. 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
|
@DavidSpickett @jasonmolenda I uploaded a new change. It has:
It also changed the commit message to emphasize changing disassemble -b format |
LLDB uses the LLVM disassembler to determine the size of instructions and to do the actual disassembly. Currently, if the LLVM disassembler can't disassemble an instruction, LLDB will ignore the instruction size, assume the instruction size is the minimum size for that device, print no useful opcode, and print nothing for the instruction. This patch changes this behavior to separate the instruction size and "can't disassemble". If the LLVM disassembler knows the size, but can't dissasemble the instruction, LLDB will use that size. It will print out the opcode, and will print "<unknown>" for the instruction. This is much more useful to both a user and a script. The impetus behind this change is to clean up RISC-V disassembly when the LLVM disassembler doesn't understand all of the instructions. 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
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.