Bug 30863 - Step doesn't stop with conditional breakpoint on the next line

Differential Revisions:
  https://reviews.llvm.org/D26497 (committed r290168, temporary reverted r290197)
  https://reviews.llvm.org/D28945 (fix for Ubuntu tests fail)

llvm-svn: 292930

lldb/include/lldb/Target/Thread.h

@@ -126,6 +126,7 @@ public:
                            // bit of data.
     lldb::StopInfoSP stop_info_sp; // You have to restore the stop info or you
                                    // might continue with the wrong signals.
+    std::vector<lldb::ThreadPlanSP> m_completed_plan_stack;
     lldb::RegisterCheckpointSP
         register_backup_sp; // You need to restore the registers, of course...
     uint32_t current_inlined_depth;
@@ -1028,6 +1029,15 @@ public:
   //------------------------------------------------------------------
   bool WasThreadPlanDiscarded(ThreadPlan *plan);
 
+  //------------------------------------------------------------------
+  /// Check if we have completed plan to override breakpoint stop reason
+  ///
+  /// @return
+  ///     Returns true if completed plan stack is not empty
+  ///     false otherwise.
+  //------------------------------------------------------------------
+  bool CompletedPlanOverridesBreakpoint();
+                   
   //------------------------------------------------------------------
   /// Queues a generic thread plan.
   ///
@@ -1213,6 +1223,8 @@ public:
 
   void SetStopInfo(const lldb::StopInfoSP &stop_info_sp);
 
+  void ResetStopInfo();
+
   void SetShouldReportStop(Vote vote);
 
   //----------------------------------------------------------------------

lldb/include/lldb/Target/ThreadPlan.h

@@ -40,9 +40,10 @@ namespace lldb_private {
 //  The thread maintaining a thread plan stack, and you program the actions of a
 //  particular thread
 //  by pushing plans onto the plan stack.
-//  There is always a "Current" plan, which is the head of the plan stack,
+//  There is always a "Current" plan, which is the top of the plan stack,
 //  though in some cases
-//  a plan may defer to plans higher in the stack for some piece of information.
+//  a plan may defer to plans higher in the stack for some piece of information
+//  (let us define that the plan stack grows downwards).
 //
 //  The plan stack is never empty, there is always a Base Plan which persists
 //  through the life
@@ -109,6 +110,15 @@ namespace lldb_private {
 //  plans in the time between when
 //  your plan gets unshipped and the next resume.
 //
+//  Thread State Checkpoint:
+//
+//  Note that calling functions on target process (ThreadPlanCallFunction) changes
+//  current thread state. The function can be called either by direct user demand or
+//  internally, for example lldb allocates memory on device to calculate breakpoint
+//  condition expression - on Linux it is performed by calling mmap on device.
+//  ThreadStateCheckpoint saves Thread state (stop info and completed
+//  plan stack) to restore it after completing function call.
+//
 //  Over the lifetime of the plan, various methods of the ThreadPlan are then
 //  called in response to changes of state in
 //  the process we are debugging as follows:
@@ -149,7 +159,7 @@ namespace lldb_private {
 //  If the Current plan answers "true" then it is asked if the stop should
 //  percolate all the way to the
 //  user by calling the ShouldStop method.  If the current plan doesn't explain
-//  the stop, then we query down
+//  the stop, then we query up
 //  the plan stack for a plan that does explain the stop.  The plan that does
 //  explain the stop then needs to
 //  figure out what to do about the plans below it in the stack.  If the stop is
@@ -170,7 +180,7 @@ namespace lldb_private {
 //  event it didn't directly handle
 //  it can designate itself a "Master" plan by responding true to IsMasterPlan,
 //  and then if it wants not to be
-//  discarded, it can return true to OkayToDiscard, and it and all its dependent
+//  discarded, it can return false to OkayToDiscard, and it and all its dependent
 //  plans will be preserved when
 //  we resume execution.
 //
@@ -207,7 +217,7 @@ namespace lldb_private {
 //
 //  If a plan says responds "true" to ShouldStop, then it is asked if it's job
 //  is complete by calling
-//  MischiefManaged.  If that returns true, the thread is popped from the plan
+//  MischiefManaged.  If that returns true, the plan is popped from the plan
 //  stack and added to the
 //  Completed Plan Stack.  Then the next plan in the stack is asked if it
 //  ShouldStop, and  it returns "true",
@@ -241,9 +251,9 @@ namespace lldb_private {
 //
 //  When the process stops, the thread is given a StopReason, in the form of a
 //  StopInfo object.  If there is a completed
-//  plan corresponding to the stop, then the "actual" stop reason will be
+//  plan corresponding to the stop, then the "actual" stop reason can be
 //  suppressed, and instead a StopInfoThreadPlan
-//  object will be cons'ed up from the highest completed plan in the stack.
+//  object will be cons'ed up from the top completed plan in the stack.
 //  However, if the plan doesn't want to be
 //  the stop reason, then it can call SetPlanComplete and pass in "false" for
 //  the "success" parameter.  In that case,

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/step_over_breakpoint/Makefile

@@ -0,0 +1,9 @@
+LEVEL = ../../../make
+
+CXX_SOURCES := main.cpp
+
+ifneq (,$(findstring icc,$(CC)))
+    CXXFLAGS += -debug inline-debug-info
+endif
+
+include $(LEVEL)/Makefile.rules

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

@@ -0,0 +1,119 @@
+"""
+Test that breakpoints do not affect stepping.
+Check for correct StopReason when stepping to the line with breakpoint 
+which chould be eStopReasonBreakpoint in general,
+and eStopReasonPlanComplete when breakpoint's condition fails.  
+"""
+
+from __future__ import print_function
+
+import unittest2
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class StepOverBreakpointsTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        TestBase.setUp(self)
+       
+        self.build()
+        exe = os.path.join(os.getcwd(), "a.out")
+        src = lldb.SBFileSpec("main.cpp")
+
+        # Create a target by the debugger.
+        self.target = self.dbg.CreateTarget(exe)
+        self.assertTrue(self.target, VALID_TARGET)
+
+        # Setup four breakpoints, two of them with false condition
+        self.line1 = line_number('main.cpp', "breakpoint_1")
+        self.line4 = line_number('main.cpp', "breakpoint_4")
+
+        self.breakpoint1 = self.target.BreakpointCreateByLocation(src, self.line1)        
+        self.assertTrue(
+            self.breakpoint1 and self.breakpoint1.GetNumLocations() == 1,
+            VALID_BREAKPOINT)
+
+        self.breakpoint2 = self.target.BreakpointCreateBySourceRegex("breakpoint_2", src)
+        self.breakpoint2.GetLocationAtIndex(0).SetCondition('false')
+
+        self.breakpoint3 = self.target.BreakpointCreateBySourceRegex("breakpoint_3", src)
+        self.breakpoint3.GetLocationAtIndex(0).SetCondition('false')
+
+        self.breakpoint4 = self.target.BreakpointCreateByLocation(src, self.line4)
+
+        # Start debugging
+        self.process = self.target.LaunchSimple(
+            None, None, self.get_process_working_directory())
+        self.assertIsNotNone(self.process, PROCESS_IS_VALID)
+        self.thread = lldbutil.get_one_thread_stopped_at_breakpoint(self.process, self.breakpoint1)
+        self.assertIsNotNone(self.thread, "Didn't stop at breakpoint 1.")
+
+    def test_step_instruction(self): 
+        # Count instructions between breakpoint_1 and breakpoint_4
+        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()
+        for i in range(instructions.GetSize()) :
+            addr = instructions.GetInstructionAtIndex(i).GetAddress()
+            if (addr == addr_1) : index_1 = i
+            if (addr == addr_4) : index_4 = i 
+
+        steps_expected = index_4 - index_1
+        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)
+            if (self.thread.GetStopReason() == lldb.eStopReasonBreakpoint) :
+                # we should not stop on breakpoint_2 and _3 because they have false condition
+                self.assertEquals(self.thread.GetFrameAtIndex(0).GetLineEntry().GetLine(), self.line4)
+                # breakpoint_2 and _3 should not affect step count
+                self.assertTrue(step_count >= steps_expected)
+                break
+
+        # Run the process until termination
+        self.process.Continue()
+        self.assertEquals(self.process.GetState(), lldb.eStateExited)
+
+    def test_step_over(self):
+        #lldb.DBG.EnableLog("lldb", ["step","breakpoint"])
+        
+        self.thread.StepOver()
+        # We should be stopped at the breakpoint_2 line with stop plan complete reason
+        self.assertEquals(self.process.GetState(), lldb.eStateStopped)
+        self.assertEquals(self.thread.GetStopReason(), lldb.eStopReasonPlanComplete)
+
+        self.thread.StepOver()
+        # We should be stopped at the breakpoint_3 line with stop plan complete reason
+        self.assertEquals(self.process.GetState(), lldb.eStateStopped)
+        self.assertEquals(self.thread.GetStopReason(), lldb.eStopReasonPlanComplete)
+
+        self.thread.StepOver()
+        # We should be stopped at the breakpoint_4
+        self.assertEquals(self.process.GetState(), lldb.eStateStopped)
+        self.assertEquals(self.thread.GetStopReason(), lldb.eStopReasonBreakpoint)
+        thread1 = lldbutil.get_one_thread_stopped_at_breakpoint(self.process, self.breakpoint4)
+        self.assertEquals(self.thread, thread1, "Didn't stop at breakpoint 4.")
+
+        # Check that stepping does not affect breakpoint's hit count
+        self.assertEquals(self.breakpoint1.GetHitCount(), 1)
+        self.assertEquals(self.breakpoint2.GetHitCount(), 0)
+        self.assertEquals(self.breakpoint3.GetHitCount(), 0)
+        self.assertEquals(self.breakpoint4.GetHitCount(), 1)
+
+        # Run the process until termination
+        self.process.Continue()
+        self.assertEquals(self.process.GetState(), lldb.eStateExited)
+

lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/step_over_breakpoint/main.cpp

@@ -0,0 +1,12 @@
+
+int func() { return 1; }
+
+int
+main(int argc, char const *argv[])
+{
+    int a = 0;      // breakpoint_1
+    int b = func(); // breakpoint_2
+    a = b + func(); // breakpoint_3
+    return 0;       // breakpoint_4
+}
+

lldb/source/Target/StopInfo.cpp

@@ -269,6 +269,7 @@ protected:
     if (!m_should_perform_action)
       return;
     m_should_perform_action = false;
+    bool internal_breakpoint = true;
 
     ThreadSP thread_sp(m_thread_wp.lock());
 
@@ -495,6 +496,9 @@ protected:
             if (callback_says_stop)
               m_should_stop = true;
 
+            if (m_should_stop && !bp_loc_sp->GetBreakpoint().IsInternal())
+              internal_breakpoint = false;
+                  
             // If we are going to stop for this breakpoint, then remove the
             // breakpoint.
             if (callback_says_stop && bp_loc_sp &&
@@ -526,6 +530,20 @@ protected:
               "Process::%s could not find breakpoint site id: %" PRId64 "...",
               __FUNCTION__, m_value);
       }
+
+      if ((m_should_stop == false || internal_breakpoint)
+          && thread_sp->CompletedPlanOverridesBreakpoint()) {
+        
+        // Override should_stop decision when we have
+        // completed step plan additionally to the breakpoint
+        m_should_stop = true;
+        
+        // Here we clean the preset stop info so the next
+        // GetStopInfo call will find the appropriate stop info,
+        // which should be the stop info related to the completed plan
+        thread_sp->ResetStopInfo();
+      }
+
       if (log)
         log->Printf("Process::%s returning from action with m_should_stop: %d.",
                     __FUNCTION__, m_should_stop);

lldb/source/Target/Thread.cpp

@@ -380,24 +380,32 @@ lldb::StopInfoSP Thread::GetStopInfo() {
   if (m_destroy_called)
     return m_stop_info_sp;
 
-  ThreadPlanSP plan_sp(GetCompletedPlan());
+  ThreadPlanSP completed_plan_sp(GetCompletedPlan());
   ProcessSP process_sp(GetProcess());
   const uint32_t stop_id = process_sp ? process_sp->GetStopID() : UINT32_MAX;
-  if (plan_sp && plan_sp->PlanSucceeded()) {
-    return StopInfo::CreateStopReasonWithPlan(plan_sp, GetReturnValueObject(),
-                                              GetExpressionVariable());
+
+  // Here we select the stop info according to priorirty:
+  // - m_stop_info_sp (if not trace) - preset value
+  // - completed plan stop info - new value with plan from completed plan stack
+  // - m_stop_info_sp (trace stop reason is OK now)
+  // - ask GetPrivateStopInfo to set stop info
+
+  bool have_valid_stop_info = m_stop_info_sp &&
+      m_stop_info_sp ->IsValid() &&
+      m_stop_info_stop_id == stop_id;
+  bool have_valid_completed_plan = completed_plan_sp && completed_plan_sp->PlanSucceeded();
+  bool plan_overrides_trace =
+    have_valid_stop_info && have_valid_completed_plan
+    && (m_stop_info_sp->GetStopReason() == eStopReasonTrace);
+    
+  if (have_valid_stop_info && !plan_overrides_trace) {
+    return m_stop_info_sp;
+  } else if (have_valid_completed_plan) {
+    return StopInfo::CreateStopReasonWithPlan(
+        completed_plan_sp, GetReturnValueObject(), GetExpressionVariable());
   } else {
-    if ((m_stop_info_stop_id == stop_id) || // Stop info is valid, just return
-                                            // what we have (even if empty)
-        (m_stop_info_sp &&
-         m_stop_info_sp
-             ->IsValid())) // Stop info is valid, just return what we have
-    {
-      return m_stop_info_sp;
-    } else {
-      GetPrivateStopInfo();
-      return m_stop_info_sp;
-    }
+    GetPrivateStopInfo();
+    return m_stop_info_sp;
   }
 }
 
@@ -459,6 +467,12 @@ bool Thread::StopInfoIsUpToDate() const {
                  // date...
 }
 
+void Thread::ResetStopInfo() {
+  if (m_stop_info_sp) {
+    m_stop_info_sp.reset();
+  }
+}
+
 void Thread::SetStopInfo(const lldb::StopInfoSP &stop_info_sp) {
   m_stop_info_sp = stop_info_sp;
   if (m_stop_info_sp) {
@@ -526,7 +540,8 @@ bool Thread::CheckpointThreadState(ThreadStateCheckpoint &saved_state) {
   if (process_sp)
     saved_state.orig_stop_id = process_sp->GetStopID();
   saved_state.current_inlined_depth = GetCurrentInlinedDepth();
-
+  saved_state.m_completed_plan_stack = m_completed_plan_stack;
+	
   return true;
 }
 
@@ -559,6 +574,7 @@ bool Thread::RestoreThreadStateFromCheckpoint(
   SetStopInfo(saved_state.stop_info_sp);
   GetStackFrameList()->SetCurrentInlinedDepth(
       saved_state.current_inlined_depth);
+  m_completed_plan_stack = saved_state.m_completed_plan_stack;
   return true;
 }
 
@@ -895,6 +911,9 @@ bool Thread::ShouldStop(Event *event_ptr) {
 
   if (should_stop) {
     ThreadPlan *plan_ptr = GetCurrentPlan();
+
+    // Discard the stale plans and all plans below them in the stack,
+    // plus move the completed plans to the completed plan stack
     while (!PlanIsBasePlan(plan_ptr)) {
       bool stale = plan_ptr->IsPlanStale();
       ThreadPlan *examined_plan = plan_ptr;
@@ -905,7 +924,15 @@ bool Thread::ShouldStop(Event *event_ptr) {
           log->Printf(
               "Plan %s being discarded in cleanup, it says it is already done.",
               examined_plan->GetName());
-        DiscardThreadPlansUpToPlan(examined_plan);
+        while (GetCurrentPlan() != examined_plan) {
+          DiscardPlan();
+        }
+        if (examined_plan->IsPlanComplete()) {
+          // plan is complete but does not explain the stop (example: step to a line
+          // with breakpoint), let us move the plan to completed_plan_stack anyway
+          PopPlan();
+        } else
+          DiscardPlan();
       }
     }
   }
@@ -1133,6 +1160,10 @@ bool Thread::WasThreadPlanDiscarded(ThreadPlan *plan) {
   return false;
 }
 
+bool Thread::CompletedPlanOverridesBreakpoint() {
+  return (!m_completed_plan_stack.empty()) ;
+}
+
 ThreadPlan *Thread::GetPreviousPlan(ThreadPlan *current_plan) {
   if (current_plan == nullptr)
     return nullptr;

lldb/source/Target/ThreadPlanStepInstruction.cpp

@@ -94,6 +94,15 @@ bool ThreadPlanStepInstruction::IsPlanStale() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
   StackID cur_frame_id = m_thread.GetStackFrameAtIndex(0)->GetStackID();
   if (cur_frame_id == m_stack_id) {
+    // Set plan Complete when we reach next instruction
+    uint64_t pc = m_thread.GetRegisterContext()->GetPC(0);
+    uint32_t max_opcode_size = m_thread.CalculateTarget()
+        ->GetArchitecture().GetMaximumOpcodeByteSize();
+    bool next_instruction_reached = (pc > m_instruction_addr) &&
+        (pc <= m_instruction_addr + max_opcode_size);
+    if (next_instruction_reached) {
+      SetPlanComplete();
+    }
     return (m_thread.GetRegisterContext()->GetPC(0) != m_instruction_addr);
   } else if (cur_frame_id < m_stack_id) {
     // If the current frame is younger than the start frame and we are stepping

lldb/source/Target/ThreadPlanStepRange.cpp

@@ -461,6 +461,16 @@ bool ThreadPlanStepRange::IsPlanStale() {
     // One tricky bit here is that some stubs don't push a frame, so we should.
     // check that we are in the same symbol.
     if (!InRange()) {
+      // Set plan Complete when we reach next instruction just after the range
+      lldb::addr_t addr = m_thread.GetRegisterContext()->GetPC() - 1;
+      size_t num_ranges = m_address_ranges.size();
+      for (size_t i = 0; i < num_ranges; i++) {
+        bool in_range = m_address_ranges[i].ContainsLoadAddress(
+            addr, m_thread.CalculateTarget().get());
+        if (in_range) {
+          SetPlanComplete();
+        }
+      }
       return true;
     }
   }