[lldb] Move breakpoint hit reset code to Target::CleanupProcess

[labath]

Summary

This ensures it is run regardless of the method we use to initiate the
session (previous version did not handle connects), and it is the same
place that is used for resetting watchpoints.

[jingham]

If there's some guarantee that Target::CreateProcess HAS to be the way that a process is created, then this is fine. Is there such a guarantee - the function is pretty trivial so somebody might be tempted to do the work in some other place but miss the breakpoint resetting.

[labath]

I'm not sure what kind of guarantees are you looking for. m_process_sp is a private member of the Target class, so it's not like just anyone can come in and change it. There's no way to stop code from inside the Target class from changing it without going through the CreateProcess method, but the same can be said about the calls to WillXXX methods in the process class.

[jingham]

The WillXXX have the virtue that they are at least self-documenting "Before you do XXX this will be called." As it stands somebody working on Target could be forgiven for thinking Target::CreateProcess was just a convenience method, and redo it somewhere inline forgetting the breakpoint resetting. If we are going to require CreateProcess as the only way to do this, we should at least state that somewhere.

[labath]

use CleanupProcess instead

[labath]

Here's an even better idea. I've now doing it in Target::CleanupProcess, right next to the code for resetting watchpoint hit counts.

[jingham]

That seems fine. I think it's useful to be able to see breakpoint hit counts up to the point where you start a new process. From looking at the code, it looks like putting the clear in CleanupProcess will do that. If you agree this is useful, can you add to the test that the hit count is still 1 between process.Kill and connecting to the new process? Other than that this seems fine.

[labath]

Check breakpoint hit counts after termination

[labath]

In D134882#3831137, @jingham wrote:

That seems fine. I think it's useful to be able to see breakpoint hit counts up to the point where you start a new process. From looking at the code, it looks like putting the clear in CleanupProcess will do that. If you agree this is useful, can you add to the test that the hit count is still 1 between process.Kill and connecting to the new process?

That makes sense to me. I've added the extra check.

lldb/source/Target/Process.cpp

 ListenerSP ProcessAttachInfo::GetListenerForProcess(Debugger &debugger) {
   if (m_listener_sp)
     return m_listener_sp;
   else
     return debugger.GetListener();
 }
 
 Status Process::WillLaunch(Module *module) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillLaunch(module);
 }
 
 Status Process::WillAttachToProcessWithID(lldb::pid_t pid) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithID(pid);
 }
 
 Status Process::WillAttachToProcessWithName(const char *process_name,
                                             bool wait_for_launch) {
-  GetTarget().ResetBreakpointHitCounts();
   return DoWillAttachToProcessWithName(process_name, wait_for_launch);
 }
 
 Status Process::Attach(ProcessAttachInfo &attach_info) {
   m_abi_sp.reset();
   m_process_input_reader.reset();
   m_dyld_up.reset();
   m_jit_loaders_up.reset();

lldb/source/Target/Target.cpp

 }
 
 void Target::CleanupProcess() {
   // Do any cleanup of the target we need to do between process instances.
   // NB It is better to do this before destroying the process in case the
   // clean up needs some help from the process.
   m_breakpoint_list.ClearAllBreakpointSites();
   m_internal_breakpoint_list.ClearAllBreakpointSites();
+  ResetBreakpointHitCounts();
   // Disable watchpoints just on the debugger side.
   std::unique_lock<std::recursive_mutex> lock;
   this->GetWatchpointList().GetListMutex(lock);
   DisableAllWatchpoints(false);
   ClearAllWatchpointHitCounts();
   ClearAllWatchpointHistoricValues();
   m_latest_stop_hook_id = 0;
 }

lldb/test/API/functionalities/gdb_remote_client/TestProcessConnect.py

def test_process_connect_async(self):
                         matching=False,
                         substrs=['Process', 'stopped'])
             lldbutil.expect_state_changes(self, self.dbg.GetListener(),
                                           self.process(), [lldb.eStateStopped])
         finally:
             self.dbg.GetSelectedTarget().GetProcess().Kill()
         lldbutil.expect_state_changes(self, self.dbg.GetListener(),
                                       self.process(), [lldb.eStateExited])
+    def test_breakpoint_count(self):
+        """
+        Test that breakpoint count gets reset for each new connection.
+        """
+        class MyResponder(MockGDBServerResponder):
+
+            def __init__(self):
+                super().__init__()
+                self.continued = False
+
+            def qfThreadInfo(self):
+                return "m47"
+
+            def qsThreadInfo(self):
+                return "l"
+
+            def setBreakpoint(self, packet):
+                return "OK"
+
+            def readRegister(self, reg):
+                # Pretend we're at the breakpoint after we've been resumed.
+                return "3412000000000000" if self.continued else "4747000000000000"
+
+            def cont(self):
+                self.continued = True
+                return "T05thread=47;reason:breakpoint"
+
+        # Connect to the first process and set our breakpoint.
+        self.server.responder = MyResponder()
+        target = self.createTarget("a.yaml")
+        process = self.connect(target)
+
+        bkpt = target.BreakpointCreateByAddress(0x1234)
+        self.assertTrue(bkpt.IsValid())
+        self.assertEqual(bkpt.GetNumLocations(), 1)
+
+        # "continue" the process. It should hit our breakpoint.
+        process.Continue()
+        self.assertState(process.GetState(), lldb.eStateStopped)
+        self.assertEqual(bkpt.GetHitCount(), 1)
+
+        # Now kill it. The breakpoint should still show a hit count of one.
+        process.Kill()
+        self.server.stop()
+        self.assertEqual(bkpt.GetHitCount(), 1)
+
+        # Start over, and reconnect.
+        self.server = MockGDBServer(self.server_socket_class())
+        self.server.start()
+
+        process = self.connect(target)
+
+        # The hit count should be reset.
+        self.assertEqual(bkpt.GetHitCount(), 0)