From 288712bbaca36bff6578bc839ebcdc3707662f81 Mon Sep 17 00:00:00 2001
From: Andrew Burgess <aburgess@redhat.com>
Date: Mon, 22 Nov 2021 15:16:27 +0000
Subject: [PATCH] gdb/remote: use scoped_restore to control starting_up flag

This commit makes use of a scoped_restore object to control the
remote_state::starting_up flag within the remote_target::start_remote
method.

Ideally I would have liked to create the scoped_restore inside
start_remote and just leave the restore in place until the end of the
scope, however, I'm worried that doing this would change the behaviour
of GDB.  Specifically, in start_remote, the following code is executed
once the starting_up flag has been restored to its previous value:

  if (breakpoints_should_be_inserted_now ())
    insert_breakpoints ();

I think (but am not 100% sure) that calling install_breakpoints could
end up back inside remote_target::can_download_tracepoint, which does
check the value of remote_state::starting_up.  And so, I'm concerned
that leaving the scoped_restore in place until the end of start_remote
will cause a possible change in behaviour.

To avoid this, and to leave things as close to the current behaviour
as possible, I've split remote_target::start_remote into two, there's
the main function body which moves into remote_target::start_remote_1,
this function uses the scoped_restore to change the ::starting_up
flag, then there's the old remote_target::start_remote, which now just
calls ::start_remote_1, and then does the insert_breakpoints call.

There should be no user visible changes after this commit, unless
there's a situation where the ::starting_up flag could previously have
been left set, if this was the case, then this situation should no
longer be possible.
---
 gdb/remote.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 7df904bf10b..f53e31e126e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -956,7 +956,9 @@ public: /* Remote specific methods.  */
 
   bool vcont_r_supported ();
 
-private: /* data fields */
+private:
+
+  bool start_remote_1 (int from_tty, int extended_p);
 
   /* The remote state.  Don't reference this directly.  Use the
      get_remote_state method instead.  */
@@ -4671,10 +4673,13 @@ remote_target::process_initial_stop_replies (int from_tty)
     }
 }
 
-/* Start the remote connection and sync state.  */
+/* Helper for remote_target::start_remote, start the remote connection and
+   sync state.  Return true if everything goes OK, otherwise, return false.
+   This function exists so that the scoped_restore created within it will
+   expire before we return to remote_target::start_remote.  */
 
-void
-remote_target::start_remote (int from_tty, int extended_p)
+bool
+remote_target::start_remote_1 (int from_tty, int extended_p)
 {
   REMOTE_SCOPED_DEBUG_ENTER_EXIT;
 
@@ -4687,7 +4692,8 @@ remote_target::start_remote (int from_tty, int extended_p)
      Ctrl-C before we're connected and synced up can't interrupt the
      target.  Instead, it offers to drop the (potentially wedged)
      connection.  */
-  rs->starting_up = true;
+  scoped_restore restore_starting_up_flag
+    = make_scoped_restore (&rs->starting_up, true);
 
   QUIT;
 
@@ -4825,11 +4831,7 @@ remote_target::start_remote (int from_tty, int extended_p)
 	{
 	  if (!extended_p)
 	    error (_("The target is not running (try extended-remote?)"));
-
-	  /* We're connected, but not running.  Drop out before we
-	     call start_remote.  */
-	  rs->starting_up = false;
-	  return;
+	  return false;
 	}
       else
 	{
@@ -4940,11 +4942,7 @@ remote_target::start_remote (int from_tty, int extended_p)
 	{
 	  if (!extended_p)
 	    error (_("The target is not running (try extended-remote?)"));
-
-	  /* We're connected, but not running.  Drop out before we
-	     call start_remote.  */
-	  rs->starting_up = false;
-	  return;
+	  return false;
 	}
 
       /* Report all signals during attach/startup.  */
@@ -4984,14 +4982,16 @@ remote_target::start_remote (int from_tty, int extended_p)
      previously; find out where things are at.  */
   remote_btrace_maybe_reopen ();
 
-  /* The thread and inferior lists are now synchronized with the
-     target, our symbols have been relocated, and we're merged the
-     target's tracepoints with ours.  We're done with basic start
-     up.  */
-  rs->starting_up = false;
+  return true;
+}
+
+/* Start the remote connection and sync state.  */
 
-  /* Maybe breakpoints are global and need to be inserted now.  */
-  if (breakpoints_should_be_inserted_now ())
+void
+remote_target::start_remote (int from_tty, int extended_p)
+{
+  if (start_remote_1 (from_tty, extended_p)
+      && breakpoints_should_be_inserted_now ())
     insert_breakpoints ();
 }
 
-- 
2.30.2