Fix single-stepping onto a breakpoint

[labath]

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.

[tberghammer]

Looks reasonable but should we merge TestSingleStepOntoBreakpoint.py and TestConsecutiveBreakpoints.py? I think they have quite a bit of code in common.

[abhishek.aggarwal]

source/Plugins/Process/FreeBSD/FreeBSDThread.cpp will not compile for FreeBSD. In Line 576, bp_id is undefined. Please replace it with bp_site_sp->GetID()

[labath]

Merged test cases, and updated the FreeBSD version. Please take another look.

[amccarth]

I was chasing this same bug on Windows before I noticed you were working on it. I patched in your latest diff, and the problem still occurs. In fact, now I get two failures:

FAIL: test_continue (TestConsecutiveBreakpoints.ConsecutiveBreakpointsTestCase)

Test that continue stops at the second breakpoint.

---

Traceback (most recent call last):

File "D:\src\llvm\llvm\tools\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 552, in wrapper
  return func(self, *args, **kwargs)
File "D:\src\llvm\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\breakpoint\consecutive_breakpoints\TestConsecutiveBreakpoints.py", line 58, in test_continue
  self.assertEquals(self.process.GetState(), lldb.eStateStopped)

AssertionError: 10 != 5

Config=i686-D:\src\llvm\build\ninja_release\bin\clang.exe

FAIL: test_single_step (TestConsecutiveBreakpoints.ConsecutiveBreakpointsTestCase)

Test that single step stops at the second breakpoint.

---

Traceback (most recent call last):

File "D:\src\llvm\llvm\tools\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 552, in wrapper
  return func(self, *args, **kwargs)
File "D:\src\llvm\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\breakpoint\consecutive_breakpoints\TestConsecutiveBreakpoints.py", line 76, in test_single_step
  self.assertIsNotNone(self.thread, "Expected one thread to be stopped at breakpoint 2")

AssertionError: unexpectedly None : Expected one thread to be stopped at breakpoint 2

Config=i686-D:\src\llvm\build\ninja_release\bin\clang.exe

Ran 3 tests in 25.026s

RESULT: FAILED (1 passes, 2 failures, 0 errors, 0 skipped, 0 expected failures, 0 unexpected successes)

[labath]

In D16767#341259, @amccarth wrote:

I was chasing this same bug on Windows before I noticed you were working on it. I patched in your latest diff, and the problem still occurs.

That is not surprising. These fixes (r259344, and this one) are implemented in the os-specific classes: ProcessFreeBSD (used on freebsd, obviously), and ProcessGdbRemote (used on arches using lldb-server: linux, osx). I don't really like that, as I think this functionality could be abstracted to a common place, but that seems to be the way things are done currently. You could probably fix this by doing something similar in ProcessWindows. I.e., when you get a "stopped by single-step" event from the OS, check whether there is a breakpoint under your instruction, and if it is, report that as a breakpoint hit instead.

FAIL: test_continue (TestConsecutiveBreakpoints.ConsecutiveBreakpointsTestCase)

Test that continue stops at the second breakpoint.

---

Traceback (most recent call last):

File "D:\src\llvm\llvm\tools\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 552, in wrapper
  return func(self, *args, **kwargs)
File "D:\src\llvm\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\breakpoint\consecutive_breakpoints\TestConsecutiveBreakpoints.py", line 58, in test_continue
  self.assertEquals(self.process.GetState(), lldb.eStateStopped)

AssertionError: 10 != 5

This is what the original test was doing. It just fails slightly sooner, because I added a new assert (10 is eStateExited, so your inferior exits because it failed to stop at the second breakpoint).

FAIL: test_single_step (TestConsecutiveBreakpoints.ConsecutiveBreakpointsTestCase)

Test that single step stops at the second breakpoint.

---

Traceback (most recent call last):

File "D:\src\llvm\llvm\tools\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 552, in wrapper
  return func(self, *args, **kwargs)
File "D:\src\llvm\llvm\tools\lldb\packages\Python\lldbsuite\test\functionalities\breakpoint\consecutive_breakpoints\TestConsecutiveBreakpoints.py", line 76, in test_single_step
  self.assertIsNotNone(self.thread, "Expected one thread to be stopped at breakpoint 2")

AssertionError: unexpectedly None : Expected one thread to be stopped at breakpoint 2

This is sort of testing the same thing, only it's using a single step command instead. I added it "by the way", while I was adding the third test. The failure mode is different as I didn't put the assert on the process state (I probably should do that). Hopefully you will be able to fix both failures in one go.

RESULT: FAILED (1 passes

This is the regression introduced by the first patch. Without my fix the single step operation wouldn't stop correctly because of the thread-specific breakpoint. You want to make sure you don't regress here while fixing the first two.

I am going ahead and committing this, as I don't think it will cause any regressions on your side. Let me know if you have any concerns.

[tfiala]

This change looks like it introduced a test failure on OS X:
http://lab.llvm.org:8080/green/job/lldb_build_test/16215/

Per another email on lldb-dev, the error emails were not getting generated because, while the test failure was logged and captured, it wasn't failing the build.

lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/consecutive_breakpoints/TestConsecutiveBreakpoints.py

 """
-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
 
 
 
 import unittest2
 import lldb
 import lldbsuite.test.lldbutil as lldbutil
 from lldbsuite.test.lldbtest import *
 
 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()

lldb/trunk/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp

FreeBSDThread::TraceNotify(const ProcessMessage &message)
     assert(GetRegisterContext());
     lldb::addr_t pc = GetRegisterContext()->GetPC();
     if (log)
         log->Printf ("FreeBSDThread::%s () PC=0x%8.8" PRIx64, __FUNCTION__, pc);
     lldb::BreakpointSiteSP bp_site(GetProcess()->GetBreakpointSiteList().FindByAddress(pc));
 
     // 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 (bp_site && (bp_site->ValidForThisThread(this) || GetProcess()->GetOperatingSystem () != NULL))
+        SetStopInfo(StopInfo::CreateStopReasonWithBreakpointSiteID(*this, bp_site->GetID()));
     else
     {
         POSIXBreakpointProtocol* reg_ctx = GetPOSIXBreakpointProtocol();
         if (reg_ctx)
         {
             uint32_t num_hw_wps = reg_ctx->NumSupportedHardwareWatchpoints();
             uint32_t wp_idx;
             for (wp_idx = 0; wp_idx < num_hw_wps; wp_idx++)

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (tid != LLDB_INVALID_THREAD_ID)
                     {
                         if (reason.compare("trace") == 0)
                         {
                             addr_t pc = thread_sp->GetRegisterContext()->GetPC();
                             lldb::BreakpointSiteSP bp_site_sp = thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
 
                             // 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 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);
-                                }
+                            if (bp_site_sp && bp_site_sp->ValidForThisThread(thread_sp.get()))
+                                thread_sp->SetStopInfo(
+                                    StopInfo::CreateStopReasonWithBreakpointSiteID(*thread_sp, bp_site_sp->GetID()));
                             }
                             else
                               thread_sp->SetStopInfo (StopInfo::CreateStopReasonToTrace (*thread_sp));
                             handled = true;
                         }
                         else if (reason.compare("breakpoint") == 0)
                         {
                             addr_t pc = thread_sp->GetRegisterContext()->GetPC();