Page MenuHomePhabricator

Fix single-stepping onto a breakpoint
ClosedPublic

Authored by labath on Feb 1 2016, 4:49 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 46523.Feb 1 2016, 4:49 AM
labath retitled this revision from to Fix single-stepping onto a breakpoint.
labath updated this object.
labath added reviewers: tberghammer, emaste.
tberghammer accepted this revision.Feb 1 2016, 5:23 AM
tberghammer edited edge metadata.

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

This revision is now accepted and ready to land.Feb 1 2016, 5:23 AM

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 updated this revision to Diff 46528.Feb 1 2016, 6:19 AM
labath edited edge metadata.

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

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 added a comment.Feb 2 2016, 2:43 AM

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.

This revision was automatically updated to reflect the committed changes.
tfiala added a subscriber: tfiala.Feb 2 2016, 8:14 AM

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.