[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

(cherry picked from commit 254a312)

Author: RamNalamothu

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();
 }
 

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()

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}
+  )

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());
+}

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());
   }
 }

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.
   ///