[Lldb-commits] [lldb] 254a312 - [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin
Venkata Ramanaiah Nalamothu via lldb-commits
lldb-commits at lists.llvm.org
Sun Aug 13 20:07:49 PDT 2023
Author: Venkata Ramanaiah Nalamothu
Date: 2023-08-14T08:37:41+05:30
New Revision: 254a31273a27728bb7937b27417eba083781f7eb
URL: https://github.com/llvm/llvm-project/commit/254a31273a27728bb7937b27417eba083781f7eb
DIFF: https://github.com/llvm/llvm-project/commit/254a31273a27728bb7937b27417eba083781f7eb.diff
LOG: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin
Since the info in MCInstrDesc is based on opcodes only, it is often quite
inaccurate. The MCInstrAnalysis has been added so that targets can provide
accurate info, which is based on registers used by the instruction, through
the own versions of MCInstrDesc functions.
The RISCVMCInstrAnalysis, which needs to refine several MCInstrDesc methods,
is a good example for this.
Given the llvm-objdump also uses MCInstrAnalysis, I think this change is in
the right direction.
The default implementation of MCInstrAnalysis methods forward the query to
MCInstrDesc functions. Hence, no functional change is intended/expected.
To avoid bloating up MCInstrAnalysis, only the methods provided by it and
the ones used by disassembler plugin are changed to use MCInstrAnalysis when
available.
Though I am not sure if it will be useful, making MCInstrAnalysis available
in the disassembler plugin would allow enabling symbolize operands (D84191)
feature in lldb's disassembler as well.
Reviewed By: jasonmolenda
Differential Revision: https://reviews.llvm.org/D156086
Added:
lldb/unittests/Disassembler/RISCV/CMakeLists.txt
lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
Modified:
lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
lldb/unittests/Disassembler/CMakeLists.txt
lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
llvm/include/llvm/MC/MCInstrAnalysis.h
Removed:
################################################################################
diff --git a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
index 09115cc670da78..b4fdcbd783570a 100644
--- a/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ b/lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -18,6 +18,7 @@
#include "llvm/MC/MCDisassembler/MCRelocationInfo.h"
#include "llvm/MC/MCInst.h"
#include "llvm/MC/MCInstPrinter.h"
+#include "llvm/MC/MCInstrAnalysis.h"
#include "llvm/MC/MCInstrInfo.h"
#include "llvm/MC/MCRegisterInfo.h"
#include "llvm/MC/MCSubtargetInfo.h"
@@ -75,7 +76,8 @@ class DisassemblerLLVMC::MCDisasmInstance {
std::unique_ptr<llvm::MCAsmInfo> &&asm_info_up,
std::unique_ptr<llvm::MCContext> &&context_up,
std::unique_ptr<llvm::MCDisassembler> &&disasm_up,
- std::unique_ptr<llvm::MCInstPrinter> &&instr_printer_up);
+ std::unique_ptr<llvm::MCInstPrinter> &&instr_printer_up,
+ std::unique_ptr<llvm::MCInstrAnalysis> &&instr_analysis_up);
std::unique_ptr<llvm::MCInstrInfo> m_instr_info_up;
std::unique_ptr<llvm::MCRegisterInfo> m_reg_info_up;
@@ -84,6 +86,7 @@ class DisassemblerLLVMC::MCDisasmInstance {
std::unique_ptr<llvm::MCContext> m_context_up;
std::unique_ptr<llvm::MCDisassembler> m_disasm_up;
std::unique_ptr<llvm::MCInstPrinter> m_instr_printer_up;
+ std::unique_ptr<llvm::MCInstrAnalysis> m_instr_analysis_up;
};
namespace x86 {
@@ -1287,11 +1290,15 @@ DisassemblerLLVMC::MCDisasmInstance::Create(const char *triple, const char *cpu,
if (!instr_printer_up)
return Instance();
- return Instance(
- new MCDisasmInstance(std::move(instr_info_up), std::move(reg_info_up),
- std::move(subtarget_info_up), std::move(asm_info_up),
- std::move(context_up), std::move(disasm_up),
- std::move(instr_printer_up)));
+ // Not all targets may have registered createMCInstrAnalysis().
+ std::unique_ptr<llvm::MCInstrAnalysis> instr_analysis_up(
+ curr_target->createMCInstrAnalysis(instr_info_up.get()));
+
+ return Instance(new MCDisasmInstance(
+ std::move(instr_info_up), std::move(reg_info_up),
+ std::move(subtarget_info_up), std::move(asm_info_up),
+ std::move(context_up), std::move(disasm_up), std::move(instr_printer_up),
+ std::move(instr_analysis_up)));
}
DisassemblerLLVMC::MCDisasmInstance::MCDisasmInstance(
@@ -1301,13 +1308,15 @@ DisassemblerLLVMC::MCDisasmInstance::MCDisasmInstance(
std::unique_ptr<llvm::MCAsmInfo> &&asm_info_up,
std::unique_ptr<llvm::MCContext> &&context_up,
std::unique_ptr<llvm::MCDisassembler> &&disasm_up,
- std::unique_ptr<llvm::MCInstPrinter> &&instr_printer_up)
+ std::unique_ptr<llvm::MCInstPrinter> &&instr_printer_up,
+ std::unique_ptr<llvm::MCInstrAnalysis> &&instr_analysis_up)
: m_instr_info_up(std::move(instr_info_up)),
m_reg_info_up(std::move(reg_info_up)),
m_subtarget_info_up(std::move(subtarget_info_up)),
m_asm_info_up(std::move(asm_info_up)),
m_context_up(std::move(context_up)), m_disasm_up(std::move(disasm_up)),
- m_instr_printer_up(std::move(instr_printer_up)) {
+ m_instr_printer_up(std::move(instr_printer_up)),
+ m_instr_analysis_up(std::move(instr_analysis_up)) {
assert(m_instr_info_up && m_reg_info_up && m_subtarget_info_up &&
m_asm_info_up && m_context_up && m_disasm_up && m_instr_printer_up);
}
@@ -1365,6 +1374,8 @@ void DisassemblerLLVMC::MCDisasmInstance::SetStyle(
bool DisassemblerLLVMC::MCDisasmInstance::CanBranch(
llvm::MCInst &mc_inst) const {
+ if (m_instr_analysis_up)
+ return m_instr_analysis_up->mayAffectControlFlow(mc_inst, *m_reg_info_up);
return m_instr_info_up->get(mc_inst.getOpcode())
.mayAffectControlFlow(mc_inst, *m_reg_info_up);
}
@@ -1375,6 +1386,8 @@ bool DisassemblerLLVMC::MCDisasmInstance::HasDelaySlot(
}
bool DisassemblerLLVMC::MCDisasmInstance::IsCall(llvm::MCInst &mc_inst) const {
+ if (m_instr_analysis_up)
+ return m_instr_analysis_up->isCall(mc_inst);
return m_instr_info_up->get(mc_inst.getOpcode()).isCall();
}
diff --git a/lldb/unittests/Disassembler/CMakeLists.txt b/lldb/unittests/Disassembler/CMakeLists.txt
index 39c43607752cc3..208f1807427f4e 100644
--- a/lldb/unittests/Disassembler/CMakeLists.txt
+++ b/lldb/unittests/Disassembler/CMakeLists.txt
@@ -5,3 +5,7 @@ endif()
if("X86" IN_LIST LLVM_TARGETS_TO_BUILD)
add_subdirectory(x86)
endif()
+
+if("RISCV" IN_LIST LLVM_TARGETS_TO_BUILD)
+ add_subdirectory(RISCV)
+endif()
diff --git a/lldb/unittests/Disassembler/RISCV/CMakeLists.txt b/lldb/unittests/Disassembler/RISCV/CMakeLists.txt
new file mode 100644
index 00000000000000..5bcc3e948335c6
--- /dev/null
+++ b/lldb/unittests/Disassembler/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_unittest(MCDisasmInstanceRISCVTests
+ TestMCDisasmInstanceRISCV.cpp
+ LINK_LIBS
+ lldbCore
+ lldbSymbol
+ lldbTarget
+ lldbPluginDisassemblerLLVMC
+ lldbPluginProcessUtility
+ LINK_COMPONENTS
+ Support
+ ${LLVM_TARGETS_TO_BUILD}
+ )
diff --git a/lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp b/lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
new file mode 100644
index 00000000000000..5534a66f6f3447
--- /dev/null
+++ b/lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -0,0 +1,90 @@
+//===-- TestMCDisasmInstanceRISCV.cpp -------------------------------------===//
+
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/TargetSelect.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Core/Address.h"
+#include "lldb/Core/Disassembler.h"
+#include "lldb/Target/ExecutionContext.h"
+#include "lldb/Utility/ArchSpec.h"
+
+#include "Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+class TestMCDisasmInstanceRISCV : public testing::Test {
+public:
+ static void SetUpTestCase();
+ static void TearDownTestCase();
+
+protected:
+};
+
+void TestMCDisasmInstanceRISCV::SetUpTestCase() {
+ llvm::InitializeAllTargets();
+ llvm::InitializeAllAsmPrinters();
+ llvm::InitializeAllTargetMCs();
+ llvm::InitializeAllDisassemblers();
+ DisassemblerLLVMC::Initialize();
+}
+
+void TestMCDisasmInstanceRISCV::TearDownTestCase() {
+ DisassemblerLLVMC::Terminate();
+}
+
+TEST_F(TestMCDisasmInstanceRISCV, TestRISCV32Instruction) {
+ ArchSpec arch("riscv32-*-linux");
+
+ const unsigned num_of_instructions = 5;
+ uint8_t data[] = {
+ 0xef, 0x00, 0x00, 0x00, // call -- jal x1, 0
+ 0xe7, 0x00, 0x00, 0x00, // call -- jalr x1, x0, 0
+ 0x6f, 0x00, 0x00, 0x00, // jump -- jal x0, 0
+ 0x67, 0x00, 0x00, 0x00, // jump -- jalr x0, x0, 0
+ 0x67, 0x80, 0x00, 0x00 // ret -- jalr x0, x1, 0
+ };
+
+ DisassemblerSP disass_sp;
+ Address start_addr(0x100);
+ disass_sp =
+ Disassembler::DisassembleBytes(arch, nullptr, nullptr, start_addr, &data,
+ sizeof (data), num_of_instructions, false);
+
+ // If we failed to get a disassembler, we can assume it is because
+ // the llvm we linked against was not built with the riscv target,
+ // and we should skip these tests without marking anything as failing.
+ if (!disass_sp)
+ return;
+
+ const InstructionList inst_list(disass_sp->GetInstructionList());
+ EXPECT_EQ(num_of_instructions, inst_list.GetSize());
+
+ InstructionSP inst_sp;
+ inst_sp = inst_list.GetInstructionAtIndex(0);
+ EXPECT_TRUE(inst_sp->IsCall());
+ EXPECT_TRUE(inst_sp->DoesBranch());
+
+ inst_sp = inst_list.GetInstructionAtIndex(1);
+ EXPECT_TRUE(inst_sp->IsCall());
+ EXPECT_TRUE(inst_sp->DoesBranch());
+
+ inst_sp = inst_list.GetInstructionAtIndex(2);
+ EXPECT_FALSE(inst_sp->IsCall());
+ EXPECT_TRUE(inst_sp->DoesBranch());
+
+ inst_sp = inst_list.GetInstructionAtIndex(3);
+ EXPECT_FALSE(inst_sp->IsCall());
+ EXPECT_TRUE(inst_sp->DoesBranch());
+
+ inst_sp = inst_list.GetInstructionAtIndex(4);
+ EXPECT_FALSE(inst_sp->IsCall());
+ EXPECT_TRUE(inst_sp->DoesBranch());
+}
diff --git a/lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp b/lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
index 9cce3e26554692..54433e6c8493da 100644
--- a/lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ b/lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -129,17 +129,28 @@ TEST_F(TestGetControlFlowKindx86, TestX86_64Instruction) {
// If we failed to get a disassembler, we can assume it is because
// the llvm we linked against was not built with the i386 target,
// and we should skip these tests without marking anything as failing.
-
- if (disass_sp) {
- const InstructionList inst_list(disass_sp->GetInstructionList());
- EXPECT_EQ(num_of_instructions, inst_list.GetSize());
-
- for (size_t i = 0; i < num_of_instructions; ++i) {
- InstructionSP inst_sp;
- inst_sp = inst_list.GetInstructionAtIndex(i);
- ExecutionContext exe_ctx (nullptr, nullptr, nullptr);
- InstructionControlFlowKind kind = inst_sp->GetControlFlowKind(&exe_ctx);
- EXPECT_EQ(kind, result[i]);
- }
+ if (!disass_sp)
+ return;
+
+ const InstructionList inst_list(disass_sp->GetInstructionList());
+ EXPECT_EQ(num_of_instructions, inst_list.GetSize());
+
+ for (size_t i = 0; i < num_of_instructions; ++i) {
+ InstructionSP inst_sp;
+ inst_sp = inst_list.GetInstructionAtIndex(i);
+ ExecutionContext exe_ctx(nullptr, nullptr, nullptr);
+ InstructionControlFlowKind kind = inst_sp->GetControlFlowKind(&exe_ctx);
+ EXPECT_EQ(kind, result[i]);
+
+ // Also, test the DisassemblerLLVMC::MCDisasmInstance methods.
+ if (kind == eInstructionControlFlowKindReturn)
+ EXPECT_FALSE(inst_sp->IsCall());
+ if (kind == eInstructionControlFlowKindCall)
+ EXPECT_TRUE(inst_sp->IsCall());
+ if (kind == eInstructionControlFlowKindCall ||
+ kind == eInstructionControlFlowKindJump ||
+ kind == eInstructionControlFlowKindCondJump ||
+ kind == eInstructionControlFlowKindReturn)
+ EXPECT_TRUE(inst_sp->DoesBranch());
}
}
diff --git a/llvm/include/llvm/MC/MCInstrAnalysis.h b/llvm/include/llvm/MC/MCInstrAnalysis.h
index aca0f4daeeee89..c3c675c39c5590 100644
--- a/llvm/include/llvm/MC/MCInstrAnalysis.h
+++ b/llvm/include/llvm/MC/MCInstrAnalysis.h
@@ -18,6 +18,7 @@
#include "llvm/MC/MCInst.h"
#include "llvm/MC/MCInstrDesc.h"
#include "llvm/MC/MCInstrInfo.h"
+#include "llvm/MC/MCRegisterInfo.h"
#include <cstdint>
#include <vector>
@@ -64,6 +65,17 @@ class MCInstrAnalysis {
return Info->get(Inst.getOpcode()).isTerminator();
}
+ virtual bool mayAffectControlFlow(const MCInst &Inst,
+ const MCRegisterInfo &MCRI) const {
+ if (isBranch(Inst) || isCall(Inst) || isReturn(Inst) ||
+ isIndirectBranch(Inst))
+ return true;
+ unsigned PC = MCRI.getProgramCounter();
+ if (PC == 0)
+ return false;
+ return Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI);
+ }
+
/// Returns true if at least one of the register writes performed by
/// \param Inst implicitly clears the upper portion of all super-registers.
///
More information about the lldb-commits
mailing list