public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c
  2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
                   ` (4 preceding siblings ...)
  2019-02-07  9:40 ` [PATCH 3/8] C++-ify ravenscar_arch_ops Tom Tromey
@ 2019-02-07  9:40 ` Tom Tromey
  2019-02-07 17:23   ` Pedro Alves
  2019-02-07  9:40 ` [PATCH 2/8] Exception safety " Tom Tromey
  2019-02-07 14:22 ` [PATCH 1/8] Fix some typos " Tom Tromey
  7 siblings, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2019-02-07  9:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

From: Tom Tromey <tromey@adacore.com>

ravenscar-thread.c intercepts resume and wait target requests and
replaces the requested ptid with the ptid of the underlying CPU.
However, this is incorrect when a request is made with a wildcard
ptid.

This patch adds a special case to ravenscar-thread.c for
minus_one_ptid.  I don't believe a special case for process wildcards
is necessary, so I have not added that.

gdb/ChangeLog
2019-02-07  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_thread_target::resume)
	(ravenscar_thread_target::wait): Special case wildcard requests.
---
 gdb/ChangeLog          |  5 +++++
 gdb/ravenscar-thread.c | 20 ++++++++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 2e8d76d4ae8..4b07b00aae2 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -323,8 +323,14 @@ void
 ravenscar_thread_target::resume (ptid_t ptid, int step,
 				 enum gdb_signal siggnal)
 {
-  inferior_ptid = m_base_ptid;
-  beneath ()->resume (m_base_ptid, step, siggnal);
+  /* If we see a wildcard resume, we simply pass that on.  Otherwise,
+     arrange to resume the base ptid.  */
+  if (ptid != minus_one_ptid)
+    {
+      inferior_ptid = m_base_ptid;
+      ptid = m_base_ptid;
+    }
+  beneath ()->resume (ptid, step, siggnal);
 }
 
 ptid_t
@@ -334,8 +340,12 @@ ravenscar_thread_target::wait (ptid_t ptid,
 {
   ptid_t event_ptid;
 
-  inferior_ptid = m_base_ptid;
-  event_ptid = beneath ()->wait (m_base_ptid, status, 0);
+  if (ptid != minus_one_ptid)
+    {
+      inferior_ptid = m_base_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.
 
@@ -350,6 +360,8 @@ ravenscar_thread_target::wait (ptid_t ptid,
       this->update_thread_list ();
       this->update_inferior_ptid ();
     }
+  else
+    inferior_ptid = m_base_ptid;
   return inferior_ptid;
 }
 
-- 
2.17.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 5/8] Minor C++-ification in ravenscar-thread.c
  2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
                   ` (2 preceding siblings ...)
  2019-02-07  9:40 ` [PATCH 6/8] Add push_target overload Tom Tromey
@ 2019-02-07  9:40 ` Tom Tromey
  2019-02-07  9:40 ` [PATCH 3/8] C++-ify ravenscar_arch_ops Tom Tromey
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2019-02-07  9:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

From: Tom Tromey <tromey@adacore.com>

This changes some functions in ravenscar-thread.c to return "bool"
rather than int, where appropriate, and also changes "(void)" to "()".

gdb/ChangeLog
2019-02-07  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (is_ravenscar_task)
	(ravenscar_task_is_currently_active): Return bool.
	(ravenscar_update_inferior_ptid, get_running_thread_msymbol)
	(_initialize_ravenscar): Remove "(void)".
	(has_ravenscar_runtime, ravenscar_runtime_initialized): Likewise.
	Return bool.
---
 gdb/ChangeLog          |  9 +++++++++
 gdb/ravenscar-thread.c | 30 +++++++++++++-----------------
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 37b799540f7..b0de509ad0c 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -121,15 +121,11 @@ struct ravenscar_thread_target final : public target_ops
 static ravenscar_thread_target ravenscar_ops;
 
 static ptid_t ravenscar_active_task (int cpu);
-static void ravenscar_update_inferior_ptid (void);
-static int has_ravenscar_runtime (void);
-static int ravenscar_runtime_initialized (void);
-static void ravenscar_inferior_created (struct target_ops *target,
-					int from_tty);
+static bool ravenscar_runtime_initialized ();
 
-/* Return nonzero iff PTID corresponds to a ravenscar task.  */
+/* Return true iff PTID corresponds to a ravenscar task.  */
 
-static int
+static bool
 is_ravenscar_task (ptid_t ptid)
 {
   /* By construction, ravenscar tasks have their LWP set to zero.
@@ -169,7 +165,7 @@ ravenscar_get_thread_base_cpu (ptid_t ptid)
   return base_cpu;
 }
 
-/* Given a ravenscar task (identified by its ptid_t PTID), return nonzero
+/* Given a ravenscar task (identified by its ptid_t PTID), return true
    if this task is the currently active task on the cpu that task is
    running on.
 
@@ -179,7 +175,7 @@ ravenscar_get_thread_base_cpu (ptid_t ptid)
    that task's registers are in the CPU bank.  Otherwise, the task
    is currently suspended, and its registers have been saved in memory.  */
 
-static int
+static bool
 ravenscar_task_is_currently_active (ptid_t ptid)
 {
   ptid_t active_task_ptid
@@ -210,7 +206,7 @@ get_base_thread_from_ravenscar_task (ptid_t ptid)
    update inferior_ptid accordingly.  */
 
 static void
-ravenscar_update_inferior_ptid (void)
+ravenscar_update_inferior_ptid ()
 {
   int base_cpu;
 
@@ -242,7 +238,7 @@ ravenscar_update_inferior_ptid (void)
    Return NULL if not found.  */
 
 static struct bound_minimal_symbol
-get_running_thread_msymbol (void)
+get_running_thread_msymbol ()
 {
   struct bound_minimal_symbol msym;
 
@@ -260,8 +256,8 @@ get_running_thread_msymbol (void)
 /* Return True if the Ada Ravenscar run-time can be found in the
    application.  */
 
-static int
-has_ravenscar_runtime (void)
+static bool
+has_ravenscar_runtime ()
 {
   struct bound_minimal_symbol msym_ravenscar_runtime_initializer
     = lookup_minimal_symbol (ravenscar_runtime_initializer, NULL, NULL);
@@ -280,10 +276,10 @@ has_ravenscar_runtime (void)
 /* Return True if the Ada Ravenscar run-time can be found in the
    application, and if it has been initialized on target.  */
 
-static int
-ravenscar_runtime_initialized (void)
+static bool
+ravenscar_runtime_initialized ()
 {
-  return (!(ravenscar_active_task (1) == null_ptid));
+  return ravenscar_active_task (1) != null_ptid;
 }
 
 /* Return the ID of the thread that is currently running.
@@ -588,7 +584,7 @@ Support for Ravenscar task/thread switching is disabled\n"));
    init.c.  */
 
 void
-_initialize_ravenscar (void)
+_initialize_ravenscar ()
 {
   base_ptid = null_ptid;
 
-- 
2.17.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 2/8] Exception safety in ravenscar-thread.c
  2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
                   ` (5 preceding siblings ...)
  2019-02-07  9:40 ` [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c Tom Tromey
@ 2019-02-07  9:40 ` Tom Tromey
  2019-02-07 14:22 ` [PATCH 1/8] Fix some typos " Tom Tromey
  7 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2019-02-07  9:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

From: Tom Tromey <tromey@adacore.com>

This changes some code in ravenscar-thread.c to use scoped_restore.  I
am not sure if it matters in practice, but this makes these methods
exception-safe in case the methods lower in the target stack can
throw.

gdb/ChangeLog
2019-02-07  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (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.
---
 gdb/ChangeLog          |  8 +++++++
 gdb/ravenscar-thread.c | 50 +++++++++++++-----------------------------
 2 files changed, 23 insertions(+), 35 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 32a4aa8d16a..9d708fd8581 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -464,13 +464,9 @@ ravenscar_thread_target::prepare_to_store (struct regcache *regcache)
 bool
 ravenscar_thread_target::stopped_by_sw_breakpoint ()
 {
-  ptid_t saved_ptid = inferior_ptid;
-  bool result;
-
-  inferior_ptid = get_base_thread_from_ravenscar_task (saved_ptid);
-  result = beneath ()->stopped_by_sw_breakpoint ();
-  inferior_ptid = saved_ptid;
-  return result;
+  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
+  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  return beneath ()->stopped_by_sw_breakpoint ();
 }
 
 /* Implement the to_stopped_by_hw_breakpoint target_ops "method".  */
@@ -478,13 +474,9 @@ ravenscar_thread_target::stopped_by_sw_breakpoint ()
 bool
 ravenscar_thread_target::stopped_by_hw_breakpoint ()
 {
-  ptid_t saved_ptid = inferior_ptid;
-  bool result;
-
-  inferior_ptid = get_base_thread_from_ravenscar_task (saved_ptid);
-  result = beneath ()->stopped_by_hw_breakpoint ();
-  inferior_ptid = saved_ptid;
-  return result;
+  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
+  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  return beneath ()->stopped_by_hw_breakpoint ();
 }
 
 /* Implement the to_stopped_by_watchpoint target_ops "method".  */
@@ -492,13 +484,9 @@ ravenscar_thread_target::stopped_by_hw_breakpoint ()
 bool
 ravenscar_thread_target::stopped_by_watchpoint ()
 {
-  ptid_t saved_ptid = inferior_ptid;
-  bool result;
-
-  inferior_ptid = get_base_thread_from_ravenscar_task (saved_ptid);
-  result = beneath ()->stopped_by_watchpoint ();
-  inferior_ptid = saved_ptid;
-  return result;
+  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
+  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  return beneath ()->stopped_by_watchpoint ();
 }
 
 /* Implement the to_stopped_data_address target_ops "method".  */
@@ -506,13 +494,9 @@ ravenscar_thread_target::stopped_by_watchpoint ()
 bool
 ravenscar_thread_target::stopped_data_address (CORE_ADDR *addr_p)
 {
-  ptid_t saved_ptid = inferior_ptid;
-  bool result;
-
-  inferior_ptid = get_base_thread_from_ravenscar_task (saved_ptid);
-  result = beneath ()->stopped_data_address (addr_p);
-  inferior_ptid = saved_ptid;
-  return result;
+  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
+  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  return beneath ()->stopped_data_address (addr_p);
 }
 
 void
@@ -528,13 +512,9 @@ ravenscar_thread_target::mourn_inferior ()
 int
 ravenscar_thread_target::core_of_thread (ptid_t ptid)
 {
-  ptid_t saved_ptid = inferior_ptid;
-  int result;
-
-  inferior_ptid = get_base_thread_from_ravenscar_task (saved_ptid);
-  result = beneath ()->core_of_thread (inferior_ptid);
-  inferior_ptid = saved_ptid;
-  return result;
+  scoped_restore save_ptid = make_scoped_restore (&inferior_ptid);
+  inferior_ptid = get_base_thread_from_ravenscar_task (inferior_ptid);
+  return beneath ()->core_of_thread (inferior_ptid);
 }
 
 /* Observer on inferior_created: push ravenscar thread stratum if needed.  */
-- 
2.17.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 4/8] Fix formatting in ravenscar-thread.c
  2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
  2019-02-07  9:40 ` [PATCH 7/8] Make the ravenscar thread target multi-target-ready Tom Tromey
@ 2019-02-07  9:40 ` Tom Tromey
  2019-02-07  9:40 ` [PATCH 6/8] Add push_target overload Tom Tromey
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2019-02-07  9:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

From: Tom Tromey <tromey@adacore.com>

This fixes some incorrect formatting in ravenscar-thread.c.

gdb/ChangeLog
2019-02-07  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (ravenscar_runtime_initializer)
	(has_ravenscar_runtime, get_running_thread_id)
	(ravenscar_thread_target::resume): Fix indentation.
---
 gdb/ChangeLog          |  6 ++++++
 gdb/ravenscar-thread.c | 23 ++++++++++++-----------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 0dc50a41429..37b799540f7 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -70,8 +70,8 @@ static const char running_thread_name[] = "__gnat_running_thread_table";
 static const char known_tasks_name[] = "system__tasking__debug__known_tasks";
 static const char first_task_name[] = "system__tasking__debug__first_task";
 
-static const char ravenscar_runtime_initializer[] =
-  "system__bb__threads__initialize";
+static const char ravenscar_runtime_initializer[]
+  = "system__bb__threads__initialize";
 
 static const target_info ravenscar_target_info = {
   "ravenscar",
@@ -263,12 +263,12 @@ get_running_thread_msymbol (void)
 static int
 has_ravenscar_runtime (void)
 {
-  struct bound_minimal_symbol msym_ravenscar_runtime_initializer =
-    lookup_minimal_symbol (ravenscar_runtime_initializer, NULL, NULL);
-  struct bound_minimal_symbol msym_known_tasks =
-    lookup_minimal_symbol (known_tasks_name, NULL, NULL);
-  struct bound_minimal_symbol msym_first_task =
-    lookup_minimal_symbol (first_task_name, NULL, NULL);
+  struct bound_minimal_symbol msym_ravenscar_runtime_initializer
+    = lookup_minimal_symbol (ravenscar_runtime_initializer, NULL, NULL);
+  struct bound_minimal_symbol msym_known_tasks
+    = lookup_minimal_symbol (known_tasks_name, NULL, NULL);
+  struct bound_minimal_symbol msym_first_task
+    = lookup_minimal_symbol (first_task_name, NULL, NULL);
   struct bound_minimal_symbol msym_running_thread
     = get_running_thread_msymbol ();
 
@@ -297,8 +297,8 @@ get_running_thread_id (int cpu)
   int buf_size;
   gdb_byte *buf;
   CORE_ADDR object_addr;
-  struct type *builtin_type_void_data_ptr =
-    builtin_type (target_gdbarch ())->builtin_data_ptr;
+  struct type *builtin_type_void_data_ptr
+    = builtin_type (target_gdbarch ())->builtin_data_ptr;
 
   if (!object_msym.minsym)
     return 0;
@@ -313,7 +313,8 @@ get_running_thread_id (int cpu)
 }
 
 void
-ravenscar_thread_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
+ravenscar_thread_target::resume (ptid_t ptid, int step,
+				 enum gdb_signal siggnal)
 {
   inferior_ptid = base_ptid;
   beneath ()->resume (base_ptid, step, siggnal);
-- 
2.17.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 7/8] Make the ravenscar thread target multi-target-ready
  2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
@ 2019-02-07  9:40 ` Tom Tromey
  2019-02-07 17:23   ` Pedro Alves
  2019-02-07  9:40 ` [PATCH 4/8] Fix formatting in ravenscar-thread.c Tom Tromey
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2019-02-07  9:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

From: Tom Tromey <tromey@adacore.com>

This changes ravenscar-thread.c to make it ready for multi-target.
This is done by moving globals into the target, and then arranging to
allocate the target with "new" and delete the target in its "close"
method.

gdb/ChangeLog
2019-02-07  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c (base_ptid): Remove.
	(struct ravenscar_thread_target) <close>: New method.
	<m_base_ptid>: New member.
	<update_inferior_ptid, active_task, task_is_currently_active,
	runtime_initialized>: Declare methods.
	<ravenscar_thread_target>: Add constructor.
	(ravenscar_thread_target::task_is_currently_active)
	(ravenscar_thread_target::update_inferior_ptid)
	(ravenscar_runtime_initialized): Rename.  Now methods.
	(ravenscar_thread_target::resume, ravenscar_thread_target::wait)
	(ravenscar_thread_target::update_thread_list): Update.
	(ravenscar_thread_target::active_task): Now method.
	(ravenscar_thread_target::store_registers)
	(ravenscar_thread_target::prepare_to_store)
	(ravenscar_thread_target::prepare_to_store)
	(ravenscar_thread_target::mourn_inferior): Update.
	(ravenscar_inferior_created): Use "new" to create target.
	(ravenscar_thread_target::get_ada_task_ptid): Update.
	(_initialize_ravenscar): Don't initialize base_ptid.
	(ravenscar_ops): Remove global.
---
 gdb/ChangeLog          | 23 ++++++++++
 gdb/ravenscar-thread.c | 99 +++++++++++++++++++++++-------------------
 2 files changed, 77 insertions(+), 45 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index b0de509ad0c..2e8d76d4ae8 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -60,11 +60,6 @@
 /* If non-null, ravenscar task support is enabled.  */
 static int ravenscar_task_support = 1;
 
-/* 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.  */
-static ptid_t base_ptid;
-
 static const char running_thread_name[] = "__gnat_running_thread_table";
 
 static const char known_tasks_name[] = "system__tasking__debug__known_tasks";
@@ -81,6 +76,11 @@ static const target_info ravenscar_target_info = {
 
 struct ravenscar_thread_target final : public target_ops
 {
+  ravenscar_thread_target ()
+  {
+    update_inferior_ptid ();
+  }
+
   const target_info &info () const override
   { return ravenscar_target_info; }
 
@@ -115,13 +115,24 @@ struct ravenscar_thread_target final : public target_ops
   ptid_t get_ada_task_ptid (long lwp, long thread) override;
 
   void mourn_inferior () override;
-};
 
-/* This module's target-specific operations.  */
-static ravenscar_thread_target ravenscar_ops;
+  void close () override
+  {
+    delete this;
+  }
+
+private:
 
-static ptid_t ravenscar_active_task (int cpu);
-static bool ravenscar_runtime_initialized ();
+  /* 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;
+
+  void update_inferior_ptid ();
+  ptid_t active_task (int cpu);
+  bool task_is_currently_active (ptid_t ptid);
+  bool runtime_initialized ();
+};
 
 /* Return true iff PTID corresponds to a ravenscar task.  */
 
@@ -175,11 +186,11 @@ ravenscar_get_thread_base_cpu (ptid_t ptid)
    that task's registers are in the CPU bank.  Otherwise, the task
    is currently suspended, and its registers have been saved in memory.  */
 
-static bool
-ravenscar_task_is_currently_active (ptid_t ptid)
+bool
+ravenscar_thread_target::task_is_currently_active (ptid_t ptid)
 {
   ptid_t active_task_ptid
-    = ravenscar_active_task (ravenscar_get_thread_base_cpu (ptid));
+    = active_task (ravenscar_get_thread_base_cpu (ptid));
 
   return ptid == active_task_ptid;
 }
@@ -205,24 +216,24 @@ get_base_thread_from_ravenscar_task (ptid_t ptid)
 /* Fetch the ravenscar running thread from target memory and
    update inferior_ptid accordingly.  */
 
-static void
-ravenscar_update_inferior_ptid ()
+void
+ravenscar_thread_target::update_inferior_ptid ()
 {
   int base_cpu;
 
-  base_ptid = inferior_ptid;
+  m_base_ptid = inferior_ptid;
 
   gdb_assert (!is_ravenscar_task (inferior_ptid));
-  base_cpu = ravenscar_get_thread_base_cpu (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 (!ravenscar_runtime_initialized ())
+  if (!runtime_initialized ())
     return;
 
-  /* Make sure we set base_ptid before calling ravenscar_active_task
+  /* Make sure we set m_base_ptid before calling active_task
      as the latter relies on it.  */
-  inferior_ptid = ravenscar_active_task (base_cpu);
+  inferior_ptid = active_task (base_cpu);
   gdb_assert (inferior_ptid != null_ptid);
 
   /* The running thread may not have been added to
@@ -276,10 +287,10 @@ has_ravenscar_runtime ()
 /* Return True if the Ada Ravenscar run-time can be found in the
    application, and if it has been initialized on target.  */
 
-static bool
-ravenscar_runtime_initialized ()
+bool
+ravenscar_thread_target::runtime_initialized ()
 {
-  return ravenscar_active_task (1) != null_ptid;
+  return active_task (1) != null_ptid;
 }
 
 /* Return the ID of the thread that is currently running.
@@ -312,8 +323,8 @@ void
 ravenscar_thread_target::resume (ptid_t ptid, int step,
 				 enum gdb_signal siggnal)
 {
-  inferior_ptid = base_ptid;
-  beneath ()->resume (base_ptid, step, siggnal);
+  inferior_ptid = m_base_ptid;
+  beneath ()->resume (m_base_ptid, step, siggnal);
 }
 
 ptid_t
@@ -323,8 +334,8 @@ ravenscar_thread_target::wait (ptid_t ptid,
 {
   ptid_t event_ptid;
 
-  inferior_ptid = base_ptid;
-  event_ptid = beneath ()->wait (base_ptid, status, 0);
+  inferior_ptid = m_base_ptid;
+  event_ptid = beneath ()->wait (m_base_ptid, status, 0);
   /* Find any new threads that might have been created, and update
      inferior_ptid to the active thread.
 
@@ -337,7 +348,7 @@ ravenscar_thread_target::wait (ptid_t ptid,
     {
       inferior_ptid = event_ptid;
       this->update_thread_list ();
-      ravenscar_update_inferior_ptid ();
+      this->update_inferior_ptid ();
     }
   return inferior_ptid;
 }
@@ -359,21 +370,21 @@ ravenscar_thread_target::update_thread_list ()
 
   /* Do not clear the thread list before adding the Ada task, to keep
      the thread that the process stratum has included into it
-     (base_ptid) and the running thread, that may not have been included
+     (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);
 }
 
-static ptid_t
-ravenscar_active_task (int cpu)
+ptid_t
+ravenscar_thread_target::active_task (int cpu)
 {
   CORE_ADDR tid = get_running_thread_id (cpu);
 
   if (tid == 0)
     return null_ptid;
   else
-    return ptid_t (base_ptid.pid (), 0, tid);
+    return ptid_t (m_base_ptid.pid (), 0, tid);
 }
 
 const char *
@@ -403,9 +414,9 @@ ravenscar_thread_target::fetch_registers (struct regcache *regcache, int regnum)
 {
   ptid_t ptid = regcache->ptid ();
 
-  if (ravenscar_runtime_initialized ()
+  if (runtime_initialized ()
       && is_ravenscar_task (ptid)
-      && !ravenscar_task_is_currently_active (ptid))
+      && !task_is_currently_active (ptid))
     {
       struct gdbarch *gdbarch = regcache->arch ();
       struct ravenscar_arch_ops *arch_ops
@@ -423,9 +434,9 @@ ravenscar_thread_target::store_registers (struct regcache *regcache,
 {
   ptid_t ptid = regcache->ptid ();
 
-  if (ravenscar_runtime_initialized ()
+  if (runtime_initialized ()
       && is_ravenscar_task (ptid)
-      && !ravenscar_task_is_currently_active (ptid))
+      && !task_is_currently_active (ptid))
     {
       struct gdbarch *gdbarch = regcache->arch ();
       struct ravenscar_arch_ops *arch_ops
@@ -442,9 +453,9 @@ ravenscar_thread_target::prepare_to_store (struct regcache *regcache)
 {
   ptid_t ptid = regcache->ptid ();
 
-  if (ravenscar_runtime_initialized ()
+  if (runtime_initialized ()
       && is_ravenscar_task (ptid)
-      && !ravenscar_task_is_currently_active (ptid))
+      && !task_is_currently_active (ptid))
     {
       struct gdbarch *gdbarch = regcache->arch ();
       struct ravenscar_arch_ops *arch_ops
@@ -499,9 +510,9 @@ ravenscar_thread_target::stopped_data_address (CORE_ADDR *addr_p)
 void
 ravenscar_thread_target::mourn_inferior ()
 {
-  base_ptid = null_ptid;
+  m_base_ptid = null_ptid;
   beneath ()->mourn_inferior ();
-  unpush_target (&ravenscar_ops);
+  unpush_target (this);
 }
 
 /* Implement the to_core_of_thread target_ops "method".  */
@@ -533,14 +544,14 @@ ravenscar_inferior_created (struct target_ops *target, int from_tty)
       return;
     }
 
-  ravenscar_update_inferior_ptid ();
-  push_target (&ravenscar_ops);
+  target_ops_up target_holder (new ravenscar_thread_target ());
+  push_target (std::move (target_holder));
 }
 
 ptid_t
 ravenscar_thread_target::get_ada_task_ptid (long lwp, long thread)
 {
-  return ptid_t (base_ptid.pid (), 0, thread);
+  return ptid_t (m_base_ptid.pid (), 0, thread);
 }
 
 /* Command-list for the "set/show ravenscar" prefix command.  */
@@ -586,8 +597,6 @@ Support for Ravenscar task/thread switching is disabled\n"));
 void
 _initialize_ravenscar ()
 {
-  base_ptid = null_ptid;
-
   /* Notice when the inferior is created in order to push the
      ravenscar ops if needed.  */
   gdb::observers::inferior_created.attach (ravenscar_inferior_created);
-- 
2.17.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 3/8] C++-ify ravenscar_arch_ops
  2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
                   ` (3 preceding siblings ...)
  2019-02-07  9:40 ` [PATCH 5/8] Minor C++-ification in ravenscar-thread.c Tom Tromey
@ 2019-02-07  9:40 ` Tom Tromey
  2019-02-07 17:23   ` Pedro Alves
  2019-02-07  9:40 ` [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c Tom Tromey
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2019-02-07  9:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

From: Tom Tromey <tromey@adacore.com>

This turns ravenscar_arch_ops into an abstract base class and updates
all the places where it is used.  This is an improvement because it
avoids any possibility of forgetting to set one of the function
pointers.  It also makes clear that these functions aren't intended to
be changed dynamically.

gdb/ChangeLog
2019-02-07  Tom Tromey  <tromey@adacore.com>

	* sparc-ravenscar-thread.c (struct sparc_ravenscar_ops): Derive
	from ravenscar_arch_ops.
	(sparc_ravenscar_ops::fetch_registers)
	(sparc_ravenscar_ops::prepare_to_store)
	(sparc_ravenscar_ops::store_registers): Now methods.
	(sparc_ravenscar_ops): Redefine.
	* ravenscar-thread.h (struct ravenscar_arch_ops): Add virtual
	methods and destructor.  Remove members.
	* ravenscar-thread.c (ravenscar_thread_target::fetch_registers)
	(ravenscar_thread_target::store_registers)
	(ravenscar_thread_target::prepare_to_store): Update.
	* ppc-ravenscar-thread.c (ppc_ravenscar_generic_prepare_to_store):
	Remove.
	(struct ppc_ravenscar_powerpc_ops): Derive from
	ravenscar_arch_ops.
	(ppc_ravenscar_powerpc_ops::fetch_registers)
	(ppc_ravenscar_powerpc_ops::store_registers): Now methods.
	(ppc_ravenscar_powerpc_ops): Redefine.
	(struct ppc_ravenscar_e500_ops): Derive from ravenscar_arch_ops.
	(ppc_ravenscar_e500_ops::fetch_registers)
	(ppc_ravenscar_e500_ops::store_registers): Now methods.
	(ppc_ravenscar_e500_ops): Redefine.
	* aarch64-ravenscar-thread.c
	(aarch64_ravenscar_generic_prepare_to_store): Remove.
	(struct aarch64_ravenscar_ops): Derive from ravenscar_arch_ops.
	(aarch64_ravenscar_fetch_registers)
	(aarch64_ravenscar_store_registers): Now methods.
	(aarch64_ravenscar_ops): Redefine.
---
 gdb/ChangeLog                  | 31 ++++++++++++++++
 gdb/aarch64-ravenscar-thread.c | 51 ++++++++++-----------------
 gdb/ppc-ravenscar-thread.c     | 64 +++++++++++++++-------------------
 gdb/ravenscar-thread.c         |  6 ++--
 gdb/ravenscar-thread.h         | 10 ++++--
 gdb/sparc-ravenscar-thread.c   | 30 +++++++---------
 6 files changed, 100 insertions(+), 92 deletions(-)

diff --git a/gdb/aarch64-ravenscar-thread.c b/gdb/aarch64-ravenscar-thread.c
index 1234650a2b7..fa99d896ffe 100644
--- a/gdb/aarch64-ravenscar-thread.c
+++ b/gdb/aarch64-ravenscar-thread.c
@@ -133,15 +133,6 @@ aarch64_ravenscar_generic_fetch_registers
     }
 }
 
-/* to_prepare_to_store when inferior_ptid is different from the running
-   thread.  */
-
-static void
-aarch64_ravenscar_generic_prepare_to_store (struct regcache *regcache)
-{
-  /* Nothing to do.  */
-}
-
 /* to_store_registers when inferior_ptid is different from the running
    thread.  */
 
@@ -175,34 +166,28 @@ static const struct ravenscar_reg_info aarch64_reg_info =
   ARRAY_SIZE (aarch64_context_offsets),
 };
 
-/* Implement the to_fetch_registers ravenscar_arch_ops method
-   for most Aarch64 targets.  */
-
-static void
-aarch64_ravenscar_fetch_registers (struct regcache *regcache, int regnum)
+struct aarch64_ravenscar_ops : public ravenscar_arch_ops
 {
-  aarch64_ravenscar_generic_fetch_registers
-    (&aarch64_reg_info, regcache, regnum);
-}
-
-/* Implement the to_store_registers ravenscar_arch_ops method
-   for most Aarch64 targets.  */
-
-static void
-aarch64_ravenscar_store_registers (struct regcache *regcache, int regnum)
-{
-  aarch64_ravenscar_generic_store_registers
-    (&aarch64_reg_info, regcache, regnum);
-}
+  void fetch_registers (struct regcache *regcache, int regnum) override
+  {
+    aarch64_ravenscar_generic_fetch_registers
+      (&aarch64_reg_info, regcache, regnum);
+  }
+
+  void store_registers (struct regcache *regcache, int regnum) override
+  {
+    aarch64_ravenscar_generic_store_registers
+      (&aarch64_reg_info, regcache, regnum);
+  }
+
+  void prepare_to_store (struct regcache *) override
+  {
+  }
+};
 
 /* The ravenscar_arch_ops vector for most Aarch64 targets.  */
 
-static struct ravenscar_arch_ops aarch64_ravenscar_ops =
-{
-  aarch64_ravenscar_fetch_registers,
-  aarch64_ravenscar_store_registers,
-  aarch64_ravenscar_generic_prepare_to_store
-};
+static struct aarch64_ravenscar_ops aarch64_ravenscar_ops;
 
 /* Register aarch64_ravenscar_ops in GDBARCH.  */
 
diff --git a/gdb/ppc-ravenscar-thread.c b/gdb/ppc-ravenscar-thread.c
index eca80c534a5..919b32c94f4 100644
--- a/gdb/ppc-ravenscar-thread.c
+++ b/gdb/ppc-ravenscar-thread.c
@@ -169,15 +169,6 @@ ppc_ravenscar_generic_fetch_registers
     }
 }
 
-/* to_prepare_to_store when inferior_ptid is different from the running
-   thread.  */
-
-static void
-ppc_ravenscar_generic_prepare_to_store (struct regcache *regcache)
-{
-  /* Nothing to do.  */
-}
-
 /* to_store_registers when inferior_ptid is different from the running
    thread.  */
 
@@ -211,32 +202,31 @@ static const struct ravenscar_reg_info ppc_reg_info =
   ARRAY_SIZE (powerpc_context_offsets),
 };
 
-/* Implement the to_fetch_registers ravenscar_arch_ops method
-   for most PowerPC targets.  */
+struct ppc_ravenscar_powerpc_ops : public ravenscar_arch_ops
+{
+  void fetch_registers (struct regcache *, int) override;
+  void store_registers (struct regcache *, int) override;
+
+  void prepare_to_store (struct regcache *) override
+  {
+  }
+};
 
-static void
-ppc_ravenscar_powerpc_fetch_registers (struct regcache *regcache, int regnum)
+void
+ppc_ravenscar_powerpc_ops::fetch_registers (struct regcache *regcache, int regnum)
 {
   ppc_ravenscar_generic_fetch_registers (&ppc_reg_info, regcache, regnum);
 }
 
-/* Implement the to_store_registers ravenscar_arch_ops method
-   for most PowerPC targets.  */
-
-static void
-ppc_ravenscar_powerpc_store_registers (struct regcache *regcache, int regnum)
+void
+ppc_ravenscar_powerpc_ops::store_registers (struct regcache *regcache, int regnum)
 {
   ppc_ravenscar_generic_store_registers (&ppc_reg_info, regcache, regnum);
 }
 
 /* The ravenscar_arch_ops vector for most PowerPC targets.  */
 
-static struct ravenscar_arch_ops ppc_ravenscar_powerpc_ops =
-{
-  ppc_ravenscar_powerpc_fetch_registers,
-  ppc_ravenscar_powerpc_store_registers,
-  ppc_ravenscar_generic_prepare_to_store
-};
+static struct ppc_ravenscar_powerpc_ops ppc_ravenscar_powerpc_ops;
 
 /* Register ppc_ravenscar_powerpc_ops in GDBARCH.  */
 
@@ -254,11 +244,18 @@ static const struct ravenscar_reg_info e500_reg_info =
   ARRAY_SIZE (e500_context_offsets),
 };
 
-/* Implement the to_fetch_registers ravenscar_arch_ops method
-   for E500 targets.  */
+struct ppc_ravenscar_e500_ops : public ravenscar_arch_ops
+{
+  void fetch_registers (struct regcache *, int) override;
+  void store_registers (struct regcache *, int) override;
 
-static void
-ppc_ravenscar_e500_fetch_registers (struct regcache *regcache, int regnum)
+  void prepare_to_store (struct regcache *) override
+  {
+  }
+};
+
+void
+ppc_ravenscar_e500_ops::fetch_registers (struct regcache *regcache, int regnum)
 {
   ppc_ravenscar_generic_fetch_registers (&e500_reg_info, regcache, regnum);
 }
@@ -266,20 +263,15 @@ ppc_ravenscar_e500_fetch_registers (struct regcache *regcache, int regnum)
 /* Implement the to_store_registers ravenscar_arch_ops method
    for E500 targets.  */
 
-static void
-ppc_ravenscar_e500_store_registers (struct regcache *regcache, int regnum)
+void
+ppc_ravenscar_e500_ops::store_registers (struct regcache *regcache, int regnum)
 {
   ppc_ravenscar_generic_store_registers (&e500_reg_info, regcache, regnum);
 }
 
 /* The ravenscar_arch_ops vector for E500 targets.  */
 
-static struct ravenscar_arch_ops ppc_ravenscar_e500_ops =
-{
-  ppc_ravenscar_e500_fetch_registers,
-  ppc_ravenscar_e500_store_registers,
-  ppc_ravenscar_generic_prepare_to_store
-};
+static struct ppc_ravenscar_e500_ops ppc_ravenscar_e500_ops;
 
 /* Register ppc_ravenscar_e500_ops in GDBARCH.  */
 
diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index 9d708fd8581..0dc50a41429 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -414,7 +414,7 @@ ravenscar_thread_target::fetch_registers (struct regcache *regcache, int regnum)
       struct ravenscar_arch_ops *arch_ops
 	= gdbarch_ravenscar_ops (gdbarch);
 
-      arch_ops->to_fetch_registers (regcache, regnum);
+      arch_ops->fetch_registers (regcache, regnum);
     }
   else
     beneath ()->fetch_registers (regcache, regnum);
@@ -434,7 +434,7 @@ ravenscar_thread_target::store_registers (struct regcache *regcache,
       struct ravenscar_arch_ops *arch_ops
 	= gdbarch_ravenscar_ops (gdbarch);
 
-      arch_ops->to_store_registers (regcache, regnum);
+      arch_ops->store_registers (regcache, regnum);
     }
   else
     beneath ()->store_registers (regcache, regnum);
@@ -453,7 +453,7 @@ ravenscar_thread_target::prepare_to_store (struct regcache *regcache)
       struct ravenscar_arch_ops *arch_ops
 	= gdbarch_ravenscar_ops (gdbarch);
 
-      arch_ops->to_prepare_to_store (regcache);
+      arch_ops->prepare_to_store (regcache);
     }
   else
     beneath ()->prepare_to_store (regcache);
diff --git a/gdb/ravenscar-thread.h b/gdb/ravenscar-thread.h
index 8aab0a124f2..f0c163c5f0b 100644
--- a/gdb/ravenscar-thread.h
+++ b/gdb/ravenscar-thread.h
@@ -24,9 +24,13 @@
 
 struct ravenscar_arch_ops
 {
-  void (*to_fetch_registers) (struct regcache *, int);
-  void (*to_store_registers) (struct regcache *, int);
-  void (*to_prepare_to_store) (struct regcache *);
+  virtual ~ravenscar_arch_ops ()
+  {
+  }
+
+  virtual void fetch_registers (struct regcache *, int) = 0;
+  virtual void store_registers (struct regcache *, int) = 0;
+  virtual void prepare_to_store (struct regcache *) = 0;
 };
 
 #endif /* !defined (RAVENSCAR_THREAD_H) */
diff --git a/gdb/sparc-ravenscar-thread.c b/gdb/sparc-ravenscar-thread.c
index e09e453eabf..4b26f55490c 100644
--- a/gdb/sparc-ravenscar-thread.c
+++ b/gdb/sparc-ravenscar-thread.c
@@ -25,11 +25,12 @@
 #include "ravenscar-thread.h"
 #include "sparc-ravenscar-thread.h"
 
-static void sparc_ravenscar_fetch_registers (struct regcache *regcache,
-                                             int regnum);
-static void sparc_ravenscar_store_registers (struct regcache *regcache,
-                                             int regnum);
-static void sparc_ravenscar_prepare_to_store (struct regcache *regcache);
+struct sparc_ravenscar_ops : public ravenscar_arch_ops
+{
+  void fetch_registers (struct regcache *, int) override;
+  void store_registers (struct regcache *, int) override;
+  void prepare_to_store (struct regcache *) override;
+};
 
 /* Register offsets from a referenced address (exempli gratia the
    Thread_Descriptor).  The referenced address depends on the register
@@ -100,8 +101,8 @@ register_in_thread_descriptor_p (int regnum)
 /* to_fetch_registers when inferior_ptid is different from the running
    thread.  */
 
-static void
-sparc_ravenscar_fetch_registers (struct regcache *regcache, int regnum)
+void
+sparc_ravenscar_ops::fetch_registers (struct regcache *regcache, int regnum)
 {
   struct gdbarch *gdbarch = regcache->arch ();
   const int sp_regnum = gdbarch_sp_regnum (gdbarch);
@@ -143,8 +144,8 @@ sparc_ravenscar_fetch_registers (struct regcache *regcache, int regnum)
 /* to_prepare_to_store when inferior_ptid is different from the running
    thread.  */
 
-static void
-sparc_ravenscar_prepare_to_store (struct regcache *regcache)
+void
+sparc_ravenscar_ops::prepare_to_store (struct regcache *regcache)
 {
   /* Nothing to do.  */
 }
@@ -152,8 +153,8 @@ sparc_ravenscar_prepare_to_store (struct regcache *regcache)
 /* to_store_registers when inferior_ptid is different from the running
    thread.  */
 
-static void
-sparc_ravenscar_store_registers (struct regcache *regcache, int regnum)
+void
+sparc_ravenscar_ops::store_registers (struct regcache *regcache, int regnum)
 {
   struct gdbarch *gdbarch = regcache->arch ();
   int buf_size = register_size (gdbarch, regnum);
@@ -178,12 +179,7 @@ sparc_ravenscar_store_registers (struct regcache *regcache, int regnum)
                 buf_size);
 }
 
-static struct ravenscar_arch_ops sparc_ravenscar_ops =
-{
-  sparc_ravenscar_fetch_registers,
-  sparc_ravenscar_store_registers,
-  sparc_ravenscar_prepare_to_store
-};
+static struct sparc_ravenscar_ops sparc_ravenscar_ops;
 
 /* Register ravenscar_arch_ops in GDBARCH.  */
 
-- 
2.17.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 6/8] Add push_target overload
  2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
  2019-02-07  9:40 ` [PATCH 7/8] Make the ravenscar thread target multi-target-ready Tom Tromey
  2019-02-07  9:40 ` [PATCH 4/8] Fix formatting in ravenscar-thread.c Tom Tromey
@ 2019-02-07  9:40 ` Tom Tromey
  2019-02-07 17:23   ` Pedro Alves
  2019-02-07  9:40 ` [PATCH 5/8] Minor C++-ification in ravenscar-thread.c Tom Tromey
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2019-02-07  9:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

From: Tom Tromey <tromey@adacore.com>

This adds a push_target overload that takes a "target_ops_up &&".
This removes some calls to release a target_ops_up, and makes the
intent here clearer.

gdb/ChangeLog
2019-02-07  Tom Tromey  <tromey@adacore.com>

	* target.h (push_target): Declare new overload.
	* target.c (push_target): New overload, taking an rvalue reference.
	* remote.c (remote_target::open_1): Use push_target overload.
	* corelow.c (core_target_open): Use push_target overload.
---
 gdb/ChangeLog | 7 +++++++
 gdb/corelow.c | 3 +--
 gdb/remote.c  | 4 +---
 gdb/target.c  | 9 +++++++++
 gdb/target.h  | 3 +++
 5 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 52d6d95b3c0..6a29d6a2328 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -417,8 +417,7 @@ core_target_open (const char *arg, int from_tty)
   if (!exec_bfd)
     set_gdbarch_from_file (core_bfd);
 
-  push_target (target);
-  target_holder.release ();
+  push_target (std::move (target_holder));
 
   inferior_ptid = null_ptid;
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 18e678d07af..95201eeab5c 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5547,9 +5547,7 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
     }
 
   /* Switch to using the remote target now.  */
-  push_target (remote);
-  /* The target stack owns the target now.  */
-  target_holder.release ();
+  push_target (std::move (target_holder));
 
   /* Register extra event sources in the event loop.  */
   rs->remote_async_inferior_event_token
diff --git a/gdb/target.c b/gdb/target.c
index c1ab07f7608..116510e8cb8 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -585,6 +585,15 @@ push_target (struct target_ops *t)
   g_target_stack.push (t);
 }
 
+/* See target.h  */
+
+void
+push_target (target_ops_up &&t)
+{
+  g_target_stack.push (t.get ());
+  t.release ();
+}
+
 /* See target.h.  */
 
 int
diff --git a/gdb/target.h b/gdb/target.h
index 96413aa6c3a..c95151a4044 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2333,6 +2333,9 @@ extern void add_deprecated_target_alias (const target_info &info,
 
 extern void push_target (struct target_ops *);
 
+/* An overload that deletes the target on failure.  */
+extern void push_target (target_ops_up &&);
+
 extern int unpush_target (struct target_ops *);
 
 extern void target_pre_inferior (int);
-- 
2.17.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 0/8] Clean up ravenscar-thread.c
@ 2019-02-07 14:22 Tom Tromey
  2019-02-07  9:40 ` [PATCH 7/8] Make the ravenscar thread target multi-target-ready Tom Tromey
                   ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Tom Tromey @ 2019-02-07 14:22 UTC (permalink / raw)
  To: gdb-patches

This series cleans up a few issues that I noticed in
ravenscar-thread.c.  Most of it is fairly straightforward, with some
patches being trivial.

There is one more general change in here, to add a push_target
overload.  See patch #6.

Finally, this series fixes a bug in ravenscar-thread, where wildcard
requests (to resume or wait) would not be respected.  This caused
problems when testing with qemu.

I tested this using the AdaCore internal test suite, which has some
Ravenscar tests.

Tom


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/8] Fix some typos in ravenscar-thread.c
  2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
                   ` (6 preceding siblings ...)
  2019-02-07  9:40 ` [PATCH 2/8] Exception safety " Tom Tromey
@ 2019-02-07 14:22 ` Tom Tromey
  7 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2019-02-07 14:22 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

From: Tom Tromey <tromey@adacore.com>

This fixes some typos I noticed in ravenscar-thread.c.

gdb/ChangeLog
2019-02-07  Tom Tromey  <tromey@adacore.com>

	* ravenscar-thread.c: Fix some typos.
---
 gdb/ChangeLog          |  4 ++++
 gdb/ravenscar-thread.c | 10 +++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
index bc7f9d6c658..32a4aa8d16a 100644
--- a/gdb/ravenscar-thread.c
+++ b/gdb/ravenscar-thread.c
@@ -36,7 +36,7 @@
 
    The typical situation is when debugging a bare-metal target over
    the remote protocol. In that situation, the system does not know
-   about high-level comcepts such as threads, only about some code
+   about high-level concepts such as threads, only about some code
    running on one or more CPUs. And since the remote protocol does not
    provide any handling for CPUs, the de facto standard for handling
    them is to have one thread per CPU, where the thread's ptid has
@@ -44,14 +44,14 @@
    2 for the second one, etc).  This module will make that assumption.
 
    This module then creates and maintains the list of threads based
-   on the list of Ada tasks, with one thread per Ada tasks. The convention
+   on the list of Ada tasks, with one thread per Ada task. The convention
    is that threads corresponding to the CPUs (see assumption above)
-   have a ptid_t of the form (PID, LWP, 0), which threads corresponding
+   have a ptid_t of the form (PID, LWP, 0), while threads corresponding
    to our Ada tasks have a ptid_t of the form (PID, 0, TID) where TID
    is the Ada task's ID as extracted from Ada runtime information.
 
-   Switching to a given Ada tasks (or its underlying thread) is performed
-   by fetching the registers of that tasks from the memory area where
+   Switching to a given Ada task (or its underlying thread) is performed
+   by fetching the registers of that task from the memory area where
    the registers were saved.  For any of the other operations, the
    operation is performed by first finding the CPU on which the task
    is running, switching to its corresponding ptid, and then performing
-- 
2.17.2

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/8] Make the ravenscar thread target multi-target-ready
  2019-02-07  9:40 ` [PATCH 7/8] Make the ravenscar thread target multi-target-ready Tom Tromey
@ 2019-02-07 17:23   ` Pedro Alves
  0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2019-02-07 17:23 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches; +Cc: Tom Tromey

On 02/07/2019 09:40 AM, Tom Tromey wrote:
> From: Tom Tromey <tromey@adacore.com>
> 
> This changes ravenscar-thread.c to make it ready for multi-target.
> This is done by moving globals into the target, and then arranging to
> allocate the target with "new" and delete the target in its "close"
> method.

Looks good to me, thanks for doing this.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 6/8] Add push_target overload
  2019-02-07  9:40 ` [PATCH 6/8] Add push_target overload Tom Tromey
@ 2019-02-07 17:23   ` Pedro Alves
  0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2019-02-07 17:23 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches; +Cc: Tom Tromey

On 02/07/2019 09:40 AM, Tom Tromey wrote:
> From: Tom Tromey <tromey@adacore.com>
> 
> This adds a push_target overload that takes a "target_ops_up &&".
> This removes some calls to release a target_ops_up, and makes the
> intent here clearer.

I like it.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/8] C++-ify ravenscar_arch_ops
  2019-02-07  9:40 ` [PATCH 3/8] C++-ify ravenscar_arch_ops Tom Tromey
@ 2019-02-07 17:23   ` Pedro Alves
  2019-02-11 18:56     ` Tom Tromey
  0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2019-02-07 17:23 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches; +Cc: Tom Tromey

On 02/07/2019 09:40 AM, Tom Tromey wrote:
> From: Tom Tromey <tromey@adacore.com>
> 
> This turns ravenscar_arch_ops into an abstract base class and updates
> all the places where it is used.  This is an improvement because it
> avoids any possibility of forgetting to set one of the function
> pointers.  It also makes clear that these functions aren't intended to
> be changed dynamically.

LGTM, though I'd probably eliminate the prepare_to_store method or
make it not-abstract -- seems that no architecture does anything with it.

Thanks,
Pedro Alves

> 
> gdb/ChangeLog
> 2019-02-07  Tom Tromey  <tromey@adacore.com>
> 
> 	* sparc-ravenscar-thread.c (struct sparc_ravenscar_ops): Derive
> 	from ravenscar_arch_ops.
> 	(sparc_ravenscar_ops::fetch_registers)
> 	(sparc_ravenscar_ops::prepare_to_store)
> 	(sparc_ravenscar_ops::store_registers): Now methods.
> 	(sparc_ravenscar_ops): Redefine.
> 	* ravenscar-thread.h (struct ravenscar_arch_ops): Add virtual
> 	methods and destructor.  Remove members.
> 	* ravenscar-thread.c (ravenscar_thread_target::fetch_registers)
> 	(ravenscar_thread_target::store_registers)
> 	(ravenscar_thread_target::prepare_to_store): Update.
> 	* ppc-ravenscar-thread.c (ppc_ravenscar_generic_prepare_to_store):
> 	Remove.
> 	(struct ppc_ravenscar_powerpc_ops): Derive from
> 	ravenscar_arch_ops.
> 	(ppc_ravenscar_powerpc_ops::fetch_registers)
> 	(ppc_ravenscar_powerpc_ops::store_registers): Now methods.
> 	(ppc_ravenscar_powerpc_ops): Redefine.
> 	(struct ppc_ravenscar_e500_ops): Derive from ravenscar_arch_ops.
> 	(ppc_ravenscar_e500_ops::fetch_registers)
> 	(ppc_ravenscar_e500_ops::store_registers): Now methods.
> 	(ppc_ravenscar_e500_ops): Redefine.
> 	* aarch64-ravenscar-thread.c
> 	(aarch64_ravenscar_generic_prepare_to_store): Remove.
> 	(struct aarch64_ravenscar_ops): Derive from ravenscar_arch_ops.
> 	(aarch64_ravenscar_fetch_registers)
> 	(aarch64_ravenscar_store_registers): Now methods.
> 	(aarch64_ravenscar_ops): Redefine.
> ---
>  gdb/ChangeLog                  | 31 ++++++++++++++++
>  gdb/aarch64-ravenscar-thread.c | 51 ++++++++++-----------------
>  gdb/ppc-ravenscar-thread.c     | 64 +++++++++++++++-------------------
>  gdb/ravenscar-thread.c         |  6 ++--
>  gdb/ravenscar-thread.h         | 10 ++++--
>  gdb/sparc-ravenscar-thread.c   | 30 +++++++---------
>  6 files changed, 100 insertions(+), 92 deletions(-)
> 
> diff --git a/gdb/aarch64-ravenscar-thread.c b/gdb/aarch64-ravenscar-thread.c
> index 1234650a2b7..fa99d896ffe 100644
> --- a/gdb/aarch64-ravenscar-thread.c
> +++ b/gdb/aarch64-ravenscar-thread.c
> @@ -133,15 +133,6 @@ aarch64_ravenscar_generic_fetch_registers
>      }
>  }
>  
> -/* to_prepare_to_store when inferior_ptid is different from the running
> -   thread.  */
> -
> -static void
> -aarch64_ravenscar_generic_prepare_to_store (struct regcache *regcache)
> -{
> -  /* Nothing to do.  */
> -}
> -
>  /* to_store_registers when inferior_ptid is different from the running
>     thread.  */
>  
> @@ -175,34 +166,28 @@ static const struct ravenscar_reg_info aarch64_reg_info =
>    ARRAY_SIZE (aarch64_context_offsets),
>  };
>  
> -/* Implement the to_fetch_registers ravenscar_arch_ops method
> -   for most Aarch64 targets.  */
> -
> -static void
> -aarch64_ravenscar_fetch_registers (struct regcache *regcache, int regnum)
> +struct aarch64_ravenscar_ops : public ravenscar_arch_ops
>  {
> -  aarch64_ravenscar_generic_fetch_registers
> -    (&aarch64_reg_info, regcache, regnum);
> -}
> -
> -/* Implement the to_store_registers ravenscar_arch_ops method
> -   for most Aarch64 targets.  */
> -
> -static void
> -aarch64_ravenscar_store_registers (struct regcache *regcache, int regnum)
> -{
> -  aarch64_ravenscar_generic_store_registers
> -    (&aarch64_reg_info, regcache, regnum);
> -}
> +  void fetch_registers (struct regcache *regcache, int regnum) override
> +  {
> +    aarch64_ravenscar_generic_fetch_registers
> +      (&aarch64_reg_info, regcache, regnum);
> +  }
> +
> +  void store_registers (struct regcache *regcache, int regnum) override
> +  {
> +    aarch64_ravenscar_generic_store_registers
> +      (&aarch64_reg_info, regcache, regnum);
> +  }
> +
> +  void prepare_to_store (struct regcache *) override
> +  {
> +  }
> +};
>  
>  /* The ravenscar_arch_ops vector for most Aarch64 targets.  */
>  
> -static struct ravenscar_arch_ops aarch64_ravenscar_ops =
> -{
> -  aarch64_ravenscar_fetch_registers,
> -  aarch64_ravenscar_store_registers,
> -  aarch64_ravenscar_generic_prepare_to_store
> -};
> +static struct aarch64_ravenscar_ops aarch64_ravenscar_ops;
>  
>  /* Register aarch64_ravenscar_ops in GDBARCH.  */
>  
> diff --git a/gdb/ppc-ravenscar-thread.c b/gdb/ppc-ravenscar-thread.c
> index eca80c534a5..919b32c94f4 100644
> --- a/gdb/ppc-ravenscar-thread.c
> +++ b/gdb/ppc-ravenscar-thread.c
> @@ -169,15 +169,6 @@ ppc_ravenscar_generic_fetch_registers
>      }
>  }
>  
> -/* to_prepare_to_store when inferior_ptid is different from the running
> -   thread.  */
> -
> -static void
> -ppc_ravenscar_generic_prepare_to_store (struct regcache *regcache)
> -{
> -  /* Nothing to do.  */
> -}
> -
>  /* to_store_registers when inferior_ptid is different from the running
>     thread.  */
>  
> @@ -211,32 +202,31 @@ static const struct ravenscar_reg_info ppc_reg_info =
>    ARRAY_SIZE (powerpc_context_offsets),
>  };
>  
> -/* Implement the to_fetch_registers ravenscar_arch_ops method
> -   for most PowerPC targets.  */
> +struct ppc_ravenscar_powerpc_ops : public ravenscar_arch_ops
> +{
> +  void fetch_registers (struct regcache *, int) override;
> +  void store_registers (struct regcache *, int) override;
> +
> +  void prepare_to_store (struct regcache *) override
> +  {
> +  }
> +};
>  
> -static void
> -ppc_ravenscar_powerpc_fetch_registers (struct regcache *regcache, int regnum)
> +void
> +ppc_ravenscar_powerpc_ops::fetch_registers (struct regcache *regcache, int regnum)
>  {
>    ppc_ravenscar_generic_fetch_registers (&ppc_reg_info, regcache, regnum);
>  }
>  
> -/* Implement the to_store_registers ravenscar_arch_ops method
> -   for most PowerPC targets.  */
> -
> -static void
> -ppc_ravenscar_powerpc_store_registers (struct regcache *regcache, int regnum)
> +void
> +ppc_ravenscar_powerpc_ops::store_registers (struct regcache *regcache, int regnum)
>  {
>    ppc_ravenscar_generic_store_registers (&ppc_reg_info, regcache, regnum);
>  }
>  
>  /* The ravenscar_arch_ops vector for most PowerPC targets.  */
>  
> -static struct ravenscar_arch_ops ppc_ravenscar_powerpc_ops =
> -{
> -  ppc_ravenscar_powerpc_fetch_registers,
> -  ppc_ravenscar_powerpc_store_registers,
> -  ppc_ravenscar_generic_prepare_to_store
> -};
> +static struct ppc_ravenscar_powerpc_ops ppc_ravenscar_powerpc_ops;
>  
>  /* Register ppc_ravenscar_powerpc_ops in GDBARCH.  */
>  
> @@ -254,11 +244,18 @@ static const struct ravenscar_reg_info e500_reg_info =
>    ARRAY_SIZE (e500_context_offsets),
>  };
>  
> -/* Implement the to_fetch_registers ravenscar_arch_ops method
> -   for E500 targets.  */
> +struct ppc_ravenscar_e500_ops : public ravenscar_arch_ops
> +{
> +  void fetch_registers (struct regcache *, int) override;
> +  void store_registers (struct regcache *, int) override;
>  
> -static void
> -ppc_ravenscar_e500_fetch_registers (struct regcache *regcache, int regnum)
> +  void prepare_to_store (struct regcache *) override
> +  {
> +  }
> +};
> +
> +void
> +ppc_ravenscar_e500_ops::fetch_registers (struct regcache *regcache, int regnum)
>  {
>    ppc_ravenscar_generic_fetch_registers (&e500_reg_info, regcache, regnum);
>  }
> @@ -266,20 +263,15 @@ ppc_ravenscar_e500_fetch_registers (struct regcache *regcache, int regnum)
>  /* Implement the to_store_registers ravenscar_arch_ops method
>     for E500 targets.  */
>  
> -static void
> -ppc_ravenscar_e500_store_registers (struct regcache *regcache, int regnum)
> +void
> +ppc_ravenscar_e500_ops::store_registers (struct regcache *regcache, int regnum)
>  {
>    ppc_ravenscar_generic_store_registers (&e500_reg_info, regcache, regnum);
>  }
>  
>  /* The ravenscar_arch_ops vector for E500 targets.  */
>  
> -static struct ravenscar_arch_ops ppc_ravenscar_e500_ops =
> -{
> -  ppc_ravenscar_e500_fetch_registers,
> -  ppc_ravenscar_e500_store_registers,
> -  ppc_ravenscar_generic_prepare_to_store
> -};
> +static struct ppc_ravenscar_e500_ops ppc_ravenscar_e500_ops;
>  
>  /* Register ppc_ravenscar_e500_ops in GDBARCH.  */
>  
> diff --git a/gdb/ravenscar-thread.c b/gdb/ravenscar-thread.c
> index 9d708fd8581..0dc50a41429 100644
> --- a/gdb/ravenscar-thread.c
> +++ b/gdb/ravenscar-thread.c
> @@ -414,7 +414,7 @@ ravenscar_thread_target::fetch_registers (struct regcache *regcache, int regnum)
>        struct ravenscar_arch_ops *arch_ops
>  	= gdbarch_ravenscar_ops (gdbarch);
>  
> -      arch_ops->to_fetch_registers (regcache, regnum);
> +      arch_ops->fetch_registers (regcache, regnum);
>      }
>    else
>      beneath ()->fetch_registers (regcache, regnum);
> @@ -434,7 +434,7 @@ ravenscar_thread_target::store_registers (struct regcache *regcache,
>        struct ravenscar_arch_ops *arch_ops
>  	= gdbarch_ravenscar_ops (gdbarch);
>  
> -      arch_ops->to_store_registers (regcache, regnum);
> +      arch_ops->store_registers (regcache, regnum);
>      }
>    else
>      beneath ()->store_registers (regcache, regnum);
> @@ -453,7 +453,7 @@ ravenscar_thread_target::prepare_to_store (struct regcache *regcache)
>        struct ravenscar_arch_ops *arch_ops
>  	= gdbarch_ravenscar_ops (gdbarch);
>  
> -      arch_ops->to_prepare_to_store (regcache);
> +      arch_ops->prepare_to_store (regcache);
>      }
>    else
>      beneath ()->prepare_to_store (regcache);
> diff --git a/gdb/ravenscar-thread.h b/gdb/ravenscar-thread.h
> index 8aab0a124f2..f0c163c5f0b 100644
> --- a/gdb/ravenscar-thread.h
> +++ b/gdb/ravenscar-thread.h
> @@ -24,9 +24,13 @@
>  
>  struct ravenscar_arch_ops
>  {
> -  void (*to_fetch_registers) (struct regcache *, int);
> -  void (*to_store_registers) (struct regcache *, int);
> -  void (*to_prepare_to_store) (struct regcache *);
> +  virtual ~ravenscar_arch_ops ()
> +  {
> +  }
> +
> +  virtual void fetch_registers (struct regcache *, int) = 0;
> +  virtual void store_registers (struct regcache *, int) = 0;
> +  virtual void prepare_to_store (struct regcache *) = 0;
>  };
>  
>  #endif /* !defined (RAVENSCAR_THREAD_H) */
> diff --git a/gdb/sparc-ravenscar-thread.c b/gdb/sparc-ravenscar-thread.c
> index e09e453eabf..4b26f55490c 100644
> --- a/gdb/sparc-ravenscar-thread.c
> +++ b/gdb/sparc-ravenscar-thread.c
> @@ -25,11 +25,12 @@
>  #include "ravenscar-thread.h"
>  #include "sparc-ravenscar-thread.h"
>  
> -static void sparc_ravenscar_fetch_registers (struct regcache *regcache,
> -                                             int regnum);
> -static void sparc_ravenscar_store_registers (struct regcache *regcache,
> -                                             int regnum);
> -static void sparc_ravenscar_prepare_to_store (struct regcache *regcache);
> +struct sparc_ravenscar_ops : public ravenscar_arch_ops
> +{
> +  void fetch_registers (struct regcache *, int) override;
> +  void store_registers (struct regcache *, int) override;
> +  void prepare_to_store (struct regcache *) override;
> +};
>  
>  /* Register offsets from a referenced address (exempli gratia the
>     Thread_Descriptor).  The referenced address depends on the register
> @@ -100,8 +101,8 @@ register_in_thread_descriptor_p (int regnum)
>  /* to_fetch_registers when inferior_ptid is different from the running
>     thread.  */
>  
> -static void
> -sparc_ravenscar_fetch_registers (struct regcache *regcache, int regnum)
> +void
> +sparc_ravenscar_ops::fetch_registers (struct regcache *regcache, int regnum)
>  {
>    struct gdbarch *gdbarch = regcache->arch ();
>    const int sp_regnum = gdbarch_sp_regnum (gdbarch);
> @@ -143,8 +144,8 @@ sparc_ravenscar_fetch_registers (struct regcache *regcache, int regnum)
>  /* to_prepare_to_store when inferior_ptid is different from the running
>     thread.  */
>  
> -static void
> -sparc_ravenscar_prepare_to_store (struct regcache *regcache)
> +void
> +sparc_ravenscar_ops::prepare_to_store (struct regcache *regcache)
>  {
>    /* Nothing to do.  */
>  }
> @@ -152,8 +153,8 @@ sparc_ravenscar_prepare_to_store (struct regcache *regcache)
>  /* to_store_registers when inferior_ptid is different from the running
>     thread.  */
>  
> -static void
> -sparc_ravenscar_store_registers (struct regcache *regcache, int regnum)
> +void
> +sparc_ravenscar_ops::store_registers (struct regcache *regcache, int regnum)
>  {
>    struct gdbarch *gdbarch = regcache->arch ();
>    int buf_size = register_size (gdbarch, regnum);
> @@ -178,12 +179,7 @@ sparc_ravenscar_store_registers (struct regcache *regcache, int regnum)
>                  buf_size);
>  }
>  
> -static struct ravenscar_arch_ops sparc_ravenscar_ops =
> -{
> -  sparc_ravenscar_fetch_registers,
> -  sparc_ravenscar_store_registers,
> -  sparc_ravenscar_prepare_to_store
> -};
> +static struct sparc_ravenscar_ops sparc_ravenscar_ops;
>  
>  /* Register ravenscar_arch_ops in GDBARCH.  */
>  
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c
  2019-02-07  9:40 ` [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c Tom Tromey
@ 2019-02-07 17:23   ` Pedro Alves
  2019-02-09  4:41     ` Joel Brobecker
  2019-02-15 20:39     ` Tom Tromey
  0 siblings, 2 replies; 19+ messages in thread
From: Pedro Alves @ 2019-02-07 17:23 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches; +Cc: Tom Tromey

On 02/07/2019 09:40 AM, Tom Tromey wrote:
> From: Tom Tromey <tromey@adacore.com>
> 
> ravenscar-thread.c intercepts resume and wait target requests and
> replaces the requested ptid with the ptid of the underlying CPU.
> However, this is incorrect when a request is made with a wildcard
> ptid.

Can you clarify a bit more what went wrong?
IIRC, the base target always has one thread/cpu only, anyway?  What
difference does the patch make in practice?

The reason I'm wondering is because the patch makes me wonder about
a step request with no sched-lock, which is the default (*).
In that case, you'll have:

 - the thread to step is in inferior_ptid
 - ptid == minus_one_ptid (indicating everything resumes / no schedlock)

So seems like after this patch the lower layer might get a request to step
an unknown inferior_ptid?  Might not happen by luck/accident.
I'd suspect that you'd want to do instead:

  inferior_ptid = m_base_ptid;
  /* If we see a wildcard resume, we simply pass that on.  Otherwise,
     arrange to resume the base ptid.  */
  if (ptid != minus_one_ptid)
    ptid = m_base_ptid;
  beneath ()->resume (ptid, step, siggnal);

I.e., always flip inferior_ptid.

(*) since ravenscar-thread.c doesn't know how to interact with
    the ravenscar runtime to specify which threads are resumable,
    "schedlock on" most certainly doesn't work properly.  E.g.,
      "task 1 stops; set scheduler-locking on; task 2; step"
    seemingly will step task 1 instead of 2, AFAICT.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c
  2019-02-07 17:23   ` Pedro Alves
@ 2019-02-09  4:41     ` Joel Brobecker
  2019-02-14 13:35       ` Pedro Alves
  2019-02-15 20:39     ` Tom Tromey
  1 sibling, 1 reply; 19+ messages in thread
From: Joel Brobecker @ 2019-02-09  4:41 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Tom Tromey, gdb-patches, Tom Tromey

> > ravenscar-thread.c intercepts resume and wait target requests and
> > replaces the requested ptid with the ptid of the underlying CPU.
> > However, this is incorrect when a request is made with a wildcard
> > ptid.
> 
> Can you clarify a bit more what went wrong?
> IIRC, the base target always has one thread/cpu only, anyway?  What
> difference does the patch make in practice?

This happens when debugging application on a multi-CPU target,
using QEMU, where some tasks are allocated to one CPU, and others
are allocated to the other.

When stopping, QEMU tells us about one thread per CPU, so we start
with one possible base_ptid per CPU.  Previously, sending a resume
order for one thread actually resumed execution on all the CPUs.
But during an upgrade, which behavior was changed so that sending
a resume order for one thread only wakes the corresponding CPU up.

At the user level, we noticed the issue because we had a test were
we insert a breakpoint one some code which is only run from, say,
CPU #2, whereas we unfortunately resumed the execution after having
stopped somewhere in CPU #1. As a result, we sent an order to resume
CPU #1, which starves CPU #2 forever, because the code in CPU #1
waits for some of the Ada tasks allocated to CPU #2 (and we never
reach our breakpoint either).

> The reason I'm wondering is because the patch makes me wonder about
> a step request with no sched-lock, which is the default (*).
> In that case, you'll have:
> 
>  - the thread to step is in inferior_ptid
>  - ptid == minus_one_ptid (indicating everything resumes / no schedlock)
> 
> So seems like after this patch the lower layer might get a request to step
> an unknown inferior_ptid?  Might not happen by luck/accident.
> I'd suspect that you'd want to do instead:
> 
>   inferior_ptid = m_base_ptid;
>   /* If we see a wildcard resume, we simply pass that on.  Otherwise,
>      arrange to resume the base ptid.  */
>   if (ptid != minus_one_ptid)
>     ptid = m_base_ptid;
>   beneath ()->resume (ptid, step, siggnal);
> 
> I.e., always flip inferior_ptid.
> 
> (*) since ravenscar-thread.c doesn't know how to interact with
>     the ravenscar runtime to specify which threads are resumable,
>     "schedlock on" most certainly doesn't work properly.  E.g.,
>       "task 1 stops; set scheduler-locking on; task 2; step"
>     seemingly will step task 1 instead of 2, AFAICT.

That's correct (unless if you are in the particular situation where
you have one task per CPU).

-- 
Joel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/8] C++-ify ravenscar_arch_ops
  2019-02-07 17:23   ` Pedro Alves
@ 2019-02-11 18:56     ` Tom Tromey
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2019-02-11 18:56 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Tom Tromey, gdb-patches, Tom Tromey

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> This turns ravenscar_arch_ops into an abstract base class and updates
>> all the places where it is used.  This is an improvement because it
>> avoids any possibility of forgetting to set one of the function
>> pointers.  It also makes clear that these functions aren't intended to
>> be changed dynamically.

Pedro> LGTM, though I'd probably eliminate the prepare_to_store method or
Pedro> make it not-abstract -- seems that no architecture does anything with it.

I had noticed that as well, but decided to leave it in on the theory
that maybe it is good to keep the interfaces "parallel".  But, I don't
think it matters all that much, because if it is needed in the future,
it's easy enough to add it.  So, I'll remove it.

Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c
  2019-02-09  4:41     ` Joel Brobecker
@ 2019-02-14 13:35       ` Pedro Alves
  2019-02-15 20:43         ` Tom Tromey
  0 siblings, 1 reply; 19+ messages in thread
From: Pedro Alves @ 2019-02-14 13:35 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Tom Tromey, gdb-patches, Tom Tromey

On 02/09/2019 04:41 AM, Joel Brobecker wrote:
>>> ravenscar-thread.c intercepts resume and wait target requests and
>>> replaces the requested ptid with the ptid of the underlying CPU.
>>> However, this is incorrect when a request is made with a wildcard
>>> ptid.
>>
>> Can you clarify a bit more what went wrong?
>> IIRC, the base target always has one thread/cpu only, anyway?  What
>> difference does the patch make in practice?
> 
> This happens when debugging application on a multi-CPU target,
> using QEMU, where some tasks are allocated to one CPU, and others
> are allocated to the other.
> 
> When stopping, QEMU tells us about one thread per CPU, so we start
> with one possible base_ptid per CPU.  Previously, sending a resume
> order for one thread actually resumed execution on all the CPUs.
> But during an upgrade, which behavior was changed so that sending
> a resume order for one thread only wakes the corresponding CPU up.

Interesting -- that clarifies the "why changed that made this
noticeable now" question I had in my mind.  It's the sort of thing that
I think is worth it to include in the commit log.

> 
> At the user level, we noticed the issue because we had a test were
> we insert a breakpoint one some code which is only run from, say,
> CPU #2, whereas we unfortunately resumed the execution after having
> stopped somewhere in CPU #1. As a result, we sent an order to resume
> CPU #1, which starves CPU #2 forever, because the code in CPU #1
> waits for some of the Ada tasks allocated to CPU #2 (and we never
> reach our breakpoint either).

I see.  For some reason I recalled that Ravenscar didn't support SMP.

> 
>> The reason I'm wondering is because the patch makes me wonder about
>> a step request with no sched-lock, which is the default (*).
>> In that case, you'll have:
>>
>>  - the thread to step is in inferior_ptid
>>  - ptid == minus_one_ptid (indicating everything resumes / no schedlock)
>>
>> So seems like after this patch the lower layer might get a request to step
>> an unknown inferior_ptid?  Might not happen by luck/accident.
>> I'd suspect that you'd want to do instead:
>>
>>   inferior_ptid = m_base_ptid;
>>   /* If we see a wildcard resume, we simply pass that on.  Otherwise,
>>      arrange to resume the base ptid.  */
>>   if (ptid != minus_one_ptid)
>>     ptid = m_base_ptid;
>>   beneath ()->resume (ptid, step, siggnal);
>>
>> I.e., always flip inferior_ptid.

This comment still applies, I think.

BTW, the main reason I asked is because I have a change in the multi-target
branch that makes infrun switch inferior_ptid to null_ptid before
handling events, which exposed a number of bad assumptions that could lead
to accessing the wrong target.  Ravenscar may end up affected, but I'm
not certain.

>>
>> (*) since ravenscar-thread.c doesn't know how to interact with
>>     the ravenscar runtime to specify which threads are resumable,
>>     "schedlock on" most certainly doesn't work properly.  E.g.,
>>       "task 1 stops; set scheduler-locking on; task 2; step"
>>     seemingly will step task 1 instead of 2, AFAICT.
> 
> That's correct (unless if you are in the particular situation where
> you have one task per CPU).
Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c
  2019-02-07 17:23   ` Pedro Alves
  2019-02-09  4:41     ` Joel Brobecker
@ 2019-02-15 20:39     ` Tom Tromey
  2019-02-20 19:23       ` Pedro Alves
  1 sibling, 1 reply; 19+ messages in thread
From: Tom Tromey @ 2019-02-15 20:39 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Tom Tromey, gdb-patches, Tom Tromey

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> The reason I'm wondering is because the patch makes me wonder about
Pedro> a step request with no sched-lock, which is the default (*).
Pedro> In that case, you'll have:

I think the default is "replay", which is normally like "off" -- but
there's a Fedora patch to change the default to "step"?

Anyway, not super important...

Pedro> So seems like after this patch the lower layer might get a request to step
Pedro> an unknown inferior_ptid?  Might not happen by luck/accident.
Pedro> I'd suspect that you'd want to do instead:
[...]

Yes, I agree.  Thanks for pointing this out.  I've made this change.

Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c
  2019-02-14 13:35       ` Pedro Alves
@ 2019-02-15 20:43         ` Tom Tromey
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Tromey @ 2019-02-15 20:43 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Joel Brobecker, Tom Tromey, gdb-patches, Tom Tromey

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Interesting -- that clarifies the "why changed that made this
Pedro> noticeable now" question I had in my mind.  It's the sort of thing that
Pedro> I think is worth it to include in the commit log.

I'm copying Joel's text into the commit message :)

Pedro> BTW, the main reason I asked is because I have a change in the multi-target
Pedro> branch that makes infrun switch inferior_ptid to null_ptid before
Pedro> handling events, which exposed a number of bad assumptions that could lead
Pedro> to accessing the wrong target.  Ravenscar may end up affected, but I'm
Pedro> not certain.

FWIW, ravenscar-thread.c doesn't seem to intrinsically care about
inferior_ptid -- it is just setting it for the benefit of any lower
layers on the target stack.  So, if the rest of the stack is changed so
as not to rely on this, then ravenscar-thread can follow.

I'm re-testing this series using the AdaCore internal test suite, which
exercises ravenscar-thread; I'll push them if this goes well.

thanks,
Tom

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c
  2019-02-15 20:39     ` Tom Tromey
@ 2019-02-20 19:23       ` Pedro Alves
  0 siblings, 0 replies; 19+ messages in thread
From: Pedro Alves @ 2019-02-20 19:23 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches, Tom Tromey

On 02/15/2019 08:39 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> The reason I'm wondering is because the patch makes me wonder about
> Pedro> a step request with no sched-lock, which is the default (*).
> Pedro> In that case, you'll have:
> 
> I think the default is "replay", which is normally like "off"

Yes, replay is basically "off", except when you're replaying it behaves
like "step".  What I mean is that by default when you step all threads
run unlocked.

> -- but
> there's a Fedora patch to change the default to "step"?

Yeah.  On my list of patches to drop from Fedora, actually.

> 
> Anyway, not super important...
> 
> Pedro> So seems like after this patch the lower layer might get a request to step
> Pedro> an unknown inferior_ptid?  Might not happen by luck/accident.
> Pedro> I'd suspect that you'd want to do instead:
> [...]
> 
> Yes, I agree.  Thanks for pointing this out.  I've made this change.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-02-20 19:23 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-07 14:22 [PATCH 0/8] Clean up ravenscar-thread.c Tom Tromey
2019-02-07  9:40 ` [PATCH 7/8] Make the ravenscar thread target multi-target-ready Tom Tromey
2019-02-07 17:23   ` Pedro Alves
2019-02-07  9:40 ` [PATCH 4/8] Fix formatting in ravenscar-thread.c Tom Tromey
2019-02-07  9:40 ` [PATCH 6/8] Add push_target overload Tom Tromey
2019-02-07 17:23   ` Pedro Alves
2019-02-07  9:40 ` [PATCH 5/8] Minor C++-ification in ravenscar-thread.c Tom Tromey
2019-02-07  9:40 ` [PATCH 3/8] C++-ify ravenscar_arch_ops Tom Tromey
2019-02-07 17:23   ` Pedro Alves
2019-02-11 18:56     ` Tom Tromey
2019-02-07  9:40 ` [PATCH 8/8] Special-case wildcard requests in ravenscar-thread.c Tom Tromey
2019-02-07 17:23   ` Pedro Alves
2019-02-09  4:41     ` Joel Brobecker
2019-02-14 13:35       ` Pedro Alves
2019-02-15 20:43         ` Tom Tromey
2019-02-15 20:39     ` Tom Tromey
2019-02-20 19:23       ` Pedro Alves
2019-02-07  9:40 ` [PATCH 2/8] Exception safety " Tom Tromey
2019-02-07 14:22 ` [PATCH 1/8] Fix some typos " Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).