[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Thank you @jasonmolenda.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG254a31273a27: [lldb][NFC] Use MCInstrAnalysis when available 
in the disassembler plugin (authored by RamNalamothu).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/unittests/Disassembler/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,17 @@
 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.
   ///
Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -129,17 +129,28 @@
   // 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());
   }
 }
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- /dev/null
+++ 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::In

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu created this revision.
Herald added subscribers: luismarques, s.egerton, PkmX, simoncook, arichardson.
Herald added a project: All.
RamNalamothu requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, wangpc.
Herald added projects: LLDB, LLVM.

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 feature
in lldb as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,19 @@
 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;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;
+return false;
+  }
+
   /// Returns true if at least one of the register writes performed by
   /// \param Inst implicitly clears the upper portion of all super-registers.
   ///
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ 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 @@
std::unique_ptr &&asm_info_up,
std::unique_ptr &&context_up,
std::unique_ptr &&disasm_up,
-   std::unique_ptr &&instr_printer_up);
+   std::unique_ptr &&instr_printer_up,
+   std::unique_ptr &&instr_analysis_up);
 
   std::unique_ptr m_instr_info_up;
   std::unique_ptr m_reg_info_up;
@@ -84,6 +86,7 @@
   std::unique_ptr m_context_up;
   std::unique_ptr m_disasm_up;
   std::unique_ptr m_instr_printer_up;
+  std::unique_ptr m_instr_analysis_up;
 };
 
 namespace x86 {
@@ -1287,11 +1290,15 @@
   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 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 @@
 std::unique_ptr &&asm_info_up,
 std::unique_ptr &&context_up,
 std::unique_ptr &&disasm_up,
-std::unique_ptr &&instr_printer_up)
+std::unique_ptr &&instr_printer_up,
+std::unique_ptr &&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_

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 543406.
RamNalamothu added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,19 @@
 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;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;
+return false;
+  }
+
   /// Returns true if at least one of the register writes performed by
   /// \param Inst implicitly clears the upper portion of all super-registers.
   ///
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ 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 @@
std::unique_ptr &&asm_info_up,
std::unique_ptr &&context_up,
std::unique_ptr &&disasm_up,
-   std::unique_ptr &&instr_printer_up);
+   std::unique_ptr &&instr_printer_up,
+   std::unique_ptr &&instr_analysis_up);
 
   std::unique_ptr m_instr_info_up;
   std::unique_ptr m_reg_info_up;
@@ -84,6 +86,7 @@
   std::unique_ptr m_context_up;
   std::unique_ptr m_disasm_up;
   std::unique_ptr m_instr_printer_up;
+  std::unique_ptr m_instr_analysis_up;
 };
 
 namespace x86 {
@@ -1287,11 +1290,15 @@
   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 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 @@
 std::unique_ptr &&asm_info_up,
 std::unique_ptr &&context_up,
 std::unique_ptr &&disasm_up,
-std::unique_ptr &&instr_printer_up)
+std::unique_ptr &&instr_printer_up,
+std::unique_ptr &&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 @@
 
 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::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-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

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.




Comment at: llvm/include/llvm/MC/MCInstrAnalysis.h:80
+  }
+
   /// Returns true if at least one of the register writes performed by

Does this have test coverage already?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu marked an inline comment as done.
RamNalamothu added inline comments.



Comment at: llvm/include/llvm/MC/MCInstrAnalysis.h:80
+  }
+
   /// Returns true if at least one of the register writes performed by

DavidSpickett wrote:
> Does this have test coverage already?
I think `llvm/tools/llvm-cfi-verify` has enough test coverage for this method.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 545753.
RamNalamothu added a comment.

Add lldb test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,19 @@
 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;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;
+return false;
+  }
+
   /// Returns true if at least one of the register writes performed by
   /// \param Inst implicitly clears the upper portion of all super-registers.
   ///
Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -140,6 +140,17 @@
   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());
 }
   }
 }
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ 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 @@
std::unique_ptr &&asm_info_up,
std::unique_ptr &&context_up,
std::unique_ptr &&disasm_up,
-   std::unique_ptr &&instr_printer_up);
+   std::unique_ptr &&instr_printer_up,
+   std::unique_ptr &&instr_analysis_up);
 
   std::unique_ptr m_instr_info_up;
   std::unique_ptr m_reg_info_up;
@@ -84,6 +86,7 @@
   std::unique_ptr m_context_up;
   std::unique_ptr m_disasm_up;
   std::unique_ptr m_instr_printer_up;
+  std::unique_ptr m_instr_analysis_up;
 };
 
 namespace x86 {
@@ -1287,11 +1290,15 @@
   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 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 @@
 std::unique_ptr &&asm_info_up,
 std::unique_ptr &&context_up,
 std::unique_ptr &&disasm_up,
-std::unique_ptr &&instr_printer_up)
+std::unique_ptr &&instr_printer_up,
+std::unique_ptr &&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_u

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 546173.
RamNalamothu added a comment.
Herald added subscribers: luke, frasercrmck, sameer.abuasal, Jim, jocewei, 
the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, 
niosHD, sabuasal, johnrusso, rbar.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/unittests/Disassembler/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,19 @@
 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;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;
+return false;
+  }
+
   /// Returns true if at least one of the register writes performed by
   /// \param Inst implicitly clears the upper portion of all super-registers.
   ///
Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -140,6 +140,17 @@
   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());
 }
   }
 }
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- /dev/null
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -0,0 +1,100 @@
+//===-- TextX86GetControlFlowKind.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, 0

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 546177.
RamNalamothu added a comment.

Fix RISC-V test name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/unittests/Disassembler/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,19 @@
 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;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;
+return false;
+  }
+
   /// Returns true if at least one of the register writes performed by
   /// \param Inst implicitly clears the upper portion of all super-registers.
   ///
Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -140,6 +140,17 @@
   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());
 }
   }
 }
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- /dev/null
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -0,0 +1,101 @@
+//===-- 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.
+

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 546297.
RamNalamothu added a comment.

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

Fixed the format issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/unittests/Disassembler/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,19 @@
 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;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;
+return false;
+  }
+
   /// Returns true if at least one of the register writes performed by
   /// \param Inst implicitly clears the upper portion of all super-registers.
   ///
Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -140,6 +140,17 @@
   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());
 }
   }
 }
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- /dev/null
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -0,0 +1,100 @@
+//===-- 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

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 546306.
RamNalamothu added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/unittests/Disassembler/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,19 @@
 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;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;
+return false;
+  }
+
   /// Returns true if at least one of the register writes performed by
   /// \param Inst implicitly clears the upper portion of all super-registers.
   ///
Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -140,6 +140,17 @@
   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());
 }
   }
 }
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- /dev/null
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -0,0 +1,89 @@
+//===-- 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 withou

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp:23
+
+class TestMCDisasmInstanceRISCV : public testing::Test {
+public:

Place all classes and test methods in an anonymous namespace.



Comment at: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp:64
+  // and we should skip these tests without marking anything as failing.
+  if (disass_sp) {
+const InstructionList inst_list (disass_sp->GetInstructionList());

Early return



Comment at: llvm/include/llvm/MC/MCInstrAnalysis.h:76
+  return false;
+if (Info->get(Inst.getOpcode()).hasDefOfPhysReg(Inst, PC, MCRI))
+  return true;

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 546688.
RamNalamothu added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/unittests/Disassembler/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/CMakeLists.txt
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
  llvm/include/llvm/MC/MCInstrAnalysis.h

Index: llvm/include/llvm/MC/MCInstrAnalysis.h
===
--- llvm/include/llvm/MC/MCInstrAnalysis.h
+++ 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 
 #include 
 
@@ -64,6 +65,17 @@
 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.
   ///
Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -129,17 +129,28 @@
   // 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());
   }
 }
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- /dev/null
+++ 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();

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu marked 3 inline comments as done.
RamNalamothu added a comment.

Thanks for the comments @MaskRay.




Comment at: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp:23
+
+class TestMCDisasmInstanceRISCV : public testing::Test {
+public:

MaskRay wrote:
> Place all classes and test methods in an anonymous namespace.
All the disassembler tests need this change. Will address that in a separate 
patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D156086: [lldb][NFC] Use MCInstrAnalysis when available in the disassembler plugin

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu marked an inline comment as done.
RamNalamothu added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156086/new/

https://reviews.llvm.org/D156086

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits