[2/4] gdb/remote: better management of remote_state::starting_up flag

Message ID 9ad1f89171f822a7107ef0ad34f5363febff53d0.1623268999.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • gdb/remote: spot stop packets sent for the wrong thread
Related show

Commit Message

Andrew Burgess June 9, 2021, 8:06 p.m.
Use scoped_restore to ensure the starting_up flag is always reset to
false when leaving remote_target::start_remote.

There was no bug that inspired this change, rather, while looking at
how this flag was used I spotted that there are a couple of error()
calls in remote_target::start_remote, which would leave GDB with the
remote_state::starting_up flag set to true, which doesn't seem right.

At the end of remote_target::start_remote we were previously setting
remote_state::starting_up back to false before the end of the
function, I've retained this behaviour for now, it should be harmless
to set the starting_up flag back to false and then (later) have the
scoped_restore also set the flag back to false.

I don't believe there should be any user visible changes after this
commit.  In the non-error case I think this is obviously true, the
starting_up flag was set to true on entry to ::start_remote, and was
set back to false before we returned, this is still the case.

In the error () case we would previously leave the ::starting_up flag
set to true, however, this only effected the Ctrl-C handling, thread
creation, or tracepoint download.  As throwing an error during
::start_remote would cause the remote_target to be popped from the
target stack, we would never enter any of the code that checked the
::starting_up flag.  And so, I claim, we will not see any changes in
the error () case either.

gdb/ChangeLog:

	* remote.c (remote_target::start_remote): Use scoped_restore to
	manage remote_state::starting_up flag.
---
 gdb/ChangeLog |  5 +++++
 gdb/remote.c  | 13 +++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.25.4

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 531d43a692b..97268151a59 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4667,7 +4667,9 @@  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;
+  gdb_assert (!rs->starting_up);
+  scoped_restore starting_up_restore
+    = make_scoped_restore (&rs->starting_up, true);
 
   QUIT;
 
@@ -4808,7 +4810,6 @@  remote_target::start_remote (int from_tty, int extended_p)
 
 	  /* We're connected, but not running.  Drop out before we
 	     call start_remote.  */
-	  rs->starting_up = false;
 	  return;
 	}
       else
@@ -4923,7 +4924,6 @@  remote_target::start_remote (int from_tty, int extended_p)
 
 	  /* We're connected, but not running.  Drop out before we
 	     call start_remote.  */
-	  rs->starting_up = false;
 	  return;
 	}
 
@@ -4967,7 +4967,12 @@  remote_target::start_remote (int from_tty, int extended_p)
   /* 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.  */
+     up.
+
+     We manually set the starting_up flag here despite having
+     STARTING_UP_RESTORE which will do this for us at the end of our
+     scope, now we are fully connected we want things like Ctrl-C handling
+     to behave as normal during the following code.  */
   rs->starting_up = false;
 
   /* Maybe breakpoints are global and need to be inserted now.  */