[lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[RamNalamothu]

Summary

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.

[RamNalamothu]

Refer to the commit which added symbolize operands feature in llvm-objdump.

[DavidSpickett]

What I understand is that MCInstrAnalysis defaults to using MCInstrDesc, however not all targets have MCInstrAnalysis at all. So we have this awkwardness here with the two pointers, which is fine.

Sounds good to me, I am just wondering about the changes over in llvm.

llvm/include/llvm/MC/MCInstrAnalysis.h

80

Does this have test coverage already?

[RamNalamothu]

llvm/include/llvm/MC/MCInstrAnalysis.h

80

I think llvm/tools/llvm-cfi-verify has enough test coverage for this method.

[jasonmolenda]

I'm not super familiar with the MCInst class from llvm, and hadn't heard of MCInstrAnalysis. I was looking through the llvm targets - are these MCInstrAnalysis primitives going to be implemented for all targets we support today? I see them defined for e.g. MIPS and RISCV, but I'm not sure AArch64 does. Does isBranch include other variants like isUnconditionalBranch? You check if we got an MCInstrAnalysis for this target - do we get none for a target like AArch64 and fall back to our current behavior?

[RamNalamothu]

In D156086#4529791, @jasonmolenda wrote:

I'm not super familiar with the MCInst class from llvm, and hadn't heard of MCInstrAnalysis. I was looking through the llvm targets - are these MCInstrAnalysis primitives going to be implemented for all targets we support today?

Except few, all the other targets implement MCInstrAnalysis.

I see them defined for e.g. MIPS and RISCV, but I'm not sure AArch64 does.

AArch64 does implement it.
AArch64
AMDGPU
PowerPC
X86

Does isBranch include other variants like isUnconditionalBranch?

No. They are implemented as separate methods. You can see that with a full context diff of MCInstrAnalysis.h changes in this revision or MCInstrAnalysis.h

You check if we got an MCInstrAnalysis for this target - do we get none for a target like AArch64 and fall back to our current behavior?

If the target doesn't implement MCInstrAnalysis or implements but doesn't override the methods, we fall back to the current behavior because the default implementation of MCInstrAnalysis methods forward the query to MCInstrDesc functions which the current behavior relies on.

[jasonmolenda]

In D156086#4530507, @RamNalamothu wrote:

In D156086#4529791, @jasonmolenda wrote:

Does isBranch include other variants like isUnconditionalBranch?

No. They are implemented as separate methods. You can see that with a full context diff of MCInstrAnalysis.h changes in this revision or MCInstrAnalysis.h

mayAffectControlFlow doesn't test for isUnconditionalBranch. Is that a problem? I didn't look through the different property check methods like this, but I happened to notice this one and see it wasn't detected in mayAffectControlFlow. Maybe I misunderstood something.

[MaskRay]

It seems that a lldb specific test is needed. Adding a new method to llvm/include/llvm/MC/MCInstrAnalysis.h is fine with me, though I haven't checked the semantics.

[RamNalamothu]

In D156086#4536992, @jasonmolenda wrote:

In D156086#4530507, @RamNalamothu wrote:

In D156086#4529791, @jasonmolenda wrote:

Does isBranch include other variants like isUnconditionalBranch?

No. They are implemented as separate methods. You can see that with a full context diff of MCInstrAnalysis.h changes in this revision or MCInstrAnalysis.h

mayAffectControlFlow doesn't test for isUnconditionalBranch. Is that a problem? I didn't look through the different property check methods like this, but I happened to notice this one and see it wasn't detected in mayAffectControlFlow. Maybe I misunderstood something.

The idea is MCInstrAnalysis's default implementation just replicates what MCInstrDesc does (MCInstrDesc::mayAffectControlFlow) and the individual targets can refine those methods as needed.

In D156086#4537284, @MaskRay wrote:

It seems that a lldb specific test is needed. Adding a new method to llvm/include/llvm/MC/MCInstrAnalysis.h is fine with me, though I haven't checked the semantics.

I will try to add a lldb specific test.

[RamNalamothu]

Add lldb test.

[RamNalamothu]

Added RISC-V specific test which overrides MCInstrAnalysis methods and uses the newly added code path.

Tests in lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp use MCInstrAnalysis default methods and need old behavior before this patch.
Tests in lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp use RISC-V overridden MCInstrAnalysis methods and need new behavior from this patch.

Does this look good now?

[RamNalamothu]

Fix RISC-V test name.

[RamNalamothu]

Oops. Didn't notice the changed formatting of the comment after git-clang-format.

Fixed the format issue.

[RamNalamothu]

Remove left overs of copy-paste. Apologies for so many updates.

[MaskRay]

lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp

24

Place all classes and test methods in an anonymous namespace.

65

Early return

llvm/include/llvm/MC/MCInstrAnalysis.h

76

return Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI)

[RamNalamothu]

Address review comments.

[RamNalamothu]

Thanks for the comments @MaskRay.

lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp

24

All the disassembler tests need this change. Will address that in a separate patch.

[RamNalamothu]

Ping.

[RamNalamothu]

I think I have addressed all the review feedback. Ok to land?

[jasonmolenda]

The lldb side looks good to me, and I think you've addressed the llvm parts.

[RamNalamothu]

Thank you @jasonmolenda.

If there are no further comments on llvm side, I will land this in a couple of days.

lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

 #include "llvm/ADT/StringExtras.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDisassembler/MCDisassembler.h"
 #include "llvm/MC/MCDisassembler/MCExternalSymbolizer.h"
 #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"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/TargetSelect.h"

 
 private:
   MCDisasmInstance(std::unique_ptr<llvm::MCInstrInfo> &&instr_info_up,
                    std::unique_ptr<llvm::MCRegisterInfo> &&reg_info_up,
                    std::unique_ptr<llvm::MCSubtargetInfo> &&subtarget_info_up,
                    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;
   std::unique_ptr<llvm::MCSubtargetInfo> m_subtarget_info_up;
   std::unique_ptr<llvm::MCAsmInfo> m_asm_info_up;
   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 {
 
 /// These are the three values deciding instruction control flow kind.
 /// InstructionLengthDecode function decodes an instruction and get this struct.
 ///
 /// primary_opcode
DisassemblerLLVMC::MCDisasmInstance::Create(const char *triple, const char *cpu,
 
   std::unique_ptr<llvm::MCInstPrinter> instr_printer_up(
       curr_target->createMCInstPrinter(llvm::Triple{triple},
                                        asm_printer_variant, *asm_info_up,
                                        *instr_info_up, *reg_info_up));
   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(context_up), std::move(disasm_up), std::move(instr_printer_up),
+      std::move(instr_analysis_up)));
 }
 
 DisassemblerLLVMC::MCDisasmInstance::MCDisasmInstance(
     std::unique_ptr<llvm::MCInstrInfo> &&instr_info_up,
     std::unique_ptr<llvm::MCRegisterInfo> &&reg_info_up,
     std::unique_ptr<llvm::MCSubtargetInfo> &&subtarget_info_up,
     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);
 }
 
 uint64_t DisassemblerLLVMC::MCDisasmInstance::GetMCInst(
     const uint8_t *opcode_data, size_t opcode_data_len, lldb::addr_t pc,
     llvm::MCInst &mc_inst) const {
   llvm::ArrayRef<uint8_t> data(opcode_data, opcode_data_len);
void DisassemblerLLVMC::MCDisasmInstance::SetStyle(
   case eHexStyleAsm:
     m_instr_printer_up->setPrintHexStyle(llvm::HexStyle::Asm);
     break;
   }
 }
 
 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);
 }
 
 bool DisassemblerLLVMC::MCDisasmInstance::HasDelaySlot(
     llvm::MCInst &mc_inst) const {
   return m_instr_info_up->get(mc_inst.getOpcode()).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();
 }
 
 bool DisassemblerLLVMC::MCDisasmInstance::IsLoad(llvm::MCInst &mc_inst) const {
   return m_instr_info_up->get(mc_inst.getOpcode()).mayLoad();
 }
 
 bool DisassemblerLLVMC::MCDisasmInstance::IsAuthenticated(

lldb/unittests/Disassembler/CMakeLists.txt

 if("ARM" IN_LIST LLVM_TARGETS_TO_BUILD)
   add_subdirectory(ARM)
 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

+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

+//===-- 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

TEST_F(TestGetControlFlowKindx86, TestX86_64Instruction) {
   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 i386 target,
   // and we should skip these tests without marking anything as failing.
+  if (!disass_sp)
+    return;
 
-  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]);
-    }
+    ExecutionContext exe_ctx(nullptr, nullptr, nullptr);
+
+    // 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

 
 #ifndef LLVM_MC_MCINSTRANALYSIS_H
 #define LLVM_MC_MCINSTRANALYSIS_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrDesc.h"
 #include "llvm/MC/MCInstrInfo.h"
+#include "llvm/MC/MCRegisterInfo.h"
 #include <cstdint>
 #include <vector>
 
 namespace llvm {
 
 class MCRegisterInfo;
 class Triple;
 
public:
   virtual bool isReturn(const MCInst &Inst) const {
     return Info->get(Inst.getOpcode()).isReturn();
   }
 
   virtual bool isTerminator(const MCInst &Inst) const {
     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.
   ///
   /// Example: on X86-64, a write to EAX implicitly clears the upper half of
   /// RAX. Also (still on x86) an XMM write perfomed by an AVX 128-bit
   /// instruction implicitly clears the upper portion of the correspondent
   /// YMM register.
   ///
   /// This method also updates an APInt which is used as mask of register
   /// writes. There is one bit for every explicit/implicit write performed by