[26/28] Don't write to inferior_ptid in ravenscar-thread.c

Message ID 20200414175434.8047-27-palves@redhat.com
State New
Headers show
Series
  • Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)
Related show

Commit Message

Simon Marchi via Gdb-patches April 14, 2020, 5:54 p.m.
gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* ravenscar-thread.c (ravenscar_thread_target): Update.
	(ravenscar_thread_target::update_inferior_ptid): Rename to ...
	(ravenscar_thread_target::add_active_thread): ... this.  Don't
	set m_base_ptid here.  Update to avoid referencing inferior_ptid.
	(ravenscar_thread_target::wait): Don't write to inferior_ptid.
---
 gdb/ravenscar-thread.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

-- 
2.14.5

Comments

Tom Tromey April 17, 2020, 6:45 p.m. | #1
>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:


Pedro>    ravenscar_thread_target ()
Pedro> +    : m_base_ptid (inferior_ptid)
Pedro>    {
Pedro> -    update_inferior_ptid ();
Pedro> +    thread_info *thr = add_active_thread ();
Pedro> +    if (thr != nullptr)
Pedro> +      switch_to_thread (thr);
Pedro>    }

I wonder if this will help with the announcement problem fixed here:

https://sourceware.org/pipermail/gdb-patches/2019-March/156455.html

It's not all that important, I still want to try to get that series in.
It's held up on

https://sourceware.org/pipermail/gdb-patches/2019-March/156453.html

Tom
Simon Marchi via Gdb-patches June 18, 2020, 8 p.m. | #2
On 4/17/20 7:45 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:

> 

> Pedro>    ravenscar_thread_target ()

> Pedro> +    : m_base_ptid (inferior_ptid)

> Pedro>    {

> Pedro> -    update_inferior_ptid ();

> Pedro> +    thread_info *thr = add_active_thread ();

> Pedro> +    if (thr != nullptr)

> Pedro> +      switch_to_thread (thr);

> Pedro>    }

> 

> I wonder if this will help with the announcement problem fixed here:

> 

> https://sourceware.org/pipermail/gdb-patches/2019-March/156455.html

> 


I don't think so.  update_inferior_ptid added a thread, and
add_active_thread will still add it.

> It's not all that important, I still want to try to get that series in.

> It's held up on

> 

> https://sourceware.org/pipermail/gdb-patches/2019-March/156453.html

> 


Offhand that seemed like a gross hack to me when I first saw it, which is
why I've had trouble responding.  :-(  But maybe I just misunderstood it.

Thanks,
Pedro Alves
Tom Tromey June 18, 2020, 9:38 p.m. | #3
>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:


>> https://sourceware.org/pipermail/gdb-patches/2019-March/156453.html


Pedro> Offhand that seemed like a gross hack to me when I first saw it, which is
Pedro> why I've had trouble responding.  :-(  But maybe I just misunderstood it.

Yeah, it pretty much is.  On the other hand it helps in this situation.

Probably the gross hack is ravenscar-tdep.c, which is a sort of "green
thread" layer.  Maybe there's some more principled way to tackle this.

Tom

Patch

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index fd3beb03ec..93e02b1f40 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -77,8 +77,11 @@  static const target_info ravenscar_target_info = {
 struct ravenscar_thread_target final : public target_ops
 {
   ravenscar_thread_target ()
+    : m_base_ptid (inferior_ptid)
   {
-    update_inferior_ptid ();
+    thread_info *thr = add_active_thread ();
+    if (thr != nullptr)
+      switch_to_thread (thr);
   }
 
   const target_info &info () const override
@@ -126,9 +129,9 @@  struct ravenscar_thread_target final : public target_ops
   /* PTID of the last thread that received an event.
      This can be useful to determine the associated task that received
      the event, to make it the current task.  */
-  ptid_t m_base_ptid = null_ptid;
+  ptid_t m_base_ptid;
 
-  void update_inferior_ptid ();
+  thread_info *add_active_thread ();
   ptid_t active_task (int cpu);
   bool task_is_currently_active (ptid_t ptid);
   bool runtime_initialized ();
@@ -213,37 +216,36 @@  get_base_thread_from_ravenscar_task (ptid_t ptid)
   return ptid_t (ptid.pid (), base_cpu, 0);
 }
 
-/* Fetch the ravenscar running thread from target memory and
-   update inferior_ptid accordingly.  */
+/* Fetch the ravenscar running thread from target memory, make sure
+   there's a corresponding thread in the thread list, and return it.
+   If the runtime is not initialized, return NULL.  */
 
-void
-ravenscar_thread_target::update_inferior_ptid ()
+thread_info *
+ravenscar_thread_target::add_active_thread ()
 {
   process_stratum_target *proc_target
     = as_process_stratum_target (this->beneath ());
 
   int base_cpu;
 
-  m_base_ptid = inferior_ptid;
-
-  gdb_assert (!is_ravenscar_task (inferior_ptid));
+  gdb_assert (!is_ravenscar_task (m_base_ptid));
   base_cpu = ravenscar_get_thread_base_cpu (m_base_ptid);
 
-  /* If the runtime has not been initialized yet, the inferior_ptid is
-     the only ptid that there is.  */
   if (!runtime_initialized ())
-    return;
+    return nullptr;
 
   /* Make sure we set m_base_ptid before calling active_task
      as the latter relies on it.  */
-  inferior_ptid = active_task (base_cpu);
-  gdb_assert (inferior_ptid != null_ptid);
+  ptid_t active_ptid = active_task (base_cpu);
+  gdb_assert (active_ptid != null_ptid);
 
   /* The running thread may not have been added to
      system.tasking.debug's list yet; so ravenscar_update_thread_list
      may not always add it to the thread list.  Add it here.  */
-  if (!find_thread_ptid (proc_target, inferior_ptid))
-    add_thread (proc_target, inferior_ptid);
+  thread_info *active_thr = find_thread_ptid (proc_target, active_ptid);
+  if (active_thr == nullptr)
+    active_thr = add_thread (proc_target, active_ptid);
+  return active_thr;
 }
 
 /* The Ravenscar Runtime exports a symbol which contains the ID of
@@ -343,12 +345,11 @@  ravenscar_thread_target::wait (ptid_t ptid,
     = as_process_stratum_target (this->beneath ());
   ptid_t event_ptid;
 
-  inferior_ptid = m_base_ptid;
   if (ptid != minus_one_ptid)
     ptid = m_base_ptid;
   event_ptid = beneath->wait (ptid, status, 0);
-  /* Find any new threads that might have been created, and update
-     inferior_ptid to the active thread.
+  /* Find any new threads that might have been created, and return the
+     active thread.
 
      Only do it if the program is still alive, though.  Otherwise,
      this causes problems when debugging through the remote protocol,
@@ -357,13 +358,11 @@  ravenscar_thread_target::wait (ptid_t ptid,
   if (status->kind != TARGET_WAITKIND_EXITED
       && status->kind != TARGET_WAITKIND_SIGNALLED)
     {
-      inferior_ptid = event_ptid;
+      m_base_ptid = event_ptid;
       this->update_thread_list ();
-      this->update_inferior_ptid ();
+      return this->add_active_thread ()->ptid;
     }
-  else
-    inferior_ptid = m_base_ptid;
-  return inferior_ptid;
+  return m_base_ptid;
 }
 
 /* Add the thread associated to the given TASK to the thread list