[Lldb-commits] [PATCH] D50304: [lldb] CommandObjectThreadUntil should set breakpoint at either on exact or the nearest subsequent line number but not on all the subsequent line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

ramana-nvr added a comment.

I do not have the commit permission. Could someone help commit this patch?


https://reviews.llvm.org/D50304



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

[Lldb-commits] [PATCH] D50304: [lldb] CommandObjectThreadUntil should set breakpoint at either on exact or the nearest subsequent line number but not on all the subsequent line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

ramana-nvr created this revision.
ramana-nvr added a reviewer: jingham.

The requirements for "thread until " are:

a) If any code contributed by  or the nearest subsequent of  is executed before leaving the function, stop
b) If you end up leaving the function w/o triggering (a), then stop

In case of (a), since the  may have multiple entries in the line 
table and the compiler might have scheduled/moved the relevant code across, and 
the lldb does not know the control flow, set breakpoints on all the line table 
entries of best match of  i.e. exact or the nearest subsequent 
line.

Along with the above, currently, CommandObjectThreadUntil is also setting the 
breakpoints on all the subsequent line numbers after the best match and this 
latter part is wrong.

This issue is discussed at 
http://lists.llvm.org/pipermail/lldb-dev/2018-August/013979.html.


https://reviews.llvm.org/D50304

Files:
  source/Commands/CommandObjectThread.cpp


Index: source/Commands/CommandObjectThread.cpp
===
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -1227,11 +1227,26 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
&end_ptr);
 
+// Since not all source lines will contribute code, check if we are
+// setting the breakpoint on the exact line number or the nearest
+// subsequent line number and then set breakpoints at all the line
+// entries of the chosen line number (exact or nearest subsequent).
 for (uint32_t line_number : line_numbers) {
+  LineEntry line_entry;
+  bool exact = true;
   uint32_t start_idx_ptr = index_ptr;
+  start_idx_ptr = sc.comp_unit->FindLineEntry(
+  index_ptr, line_number, sc.comp_unit, exact, &line_entry);
+  if (start_idx_ptr == UINT32_MAX) {
+exact = false;
+start_idx_ptr = sc.comp_unit->FindLineEntry(
+index_ptr, line_number, sc.comp_unit, exact, &line_entry);
+if (start_idx_ptr != UINT32_MAX)
+  line_number = line_entry.line;
+  }
+  exact = true;
+  start_idx_ptr = index_ptr;
   while (start_idx_ptr <= end_ptr) {
-LineEntry line_entry;
-const bool exact = false;
 start_idx_ptr = sc.comp_unit->FindLineEntry(
 start_idx_ptr, line_number, sc.comp_unit, exact, &line_entry);
 if (start_idx_ptr == UINT32_MAX)


Index: source/Commands/CommandObjectThread.cpp
===
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -1227,11 +1227,26 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
&end_ptr);
 
+// Since not all source lines will contribute code, check if we are
+// setting the breakpoint on the exact line number or the nearest
+// subsequent line number and then set breakpoints at all the line
+// entries of the chosen line number (exact or nearest subsequent).
 for (uint32_t line_number : line_numbers) {
+  LineEntry line_entry;
+  bool exact = true;
   uint32_t start_idx_ptr = index_ptr;
+  start_idx_ptr = sc.comp_unit->FindLineEntry(
+  index_ptr, line_number, sc.comp_unit, exact, &line_entry);
+  if (start_idx_ptr == UINT32_MAX) {
+exact = false;
+start_idx_ptr = sc.comp_unit->FindLineEntry(
+index_ptr, line_number, sc.comp_unit, exact, &line_entry);
+if (start_idx_ptr != UINT32_MAX)
+  line_number = line_entry.line;
+  }
+  exact = true;
+  start_idx_ptr = index_ptr;
   while (start_idx_ptr <= end_ptr) {
-LineEntry line_entry;
-const bool exact = false;
 start_idx_ptr = sc.comp_unit->FindLineEntry(
 start_idx_ptr, line_number, sc.comp_unit, exact, &line_entry);
 if (start_idx_ptr == UINT32_MAX)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50304: [lldb] CommandObjectThreadUntil should set breakpoint at either on exact or the nearest subsequent line number but not on all the subsequent line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

ramana-nvr updated this revision to Diff 159517.
ramana-nvr added a comment.

Yes, updated the patch accordingly.


https://reviews.llvm.org/D50304

Files:
  source/Commands/CommandObjectThread.cpp


Index: source/Commands/CommandObjectThread.cpp
===
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -1227,11 +1227,21 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
&end_ptr);
 
+// Since not all source lines will contribute code, check if we are
+// setting the breakpoint on the exact line number or the nearest
+// subsequent line number and set breakpoints at all the line table
+// entries of the chosen line number (exact or nearest subsequent).
 for (uint32_t line_number : line_numbers) {
+  LineEntry line_entry;
+  bool exact = false;
   uint32_t start_idx_ptr = index_ptr;
+  start_idx_ptr = sc.comp_unit->FindLineEntry(
+  index_ptr, line_number, sc.comp_unit, exact, &line_entry);
+  if (start_idx_ptr != UINT32_MAX)
+line_number = line_entry.line;
+  exact = true;
+  start_idx_ptr = index_ptr;
   while (start_idx_ptr <= end_ptr) {
-LineEntry line_entry;
-const bool exact = false;
 start_idx_ptr = sc.comp_unit->FindLineEntry(
 start_idx_ptr, line_number, sc.comp_unit, exact, &line_entry);
 if (start_idx_ptr == UINT32_MAX)


Index: source/Commands/CommandObjectThread.cpp
===
--- source/Commands/CommandObjectThread.cpp
+++ source/Commands/CommandObjectThread.cpp
@@ -1227,11 +1227,21 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
&end_ptr);
 
+// Since not all source lines will contribute code, check if we are
+// setting the breakpoint on the exact line number or the nearest
+// subsequent line number and set breakpoints at all the line table
+// entries of the chosen line number (exact or nearest subsequent).
 for (uint32_t line_number : line_numbers) {
+  LineEntry line_entry;
+  bool exact = false;
   uint32_t start_idx_ptr = index_ptr;
+  start_idx_ptr = sc.comp_unit->FindLineEntry(
+  index_ptr, line_number, sc.comp_unit, exact, &line_entry);
+  if (start_idx_ptr != UINT32_MAX)
+line_number = line_entry.line;
+  exact = true;
+  start_idx_ptr = index_ptr;
   while (start_idx_ptr <= end_ptr) {
-LineEntry line_entry;
-const bool exact = false;
 start_idx_ptr = sc.comp_unit->FindLineEntry(
 start_idx_ptr, line_number, sc.comp_unit, exact, &line_entry);
 if (start_idx_ptr == UINT32_MAX)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48704: [LLDB] Fix for "Bug 37950: ExecutionContext::GetByteOrder() always returns endian::InlHostByteOrder()"

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

ramana-nvr added a comment.

Sorry, I don't have a test case for this and I am yet to understand lldb test 
suite to create one.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D48704



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://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 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

[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

[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

[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

[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

[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

[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] D158971: [lldb][NFC] Put disassembler test classes and methods in anonymous namespace

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu created this revision.
RamNalamothu added reviewers: DavidSpickett, MaskRay.
Herald added subscribers: luke, sunshaoce, frasercrmck, luismarques, apazos, 
sameer.abuasal, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, 
MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, niosHD, sabuasal, 
simoncook, johnrusso, rbar, asb.
Herald added a project: All.
RamNalamothu requested review of this revision.
Herald added subscribers: lldb-commits, wangpc.
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158971

Files:
  lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
  lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp


Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestGetControlFlowKindx86 : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestGetControlFlowKindx86::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestGetControlFlowKindx86, TestX86_64Instruction) {
   ArchSpec arch("x86_64-*-linux");
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestMCDisasmInstanceRISCV : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestMCDisasmInstanceRISCV::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestMCDisasmInstanceRISCV, TestRISCV32Instruction) {
   ArchSpec arch("riscv32-*-linux");
Index: lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
===
--- lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
+++ lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestArmv7Disassembly : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -42,6 +43,7 @@
 void TestArmv7Disassembly::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestArmv7Disassembly, TestCortexFPDisass) {
   ArchSpec arch("armv7em--");
Index: lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
===
--- lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
+++ lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestArm64Disassembly : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -42,6 +43,7 @@
 void TestArm64Disassembly::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestArm64Disassembly, TestArmv81Instruction) {
   ArchSpec arch("arm64-apple-ios");


Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestGetControlFlowKindx86 : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestGetControlFlowKindx86::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestGetControlFlowKindx86, TestX86_64Instruction) {
   ArchSpec arch("x86_64-*-linux");
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestMCDisasmInstanceRISCV : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestMCDisasmInstanceRISCV::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestMCDisasmInstanceRISCV, TestRISCV32Instruction) {
   ArchSpec arch("riscv32-*-linux");
Index: lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
===
--- lldb/unitt

[Lldb-commits] [PATCH] D158971: [lldb][NFC] Put disassembler test classes and methods in anonymous namespace

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

In D158971#4624001 , @DavidSpickett 
wrote:

> And this is being done for what reason?

This is a follow-up on https://reviews.llvm.org/D156086#inline-1518306 comment.
I see that most of the lldb unit tests follow this style.

> I seem to remember something about static and anonymous namespace in the 
> developer guide, please cite that if so.

Are you referring to 
https://llvm.org/docs/CodingStandards.html#anonymous-namespaces?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158971

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

[Lldb-commits] [PATCH] D158971: [lldb][NFC] Put disassembler test classes and methods in anonymous namespace

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

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

Update commit message to refer to anonymous namespace guideline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158971

Files:
  lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
  lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp


Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestGetControlFlowKindx86 : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestGetControlFlowKindx86::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestGetControlFlowKindx86, TestX86_64Instruction) {
   ArchSpec arch("x86_64-*-linux");
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestMCDisasmInstanceRISCV : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestMCDisasmInstanceRISCV::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestMCDisasmInstanceRISCV, TestRISCV32Instruction) {
   ArchSpec arch("riscv32-*-linux");
Index: lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
===
--- lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
+++ lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestArmv7Disassembly : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -42,6 +43,7 @@
 void TestArmv7Disassembly::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestArmv7Disassembly, TestCortexFPDisass) {
   ArchSpec arch("armv7em--");
Index: lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
===
--- lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
+++ lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestArm64Disassembly : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -42,6 +43,7 @@
 void TestArm64Disassembly::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestArm64Disassembly, TestArmv81Instruction) {
   ArchSpec arch("arm64-apple-ios");


Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestGetControlFlowKindx86 : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestGetControlFlowKindx86::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestGetControlFlowKindx86, TestX86_64Instruction) {
   ArchSpec arch("x86_64-*-linux");
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestMCDisasmInstanceRISCV : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestMCDisasmInstanceRISCV::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestMCDisasmInstanceRISCV, TestRISCV32Instruction) {
   ArchSpec arch("riscv32-*-linux");
Index: lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
===
--- lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
+++ lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestArmv7Disassembly : public testing::Test {
 public:
   static void SetUpTestC

[Lldb-commits] [PATCH] D158971: [lldb][NFC] Put disassembler test classes and methods in anonymous namespace

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdfa8a15d7886: [lldb][NFC] Put disassembler test classes and 
methods in anonymous namespace (authored by RamNalamothu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158971

Files:
  lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
  lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
  lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
  lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp


Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestGetControlFlowKindx86 : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestGetControlFlowKindx86::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestGetControlFlowKindx86, TestX86_64Instruction) {
   ArchSpec arch("x86_64-*-linux");
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestMCDisasmInstanceRISCV : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestMCDisasmInstanceRISCV::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestMCDisasmInstanceRISCV, TestRISCV32Instruction) {
   ArchSpec arch("riscv32-*-linux");
Index: lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
===
--- lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
+++ lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestArmv7Disassembly : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -42,6 +43,7 @@
 void TestArmv7Disassembly::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestArmv7Disassembly, TestCortexFPDisass) {
   ArchSpec arch("armv7em--");
Index: lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
===
--- lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
+++ lldb/unittests/Disassembler/ARM/TestArm64Disassembly.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestArm64Disassembly : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -42,6 +43,7 @@
 void TestArm64Disassembly::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestArm64Disassembly, TestArmv81Instruction) {
   ArchSpec arch("arm64-apple-ios");


Index: lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
===
--- lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
+++ lldb/unittests/Disassembler/x86/TestGetControlFlowKindx86.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestGetControlFlowKindx86 : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestGetControlFlowKindx86::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestGetControlFlowKindx86, TestX86_64Instruction) {
   ArchSpec arch("x86_64-*-linux");
Index: lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
===
--- lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
+++ lldb/unittests/Disassembler/RISCV/TestMCDisasmInstanceRISCV.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestMCDisasmInstanceRISCV : public testing::Test {
 public:
   static void SetUpTestCase();
@@ -39,6 +40,7 @@
 void TestMCDisasmInstanceRISCV::TearDownTestCase() {
   DisassemblerLLVMC::Terminate();
 }
+} // namespace
 
 TEST_F(TestMCDisasmInstanceRISCV, TestRISCV32Instruction) {
   ArchSpec arch("riscv32-*-linux");
Index: lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
===
--- lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
+++ lldb/unittests/Disassembler/ARM/TestArmv7Disassembly.cpp
@@ -20,6 +20,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
 class TestArmv7Disass

[Lldb-commits] [PATCH] D158971: [lldb][NFC] Put disassembler test classes and methods in anonymous namespace

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

In D158971#4633434 , @RamNalamothu 
wrote:

> Update commit message to refer to anonymous namespace guideline.

Ah, seems like I have messed up with an arcanist option and the commit message 
update didn't get picked here/in the actual commit. Apologies for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158971

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

@jingham

Sorry for spamming. But I am not sure if you have received my previous messages.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Thank you.

In D50304#3608740 , @jingham wrote:

> In D50304#3592849 , @RamNalamothu 
> wrote:
>
>> In D50304#3586710 , @jingham wrote:
>>
>>> For some reason I'm not getting mail notifications for review changes, 
>>> sorry about that.
>>>
>>> This is certainly better than the original implementation.  Among other 
>>> things, if we find an exact match, we really shouldn't be doing any more 
>>> inexact matches, so after the first exact hit it should have switched exact 
>>> to true.
>>>
>>> But if you had line tables laid out like (this is in increasing order of 
>>> address:
>>>
>>> 10
>>> 20
>>> 30
>>> 10
>>> 16
>>>
>>> and you did "thread until 15", this would find the inexact match at 20, 
>>> switch to an exact match for line 20 and find no other matches.  But the 
>>> gap between 10 & 16 in the line table is maybe an even more plausible place 
>>> to put the line 15 until breakpoint, so maybe we did want to throw a 
>>> breakpoint there as well?
>>
>> @jingham 
>> Nope, with the current patch, we would find the inexact match at 16.
>
> Ah, my bad.  FindLineEntry does end up in FindLineEntryByFileIndexImpl which 
> does the closest match.
>
>>> Regular breakpoint setting has to move inexact breakpoints in much the same 
>>> way.  The code in the BreakpointResolverFileLine::SearchCallback ends up 
>>> calling CompileUnit::ResolveSymbolContext to get the "best" inexact match.  
>>> Maybe it would be better to not do this by hand here in the Until command, 
>>> but reuse the code that we use to move break points more generally?
>>
>> Be it CompileUnit::ResolveSymbolContext or CompileUnit::FindLineEntry, we 
>> end up calling LineTable::FindLineEntryIndexByFileIndex which finds "Exact 
>> match always wins.  Otherwise try to find the closest line > the desired 
>> line." when we pass `exact = false` to it.
>> Given that and we anyway have to extract address list, from what we get out 
>> of CompileUnit::ResolveSymbolContext, for the thread until thread plan to 
>> work with, probably the additional overhead to use 
>> CompileUnit::ResolveSymbolContext here does not make sense. Or am I missing 
>> something?
>
> Finding these addresses happens once in response to a user command, so I 
> highly doubt that the overhead matters one way or the other.  Both end up 
> doing the closest match (I misremembered where that was done) which is the 
> main thing.  ResolveSymbolContext does more work to handle inlined files, but 
> there's no way to do "thread until" from a main CU into inlined source so 
> that doesn't matter.

Make sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa57b62deef37: [lldb] Fix thread step until to not set 
breakpoint(s) on incorrect line numbers (authored by RamNalamothu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

Files:
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/test/API/functionalities/thread/step_until/TestStepUntil.py


Index: lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
===
--- lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
+++ lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
@@ -19,7 +19,7 @@
 self.greater_than_two = line_number('main.c', 'Greater than or equal 
to 2.')
 self.back_out_in_main = line_number('main.c', 'Back out in main')
 
-def do_until (self, args, until_lines, expected_linenum):
+def common_setup (self, args):
 self.build()
 exe = self.getBuildArtifact("a.out")
 
@@ -48,7 +48,8 @@
 thread = threads[0]
 return thread
 
-thread = self.common_setup(None)
+def do_until (self, args, until_lines, expected_linenum):
+thread = self.common_setup(args)
 
 cmd_interp = self.dbg.GetCommandInterpreter()
 ret_obj = lldb.SBCommandReturnObject()
@@ -77,7 +78,7 @@
 self.do_until(None, [self.less_than_two, self.greater_than_two], 
self.less_than_two)
 
 def test_missing_one (self):
-"""Test thread step until - targeting one line and missing it."""
+"""Test thread step until - targeting one line and missing it by 
stepping out to call site"""
 self.do_until(["foo", "bar", "baz"], [self.less_than_two], 
self.back_out_in_main)
 
 
Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -1033,11 +1033,21 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
&end_ptr);
 
+// Since not all source lines will contribute code, check if we are
+// setting the breakpoint on the exact line number or the nearest
+// subsequent line number and set breakpoints at all the line table
+// entries of the chosen line number (exact or nearest subsequent).
 for (uint32_t line_number : line_numbers) {
+  LineEntry line_entry;
+  bool exact = false;
   uint32_t start_idx_ptr = index_ptr;
+  start_idx_ptr = sc.comp_unit->FindLineEntry(
+  index_ptr, line_number, nullptr, exact, &line_entry);
+  if (start_idx_ptr != UINT32_MAX)
+line_number = line_entry.line;
+  exact = true;
+  start_idx_ptr = index_ptr;
   while (start_idx_ptr <= end_ptr) {
-LineEntry line_entry;
-const bool exact = false;
 start_idx_ptr = sc.comp_unit->FindLineEntry(
 start_idx_ptr, line_number, nullptr, exact, &line_entry);
 if (start_idx_ptr == UINT32_MAX)


Index: lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
===
--- lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
+++ lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
@@ -19,7 +19,7 @@
 self.greater_than_two = line_number('main.c', 'Greater than or equal to 2.')
 self.back_out_in_main = line_number('main.c', 'Back out in main')
 
-def do_until (self, args, until_lines, expected_linenum):
+def common_setup (self, args):
 self.build()
 exe = self.getBuildArtifact("a.out")
 
@@ -48,7 +48,8 @@
 thread = threads[0]
 return thread
 
-thread = self.common_setup(None)
+def do_until (self, args, until_lines, expected_linenum):
+thread = self.common_setup(args)
 
 cmd_interp = self.dbg.GetCommandInterpreter()
 ret_obj = lldb.SBCommandReturnObject()
@@ -77,7 +78,7 @@
 self.do_until(None, [self.less_than_two, self.greater_than_two], self.less_than_two)
 
 def test_missing_one (self):
-"""Test thread step until - targeting one line and missing it."""
+"""Test thread step until - targeting one line and missing it by stepping out to call site"""
 self.do_until(["foo", "bar", "baz"], [self.less_than_two], self.back_out_in_main)
 
 
Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -1033,11 +1033,21 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
   

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

In D50304#3611390 , @goncharov wrote:

> FYI this caused a test breakage at lldb-aarch64-ubuntu, 
> https://lab.llvm.org/buildbot/#/builders/96/builds/25128. I have reverted 
> this in fe6db8d03ff16a65f57af24d2cb04f489e2e9b0c 
> 

Thank you.

Interestingly, the test is not failing for `x86-64` and `arm` but fails for 
only `aarch64`. I don't have an `aarch64' system and need to find an 
alternative way to get access to the system.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

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

Make the test case insensitive to compiler optimizations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

Files:
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
  lldb/test/API/functionalities/thread/step_until/main.c


Index: lldb/test/API/functionalities/thread/step_until/main.c
===
--- lldb/test/API/functionalities/thread/step_until/main.c
+++ lldb/test/API/functionalities/thread/step_until/main.c
@@ -1,20 +1,25 @@
 #include 
 
-void call_me(int argc)
-{
+/* The return statements are purposefully so simple, and
+ * unrelated to the program, just to achieve consistent
+ * debug line tables, across platforms, that are not
+ * dependent on compiler optimzations. */
+int call_me(int argc) {
   printf ("At the start, argc: %d.\n", argc);
 
   if (argc < 2)
-printf("Less than 2.\n");
+return 1; /* Less than 2. */
   else
-printf("Greater than or equal to 2.\n");
+return argc; /* Greater than or equal to 2. */
 }
 
 int
 main(int argc, char **argv)
 {
-  call_me(argc);
-  printf("Back out in main.\n");
+  int res = 0;
+  res = call_me(argc); /* Back out in main. */
+  if (res)
+printf("Result: %d. \n", res);
 
   return 0;
 }
Index: lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
===
--- lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
+++ lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
@@ -19,7 +19,7 @@
 self.greater_than_two = line_number('main.c', 'Greater than or equal 
to 2.')
 self.back_out_in_main = line_number('main.c', 'Back out in main')
 
-def do_until (self, args, until_lines, expected_linenum):
+def common_setup (self, args):
 self.build()
 exe = self.getBuildArtifact("a.out")
 
@@ -48,7 +48,8 @@
 thread = threads[0]
 return thread
 
-thread = self.common_setup(None)
+def do_until (self, args, until_lines, expected_linenum):
+thread = self.common_setup(args)
 
 cmd_interp = self.dbg.GetCommandInterpreter()
 ret_obj = lldb.SBCommandReturnObject()
@@ -77,7 +78,7 @@
 self.do_until(None, [self.less_than_two, self.greater_than_two], 
self.less_than_two)
 
 def test_missing_one (self):
-"""Test thread step until - targeting one line and missing it."""
+"""Test thread step until - targeting one line and missing it by 
stepping out to call site"""
 self.do_until(["foo", "bar", "baz"], [self.less_than_two], 
self.back_out_in_main)
 
 
Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -1033,11 +1033,21 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
&end_ptr);
 
+// Since not all source lines will contribute code, check if we are
+// setting the breakpoint on the exact line number or the nearest
+// subsequent line number and set breakpoints at all the line table
+// entries of the chosen line number (exact or nearest subsequent).
 for (uint32_t line_number : line_numbers) {
+  LineEntry line_entry;
+  bool exact = false;
   uint32_t start_idx_ptr = index_ptr;
+  start_idx_ptr = sc.comp_unit->FindLineEntry(
+  index_ptr, line_number, nullptr, exact, &line_entry);
+  if (start_idx_ptr != UINT32_MAX)
+line_number = line_entry.line;
+  exact = true;
+  start_idx_ptr = index_ptr;
   while (start_idx_ptr <= end_ptr) {
-LineEntry line_entry;
-const bool exact = false;
 start_idx_ptr = sc.comp_unit->FindLineEntry(
 start_idx_ptr, line_number, nullptr, exact, &line_entry);
 if (start_idx_ptr == UINT32_MAX)


Index: lldb/test/API/functionalities/thread/step_until/main.c
===
--- lldb/test/API/functionalities/thread/step_until/main.c
+++ lldb/test/API/functionalities/thread/step_until/main.c
@@ -1,20 +1,25 @@
 #include 
 
-void call_me(int argc)
-{
+/* The return statements are purposefully so simple, and
+ * unrelated to the program, just to achieve consistent
+ * debug line tables, across platforms, that are not
+ * dependent on compiler optimzations. */
+int call_me(int argc) {
   printf ("At the start, argc: %d.\n", argc);
 
   if (argc < 2)
-printf("Less than 2.\n");
+return 1; /* Less than 2. */
   else
-printf("Greater than or equal to 2.\n");
+retur

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

In D50304#3641971 , @DavidSpickett 
wrote:

>> then we may need to find someone with an arm machine.
>
> I tried this patch on AArch64 and did not get any new failures. You can go 
> ahead and reland. If there's still issues I can help fix them.

Thank you very much @DavidSpickett.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG419cc0a0b2ab: [lldb] Fix thread step until to not set 
breakpoint(s) on incorrect line numbers (authored by RamNalamothu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

Files:
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
  lldb/test/API/functionalities/thread/step_until/main.c


Index: lldb/test/API/functionalities/thread/step_until/main.c
===
--- lldb/test/API/functionalities/thread/step_until/main.c
+++ lldb/test/API/functionalities/thread/step_until/main.c
@@ -1,20 +1,25 @@
 #include 
 
-void call_me(int argc)
-{
+/* The return statements are purposefully so simple, and
+ * unrelated to the program, just to achieve consistent
+ * debug line tables, across platforms, that are not
+ * dependent on compiler optimzations. */
+int call_me(int argc) {
   printf ("At the start, argc: %d.\n", argc);
 
   if (argc < 2)
-printf("Less than 2.\n");
+return 1; /* Less than 2. */
   else
-printf("Greater than or equal to 2.\n");
+return argc; /* Greater than or equal to 2. */
 }
 
 int
 main(int argc, char **argv)
 {
-  call_me(argc);
-  printf("Back out in main.\n");
+  int res = 0;
+  res = call_me(argc); /* Back out in main. */
+  if (res)
+printf("Result: %d. \n", res);
 
   return 0;
 }
Index: lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
===
--- lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
+++ lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
@@ -19,7 +19,7 @@
 self.greater_than_two = line_number('main.c', 'Greater than or equal 
to 2.')
 self.back_out_in_main = line_number('main.c', 'Back out in main')
 
-def do_until (self, args, until_lines, expected_linenum):
+def common_setup (self, args):
 self.build()
 exe = self.getBuildArtifact("a.out")
 
@@ -48,7 +48,8 @@
 thread = threads[0]
 return thread
 
-thread = self.common_setup(None)
+def do_until (self, args, until_lines, expected_linenum):
+thread = self.common_setup(args)
 
 cmd_interp = self.dbg.GetCommandInterpreter()
 ret_obj = lldb.SBCommandReturnObject()
@@ -77,7 +78,7 @@
 self.do_until(None, [self.less_than_two, self.greater_than_two], 
self.less_than_two)
 
 def test_missing_one (self):
-"""Test thread step until - targeting one line and missing it."""
+"""Test thread step until - targeting one line and missing it by 
stepping out to call site"""
 self.do_until(["foo", "bar", "baz"], [self.less_than_two], 
self.back_out_in_main)
 
 
Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -1033,11 +1033,21 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
&end_ptr);
 
+// Since not all source lines will contribute code, check if we are
+// setting the breakpoint on the exact line number or the nearest
+// subsequent line number and set breakpoints at all the line table
+// entries of the chosen line number (exact or nearest subsequent).
 for (uint32_t line_number : line_numbers) {
+  LineEntry line_entry;
+  bool exact = false;
   uint32_t start_idx_ptr = index_ptr;
+  start_idx_ptr = sc.comp_unit->FindLineEntry(
+  index_ptr, line_number, nullptr, exact, &line_entry);
+  if (start_idx_ptr != UINT32_MAX)
+line_number = line_entry.line;
+  exact = true;
+  start_idx_ptr = index_ptr;
   while (start_idx_ptr <= end_ptr) {
-LineEntry line_entry;
-const bool exact = false;
 start_idx_ptr = sc.comp_unit->FindLineEntry(
 start_idx_ptr, line_number, nullptr, exact, &line_entry);
 if (start_idx_ptr == UINT32_MAX)


Index: lldb/test/API/functionalities/thread/step_until/main.c
===
--- lldb/test/API/functionalities/thread/step_until/main.c
+++ lldb/test/API/functionalities/thread/step_until/main.c
@@ -1,20 +1,25 @@
 #include 
 
-void call_me(int argc)
-{
+/* The return statements are purposefully so simple, and
+ * unrelated to the program, just to achieve consistent
+ * debug line tables, across platforms, that are not
+ * dependent on compiler optimzations. */
+int call_me(int argc) {
   printf ("At the start, argc: %d.\n", argc);
 
   if (argc < 2)
-printf("Less than 2.\n");
+return 1; /* Less 

[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a subscriber: labath.
RamNalamothu added a comment.

@jingham @labath

I have tried to add a test but it's becoming difficult for me to come-up with a 
failing test scenario. Is it okay to commit this without a test?

Otherwise, I could add the following or error capturing

  diff --git a/lldb/source/Commands/CommandObjectThread.cpp 
b/lldb/source/Commands/CommandObjectThread.cpp
  index 95a8301a318a..f91188f4446b 100644
  --- a/lldb/source/Commands/CommandObjectThread.cpp
  +++ b/lldb/source/Commands/CommandObjectThread.cpp
  @@ -1096,7 +1096,7 @@ protected:
   return false;
 }
   
  -  process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx);
  +  
assert(process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx));
   
 StreamString stream;
 Status error;

which will break `thread until` functionality and then I can add the patch 
fixing it immediately.


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

https://reviews.llvm.org/D48865

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

[Lldb-commits] [PATCH] D126596: [lldb, test] Fix typos in the lldb tests

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu created this revision.
RamNalamothu added reviewers: jingham, labath.
Herald added a subscriber: wenlei.
Herald added a project: All.
RamNalamothu requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126596

Files:
  lldb/test/API/commands/expression/dont_allow_jit/TestAllowJIT.py
  lldb/test/API/commands/frame/language/TestGuessLanguage.py
  lldb/test/API/commands/frame/var/TestFrameVar.py
  lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
  lldb/test/API/commands/target/stop-hooks/TestStopHooks.py
  lldb/test/API/functionalities/history/TestHistoryRecall.py
  
lldb/test/API/functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingOutWithArtificialFrames.py
  lldb/test/API/functionalities/var_path/TestVarPath.py
  lldb/test/API/lang/c/find_struct_type/TestFindStructTypes.py
  lldb/test/API/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
  lldb/test/API/macosx/save_crashlog/TestSaveCrashlog.py
  lldb/test/API/macosx/version_zero/TestGetVersionZeroVersion.py
  lldb/test/API/sample_test/TestSampleTest.py

Index: lldb/test/API/sample_test/TestSampleTest.py
===
--- lldb/test/API/sample_test/TestSampleTest.py
+++ lldb/test/API/sample_test/TestSampleTest.py
@@ -13,7 +13,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/macosx/version_zero/TestGetVersionZeroVersion.py
===
--- lldb/test/API/macosx/version_zero/TestGetVersionZeroVersion.py
+++ lldb/test/API/macosx/version_zero/TestGetVersionZeroVersion.py
@@ -14,7 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/macosx/save_crashlog/TestSaveCrashlog.py
===
--- lldb/test/API/macosx/save_crashlog/TestSaveCrashlog.py
+++ lldb/test/API/macosx/save_crashlog/TestSaveCrashlog.py
@@ -14,7 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
===
--- lldb/test/API/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
+++ lldb/test/API/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
@@ -14,7 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/lang/c/find_struct_type/TestFindStructTypes.py
===
--- lldb/test/API/lang/c/find_struct_type/TestFindStructTypes.py
+++ lldb/test/API/lang/c/find_struct_type/TestFindStructTypes.py
@@ -13,7 +13,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/functionalities/var_path/TestVarPath.py
===
--- lldb/test/API/functionalities/var_path/TestVarPath.py
+++ lldb/test/API/functionalities/var_path/TestVarPath.py
@@ -13,7 +13,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingOutWithArtificialFrames.py
===
--- lldb/test/API/functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingOutWithArtificialFrames.py
+++ lldb/test/API/functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingO

[Lldb-commits] [PATCH] D126596: [lldb, test] Fix typos in the lldb tests

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc6ad6901734f: [lldb, test]  Fix typos in the lldb tests 
(authored by RamNalamothu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126596

Files:
  lldb/test/API/commands/expression/dont_allow_jit/TestAllowJIT.py
  lldb/test/API/commands/frame/language/TestGuessLanguage.py
  lldb/test/API/commands/frame/var/TestFrameVar.py
  lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
  lldb/test/API/commands/target/stop-hooks/TestStopHooks.py
  lldb/test/API/functionalities/history/TestHistoryRecall.py
  
lldb/test/API/functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingOutWithArtificialFrames.py
  lldb/test/API/functionalities/var_path/TestVarPath.py
  lldb/test/API/lang/c/find_struct_type/TestFindStructTypes.py
  lldb/test/API/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
  lldb/test/API/macosx/save_crashlog/TestSaveCrashlog.py
  lldb/test/API/macosx/version_zero/TestGetVersionZeroVersion.py
  lldb/test/API/sample_test/TestSampleTest.py

Index: lldb/test/API/sample_test/TestSampleTest.py
===
--- lldb/test/API/sample_test/TestSampleTest.py
+++ lldb/test/API/sample_test/TestSampleTest.py
@@ -13,7 +13,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/macosx/version_zero/TestGetVersionZeroVersion.py
===
--- lldb/test/API/macosx/version_zero/TestGetVersionZeroVersion.py
+++ lldb/test/API/macosx/version_zero/TestGetVersionZeroVersion.py
@@ -14,7 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/macosx/save_crashlog/TestSaveCrashlog.py
===
--- lldb/test/API/macosx/save_crashlog/TestSaveCrashlog.py
+++ lldb/test/API/macosx/save_crashlog/TestSaveCrashlog.py
@@ -14,7 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
===
--- lldb/test/API/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
+++ lldb/test/API/lang/cpp/dynamic-value-same-basename/TestDynamicValueSameBase.py
@@ -14,7 +14,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/lang/c/find_struct_type/TestFindStructTypes.py
===
--- lldb/test/API/lang/c/find_struct_type/TestFindStructTypes.py
+++ lldb/test/API/lang/c/find_struct_type/TestFindStructTypes.py
@@ -13,7 +13,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/functionalities/var_path/TestVarPath.py
===
--- lldb/test/API/functionalities/var_path/TestVarPath.py
+++ lldb/test/API/functionalities/var_path/TestVarPath.py
@@ -13,7 +13,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
-# If your test case doesn't stress debug info, the
+# If your test case doesn't stress debug info, then
 # set this to true.  That way it won't be run once for
 # each debug info format.
 NO_DEBUG_INFO_TESTCASE = True
Index: lldb/test/API/functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingOutWithArtificialFrames.py
===
--- lldb/test/API/functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingOutWithArtificialFrames.py
+++ lldb/test/API/functionalities/tail_call_frames/thread_step_out_or_return/TestSteppingOutWithArtificialFrame

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 433784.
RamNalamothu retitled this revision from "[lldb] CommandObjectThreadUntil 
should set breakpoint at either on exact or the nearest subsequent line number 
but not on all the subsequent line numbers" to "[lldb] Fix thread step until to 
not set breakpoint(s) on incorrect line numbers".
RamNalamothu edited the summary of this revision.
RamNalamothu added a comment.
Herald added a project: LLDB.

Added the test coverage.

By the way, I am surprized that it looks like no else landed into this issue 
since the time I got into it during Aug/2018.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

Files:
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/test/API/functionalities/thread/step_until/TestStepUntil.py


Index: lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
===
--- lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
+++ lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
@@ -21,7 +21,7 @@
 self.greater_than_two = line_number('main.c', 'Greater than or equal 
to 2.')
 self.back_out_in_main = line_number('main.c', 'Back out in main')
 
-def do_until (self, args, until_lines, expected_linenum):
+def common_setup (self, args):
 self.build()
 exe = self.getBuildArtifact("a.out")
 
@@ -50,7 +50,8 @@
 thread = threads[0]
 return thread
 
-thread = self.common_setup(None)
+def do_until (self, args, until_lines, expected_linenum):
+thread = self.common_setup(args)
 
 cmd_interp = self.dbg.GetCommandInterpreter()
 ret_obj = lldb.SBCommandReturnObject()
@@ -79,7 +80,7 @@
 self.do_until(None, [self.less_than_two, self.greater_than_two], 
self.less_than_two)
 
 def test_missing_one (self):
-"""Test thread step until - targeting one line and missing it."""
+"""Test thread step until - targeting one line and missing it by 
stepping out to call site"""
 self.do_until(["foo", "bar", "baz"], [self.less_than_two], 
self.back_out_in_main)
 
 
Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -1034,11 +1034,21 @@
 line_table->FindLineEntryByAddress(fun_end_addr, function_start,
&end_ptr);
 
+// Since not all source lines will contribute code, check if we are
+// setting the breakpoint on the exact line number or the nearest
+// subsequent line number and set breakpoints at all the line table
+// entries of the chosen line number (exact or nearest subsequent).
 for (uint32_t line_number : line_numbers) {
+  LineEntry line_entry;
+  bool exact = false;
   uint32_t start_idx_ptr = index_ptr;
+  start_idx_ptr = sc.comp_unit->FindLineEntry(
+  index_ptr, line_number, nullptr, exact, &line_entry);
+  if (start_idx_ptr != UINT32_MAX)
+line_number = line_entry.line;
+  exact = true;
+  start_idx_ptr = index_ptr;
   while (start_idx_ptr <= end_ptr) {
-LineEntry line_entry;
-const bool exact = false;
 start_idx_ptr = sc.comp_unit->FindLineEntry(
 start_idx_ptr, line_number, nullptr, exact, &line_entry);
 if (start_idx_ptr == UINT32_MAX)


Index: lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
===
--- lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
+++ lldb/test/API/functionalities/thread/step_until/TestStepUntil.py
@@ -21,7 +21,7 @@
 self.greater_than_two = line_number('main.c', 'Greater than or equal to 2.')
 self.back_out_in_main = line_number('main.c', 'Back out in main')
 
-def do_until (self, args, until_lines, expected_linenum):
+def common_setup (self, args):
 self.build()
 exe = self.getBuildArtifact("a.out")
 
@@ -50,7 +50,8 @@
 thread = threads[0]
 return thread
 
-thread = self.common_setup(None)
+def do_until (self, args, until_lines, expected_linenum):
+thread = self.common_setup(args)
 
 cmd_interp = self.dbg.GetCommandInterpreter()
 ret_obj = lldb.SBCommandReturnObject()
@@ -79,7 +80,7 @@
 self.do_until(None, [self.less_than_two, self.greater_than_two], self.less_than_two)
 
 def test_missing_one (self):
-"""Test thread step until - targeting one line and missing it."""
+"""Test thread step until - targeting one line and missing it by stepping out to call site"""
 self.do_until(["foo", "bar", "baz"], [self.less_than_t

[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 433966.
RamNalamothu added a comment.
Herald added a project: LLDB.

Went ahead and added the error capturing for setting incorrect thread id and 
also the fix.

Without the following change/fix

  diff --git a/lldb/source/Commands/CommandObjectThread.cpp 
b/lldb/source/Commands/CommandObjectThread.cpp
  index 04457b232f3b..11affe8a7c13 100644
  --- a/lldb/source/Commands/CommandObjectThread.cpp
  +++ b/lldb/source/Commands/CommandObjectThread.cpp
  @@ -1095,8 +1095,7 @@ protected:
   return false;
 }
   
  -  if (!process->GetThreadList().SetSelectedThreadByID(
  -  m_options.m_thread_idx)) {
  +  if (!process->GetThreadList().SetSelectedThreadByID(thread->GetID())) {
   result.AppendErrorWithFormat(
   "Failed to set the selected thread to thread id %" PRIu64 ".\n",
   thread->GetID());

the `lldb/test/API/commands/expression/formatters/TestFormatters.py:228: 
self.runCmd("thread until " + str(ret))` test would fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48865

Files:
  lldb/source/Commands/CommandObjectThread.cpp


Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -984,8 +984,8 @@
   thread->GetStackFrameAtIndex(m_options.m_frame_idx).get();
   if (frame == nullptr) {
 result.AppendErrorWithFormat(
-"Frame index %u is out of range for thread %u.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+"Frame index %u is out of range for thread id %" PRIu64 ".\n",
+m_options.m_frame_idx, thread->GetID());
 return false;
   }
 
@@ -1002,9 +1002,8 @@
 
 if (line_table == nullptr) {
   result.AppendErrorWithFormat("Failed to resolve the line table for "
-   "frame %u of thread index %u.\n",
-   m_options.m_frame_idx,
-   m_options.m_thread_idx);
+   "frame %u of thread id %" PRIu64 ".\n",
+   m_options.m_frame_idx, thread->GetID());
   return false;
 }
 
@@ -1090,13 +1089,18 @@
   return false;
 }
   } else {
-result.AppendErrorWithFormat(
-"Frame index %u of thread %u has no debug information.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+result.AppendErrorWithFormat("Frame index %u of thread id %" PRIu64
+ " has no debug information.\n",
+ m_options.m_frame_idx, thread->GetID());
 return false;
   }
 
-  process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx);
+  if (!process->GetThreadList().SetSelectedThreadByID(thread->GetID())) {
+result.AppendErrorWithFormat(
+"Failed to set the selected thread to thread id %" PRIu64 ".\n",
+thread->GetID());
+return false;
+  }
 
   StreamString stream;
   Status error;


Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -984,8 +984,8 @@
   thread->GetStackFrameAtIndex(m_options.m_frame_idx).get();
   if (frame == nullptr) {
 result.AppendErrorWithFormat(
-"Frame index %u is out of range for thread %u.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+"Frame index %u is out of range for thread id %" PRIu64 ".\n",
+m_options.m_frame_idx, thread->GetID());
 return false;
   }
 
@@ -1002,9 +1002,8 @@
 
 if (line_table == nullptr) {
   result.AppendErrorWithFormat("Failed to resolve the line table for "
-   "frame %u of thread index %u.\n",
-   m_options.m_frame_idx,
-   m_options.m_thread_idx);
+   "frame %u of thread id %" PRIu64 ".\n",
+   m_options.m_frame_idx, thread->GetID());
   return false;
 }
 
@@ -1090,13 +1089,18 @@
   return false;
 }
   } else {
-result.AppendErrorWithFormat(
-"Frame index %u of thread %u has no debug information.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+result.AppendErrorWithFormat("Frame index %u of thread id %" PRIu64
+ " has no debug information.\n",
+ m_optio

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Does this look good now to land?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Does this look good now to land?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48865

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48865

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.
Herald added a subscriber: Michael137.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.
Herald added a subscriber: Michael137.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48865

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

[Lldb-commits] [PATCH] D48865: [LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGab7fcf24849d: [LLDB] CommandObjectThreadUntil::DoExecute() 
sets the wrong selected thread ID (authored by RamNalamothu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48865

Files:
  lldb/source/Commands/CommandObjectThread.cpp


Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -984,8 +984,8 @@
   thread->GetStackFrameAtIndex(m_options.m_frame_idx).get();
   if (frame == nullptr) {
 result.AppendErrorWithFormat(
-"Frame index %u is out of range for thread %u.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+"Frame index %u is out of range for thread id %" PRIu64 ".\n",
+m_options.m_frame_idx, thread->GetID());
 return false;
   }
 
@@ -1002,9 +1002,8 @@
 
 if (line_table == nullptr) {
   result.AppendErrorWithFormat("Failed to resolve the line table for "
-   "frame %u of thread index %u.\n",
-   m_options.m_frame_idx,
-   m_options.m_thread_idx);
+   "frame %u of thread id %" PRIu64 ".\n",
+   m_options.m_frame_idx, thread->GetID());
   return false;
 }
 
@@ -1090,13 +1089,18 @@
   return false;
 }
   } else {
-result.AppendErrorWithFormat(
-"Frame index %u of thread %u has no debug information.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+result.AppendErrorWithFormat("Frame index %u of thread id %" PRIu64
+ " has no debug information.\n",
+ m_options.m_frame_idx, thread->GetID());
 return false;
   }
 
-  process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx);
+  if (!process->GetThreadList().SetSelectedThreadByID(thread->GetID())) {
+result.AppendErrorWithFormat(
+"Failed to set the selected thread to thread id %" PRIu64 ".\n",
+thread->GetID());
+return false;
+  }
 
   StreamString stream;
   Status error;


Index: lldb/source/Commands/CommandObjectThread.cpp
===
--- lldb/source/Commands/CommandObjectThread.cpp
+++ lldb/source/Commands/CommandObjectThread.cpp
@@ -984,8 +984,8 @@
   thread->GetStackFrameAtIndex(m_options.m_frame_idx).get();
   if (frame == nullptr) {
 result.AppendErrorWithFormat(
-"Frame index %u is out of range for thread %u.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+"Frame index %u is out of range for thread id %" PRIu64 ".\n",
+m_options.m_frame_idx, thread->GetID());
 return false;
   }
 
@@ -1002,9 +1002,8 @@
 
 if (line_table == nullptr) {
   result.AppendErrorWithFormat("Failed to resolve the line table for "
-   "frame %u of thread index %u.\n",
-   m_options.m_frame_idx,
-   m_options.m_thread_idx);
+   "frame %u of thread id %" PRIu64 ".\n",
+   m_options.m_frame_idx, thread->GetID());
   return false;
 }
 
@@ -1090,13 +1089,18 @@
   return false;
 }
   } else {
-result.AppendErrorWithFormat(
-"Frame index %u of thread %u has no debug information.\n",
-m_options.m_frame_idx, m_options.m_thread_idx);
+result.AppendErrorWithFormat("Frame index %u of thread id %" PRIu64
+ " has no debug information.\n",
+ m_options.m_frame_idx, thread->GetID());
 return false;
   }
 
-  process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx);
+  if (!process->GetThreadList().SetSelectedThreadByID(thread->GetID())) {
+result.AppendErrorWithFormat(
+"Failed to set the selected thread to thread id %" PRIu64 ".\n",
+thread->GetID());
+return false;
+  }
 
   StreamString stream;
   Status error;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

@jingham looks like you missed to notice this one as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

In D50304#3586710 , @jingham wrote:

> For some reason I'm not getting mail notifications for review changes, sorry 
> about that.
>
> This is certainly better than the original implementation.  Among other 
> things, if we find an exact match, we really shouldn't be doing any more 
> inexact matches, so after the first exact hit it should have switched exact 
> to true.
>
> But if you had line tables laid out like (this is in increasing order of 
> address:
>
> 10
> 20
> 30
> 10
> 16
>
> and you did "thread until 15", this would find the inexact match at 20, 
> switch to an exact match for line 20 and find no other matches.  But the gap 
> between 10 & 16 in the line table is maybe an even more plausible place to 
> put the line 15 until breakpoint, so maybe we did want to throw a breakpoint 
> there as well?

@jingham 
Nope, with the current patch, we would find the inexact match at 16.

> Regular breakpoint setting has to move inexact breakpoints in much the same 
> way.  The code in the BreakpointResolverFileLine::SearchCallback ends up 
> calling CompileUnit::ResolveSymbolContext to get the "best" inexact match.  
> Maybe it would be better to not do this by hand here in the Until command, 
> but reuse the code that we use to move break points more generally?

Be it CompileUnit::ResolveSymbolContext or CompileUnit::FindLineEntry, we end 
up calling LineTable::FindLineEntryIndexByFileIndex which finds "Exact match 
always wins.  Otherwise try to find the closest line > the desired line." when 
we pass `exact = false` to it.
Given that and we anyway have to extract address list, from what we get out of 
CompileUnit::ResolveSymbolContext, for the thread until thread plan to work 
with, probably the additional overhead to use CompileUnit::ResolveSymbolContext 
here does not make sense. Or am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50304

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

[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu created this revision.
RamNalamothu added reviewers: JDevlieghere, teemperor, DavidSpickett, zturner.
RamNalamothu requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Certain commands like 'memory write', 'register read' etc all use
the OptionGroupFormat options but the help usage text for those
options is not customized to those commands.

One such example is:

(lldb) help memory read
-s  ( --size  )

  The size in bytes to use when displaying with the selected format.

(lldb) help memory write
-s  ( --size  )

  The size in bytes to use when displaying with the selected format.

This patch allows such commands to overwrite the help text for the options
in the OptionGroupFormat group as needed and fixes help text of memory write.

Also, currently the 'memory write' command allows specifying the values when
writing the file contents to memory but the values are actually ignored. This
patch fixes that as well by erroring out when values are specified in such 
cases.

llvm.org/pr49018.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114448

Files:
  lldb/include/lldb/Interpreter/OptionGroupFormat.h
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Interpreter/CommandObject.cpp
  lldb/source/Interpreter/OptionGroupFormat.cpp
  lldb/test/API/commands/help/TestHelp.py

Index: lldb/test/API/commands/help/TestHelp.py
===
--- lldb/test/API/commands/help/TestHelp.py
+++ lldb/test/API/commands/help/TestHelp.py
@@ -225,3 +225,25 @@
 "help format",
 matching=True,
 substrs=[' -- One of the format names'])
+
+@skipIf(bugnumber="llvm.org/pr49018")
+@no_debug_info_test
+def test_help_option_group_format_options_usage(self):
+"""Test that help on commands that use OptionGroupFormat options provide relevant help specific to that command."""
+self.expect(
+"help memory read",
+matching=True,
+substrs=[
+"-f  ( --format  )", "Specify a format to be used for display.",
+"-s  ( --size  )", "The size in bytes to use when displaying with the selected format."])
+
+self.expect(
+"help memory write",
+matching=True,
+substrs=[
+"Command Options Usage:",
+"memory write [-f ] [-s ]   [ [...]]",
+"memory write -i  [-s ] [-o ] ",
+"-f  ( --format  )", "The format to use for each of the value to be written.",
+"-s  ( --size  )", "The size in bytes to write from the input file or each value."])
+
Index: lldb/source/Interpreter/OptionGroupFormat.cpp
===
--- lldb/source/Interpreter/OptionGroupFormat.cpp
+++ lldb/source/Interpreter/OptionGroupFormat.cpp
@@ -16,31 +16,76 @@
 using namespace lldb;
 using namespace lldb_private;
 
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
- uint64_t default_byte_size,
- uint64_t default_count)
+OptionGroupFormat::OptionGroupFormat(
+lldb::Format default_format, uint64_t default_byte_size,
+uint64_t default_count, OptionGroupFormatUsageTextVector usage_text_vector)
 : m_format(default_format, default_format),
   m_byte_size(default_byte_size, default_byte_size),
   m_count(default_count, default_count), m_prev_gdb_format('x'),
-  m_prev_gdb_size('w') {}
-
-static constexpr OptionDefinition g_option_table[] = {
-{LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeFormat,
- "Specify a format to be used for display."},
-{LLDB_OPT_SET_2, false, "gdb-format", 'G', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeGDBFormat,
- "Specify a format using a GDB format specifier string."},
-{LLDB_OPT_SET_3, false, "size", 's', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeByteSize,
- "The size in bytes to use when displaying with the selected format."},
-{LLDB_OPT_SET_4, false, "count", 'c', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeCount,
- "The number of total items to display."},
-};
+  m_prev_gdb_size('w'),
+  m_option_definition{
+  {LLDB_OPT_SET_1,
+   false,
+   "format",
+   'f',
+   OptionParser::eRequiredArgument,
+   nullptr,
+   {},
+   0,
+   eArgTypeFormat,
+   "Specify a format to be used for display."},
+  {LLDB_OPT_SET_2,
+   false,
+   "gdb-format",
+   'G',
+   OptionParser::eRequiredArgument,
+   nullptr,
+   {},
+   0,
+   eArgTypeGDBFormat,
+   "Specify a format using a GDB format specifier string.

[Lldb-commits] [PATCH] D114544: [lldb] Fix 'memory write' to not allow specifying values when writing file contents

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu created this revision.
RamNalamothu added reviewers: JDevlieghere, DavidSpickett, zturner, teemperor.
RamNalamothu requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Currently the 'memory write' command allows specifying the values when
writing the file contents to memory but the values are actually ignored. This
patch fixes that by erroring out when values are specified in such cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114544

Files:
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Interpreter/CommandObject.cpp
  lldb/test/API/commands/memory/write/Makefile
  lldb/test/API/commands/memory/write/TestMemoryWrite.py
  lldb/test/API/commands/memory/write/file.txt
  lldb/test/API/commands/memory/write/main.cpp

Index: lldb/test/API/commands/memory/write/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/main.cpp
@@ -0,0 +1,13 @@
+#include 
+#include 
+
+int main (int argc, char const *argv[])
+{
+char my_string[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 0};
+double my_double = 1234.5678;
+int my_ints[] = {2,4,6,8,10,12,14,16,18,20,22};
+uint64_t my_uint64s[] = {0, 1, 2, 3, 4, 5, 6, 7};
+printf("my_string=%s\n", my_string); // Set break point at this line.
+printf("my_double=%g\n", my_double);
+return 0;
+}
Index: lldb/test/API/commands/memory/write/file.txt
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/file.txt
@@ -0,0 +1 @@
+abcdefg
Index: lldb/test/API/commands/memory/write/TestMemoryWrite.py
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/TestMemoryWrite.py
@@ -0,0 +1,98 @@
+"""
+Test the 'memory write' command.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class MemoryWriteTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.cpp', '// Set break point at this line.')
+
+def build_run_stop(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+# Break in main() after the variables are assigned values.
+lldbutil.run_break_set_by_file_and_line(self,
+"main.cpp",
+self.line,
+num_expected_locations=1,
+loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+self.expect("thread list",
+STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped', 'stop reason = breakpoint'])
+
+# The breakpoint should have a hit count of 1.
+lldbutil.check_breakpoint(self, bpno = 1, expected_hit_count = 1)
+
+@no_debug_info_test
+def test_memory_write(self):
+"""Test the 'memory write' command for writing values and file contents."""
+self.build_run_stop()
+
+# (lldb) memory read --format c --size 7 --count 1 `&my_string`
+# 0x7fff5fbff990: abcdefg
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['abcdefg'])
+
+# (lldb) memory write --format c --size 7 `&my_string` ABCDEFG
+self.expect(
+"memory write --format c --size 7 `&my_string` ABCDEFG", error=False)
+
+# (lldb) memory read --format c --size 7 --count 1 `&my_string`
+# 0x7fff5fbff990: ABCDEFG
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['ABCDEFG'])
+
+# (lldb) memory write --infile file.txt --size 7 `&my_string`
+# 0x7fff5fbff990: 7 bytes were written
+self.expect(
+"memory write --infile file.txt --size 7 `&my_string`",
+substrs=['7 bytes were written'])
+
+# (lldb) memory read --format c --size 7 --count 1 `&my_string`
+# 0x7fff5fbff990: abcdefg
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['abcdefg'])
+
+# (lldb) memory write --infile file.txt --size 7 `&my_string` ABCDEFG
+# 0x7fff5fbff990: error: memory write takes only a destination address when writing file contents.
+self.expect(
+"memory write --infile file.txt --size 7 `&my_string` ABCDEFG", error=True,
+substrs=['memory write takes only a destination add

[Lldb-commits] [PATCH] D114544: [lldb] Fix 'memory write' to not allow specifying values when writing file contents

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

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

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114544

Files:
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Interpreter/CommandObject.cpp
  lldb/test/API/commands/memory/write/Makefile
  lldb/test/API/commands/memory/write/TestMemoryWrite.py
  lldb/test/API/commands/memory/write/file.txt
  lldb/test/API/commands/memory/write/main.c

Index: lldb/test/API/commands/memory/write/main.c
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/main.c
@@ -0,0 +1,7 @@
+#include 
+
+int main() {
+  char my_string[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 0};
+  printf("my_string=%s\n", my_string); // Set break point at this line.
+  return 0;
+}
Index: lldb/test/API/commands/memory/write/file.txt
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/file.txt
@@ -0,0 +1 @@
+abcdefg
Index: lldb/test/API/commands/memory/write/TestMemoryWrite.py
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/TestMemoryWrite.py
@@ -0,0 +1,83 @@
+"""
+Test the 'memory write' command.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class MemoryWriteTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+# Find the line number to break inside main().
+self.line = line_number('main.c', '// Set break point at this line.')
+
+def build_run_stop(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+# Break in main() after the variables are assigned values.
+lldbutil.run_break_set_by_file_and_line(self,
+"main.c",
+self.line,
+num_expected_locations=1,
+loc_exact=True)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+# The stop reason of the thread should be breakpoint.
+self.expect("thread list",
+STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped', 'stop reason = breakpoint'])
+
+# The breakpoint should have a hit count of 1.
+lldbutil.check_breakpoint(self, bpno = 1, expected_hit_count = 1)
+
+@no_debug_info_test
+def test_memory_write(self):
+"""Test the 'memory write' command for writing values and file contents."""
+self.build_run_stop()
+
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['abcdefg'])
+
+self.expect(
+"memory write --format c --size 7 `&my_string` ABCDEFG")
+
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['ABCDEFG'])
+
+self.expect(
+"memory write --infile file.txt --size 7 `&my_string`",
+substrs=['7 bytes were written'])
+
+self.expect(
+"memory read --format c --size 7 --count 1 `&my_string`",
+substrs=['abcdefg'])
+
+self.expect(
+"memory write --infile file.txt --size 7 `&my_string` ABCDEFG", error=True,
+substrs=['error: memory write takes only a destination address when writing file contents'])
+
+self.expect(
+"memory write --infile file.txt --size 7", error=True,
+substrs=['error: memory write takes a destination address when writing file contents'])
+
+@no_debug_info_test
+def test_memory_write_command_usage_syntax(self):
+"""Test that 'memory write' command usage syntax shows it does not take values when writing file contents."""
+self.expect(
+"help memory write",
+substrs=[
+"memory write [-f ] [-s ]   [ [...]]",
+"memory write -i  [-s ] [-o ] "])
Index: lldb/test/API/commands/memory/write/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/memory/write/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
Index: lldb/source/Interpreter/CommandObject.cpp
===
--- lldb/source/Interpreter/CommandObject.cpp
+++ lldb/source/Interpreter/CommandObject.cpp
@@ -454,6 +454,9 @@
 opt_set_mask == LLDB_OPT_SET_ALL
 ? m_arguments[i]
 : OptSetFiltered(opt_set_mask, m_

[Lldb-commits] [PATCH] D114544: [lldb] Fix 'memory write' to not allow specifying values when writing file contents

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

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

Thanks for the comments.

I wasn't familiar with the Pyhton test infrastructure so I took the files from 
`commands/memory/read` and updated as needed here. All the comments etc came 
from that.

I think I addressed all the feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114544

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

[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

In D114448#3150900 , @DavidSpickett 
wrote:

> Not sure if you're a regular contributor and already know this but in case 
> not, I tend to use 
> https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting 
> for doing clang-format stuff. It'll format only the lines you've changed.

Thanks for sharing that. I normally use `git-clang-format` (i.e. clang-format 
extension for git) which does the same job.




Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:66
+   0,
+   eArgTypeCount,
+   "The number of total items to display."},

DavidSpickett wrote:
> Not sure if clang-format has done this, or just your preferred style. Nothing 
> against either but it makes it difficult to tell if anything has changed in 
> this particular part of the diff.
> 
> In general we want clang-formatted code but if it makes the diff tricky to 
> read it's best done in a follow up change that just does formatting.
The clang-format did it. Will mark this with `clang-format off/on`.



Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:73
+  case eArgTypeFormat:
+m_format_usage_text.append(std::get<1>(usage_text_tuple));
+m_option_definition[0].usage_text = m_format_usage_text.data();

DavidSpickett wrote:
> As I said above, if this is about commands being able to *replace* the help 
> text I don't think append is needed here.
> 
> Unless std::string is helping you check whether it's been set or not, which I 
> think you could do with nullptr just as well but please correct me if I'm 
> missing some context.
I kind of took the std::string approach from `OptionGroupPythonClassWithDict` 
but as you mentioned it is not needed and will fix that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114448

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

[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu updated this revision to Diff 389884.
RamNalamothu edited the summary of this revision.
RamNalamothu added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114448

Files:
  lldb/include/lldb/Interpreter/OptionGroupFormat.h
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Interpreter/OptionGroupFormat.cpp
  lldb/test/API/commands/help/TestHelp.py

Index: lldb/test/API/commands/help/TestHelp.py
===
--- lldb/test/API/commands/help/TestHelp.py
+++ lldb/test/API/commands/help/TestHelp.py
@@ -225,3 +225,21 @@
 "help format",
 matching=True,
 substrs=[' -- One of the format names'])
+
+@no_debug_info_test
+def test_help_option_group_format_options_usage(self):
+"""Test that help on commands that use OptionGroupFormat options provide relevant help specific to that command."""
+self.expect(
+"help memory read",
+matching=True,
+substrs=[
+"-f  ( --format  )", "Specify a format to be used for display.",
+"-s  ( --size  )", "The size in bytes to use when displaying with the selected format."])
+
+self.expect(
+"help memory write",
+matching=True,
+substrs=[
+"-f  ( --format  )", "The format to use for each of the value to be written.",
+"-s  ( --size  )", "The size in bytes to write from input file or each value."])
+
Index: lldb/source/Interpreter/OptionGroupFormat.cpp
===
--- lldb/source/Interpreter/OptionGroupFormat.cpp
+++ lldb/source/Interpreter/OptionGroupFormat.cpp
@@ -16,31 +16,55 @@
 using namespace lldb;
 using namespace lldb_private;
 
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
- uint64_t default_byte_size,
- uint64_t default_count)
+OptionGroupFormat::OptionGroupFormat(
+lldb::Format default_format, uint64_t default_byte_size,
+uint64_t default_count, OptionGroupFormatUsageTextVector usage_text_vector)
 : m_format(default_format, default_format),
   m_byte_size(default_byte_size, default_byte_size),
   m_count(default_count, default_count), m_prev_gdb_format('x'),
-  m_prev_gdb_size('w') {}
+  m_prev_gdb_size('w') {
+  auto default_opt_defs = OptionGroupFormat::GetDefaultDefinitions();
+  m_option_definition[0] = default_opt_defs[0];
+  m_option_definition[1] = default_opt_defs[1];
+  m_option_definition[2] = default_opt_defs[2];
+  m_option_definition[3] = default_opt_defs[3];
 
-static constexpr OptionDefinition g_option_table[] = {
-{LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeFormat,
- "Specify a format to be used for display."},
-{LLDB_OPT_SET_2, false, "gdb-format", 'G', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeGDBFormat,
- "Specify a format using a GDB format specifier string."},
-{LLDB_OPT_SET_3, false, "size", 's', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeByteSize,
- "The size in bytes to use when displaying with the selected format."},
-{LLDB_OPT_SET_4, false, "count", 'c', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeCount,
- "The number of total items to display."},
-};
+  if (!usage_text_vector.empty()) {
+for (auto usage_text_tuple : usage_text_vector) {
+  switch (std::get<0>(usage_text_tuple)) {
+  case eArgTypeFormat:
+m_option_definition[0].usage_text = std::get<1>(usage_text_tuple);
+break;
+  case eArgTypeByteSize:
+m_option_definition[2].usage_text = std::get<1>(usage_text_tuple);
+break;
+  default:
+llvm_unreachable("Unimplemented option");
+  }
+}
+  }
+}
+
+const llvm::ArrayRef
+OptionGroupFormat::GetDefaultDefinitions() {
+  return {
+  // clang-format off
+  {LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
+   nullptr, {}, 0, eArgTypeFormat,
+   "Specify a format to be used for display."},
+  {LLDB_OPT_SET_2, false, "gdb-format", 'G',
+   OptionParser::eRequiredArgument, nullptr, {}, 0, eArgTypeGDBFormat,
+   "Specify a format using a GDB format specifier string."},
+  {LLDB_OPT_SET_3, false, "size", 's', OptionParser::eRequiredArgument,
+   nullptr, {}, 0, eArgTypeByteSize,
+   "The size in bytes to use when displaying with the selected format."},
+  {LLDB_OPT_SET_4, false, "count", 'c', OptionParser::eRequiredArgument,
+   nullptr, {}, 0, eArgTypeCount,
+   "The number of total items to display."}};
+} // clang-format on
 
 llvm::ArrayRef OptionGroupFormat::GetDefinitions() {
-  auto result = llvm

[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

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

Remove the unnecessary `if {}` gaurding around range for loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114448

Files:
  lldb/include/lldb/Interpreter/OptionGroupFormat.h
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Interpreter/OptionGroupFormat.cpp
  lldb/test/API/commands/help/TestHelp.py

Index: lldb/test/API/commands/help/TestHelp.py
===
--- lldb/test/API/commands/help/TestHelp.py
+++ lldb/test/API/commands/help/TestHelp.py
@@ -225,3 +225,21 @@
 "help format",
 matching=True,
 substrs=[' -- One of the format names'])
+
+@no_debug_info_test
+def test_help_option_group_format_options_usage(self):
+"""Test that help on commands that use OptionGroupFormat options provide relevant help specific to that command."""
+self.expect(
+"help memory read",
+matching=True,
+substrs=[
+"-f  ( --format  )", "Specify a format to be used for display.",
+"-s  ( --size  )", "The size in bytes to use when displaying with the selected format."])
+
+self.expect(
+"help memory write",
+matching=True,
+substrs=[
+"-f  ( --format  )", "The format to use for each of the value to be written.",
+"-s  ( --size  )", "The size in bytes to write from input file or each value."])
+
Index: lldb/source/Interpreter/OptionGroupFormat.cpp
===
--- lldb/source/Interpreter/OptionGroupFormat.cpp
+++ lldb/source/Interpreter/OptionGroupFormat.cpp
@@ -16,31 +16,53 @@
 using namespace lldb;
 using namespace lldb_private;
 
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
- uint64_t default_byte_size,
- uint64_t default_count)
+OptionGroupFormat::OptionGroupFormat(
+lldb::Format default_format, uint64_t default_byte_size,
+uint64_t default_count, OptionGroupFormatUsageTextVector usage_text_vector)
 : m_format(default_format, default_format),
   m_byte_size(default_byte_size, default_byte_size),
   m_count(default_count, default_count), m_prev_gdb_format('x'),
-  m_prev_gdb_size('w') {}
+  m_prev_gdb_size('w') {
+  auto default_opt_defs = OptionGroupFormat::GetDefaultDefinitions();
+  m_option_definition[0] = default_opt_defs[0];
+  m_option_definition[1] = default_opt_defs[1];
+  m_option_definition[2] = default_opt_defs[2];
+  m_option_definition[3] = default_opt_defs[3];
 
-static constexpr OptionDefinition g_option_table[] = {
-{LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeFormat,
- "Specify a format to be used for display."},
-{LLDB_OPT_SET_2, false, "gdb-format", 'G', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeGDBFormat,
- "Specify a format using a GDB format specifier string."},
-{LLDB_OPT_SET_3, false, "size", 's', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeByteSize,
- "The size in bytes to use when displaying with the selected format."},
-{LLDB_OPT_SET_4, false, "count", 'c', OptionParser::eRequiredArgument,
- nullptr, {}, 0, eArgTypeCount,
- "The number of total items to display."},
-};
+  for (auto usage_text_tuple : usage_text_vector) {
+switch (std::get<0>(usage_text_tuple)) {
+case eArgTypeFormat:
+  m_option_definition[0].usage_text = std::get<1>(usage_text_tuple);
+  break;
+case eArgTypeByteSize:
+  m_option_definition[2].usage_text = std::get<1>(usage_text_tuple);
+  break;
+default:
+  llvm_unreachable("Unimplemented option");
+}
+  }
+}
+
+const llvm::ArrayRef
+OptionGroupFormat::GetDefaultDefinitions() {
+  return {
+  // clang-format off
+  {LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
+   nullptr, {}, 0, eArgTypeFormat,
+   "Specify a format to be used for display."},
+  {LLDB_OPT_SET_2, false, "gdb-format", 'G',
+   OptionParser::eRequiredArgument, nullptr, {}, 0, eArgTypeGDBFormat,
+   "Specify a format using a GDB format specifier string."},
+  {LLDB_OPT_SET_3, false, "size", 's', OptionParser::eRequiredArgument,
+   nullptr, {}, 0, eArgTypeByteSize,
+   "The size in bytes to use when displaying with the selected format."},
+  {LLDB_OPT_SET_4, false, "count", 'c', OptionParser::eRequiredArgument,
+   nullptr, {}, 0, eArgTypeCount,
+   "The number of total items to display."}};
+} // clang-format on
 
 llvm::ArrayRef OptionGroupFormat::GetDefinitions() {
-  auto result = llvm::makeArrayRef(g_option_table);
+  auto result = llvm::makeArrayRef(m_option_d

[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added inline comments.



Comment at: lldb/source/Interpreter/OptionGroupFormat.cpp:66
+   0,
+   eArgTypeCount,
+   "The number of total items to display."},

DavidSpickett wrote:
> RamNalamothu wrote:
> > DavidSpickett wrote:
> > > Not sure if clang-format has done this, or just your preferred style. 
> > > Nothing against either but it makes it difficult to tell if anything has 
> > > changed in this particular part of the diff.
> > > 
> > > In general we want clang-formatted code but if it makes the diff tricky 
> > > to read it's best done in a follow up change that just does formatting.
> > The clang-format did it. Will mark this with `clang-format off/on`.
> My bad, what I meant was that it's fine to format it in general but that in 
> this case it should be avoided to make the diff more readable.
> 
> But now I see that this is essentially a new function so formatting it all is 
> fine. I now see the change you're making, the body of the array/function 
> isn't actually changing. You can remove the `/* clang-format...*/` and just 
> let it do its thing.
> 
> That said, what is the reason that you couldn't re-use the option table?
> 
> I have two thoughts about the function:
> 1. Do we need to remake the array each time, vs the constexpr array from 
> before.
> 2. We're returning an ArrayRef to a return value, which probably works in 
> some situations (if you immediately do a copy using the ref, then discard it) 
> but I think it's a lifetime issue overall.
> 
> I'm assuming that ArrayRef is basically std::array& in a general sense, and 
> if you returned a std::array& to something on the stack, that's a lifetime 
> issue for sure. I looked at some other option groups and they all follow the 
> pattern of returning an array ref to a const array defined outside the 
> function.
Ah, I could re-use the option table. Just an oversite from my end. Thank you 
for spotting that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114448

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

[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

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

Re-use the existing constexpr option table.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114448

Files:
  lldb/include/lldb/Interpreter/OptionGroupFormat.h
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Interpreter/OptionGroupFormat.cpp
  lldb/test/API/commands/help/TestHelp.py

Index: lldb/test/API/commands/help/TestHelp.py
===
--- lldb/test/API/commands/help/TestHelp.py
+++ lldb/test/API/commands/help/TestHelp.py
@@ -225,3 +225,21 @@
 "help format",
 matching=True,
 substrs=[' -- One of the format names'])
+
+@no_debug_info_test
+def test_help_option_group_format_options_usage(self):
+"""Test that help on commands that use OptionGroupFormat options provide relevant help specific to that command."""
+self.expect(
+"help memory read",
+matching=True,
+substrs=[
+"-f  ( --format  )", "Specify a format to be used for display.",
+"-s  ( --size  )", "The size in bytes to use when displaying with the selected format."])
+
+self.expect(
+"help memory write",
+matching=True,
+substrs=[
+"-f  ( --format  )", "The format to use for each of the value to be written.",
+"-s  ( --size  )", "The size in bytes to write from input file or each value."])
+
Index: lldb/source/Interpreter/OptionGroupFormat.cpp
===
--- lldb/source/Interpreter/OptionGroupFormat.cpp
+++ lldb/source/Interpreter/OptionGroupFormat.cpp
@@ -16,15 +16,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
- uint64_t default_byte_size,
- uint64_t default_count)
-: m_format(default_format, default_format),
-  m_byte_size(default_byte_size, default_byte_size),
-  m_count(default_count, default_count), m_prev_gdb_format('x'),
-  m_prev_gdb_size('w') {}
-
-static constexpr OptionDefinition g_option_table[] = {
+static constexpr OptionDefinition g_default_option_definitions[] = {
 {LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
  nullptr, {}, 0, eArgTypeFormat,
  "Specify a format to be used for display."},
@@ -39,8 +31,34 @@
  "The number of total items to display."},
 };
 
+OptionGroupFormat::OptionGroupFormat(
+lldb::Format default_format, uint64_t default_byte_size,
+uint64_t default_count, OptionGroupFormatUsageTextVector usage_text_vector)
+: m_format(default_format, default_format),
+  m_byte_size(default_byte_size, default_byte_size),
+  m_count(default_count, default_count), m_prev_gdb_format('x'),
+  m_prev_gdb_size('w') {
+  // Copy the default option definitions.
+  std::copy(std::begin(g_default_option_definitions),
+std::end(g_default_option_definitions),
+std::begin(m_option_definitions));
+
+  for (auto usage_text_tuple : usage_text_vector) {
+switch (std::get<0>(usage_text_tuple)) {
+case eArgTypeFormat:
+  m_option_definitions[0].usage_text = std::get<1>(usage_text_tuple);
+  break;
+case eArgTypeByteSize:
+  m_option_definitions[2].usage_text = std::get<1>(usage_text_tuple);
+  break;
+default:
+  llvm_unreachable("Unimplemented option");
+}
+  }
+}
+
 llvm::ArrayRef OptionGroupFormat::GetDefinitions() {
-  auto result = llvm::makeArrayRef(g_option_table);
+  auto result = llvm::makeArrayRef(m_option_definitions);
   if (m_byte_size.GetDefaultValue() < UINT64_MAX) {
 if (m_count.GetDefaultValue() < UINT64_MAX)
   return result;
@@ -54,7 +72,7 @@
  llvm::StringRef option_arg,
  ExecutionContext *execution_context) {
   Status error;
-  const int short_option = g_option_table[option_idx].short_option;
+  const int short_option = m_option_definitions[option_idx].short_option;
 
   switch (short_option) {
   case 'f':
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1222,7 +1222,15 @@
 interpreter, "memory write",
 "Write to the memory of the current target process.", nullptr,
 eCommandRequiresProcess | eCommandProcessMustBeLaunched),
-m_option_group(), m_format_options(eFormatBytes, 1, UINT64_MAX),
+m_option_group(),
+m_format_options(
+eFormatBytes, 1, UINT64_MAX,
+{std::make_tuple(
+ eArgTypeFo

[Lldb-commits] [PATCH] D114448: [Bug 49018][lldb] Fix incorrect help text for 'memory write' command

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7f05ff8be481: [Bug 49018][lldb] Fix incorrect help text for 
'memory write' command (authored by ramana-nvr, committed by 
RamNalamothu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114448

Files:
  lldb/include/lldb/Interpreter/OptionGroupFormat.h
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Interpreter/OptionGroupFormat.cpp
  lldb/test/API/commands/help/TestHelp.py

Index: lldb/test/API/commands/help/TestHelp.py
===
--- lldb/test/API/commands/help/TestHelp.py
+++ lldb/test/API/commands/help/TestHelp.py
@@ -225,3 +225,21 @@
 "help format",
 matching=True,
 substrs=[' -- One of the format names'])
+
+@no_debug_info_test
+def test_help_option_group_format_options_usage(self):
+"""Test that help on commands that use OptionGroupFormat options provide relevant help specific to that command."""
+self.expect(
+"help memory read",
+matching=True,
+substrs=[
+"-f  ( --format  )", "Specify a format to be used for display.",
+"-s  ( --size  )", "The size in bytes to use when displaying with the selected format."])
+
+self.expect(
+"help memory write",
+matching=True,
+substrs=[
+"-f  ( --format  )", "The format to use for each of the value to be written.",
+"-s  ( --size  )", "The size in bytes to write from input file or each value."])
+
Index: lldb/source/Interpreter/OptionGroupFormat.cpp
===
--- lldb/source/Interpreter/OptionGroupFormat.cpp
+++ lldb/source/Interpreter/OptionGroupFormat.cpp
@@ -16,15 +16,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-OptionGroupFormat::OptionGroupFormat(lldb::Format default_format,
- uint64_t default_byte_size,
- uint64_t default_count)
-: m_format(default_format, default_format),
-  m_byte_size(default_byte_size, default_byte_size),
-  m_count(default_count, default_count), m_prev_gdb_format('x'),
-  m_prev_gdb_size('w') {}
-
-static constexpr OptionDefinition g_option_table[] = {
+static constexpr OptionDefinition g_default_option_definitions[] = {
 {LLDB_OPT_SET_1, false, "format", 'f', OptionParser::eRequiredArgument,
  nullptr, {}, 0, eArgTypeFormat,
  "Specify a format to be used for display."},
@@ -39,8 +31,34 @@
  "The number of total items to display."},
 };
 
+OptionGroupFormat::OptionGroupFormat(
+lldb::Format default_format, uint64_t default_byte_size,
+uint64_t default_count, OptionGroupFormatUsageTextVector usage_text_vector)
+: m_format(default_format, default_format),
+  m_byte_size(default_byte_size, default_byte_size),
+  m_count(default_count, default_count), m_prev_gdb_format('x'),
+  m_prev_gdb_size('w') {
+  // Copy the default option definitions.
+  std::copy(std::begin(g_default_option_definitions),
+std::end(g_default_option_definitions),
+std::begin(m_option_definitions));
+
+  for (auto usage_text_tuple : usage_text_vector) {
+switch (std::get<0>(usage_text_tuple)) {
+case eArgTypeFormat:
+  m_option_definitions[0].usage_text = std::get<1>(usage_text_tuple);
+  break;
+case eArgTypeByteSize:
+  m_option_definitions[2].usage_text = std::get<1>(usage_text_tuple);
+  break;
+default:
+  llvm_unreachable("Unimplemented option");
+}
+  }
+}
+
 llvm::ArrayRef OptionGroupFormat::GetDefinitions() {
-  auto result = llvm::makeArrayRef(g_option_table);
+  auto result = llvm::makeArrayRef(m_option_definitions);
   if (m_byte_size.GetDefaultValue() < UINT64_MAX) {
 if (m_count.GetDefaultValue() < UINT64_MAX)
   return result;
@@ -54,7 +72,7 @@
  llvm::StringRef option_arg,
  ExecutionContext *execution_context) {
   Status error;
-  const int short_option = g_option_table[option_idx].short_option;
+  const int short_option = m_option_definitions[option_idx].short_option;
 
   switch (short_option) {
   case 'f':
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1222,7 +1222,15 @@
 interpreter, "memory write",
 "Write to the memory of the current target process.", nullptr,
 eCommandRequiresProcess | eCommandProcessMustBeLaunched),
-m_option_group(), m_format_options(eFormatBytes, 1, UINT64_MAX),
+m_option_group(),
+m_format_option