Fix single-stepping onto a breakpoint

Summary:
r259344 introduced a bug, where we fail to perform a single step, when the instruction we are
stepping onto contains a breakpoint which is not valid for this thread. This fixes the problem
and add a test case.

Reviewers: tberghammer, emaste

Subscribers: abhishek.aggarwal, lldb-commits, emaste

Differential Revision: http://reviews.llvm.org/D16767

llvm-svn: 259488

3 files changed, 75 insertions, 51 deletions

diff --git a/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py
index 7f5e5adb9b9..d2bd9f2812d 100644
--- a/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py
+++ b/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py
@@ -1,5 +1,5 @@
 """
-Test continue from a breakpoint when there is a breakpoint on the next instruction also.
+Test that we handle breakpoints on consecutive instructions correctly.
 """
 
 from __future__ import print_function
@@ -15,44 +15,86 @@ class ConsecutiveBreakpointsTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
-    def test (self):
-        self.build ()
-        self.consecutive_breakpoints_tests()
-
-    def consecutive_breakpoints_tests(self):
+    def prepare_test(self):
+        self.build()
         exe = os.path.join (os.getcwd(), "a.out")
 
         # Create a target by the debugger.
-        target = self.dbg.CreateTarget(exe)
-        self.assertTrue(target, VALID_TARGET)
+        self.target = self.dbg.CreateTarget(exe)
+        self.assertTrue(self.target, VALID_TARGET)
 
-        breakpoint1 = target.BreakpointCreateBySourceRegex("Set breakpoint here", lldb.SBFileSpec("main.cpp"))
-        self.assertTrue(breakpoint1 and
-                        breakpoint1.GetNumLocations() == 1,
-                        VALID_BREAKPOINT)
+        breakpoint1 = self.target.BreakpointCreateBySourceRegex("Set breakpoint here", lldb.SBFileSpec("main.cpp"))
+        self.assertTrue(breakpoint1 and breakpoint1.GetNumLocations() == 1, VALID_BREAKPOINT)
 
         # Now launch the process, and do not stop at entry point.
-        process = target.LaunchSimple (None, None, self.get_process_working_directory())
-        self.assertTrue(process, PROCESS_IS_VALID)
+        self.process = self.target.LaunchSimple (None, None, self.get_process_working_directory())
+        self.assertIsNotNone(self.process, PROCESS_IS_VALID)
 
         # We should be stopped at the first breakpoint
-        thread = lldbutil.get_one_thread_stopped_at_breakpoint(process, breakpoint1)
-        self.assertIsNotNone(thread, "Expected one thread to be stopped at breakpoint 1")
+        self.thread = lldbutil.get_one_thread_stopped_at_breakpoint(self.process, breakpoint1)
+        self.assertIsNotNone(self.thread, "Expected one thread to be stopped at breakpoint 1")
 
         # Set breakpoint to the next instruction
-        frame = thread.GetFrameAtIndex(0)
+        frame = self.thread.GetFrameAtIndex(0)
 
         address = frame.GetPCAddress()
-        instructions = target.ReadInstructions(address, 2)
+        instructions = self.target.ReadInstructions(address, 2)
         self.assertTrue(len(instructions) == 2)
-        address = instructions[1].GetAddress()
+        self.bkpt_address = instructions[1].GetAddress()
+        self.breakpoint2 = self.target.BreakpointCreateByAddress(self.bkpt_address.GetLoadAddress(self.target))
+        self.assertTrue(self.breakpoint2 and self.breakpoint2.GetNumLocations() == 1, VALID_BREAKPOINT)
+
+    def finish_test(self):
+        # Run the process until termination
+        self.process.Continue()
+        self.assertEquals(self.process.GetState(), lldb.eStateExited)
 
-        breakpoint2 = target.BreakpointCreateByAddress(address.GetLoadAddress(target))
-        process.Continue()
+    @no_debug_info_test
+    def test_continue(self):
+        """Test that continue stops at the second breakpoint."""
+        self.prepare_test()
 
+        self.process.Continue()
+        self.assertEquals(self.process.GetState(), lldb.eStateStopped)
         # We should be stopped at the second breakpoint
-        thread = lldbutil.get_one_thread_stopped_at_breakpoint(process, breakpoint2)
-        self.assertIsNotNone(thread, "Expected one thread to be stopped at breakpoint 2")
+        self.thread = lldbutil.get_one_thread_stopped_at_breakpoint(self.process, self.breakpoint2)
+        self.assertIsNotNone(self.thread, "Expected one thread to be stopped at breakpoint 2")
 
-        # Run the process until termination
-        process.Continue()
+        self.finish_test()
+
+    @no_debug_info_test
+    def test_single_step(self):
+        """Test that single step stops at the second breakpoint."""
+        self.prepare_test()
+
+        step_over = False
+        self.thread.StepInstruction(step_over)
+
+        self.assertEquals(self.process.GetState(), lldb.eStateStopped)
+        self.assertEquals(self.thread.GetFrameAtIndex(0).GetPCAddress().GetLoadAddress(self.target),
+                self.bkpt_address.GetLoadAddress(self.target))
+        self.thread = lldbutil.get_one_thread_stopped_at_breakpoint(self.process, self.breakpoint2)
+        self.assertIsNotNone(self.thread, "Expected one thread to be stopped at breakpoint 2")
+
+        self.finish_test()
+
+    @no_debug_info_test
+    def test_single_step_thread_specific(self):
+        """Test that single step stops, even though the second breakpoint is not valid."""
+        self.prepare_test()
+
+        # Choose a thread other than the current one. A non-existing thread is fine.
+        thread_index = self.process.GetNumThreads()+1
+        self.assertFalse(self.process.GetThreadAtIndex(thread_index).IsValid())
+        self.breakpoint2.SetThreadIndex(thread_index)
+
+        step_over = False
+        self.thread.StepInstruction(step_over)
+
+        self.assertEquals(self.process.GetState(), lldb.eStateStopped)
+        self.assertEquals(self.thread.GetFrameAtIndex(0).GetPCAddress().GetLoadAddress(self.target),
+                self.bkpt_address.GetLoadAddress(self.target))
+        self.assertEquals(self.thread.GetStopReason(), lldb.eStopReasonPlanComplete,
+                "Stop reason should be 'plan complete'")
+
+        self.finish_test()
diff --git a/lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp b/lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
index 46cf2bd2a78..3cb1cec6983 100644
--- a/lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
+++ b/lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp
@@ -569,20 +569,11 @@ FreeBSDThread::TraceNotify(const ProcessMessage &message)
 
     // If the current pc is a breakpoint site then set the StopInfo to Breakpoint.
     // Otherwise, set the StopInfo to Watchpoint or Trace.
-    if (bp_site)
-    {
-        lldb::break_id_t bp_id = bp_site->GetID();
-        // If we have an operating system plug-in, we might have set a thread specific breakpoint using the
-        // operating system thread ID, so we can't make any assumptions about the thread ID so we must always
-        // report the breakpoint regardless of the thread.
-        if (bp_site->ValidForThisThread(this) || GetProcess()->GetOperatingSystem () != NULL)
-            SetStopInfo (StopInfo::CreateStopReasonWithBreakpointSiteID(*this, bp_id));
-        else
-        {
-            const bool should_stop = false;
-            SetStopInfo (StopInfo::CreateStopReasonWithBreakpointSiteID(*this, bp_id, should_stop));
-        }
-    }
+    // If we have an operating system plug-in, we might have set a thread specific breakpoint using the
+    // operating system thread ID, so we can't make any assumptions about the thread ID so we must always
+    // report the breakpoint regardless of the thread.
+    if (bp_site && (bp_site->ValidForThisThread(this) || GetProcess()->GetOperatingSystem () != NULL))
+        SetStopInfo(StopInfo::CreateStopReasonWithBreakpointSiteID(*this, bp_site->GetID()));
     else
     {
         POSIXBreakpointProtocol* reg_ctx = GetPOSIXBreakpointProtocol();
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 21dad4e11b6..263639904f1 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2006,19 +2006,10 @@ ProcessGDBRemote::SetThreadStopInfo (lldb::tid_t tid,
 
                             // If the current pc is a breakpoint site then the StopInfo should be set to Breakpoint
                             // Otherwise, it will be set to Trace.
-                            if (bp_site_sp)
+                            if (bp_site_sp && bp_site_sp->ValidForThisThread(thread_sp.get()))
                             {
-                                // If the breakpoint is for this thread, then we'll report the hit, but if it is for another thread,
-                                // we can just report no reason.
-                                if (bp_site_sp->ValidForThisThread (thread_sp.get()))
-                                {
-                                    thread_sp->SetStopInfo (StopInfo::CreateStopReasonWithBreakpointSiteID (*thread_sp, bp_site_sp->GetID()));
-                                }
-                                else
-                                {
-                                    StopInfoSP invalid_stop_info_sp;
-                                    thread_sp->SetStopInfo (invalid_stop_info_sp);
-                                }
+                                thread_sp->SetStopInfo(
+                                    StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp, bp_site_sp->GetID()));
                             }
                             else
                               thread_sp->SetStopInfo (StopInfo::CreateStopReasonToTrace (*thread_sp));