[LLDB][MIPS] Fix TestStepOverBreakpoint.py failure

[nitesh.jain]

Summary

In case of MIPS, we never set breakpoint in the delay slot. Hence while doing stepping, the delay slot instruction will be skipped.

[clayborg]

Jim, can you take a look at this and see if this could be fixed in a nicer way? I would prefer to not see anything related to delay slots in a test. Can we abstract this better? Maybe ask the SBInstruction() if you can set a breakpoint on it instead of speaking in delay slot terms? Or maybe we ask the SBInstructionList to count the number of instructions between two addresses? Maybe something like:

uint32_t SBInstructionList::GetInstructionCount(const SBAddress &start, const SBAddress &end);

Or improve the test so it doesn't rely on instruction count??

[nitesh.jain]

Update diff as per suggestion

[jingham]

The substance seems fine.

I'm not sure I would guess what GetInstructionsCount with canSetBreakpoint == true would do without reading the code. You could fix this with a more explicit parameter name, but I can't think of a good name that's short enough not to be awful, something like: excludeInstructionsWhereYouCantSetBreakpoints would get it but yuck... Better to just add a header comment explaining what that parameter actually does.

It's also odd to have the SBInstructionList hold the logic determining whether you can set a breakpoint on an instruction. That should really be in SBInstruction, or even better should be in the lldb_private::Instruction class, since this seems like a generally useful bit of knowledge, and then routed through SBInstruction.

[nitesh.jain]

Update Diff as per suggestion

[clayborg]

include/lldb/API/SBInstructionList.h

36–38

comment boundaries? Either extend the '-' or format within

[jingham]

Couple tiny tweaks to the comment, and this is good to go.

include/lldb/API/SBInstructionList.h

36–38

Also grammar. Like:

Returns the number of instructions between the start and end address. If canSetBreakpoint is true then the count will be the number of instructions on which a breakpoint can be set.

[nitesh.jain]

Update diff as per suggestion.

[jingham]

Looks good, thanks!

include/lldb/API/SBAddress.h

protected:
   friend class SBThreadPlan;
   friend class SBValue;
   friend class SBQueueItem;
 
   lldb_private::Address *operator->();
 
   const lldb_private::Address *operator->() const;
 
+  friend bool operator==(const SBAddress &lhs, const SBAddress &rhs);
+
   lldb_private::Address *get();
 
   lldb_private::Address &ref();
 
   const lldb_private::Address &ref() const;
 
   SBAddress(const lldb_private::Address *lldb_object_ptr);
 
   void SetAddress(const lldb_private::Address *lldb_object_ptr);
 
 private:
   std::unique_ptr<lldb_private::Address> m_opaque_ap;
 };
 
+bool operator==(const SBAddress &lhs, const SBAddress &rhs);
+
 } // namespace lldb
 
 #endif // LLDB_SBAddress_h_

include/lldb/API/SBInstruction.h

public:
   lldb::SBData GetData(lldb::SBTarget target);
 
   size_t GetByteSize();
 
   bool DoesBranch();
 
   bool HasDelaySlot();
 
+  bool CanSetBreakpoint();
+
   void Print(FILE *out);
 
   bool GetDescription(lldb::SBStream &description);
 
   bool EmulateWithFrame(lldb::SBFrame &frame, uint32_t evaluate_options);
 
   bool DumpEmulation(const char *triple); // triple is to specify the
                                           // architecture, e.g. 'armv6' or

include/lldb/API/SBInstructionList.h

public:
   ~SBInstructionList();
 
   bool IsValid() const;
 
   size_t GetSize();
 
   lldb::SBInstruction GetInstructionAtIndex(uint32_t idx);
 
+  // ----------------------------------------------------------------------
+  // Returns the number of instructions between the start and end address.
+  // If canSetBreakpoint is true then the count will be the number of 
+  // instructions on which a breakpoint can be set.
+  // ----------------------------------------------------------------------
+  size_t GetInstructionsCount(const SBAddress &start,
+                              const SBAddress &end,
+                              bool canSetBreakpoint = false);                                   
+
   void Clear();
 
   void AppendInstruction(lldb::SBInstruction inst);
 
   void Print(FILE *out);
 
   bool GetDescription(lldb::SBStream &description);
 

include/lldb/Core/Disassembler.h

virtual void Dump(Stream *s, uint32_t max_opcode_byte_size, bool show_address,
                     const SymbolContext *prev_sym_ctx,
                     const FormatEntity::Entry *disassembly_addr_format,
                     size_t max_address_text_size);
 
   virtual bool DoesBranch() = 0;
 
   virtual bool HasDelaySlot();
 
+  bool CanSetBreakpoint ();
+
   virtual size_t Decode(const Disassembler &disassembler,
                         const DataExtractor &data,
                         lldb::offset_t data_offset) = 0;
 
   virtual void SetDescription(llvm::StringRef) {
   } // May be overridden in sub-classes that have descriptions.
 
   lldb::OptionValueSP ReadArray(FILE *in_file, Stream *out_stream,

packages/Python/lldbsuite/test/functionalities/breakpoint/step_over_breakpoint/TestStepOverBreakpoint.py

def test_step_instruction(self):
         contextList = self.target.FindFunctions('main', lldb.eFunctionNameTypeAuto)
         self.assertEquals(contextList.GetSize(), 1)
         symbolContext = contextList.GetContextAtIndex(0)
         function = symbolContext.GetFunction()
         self.assertTrue(function)
         instructions = function.GetInstructions(self.target)
         addr_1 = self.breakpoint1.GetLocationAtIndex(0).GetAddress()
         addr_4 = self.breakpoint4.GetLocationAtIndex(0).GetAddress()
-        delay_slot = 0
-        addr_1_load_address = addr_1.GetLoadAddress(self.target)
-        addr_4_load_address = addr_4.GetLoadAddress(self.target)
-        for i in range(instructions.GetSize()) :
-            addr = instructions.GetInstructionAtIndex(i).GetAddress()
-            addr_load_address = addr.GetLoadAddress(self.target)
-            if (addr == addr_1) : index_1 = i
-            if (addr == addr_4) : index_4 = i
-            if (addr_1_load_address <= addr_load_address <= addr_4_load_address):
-                if (instructions.GetInstructionAtIndex(i).HasDelaySlot()) :
-                    delay_slot = delay_slot + 1
 
-        steps_expected = index_4 - index_1 - delay_slot
+        # if third argument is true then the count will be the number of 
+        # instructions on which a breakpoint can be set.
+        # start = addr_1, end = addr_4, canSetBreakpoint = True
+        steps_expected = instructions.GetInstructionsCount(addr_1, addr_4, True)
         step_count = 0
         # Step from breakpoint_1 to breakpoint_4
         while True:
             self.thread.StepInstruction(True)
             step_count = step_count + 1
             self.assertEquals(self.process.GetState(), lldb.eStateStopped)
             self.assertTrue(self.thread.GetStopReason() == lldb.eStopReasonPlanComplete or
                             self.thread.GetStopReason() == lldb.eStopReasonBreakpoint)

scripts/interface/SBInstruction.i

public:
     GetByteSize ();
 
     bool
     DoesBranch ();
 
     bool
     HasDelaySlot ();
 
+    bool
+    CanSetBreakpoint ();
+
     void
     Print (FILE *out);
 
     bool
     GetDescription (lldb::SBStream &description);
 
     bool
     EmulateWithFrame (lldb::SBFrame &frame, uint32_t evaluate_options);

scripts/interface/SBInstructionList.i

public:
     IsValid () const;
 
     size_t
     GetSize ();
 
     lldb::SBInstruction
     GetInstructionAtIndex (uint32_t idx);
 
+    size_t GetInstructionsCount(const SBAddress &start, const SBAddress &end,
+                                bool canSetBreakpoint);
+
     void
     Clear ();
 
     void
     AppendInstruction (lldb::SBInstruction inst);
 
     void
     Print (FILE *out);

source/API/SBAddress.cpp

if (this != &rhs) {
     if (rhs.IsValid())
       ref() = rhs.ref();
     else
       m_opaque_ap.reset(new Address());
   }
   return *this;
 }
 
+bool lldb::operator==(const SBAddress &lhs, const SBAddress &rhs) {
+  if (lhs.IsValid() && rhs.IsValid())
+    return lhs.ref() == rhs.ref();
+  return false;
+}
+
 bool SBAddress::IsValid() const {
   return m_opaque_ap.get() != NULL && m_opaque_ap->IsValid();
 }
 
 void SBAddress::Clear() { m_opaque_ap.reset(new Address()); }
 
 void SBAddress::SetAddress(lldb::SBSection section, lldb::addr_t offset) {
   Address &addr = ref();

source/API/SBInstruction.cpp

 
 bool SBInstruction::HasDelaySlot() {
   lldb::InstructionSP inst_sp(GetOpaque());
   if (inst_sp)
     return inst_sp->HasDelaySlot();
   return false;
 }
 
+bool SBInstruction::CanSetBreakpoint () {
+  lldb::InstructionSP inst_sp(GetOpaque());
+  if (inst_sp)
+    return inst_sp->CanSetBreakpoint();
+  return false;
+}
+
 lldb::InstructionSP SBInstruction::GetOpaque() {
   if (m_opaque_sp)
     return m_opaque_sp->GetSP();
   else
     return lldb::InstructionSP();
 }
 
 void SBInstruction::SetOpaque(const lldb::DisassemblerSP &disasm_sp,

source/API/SBInstructionList.cpp

 //===-- SBInstructionList.cpp -----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/API/SBInstructionList.h"
 #include "lldb/API/SBInstruction.h"
+#include "lldb/API/SBAddress.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
 using namespace lldb_private;
SBInstruction SBInstructionList::GetInstructionAtIndex(uint32_t idx) {
   SBInstruction inst;
   if (m_opaque_sp && idx < m_opaque_sp->GetInstructionList().GetSize())
     inst.SetOpaque(
         m_opaque_sp,
         m_opaque_sp->GetInstructionList().GetInstructionAtIndex(idx));
   return inst;
 }
 
+size_t SBInstructionList::GetInstructionsCount(const SBAddress &start,
+                                              const SBAddress &end, 
+                                              bool canSetBreakpoint) {
+  size_t num_instructions = GetSize();
+  size_t i = 0;
+  SBAddress addr;
+  size_t lower_index = 0;
+  size_t upper_index = 0;
+  size_t instructions_to_skip = 0;
+  for (i = 0; i < num_instructions; ++i) {
+    addr = GetInstructionAtIndex(i).GetAddress();
+    if (start == addr)
+      lower_index = i;
+    if (end == addr)
+      upper_index = i;
+  }
+  if (canSetBreakpoint)
+    for (i = lower_index; i <= upper_index; ++i) {
+      SBInstruction insn = GetInstructionAtIndex(i);
+      if (!insn.CanSetBreakpoint())
+        ++instructions_to_skip;
+    }
+  return upper_index - lower_index - instructions_to_skip;
+}
+
 void SBInstructionList::Clear() { m_opaque_sp.reset(); }
 
 void SBInstructionList::AppendInstruction(SBInstruction insn) {}
 
 void SBInstructionList::SetDisassembler(const lldb::DisassemblerSP &opaque_sp) {
   m_opaque_sp = opaque_sp;
 }
 

source/Core/Disassembler.cpp

bool Instruction::DumpEmulation(const ArchSpec &arch) {
   if (insn_emulator_ap) {
     insn_emulator_ap->SetInstruction(GetOpcode(), GetAddress(), nullptr);
     return insn_emulator_ap->EvaluateInstruction(0);
   }
 
   return false;
 }
 
+bool Instruction::CanSetBreakpoint () {
+  return !HasDelaySlot();
+}
+
 bool Instruction::HasDelaySlot() {
   // Default is false.
   return false;
 }
 
 OptionValueSP Instruction::ReadArray(FILE *in_file, Stream *out_stream,
                                      OptionValue::Type data_type) {
   bool done = false;