[PATCH v2 00/12] Fix Ravenscar regressions

[PATCH v2 00/12] Fix Ravenscar regressions

[Tom Tromey]

Some changes in gdb caused regressions in AdaCore's internal Ravenscar
testing.  This is understandable, because a Ravenscar environment is a
bit of a pain to set up, and so I don't think it's tested in ordinary
gdb testing.

This series is v2 of some fixes to Ravenscar.  A few more fixes have
been added since the last series, to cope with new regressions.  I've
also dropped the "random thread switch" patch from the series.  I hope
we can discuss that one again sometime, but meanwhile I'd rather get
these changes in.

Tom

[PATCH v2 01/12] Call add_active_thread after pushing the ravenscar target

[Tom Tromey]

Currently ravenscar-thread.c calls add_active_thread before pushing
the ravenscar target.  This yields an initial thread announcement of
"[Thread 0]".  Calling add_active_thread after pushing the target
fixes this.

2019-03-26  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_thread_target): Don't call
	add_active_thread.
	(ravenscar_thread_target::add_active_thread): Now public.
	(ravenscar_inferior_created): Call add_active_thread after pushing
	the target.
---
 gdb/ChangeLog          |  8 ++++++++
 gdb/ravenscar-thread.c | 13 +++++++------
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 72253188c71..7fca51da2a7 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -79,9 +79,6 @@ struct ravenscar_thread_target final : public target_ops
   ravenscar_thread_target ()
     : m_base_ptid (inferior_ptid)
   {
-    thread_info *thr = add_active_thread ();
-    if (thr != nullptr)
-      switch_to_thread (thr);
   }
 
   const target_info &info () const override
@@ -124,6 +121,8 @@ struct ravenscar_thread_target final : public target_ops
     delete this;
   }
 
+  thread_info *add_active_thread ();
+
 private:
 
   /* PTID of the last thread that received an event.
@@ -131,7 +130,6 @@ struct ravenscar_thread_target final : public target_ops
      the event, to make it the current task.  */
   ptid_t m_base_ptid;
 
-  thread_info *add_active_thread ();
   ptid_t active_task (int cpu);
   bool task_is_currently_active (ptid_t ptid);
   bool runtime_initialized ();
@@ -548,8 +546,11 @@ ravenscar_inferior_created (struct target_ops *target, int from_tty)
       return;
     }
 
-  target_ops_up target_holder (new ravenscar_thread_target ());
-  push_target (std::move (target_holder));
+  ravenscar_thread_target *rtarget = new ravenscar_thread_target ();
+  push_target (target_ops_up (rtarget));
+  thread_info *thr = rtarget->add_active_thread ();
+  if (thr != nullptr)
+    switch_to_thread (thr);
 }
 
 ptid_t
-- 
2.26.2

[PATCH v2 02/12] Avoid crash in ravenscar_thread_target::wait

[Tom Tromey]

An earlier patch caused a Ravenscar regression in
ravenscar_thread_target::wait.  In particular, add_active_thread can
return NULL when the runtime is not initialized.

gdb/ChangeLog
2020-08-05  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_thread_target::wait): Check
	runtime_initialized.
---
 gdb/ChangeLog          | 5 +++++
 gdb/ravenscar-thread.c | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 7fca51da2a7..47001de42d3 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -354,7 +354,8 @@ ravenscar_thread_target::wait (ptid_t ptid,
      because we might try switching threads (and thus sending packets)
      after the remote has disconnected.  */
   if (status->kind != TARGET_WAITKIND_EXITED
-      && status->kind != TARGET_WAITKIND_SIGNALLED)
+      && status->kind != TARGET_WAITKIND_SIGNALLED
+      && runtime_initialized ())
     {
       m_base_ptid = event_ptid;
       this->update_thread_list ();
-- 
2.26.2

[PATCH v2 03/12] Return event_ptid from ravenscar_thread_target::wait

[Tom Tromey]

ravenscar_thread_target::wait should return the event ptid from the
wrapped "wait" call in the situation where returning the Ravenscar
thread ptid is not appropriate.  This probably does not really make a
difference in practice, but it seemed like a reasonable cleanup.

gdb/ChangeLog
2020-08-05  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_thread_target::wait): Return
	event_ptid.
---
 gdb/ChangeLog          | 5 +++++
 gdb/ravenscar-thread.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 47001de42d3..b89b5f62e5a 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -361,7 +361,7 @@ ravenscar_thread_target::wait (ptid_t ptid,
       this->update_thread_list ();
       return this->add_active_thread ()->ptid;
     }
-  return m_base_ptid;
+  return event_ptid;
 }
 
 /* Add the thread associated to the given TASK to the thread list
-- 
2.26.2

[PATCH v2 04/12] Handle case where Ada task is current but not listed

[Tom Tromey]

Currently, the ravenscar runtime can mark an Ada task as the current
task, before adding it to the list of tasks that can be read by gdb.
In this scenario, gdb can sometimes crash in
ravenscar_get_thread_base_cpu with:

../../src/gdb/ravenscar-thread.c:167: internal-error: int ravenscar_get_thread_base_cpu(ptid_t): Assertion `task_info != NULL' failed.

However, as ravenscar_get_thread_base_cpu is only called to find the
base CPU, we can simply record this when registering the thread, and
look this up later.

2019-03-26  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_thread_target) <get_base_cpu,
	get_base_thread_from_ravenscar_task>: Now methods.
	<m_cpu_map>: New member.
	(ravenscar_thread_target::get_thread_base_cpu): Rename from
	ravenscar_get_thread_base_cpu.  Check m_cpu_map.
	(ravenscar_thread_target::task_is_currently_active): Update.
	(ravenscar_thread_target::get_base_thread_from_ravenscar_task):
	Now a method.
	(ravenscar_thread_target::add_active_thread): Put initial thread
	into the m_cpu_map.
---
 gdb/ChangeLog          | 13 +++++++++++++
 gdb/ravenscar-thread.c | 40 +++++++++++++++++++++++++++++-----------
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index b89b5f62e5a..ee95a1c4bb8 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -30,6 +30,7 @@
 #include "top.h"
 #include "regcache.h"
 #include "objfiles.h"
+#include <unordered_map>
 
 /* This module provides support for "Ravenscar" tasks (Ada) when
    debugging on bare-metal targets.
@@ -133,6 +134,14 @@ struct ravenscar_thread_target final : public target_ops
   ptid_t active_task (int cpu);
   bool task_is_currently_active (ptid_t ptid);
   bool runtime_initialized ();
+  int get_thread_base_cpu (ptid_t ptid);
+  ptid_t get_base_thread_from_ravenscar_task (ptid_t ptid);
+
+  /* This maps a TID to the CPU on which it was running.  This is
+     needed because sometimes the runtime will report an active task
+     that hasn't yet been put on the list of tasks that is read by
+     ada-tasks.c.  */
+  std::unordered_map<long, int> m_cpu_map;
 };
 
 /* Return true iff PTID corresponds to a ravenscar task.  */
@@ -156,8 +165,8 @@ is_ravenscar_task (ptid_t ptid)
    This assume that PTID is a valid ptid_t.  Otherwise, a gdb_assert
    will be triggered.  */
 
-static int
-ravenscar_get_thread_base_cpu (ptid_t ptid)
+int
+ravenscar_thread_target::get_thread_base_cpu (ptid_t ptid)
 {
   int base_cpu;
 
@@ -165,8 +174,15 @@ ravenscar_get_thread_base_cpu (ptid_t ptid)
     {
       struct ada_task_info *task_info = ada_get_task_info_from_ptid (ptid);
 
-      gdb_assert (task_info != NULL);
-      base_cpu = task_info->base_cpu;
+      if (task_info != NULL)
+	base_cpu = task_info->base_cpu;
+      else
+	{
+	  auto iter = m_cpu_map.find (ptid.tid ());
+
+	  gdb_assert (iter != m_cpu_map.end ());
+	  base_cpu = iter->second;
+	}
     }
   else
     {
@@ -190,8 +206,7 @@ ravenscar_get_thread_base_cpu (ptid_t ptid)
 bool
 ravenscar_thread_target::task_is_currently_active (ptid_t ptid)
 {
-  ptid_t active_task_ptid
-    = active_task (ravenscar_get_thread_base_cpu (ptid));
+  ptid_t active_task_ptid = active_task (get_thread_base_cpu (ptid));
 
   return ptid == active_task_ptid;
 }
@@ -202,15 +217,15 @@ ravenscar_thread_target::task_is_currently_active (ptid_t ptid)
    This is the thread that corresponds to the CPU on which the task
    is running.  */
 
-static ptid_t
-get_base_thread_from_ravenscar_task (ptid_t ptid)
+ptid_t
+ravenscar_thread_target::get_base_thread_from_ravenscar_task (ptid_t ptid)
 {
   int base_cpu;
 
   if (!is_ravenscar_task (ptid))
     return ptid;
 
-  base_cpu = ravenscar_get_thread_base_cpu (ptid);
+  base_cpu = get_thread_base_cpu (ptid);
   return ptid_t (ptid.pid (), base_cpu, 0);
 }
 
@@ -227,7 +242,7 @@ ravenscar_thread_target::add_active_thread ()
   int base_cpu;
 
   gdb_assert (!is_ravenscar_task (m_base_ptid));
-  base_cpu = ravenscar_get_thread_base_cpu (m_base_ptid);
+  base_cpu = get_thread_base_cpu (m_base_ptid);
 
   if (!runtime_initialized ())
     return nullptr;
@@ -242,7 +257,10 @@ ravenscar_thread_target::add_active_thread ()
      may not always add it to the thread list.  Add it here.  */
   thread_info *active_thr = find_thread_ptid (proc_target, active_ptid);
   if (active_thr == nullptr)
-    active_thr = add_thread (proc_target, active_ptid);
+    {
+      active_thr = ::add_thread (proc_target, active_ptid);
+      m_cpu_map[active_ptid.tid ()] = base_cpu;
+    }
   return active_thr;
 }
 
-- 
2.26.2

[PATCH v2 05/12] Change names given to Ravenscar threads

[Tom Tromey]

Current a Ravenscar thread is given the same sort of name as a "CPU"
thread; they can only be distinguished by looking at the output of
"info thread".

This patch changes ravenscar-thread.c to distinguish these threads,
like:

    (gdb) continue
    Continuing.
    [New Ravenscar Thread 0x2b910]

2019-03-26  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_thread_target) <extra_thread_info>:
	Remove.
	(ravenscar_thread_target::extra_thread_info): Remove.
	(ravenscar_thread_target::pid_to_str): Mention Ravenscar in result;
	defer to target beneath for non-Ravenscar threads.
---
 gdb/ChangeLog          |  8 ++++++++
 gdb/ravenscar-thread.c | 13 ++++---------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index ee95a1c4bb8..dbcd4de81f8 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -109,8 +109,6 @@ struct ravenscar_thread_target final : public target_ops
 
   void update_thread_list () override;
 
-  const char *extra_thread_info (struct thread_info *) override;
-
   std::string pid_to_str (ptid_t) override;
 
   ptid_t get_ada_task_ptid (long lwp, long thread) override;
@@ -414,12 +412,6 @@ ravenscar_thread_target::active_task (int cpu)
     return ptid_t (m_base_ptid.pid (), 0, tid);
 }
 
-const char *
-ravenscar_thread_target::extra_thread_info (thread_info *tp)
-{
-  return "Ravenscar task";
-}
-
 bool
 ravenscar_thread_target::thread_alive (ptid_t ptid)
 {
@@ -430,7 +422,10 @@ ravenscar_thread_target::thread_alive (ptid_t ptid)
 std::string
 ravenscar_thread_target::pid_to_str (ptid_t ptid)
 {
-  return string_printf ("Thread %#x", (int) ptid.tid ());
+  if (!is_ravenscar_task (ptid))
+    return beneath ()->pid_to_str (ptid);
+
+  return string_printf ("Ravenscar Thread %#x", (int) ptid.tid ());
 }
 
 void
-- 
2.26.2

[PATCH v2 06/12] Use gdb::function_view in iterate_over_live_ada_tasks

[Tom Tromey]

This changes iterate_over_live_ada_tasks to accept a
gdb::function_view.  This is needed by a subsequent patch.

2019-03-26  Tom Tromey  <tromey@adacore.com>

	* ada-lang.h (ada_task_list_iterator_ftype): Now a
	gdb::function_view.
	(iterate_over_live_ada_tasks): Change type of argument.
	* ada-tasks.c (iterate_over_live_ada_tasks): Change type
	of argument.
---
 gdb/ChangeLog   | 8 ++++++++
 gdb/ada-lang.h  | 5 +++--
 gdb/ada-tasks.c | 2 +-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index 9be597942fd..ae313ce700a 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -375,9 +375,10 @@ extern struct ada_task_info *ada_get_task_info_from_ptid (ptid_t ptid);
 
 extern int ada_get_task_number (thread_info *thread);
 
-typedef void (ada_task_list_iterator_ftype) (struct ada_task_info *task);
+typedef gdb::function_view<void (struct ada_task_info *task)>
+  ada_task_list_iterator_ftype;
 extern void iterate_over_live_ada_tasks
-  (ada_task_list_iterator_ftype *iterator);
+  (ada_task_list_iterator_ftype iterator);
 
 extern const char *ada_get_tcb_types_info (void);
 
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 27b458767a7..d54c8b320a4 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -376,7 +376,7 @@ ada_get_task_info_from_ptid (ptid_t ptid)
    terminated yet.  */
 
 void
-iterate_over_live_ada_tasks (ada_task_list_iterator_ftype *iterator)
+iterate_over_live_ada_tasks (ada_task_list_iterator_ftype iterator)
 {
   struct ada_tasks_inferior_data *data;
 
-- 
2.26.2

[PATCH v2 07/12] Wrap xfer_partial and enable_btrace for Ravenscar

[Tom Tromey]

A gdb crash showed that the xfer_partial target method was not wrapped
for Ravenscar.  This caused remote.c to call
remote::set_general_thread with a Ravenscar "fake" ptid, which showed
up later as an event ptid.

I went through all the target methods and looked to see which ones
could call set_general_thread or set_continue_thread (but not
set_general_process, as I think Ravenscar targets aren't
multi-inferior).  This patch wraps the two that I found.

xfer_partial requires special treatment, because it can be called
recursively via get_base_thread_from_ravenscar_task.  To avoid a
recursive call, this patch changes update_thread_list to record all
tasks in the m_cpu_map, and changes get_thread_base_cpu to prefer this
map.  This avoids some memory reads.

It was unclear to me whether enable_btrace really makes sense for
Ravenscar; but at the same time it seemed harmless to add this patch.

2019-03-26  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (xfer_partial, enable_btrace, add_thread):
	New methods.
	(ravenscar_thread_target::get_thread_base_cpu): Check m_cpu_map
	first.
	(ravenscar_thread_target::add_thread): Rename from
	ravenscar_add_thread.
	(ravenscar_thread_target::update_thread_list): Use a lambda.
	(ravenscar_thread_target::xfer_partial): New method.
---
 gdb/ChangeLog          | 11 ++++++++
 gdb/ravenscar-thread.c | 64 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index dbcd4de81f8..a67a5e9a368 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -103,6 +103,13 @@ struct ravenscar_thread_target final : public target_ops
 
   bool stopped_data_address (CORE_ADDR *) override;
 
+  enum target_xfer_status xfer_partial (enum target_object object,
+					const char *annex,
+					gdb_byte *readbuf,
+					const gdb_byte *writebuf,
+					ULONGEST offset, ULONGEST len,
+					ULONGEST *xfered_len) override;
+
   bool thread_alive (ptid_t ptid) override;
 
   int core_of_thread (ptid_t ptid) override;
@@ -113,6 +120,14 @@ struct ravenscar_thread_target final : public target_ops
 
   ptid_t get_ada_task_ptid (long lwp, long thread) override;
 
+  struct btrace_target_info *enable_btrace (ptid_t ptid,
+					    const struct btrace_config *conf)
+    override
+  {
+    ptid = get_base_thread_from_ravenscar_task (ptid);
+    return beneath ()->enable_btrace (ptid, conf);
+  }
+
   void mourn_inferior () override;
 
   void close () override
@@ -134,6 +149,7 @@ struct ravenscar_thread_target final : public target_ops
   bool runtime_initialized ();
   int get_thread_base_cpu (ptid_t ptid);
   ptid_t get_base_thread_from_ravenscar_task (ptid_t ptid);
+  void add_thread (struct ada_task_info *task);
 
   /* This maps a TID to the CPU on which it was running.  This is
      needed because sometimes the runtime will report an active task
@@ -170,16 +186,18 @@ ravenscar_thread_target::get_thread_base_cpu (ptid_t ptid)
 
   if (is_ravenscar_task (ptid))
     {
-      struct ada_task_info *task_info = ada_get_task_info_from_ptid (ptid);
+      /* Prefer to not read inferior memory if possible, to avoid
+	 reentrancy problems with xfer_partial.  */
+      auto iter = m_cpu_map.find (ptid.tid ());
 
-      if (task_info != NULL)
-	base_cpu = task_info->base_cpu;
+      if (iter != m_cpu_map.end ())
+	base_cpu = iter->second;
       else
 	{
-	  auto iter = m_cpu_map.find (ptid.tid ());
+	  struct ada_task_info *task_info = ada_get_task_info_from_ptid (ptid);
 
-	  gdb_assert (iter != m_cpu_map.end ());
-	  base_cpu = iter->second;
+	  gdb_assert (task_info != NULL);
+	  base_cpu = task_info->base_cpu;
 	}
     }
   else
@@ -383,11 +401,14 @@ ravenscar_thread_target::wait (ptid_t ptid,
 /* Add the thread associated to the given TASK to the thread list
    (if the thread has already been added, this is a no-op).  */
 
-static void
-ravenscar_add_thread (struct ada_task_info *task)
+void
+ravenscar_thread_target::add_thread (struct ada_task_info *task)
 {
   if (find_thread_ptid (current_inferior (), task->ptid) == NULL)
-    add_thread (current_inferior ()->process_target (), task->ptid);
+    {
+      ::add_thread (current_inferior ()->process_target (), task->ptid);
+      m_cpu_map[task->ptid.tid ()] = task->base_cpu;
+    }
 }
 
 void
@@ -398,7 +419,10 @@ ravenscar_thread_target::update_thread_list ()
      (m_base_ptid) and the running thread, that may not have been included
      to system.tasking.debug's list yet.  */
 
-  iterate_over_live_ada_tasks (ravenscar_add_thread);
+  iterate_over_live_ada_tasks ([=] (struct ada_task_info *task)
+			       {
+				 this->add_thread (task);
+			       });
 }
 
 ptid_t
@@ -541,6 +565,26 @@ ravenscar_thread_target::core_of_thread (ptid_t ptid)
   return beneath ()->core_of_thread (inferior_ptid);
 }
 
+/* Implement the target xfer_partial method.  */
+
+enum target_xfer_status
+ravenscar_thread_target::xfer_partial (enum target_object object,
+				       const char *annex,
+				       gdb_byte *readbuf,
+				       const gdb_byte *writebuf,
+				       ULONGEST offset, ULONGEST len,
+				       ULONGEST *xfered_len)
+{
+  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
+  /* Calling get_base_thread_from_ravenscar_task can read memory from
+     the inferior.  However, that function is written to prefer our
+     internal map, so it should not result in recursive calls in
+     practice.  */
+  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  return beneath ()->xfer_partial (object, annex, readbuf, writebuf,
+				   offset, len, xfered_len);
+}
+
 /* Observer on inferior_created: push ravenscar thread stratum if needed.  */
 
 static void
-- 
2.26.2

[PATCH v2 08/12] Update Ravenscar documentation

[Tom Tromey]

This documents some recent Ravenscar changes, and further documents
the known limitation where stepping through the runtime initialization
code does not work properly.

2019-03-26  Tom Tromey  <tromey@adacore.com>

	* gdb.texinfo (Ravenscar Profile): Add examples.
	Document runtime initialization limitation.
---
 gdb/doc/ChangeLog   |  5 +++++
 gdb/doc/gdb.texinfo | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 09317fa0bd0..77013648b2f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18320,6 +18320,34 @@ using the Ravenscar Profile.
 
 @end table
 
+@cindex Ravenscar thread
+When Ravenscar task-switching is enabled, Ravenscar tasks are
+announced by @value{GDBN} as if they were threads:
+
+@smallexample
+(gdb) continue
+[New Ravenscar Thread 0x2b8f0]
+@end smallexample
+
+Both Ravenscar tasks and the underlying CPU threads will show up in
+the output of @code{info threads}:
+
+@smallexample
+(gdb) info threads
+  Id   Target Id                  Frame
+  1    Thread 1 (CPU#0 [running]) simple () at simple.adb:10
+  2    Thread 2 (CPU#1 [running]) 0x0000000000003d34 in __gnat_initialize_cpu_devices ()
+  3    Thread 3 (CPU#2 [running]) 0x0000000000003d28 in __gnat_initialize_cpu_devices ()
+  4    Thread 4 (CPU#3 [halted ]) 0x000000000000c6ec in system.task_primitives.operations.idle ()
+* 5    Ravenscar Thread 0x2b8f0   simple () at simple.adb:10
+  6    Ravenscar Thread 0x2f150   0x000000000000c6ec in system.task_primitives.operations.idle ()
+@end smallexample
+
+One known limitation of the Ravenscar support in @value{GDBN} is that
+it isn't currently possible to single-step through the runtime
+initialization sequence.  If you need to debug this code, you should
+use @code{set ravenscar task-switching off}.
+
 @node Ada Settings
 @subsubsection Ada Settings
 @cindex Ada settings
-- 
2.26.2

[PATCH v2 09/12] Fix Ravenscar "process" resume

[Tom Tromey]

A coworker noticed that gdb would send the wrong vCont packet to qemu
when debugging a Ravenscar program:

    > (gdb) thread 2
    > [Switching to thread 2 (Thread 1.2)]
    > #0  0x0000000000001000 in ?? ()

    > (gdb) c
    [...]
    > Sending packet: $vCont;c:p1.1#e2...Ack

Here, we've switched to thread 2, but the packet says to resume thread
1.

This turned out to be a bug in ravenscar_thread_target::resume, which
did not properly handle the case of a "process" resume.  In
particular, the resume method would be passed a ptid of (1, 0, 0) --
but then rewrite this to its saved ptid.

This patch fixes the problem by recognizing this case in the resume
method.

2020-07-08  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_thread_target::resume): Handle
	"is_pid" case.
---
 gdb/ChangeLog          | 5 +++++
 gdb/ravenscar-thread.c | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index a67a5e9a368..91f09209bbc 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -363,7 +363,12 @@ ravenscar_thread_target::resume (ptid_t ptid, int step,
   /* If we see a wildcard resume, we simply pass that on.  Otherwise,
      arrange to resume the base ptid.  */
   inferior_ptid = m_base_ptid;
-  if (ptid != minus_one_ptid)
+  if (ptid.is_pid ())
+    {
+      /* We only have one process, so resume all threads of it.  */
+      ptid = minus_one_ptid;
+    }
+  else if (ptid != minus_one_ptid)
     ptid = m_base_ptid;
   beneath ()->resume (ptid, step, siggnal);
 }
-- 
2.26.2

[PATCH v2 10/12] Fetch registers from correct thread in ravenscar-thread.c

[Tom Tromey]

Fabien also noticed that gdb would not report a stop correctly when
using Ravenscar.  This patch fixes the bug by making a few changes:

* ravenscar_thread_target::wait now updates the inferior ptid before
  updating the thread list.  This ensures that a new thread is
  correctly associated with the underlying CPU.

* The fetch_registers, store_registers, and prepare_to_store methods
  now save and restore the regcache's ptid before doing the operation
  on the underlying live thread.  This ensures that gdb informs the
  remote of a thread it knows about, as opposed to using a Ravenscar
  thread, which probably will not be recognized.

2020-07-08  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_thread_target::wait): Call
	update_inferior_ptid before update_thread_list.
	(temporarily_change_regcache_ptid): New class.
	(ravenscar_thread_target::fetch_registers)
	(ravenscar_thread_target::store_registers)
	(ravenscar_thread_target::prepare_to_store): Use base thread when
	forwarding operation.
---
 gdb/ChangeLog          | 10 +++++
 gdb/ravenscar-thread.c | 85 +++++++++++++++++++++++++++++++++---------
 2 files changed, 77 insertions(+), 18 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 91f09209bbc..459e5ea3ed3 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -457,20 +457,55 @@ ravenscar_thread_target::pid_to_str (ptid_t ptid)
   return string_printf ("Ravenscar Thread %#x", (int) ptid.tid ());
 }
 
+/* Temporarily set the ptid of a regcache to some other value.  When
+   this object is destroyed, the regcache's original ptid is
+   restored.  */
+
+class temporarily_change_regcache_ptid
+{
+public:
+
+  temporarily_change_regcache_ptid (struct regcache *regcache, ptid_t new_ptid)
+    : m_regcache (regcache),
+      m_save_ptid (regcache->ptid ())
+  {
+    m_regcache->set_ptid (new_ptid);
+  }
+
+  ~temporarily_change_regcache_ptid ()
+  {
+    m_regcache->set_ptid (m_save_ptid);
+  }
+
+private:
+
+  /* The regcache.  */
+  struct regcache *m_regcache;
+  /* The saved ptid.  */
+  ptid_t m_save_ptid;
+};
+
 void
 ravenscar_thread_target::fetch_registers (struct regcache *regcache, int regnum)
 {
   ptid_t ptid = regcache->ptid ();
 
-  if (runtime_initialized ()
-      && is_ravenscar_task (ptid)
-      && !task_is_currently_active (ptid))
+  if (runtime_initialized () && is_ravenscar_task (ptid))
     {
-      struct gdbarch *gdbarch = regcache->arch ();
-      struct ravenscar_arch_ops *arch_ops
-	= gdbarch_ravenscar_ops (gdbarch);
+      if (task_is_currently_active (ptid))
+	{
+	  ptid_t base = get_base_thread_from_ravenscar_task (ptid);
+	  temporarily_change_regcache_ptid changer (regcache, base);
+	  beneath ()->fetch_registers (regcache, regnum);
+	}
+      else
+	{
+	  struct gdbarch *gdbarch = regcache->arch ();
+	  struct ravenscar_arch_ops *arch_ops
+	    = gdbarch_ravenscar_ops (gdbarch);
 
-      arch_ops->fetch_registers (regcache, regnum);
+	  arch_ops->fetch_registers (regcache, regnum);
+	}
     }
   else
     beneath ()->fetch_registers (regcache, regnum);
@@ -482,15 +517,22 @@ ravenscar_thread_target::store_registers (struct regcache *regcache,
 {
   ptid_t ptid = regcache->ptid ();
 
-  if (runtime_initialized ()
-      && is_ravenscar_task (ptid)
-      && !task_is_currently_active (ptid))
+  if (runtime_initialized () && is_ravenscar_task (ptid))
     {
-      struct gdbarch *gdbarch = regcache->arch ();
-      struct ravenscar_arch_ops *arch_ops
-	= gdbarch_ravenscar_ops (gdbarch);
+      if (task_is_currently_active (ptid))
+	{
+	  ptid_t base = get_base_thread_from_ravenscar_task (ptid);
+	  temporarily_change_regcache_ptid changer (regcache, base);
+	  beneath ()->store_registers (regcache, regnum);
+	}
+      else
+	{
+	  struct gdbarch *gdbarch = regcache->arch ();
+	  struct ravenscar_arch_ops *arch_ops
+	    = gdbarch_ravenscar_ops (gdbarch);
 
-      arch_ops->store_registers (regcache, regnum);
+	  arch_ops->store_registers (regcache, regnum);
+	}
     }
   else
     beneath ()->store_registers (regcache, regnum);
@@ -501,11 +543,18 @@ ravenscar_thread_target::prepare_to_store (struct regcache *regcache)
 {
   ptid_t ptid = regcache->ptid ();
 
-  if (runtime_initialized ()
-      && is_ravenscar_task (ptid)
-      && !task_is_currently_active (ptid))
+  if (runtime_initialized () && is_ravenscar_task (ptid))
     {
-      /* Nothing.  */
+      if (task_is_currently_active (ptid))
+	{
+	  ptid_t base = get_base_thread_from_ravenscar_task (ptid);
+	  temporarily_change_regcache_ptid changer (regcache, base);
+	  beneath ()->prepare_to_store (regcache);
+	}
+      else
+	{
+	  /* Nothing.  */
+	}
     }
   else
     beneath ()->prepare_to_store (regcache);
-- 
2.26.2

[PATCH v2 11/12] Set inferior_ptid in ravenscar_thread_target::update_thread_list

[Tom Tromey]

Commit 2da4b788f ("Don't write to inferior_ptid in
ravenscar-thread.c") caused a Ravenscar regression (which, FWIW, is
understandable because Ravenscar is difficult to test).  Namely,
ravenscar_thread_target::update_thread_list calls
iterate_over_live_ada_tasks, which calls ada_build_task_list, which
uses target_has_stack -- which relies on inferior_ptid.

This patch changes update_thread_list to ensure that inferior_ptid is
set before making this call.  This avoids various failures on
Ravenscar targets.

2020-07-08  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (update_thread_list): Set inferior_ptid.
---
 gdb/ChangeLog          | 4 ++++
 gdb/ravenscar-thread.c | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 459e5ea3ed3..37df2188cf3 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -419,6 +419,12 @@ ravenscar_thread_target::add_thread (struct ada_task_info *task)
 void
 ravenscar_thread_target::update_thread_list ()
 {
+  /* iterate_over_live_ada_tasks requires that inferior_ptid be set,
+     but this isn't always the case in target methods.  So, we ensure
+     it here.  */
+  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid,
+						  m_base_ptid);
+
   /* Do not clear the thread list before adding the Ada task, to keep
      the thread that the process stratum has included into it
      (m_base_ptid) and the running thread, that may not have been included
-- 
2.26.2

[PATCH v2 12/12] Fix remaining Ravenscar regressions

[Tom Tromey]

Testing showed a few more Ravenscar regressions arising from upstream.
In particular, gdb now uses the current thread in some places where
inferior_ptid was previously used.  This patch fixes the problem by
arranging to save and restore the thread now.

gdb/ChangeLog
2020-08-05  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c
	(ravenscar_thread_target::set_base_thread_from_ravenscar_task):
	New method.
	(ravenscar_thread_target::wait): Check
	runtime_initialized.
	(ravenscar_thread_target::prepare_to_store)
	(ravenscar_thread_target::stopped_by_sw_breakpoint)
	(ravenscar_thread_target::stopped_by_hw_breakpoint)
	(ravenscar_thread_target::stopped_by_watchpoint)
	(ravenscar_thread_target::stopped_data_address)
	(ravenscar_thread_target::core_of_thread): Use
	scoped_restore_current_thread and
	set_base_thread_from_ravenscar_task.
---
 gdb/ChangeLog          | 16 ++++++++++++++++
 gdb/ravenscar-thread.c | 29 +++++++++++++++++++----------
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 37df2188cf3..387ebcb32e9 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -151,6 +151,15 @@ struct ravenscar_thread_target final : public target_ops
   ptid_t get_base_thread_from_ravenscar_task (ptid_t ptid);
   void add_thread (struct ada_task_info *task);
 
+  /* Like switch_to_thread, but uses the base ptid for the thread.  */
+  void set_base_thread_from_ravenscar_task (ptid_t ptid)
+  {
+    process_stratum_target *proc_target
+      = as_process_stratum_target (this->beneath ());
+    ptid_t underlying = get_base_thread_from_ravenscar_task (ptid);
+    switch_to_thread (find_thread_ptid (proc_target, underlying));
+  }
+
   /* This maps a TID to the CPU on which it was running.  This is
      needed because sometimes the runtime will report an active task
      that hasn't yet been put on the list of tasks that is read by
@@ -571,8 +580,8 @@ ravenscar_thread_target::prepare_to_store (struct regcache *regcache)
 bool
 ravenscar_thread_target::stopped_by_sw_breakpoint ()
 {
-  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  scoped_restore_current_thread saver;
+  set_base_thread_from_ravenscar_task (inferior_ptid);
   return beneath ()->stopped_by_sw_breakpoint ();
 }
 
@@ -581,8 +590,8 @@ ravenscar_thread_target::stopped_by_sw_breakpoint ()
 bool
 ravenscar_thread_target::stopped_by_hw_breakpoint ()
 {
-  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  scoped_restore_current_thread saver;
+  set_base_thread_from_ravenscar_task (inferior_ptid);
   return beneath ()->stopped_by_hw_breakpoint ();
 }
 
@@ -591,8 +600,8 @@ ravenscar_thread_target::stopped_by_hw_breakpoint ()
 bool
 ravenscar_thread_target::stopped_by_watchpoint ()
 {
-  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  scoped_restore_current_thread saver;
+  set_base_thread_from_ravenscar_task (inferior_ptid);
   return beneath ()->stopped_by_watchpoint ();
 }
 
@@ -601,8 +610,8 @@ ravenscar_thread_target::stopped_by_watchpoint ()
 bool
 ravenscar_thread_target::stopped_data_address (CORE_ADDR *addr_p)
 {
-  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  scoped_restore_current_thread saver;
+  set_base_thread_from_ravenscar_task (inferior_ptid);
   return beneath ()->stopped_data_address (addr_p);
 }
 
@@ -620,8 +629,8 @@ ravenscar_thread_target::mourn_inferior ()
 int
 ravenscar_thread_target::core_of_thread (ptid_t ptid)
 {
-  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
-  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  scoped_restore_current_thread saver;
+  set_base_thread_from_ravenscar_task (inferior_ptid);
   return beneath ()->core_of_thread (inferior_ptid);
 }
 
-- 
2.26.2

Re: [PATCH v2 00/12] Fix Ravenscar regressions

[Joel Brobecker]

Hi Tom,

> Some changes in gdb caused regressions in AdaCore's internal Ravenscar
> testing.  This is understandable, because a Ravenscar environment is a
> bit of a pain to set up, and so I don't think it's tested in ordinary
> gdb testing.
> 
> This series is v2 of some fixes to Ravenscar.  A few more fixes have
> been added since the last series, to cope with new regressions.  I've
> also dropped the "random thread switch" patch from the series.  I hope
> we can discuss that one again sometime, but meanwhile I'd rather get
> these changes in.

I can't approve the documentation bits, but the rest of the patch
series looks good to me.

Thank you!
-- 
Joel

Re: [PATCH v2 00/12] Fix Ravenscar regressions

[Tom Tromey]

>> This series is v2 of some fixes to Ravenscar.  A few more fixes have
>> been added since the last series, to cope with new regressions.  I've
>> also dropped the "random thread switch" patch from the series.  I hope
>> we can discuss that one again sometime, but meanwhile I'd rather get
>> these changes in.

Joel> I can't approve the documentation bits, but the rest of the patch
Joel> series looks good to me.

Thanks.  The documentation patch was actually approved here (with one
change, which is included in this update):

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

So, I am going to check in this series.

Tom

Re: [PATCH v2 00/12] Fix Ravenscar regressions

[Eli Zaretskii]

> Date: Fri, 7 Aug 2020 08:03:12 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> I can't approve the documentation bits, but the rest of the patch
> series looks good to me.

The documentation part is also OK.

Thanks.