gdb: make thread_suspend_state::stop_pc optional

Currently the stop_pc field of thread_suspect_state is a CORE_ADDR and
when we want to indicate that there is no stop_pc available we set
this field back to a special value.

There are actually two special values used, in post_create_inferior
the stop_pc is set to 0.  This is a little unfortunate, there are
plenty of embedded targets where 0 is a valid pc value.  The more
common special value for stop_pc though, is set in
thread_info::set_executing, where the value (~(CORE_ADDR) 0) is used.

This commit changes things so that the stop_pc is instead a
gdb::optional.  We can now explicitly reset the field to an
uninitialised state, we also have asserts that we don't read the
stop_pc when its in an uninitialised state (both in
gdbsupport/gdb_optional.h, when compiling with _GLIBCXX_DEBUG
defined, and in thread_info::stop_pc).

One situation where a thread will not have a stop_pc value is when the
thread is stopped as a consequence of GDB being in all stop mode, and
some other thread stopped at an interesting event.  When GDB brings
all the other threads to a stop those other threads will not have a
stop_pc set (thus avoiding an unnecessary read of the pc register).

Previously, when GDB passed through handle_one (in infrun.c) the
threads executing flag was set to false and the stop_pc field was left
unchanged, i.e. it would (previous) have been left as ~0.

Now, handle_one leaves the stop_pc with no value.

This caused a problem when we later try to set these threads running
again, in proceed() we compare the current pc with the cached stop_pc.
If the thread was stopped via handle_one then the stop_pc would have
been left as ~0, and the compare (in proceed) would (likely) fail.
Now however, this compare tries to read the stop_pc when it has no
value and this would trigger an assert.

To resolve this I've added thread_info::stop_pc_p() which returns true
if the thread has a cached stop_pc.  We should only ever call
thread_info::stop_pc() if we know that there is a cached stop_pc,
however, this doesn't mean that every call to thread_info::stop_pc()
needs to be guarded with a call to thread_info::stop_pc_p(), in most
cases we know that the thread we are looking at stopped due to some
interesting event in that thread, and so, we know that the stop_pc is
valid.

After running the testsuite I've seen no other situations where
stop_pc is read uninitialised.

There should be no user visible changes after this commit.

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index e6f383cca610e9afd0e7cc69e2114475d0105663..4b271943d50901f31c2f2771108e588016caa212 100644 (file)
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -197,9 +197,12 @@ struct thread_suspend_state
        stop_reason: if the thread's PC has changed since the thread
        last stopped, a pending breakpoint waitstatus is discarded.
 
-     - If the thread is running, this is set to -1, to avoid leaving
-       it with a stale value, to make it easier to catch bugs.  */
-  CORE_ADDR stop_pc = 0;
+     - If the thread is running, then this field has its value removed by
+       calling stop_pc.reset() (see thread_info::set_executing()).
+       Attempting to read a gdb::optional with no value is undefined
+       behaviour and will trigger an assertion error when _GLIBCXX_DEBUG is
+       defined, which should make error easier to track down.  */
+  gdb::optional<CORE_ADDR> stop_pc;
 };
 
 /* Base class for target-specific thread data.  */
@@ -327,11 +330,15 @@ public:
     m_suspend = suspend;
   }
 
-  /* Return this thread's stop PC.  */
+  /* Return this thread's stop PC.  This should only be called when it is
+     known that stop_pc has a value.  If this function is being used in a
+     situation where a thread may not have had a stop_pc assigned, then
+     stop_pc_p() can be used to check if the stop_pc is defined.  */
 
   CORE_ADDR stop_pc () const
   {
-    return m_suspend.stop_pc;
+    gdb_assert (m_suspend.stop_pc.has_value ());
+    return *m_suspend.stop_pc;
   }
 
   /* Set this thread's stop PC.  */
@@ -341,6 +348,21 @@ public:
     m_suspend.stop_pc = stop_pc;
   }
 
+  /* Remove the stop_pc stored on this thread.  */
+
+  void clear_stop_pc ()
+  {
+    m_suspend.stop_pc.reset ();
+  }
+
+  /* Return true if this thread has a cached stop pc value, otherwise
+     return false.  */
+
+  bool stop_pc_p () const
+  {
+    return m_suspend.stop_pc.has_value ();
+  }
+
   /* Return true if this thread has a pending wait status.  */
 
   bool has_pending_waitstatus () const

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index e6ee49bc43de9fe98b80e1c241c9e771f971e214..d948f4bafc5ce1a7844102a41bec3687a49b8670 100644 (file)
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -249,7 +249,7 @@ post_create_inferior (int from_tty)
      missing registers info), ignore it.  */
   thread_info *thr = inferior_thread ();
 
-  thr->set_stop_pc (0);
+  thr->clear_stop_pc ();
   try
     {
       regcache *rc = get_thread_regcache (thr);

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8e778576eabbeca26e934a52fc7cbc80c2bf23d7..d1ac9b4cbbb51955e35ad960b598240bbe87777a 100644 (file)
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3051,7 +3051,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
 
   if (addr == (CORE_ADDR) -1)
     {
-      if (pc == cur_thr->stop_pc ()
+      if (cur_thr->stop_pc_p ()
+         && pc == cur_thr->stop_pc ()
          && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here
          && execution_direction != EXEC_REVERSE)
        /* There is a breakpoint at the address we will resume at,

diff --git a/gdb/thread.c b/gdb/thread.c
index c95a9186681dfd217e330c4ec9c9b8678bf31abd..10c3dcd6991f919a561652f942b35f7c637917d9 100644 (file)
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -324,7 +324,7 @@ thread_info::set_executing (bool executing)
 {
   m_executing = executing;
   if (executing)
-    this->set_stop_pc (~(CORE_ADDR) 0);
+    this->clear_stop_pc ();
 }
 
 /* See gdbthread.h.  */