[PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache

[PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache

[Simon Marchi]

A following patch expects to be able to read the core target's auxv
data right after pushing it to the inferior's target stack.  It is not
the case today, because the read requests would hit the auxv cache,
filled earlier using (empty) auxv data obtained from the exec target.
The auxv cache is only cleared a bit later in the core target
initialization, when the inferior_appeared observers are notified.

I think it would make sense to flush an inferiors auxv cache as soone as
its target stack is modified.  The auxv cache should reflect what
reading an inferior's auxv data would look like at any point in time,
and simply speed up that process.  In other words, it should work the
same with the cache as it would work without the cache.  It seems
obvious that pushing/popping a target from the stack may change what
reading auxv for that inferior would return, and that we should
therefore flush the cache at that point.

Add an inferior_target_stack_changed observable, and attach
invalidate_auxv_cache_inf to it.  Notify this observable in the
push_target and unpush_target methods of inferior.

Change-Id: I76b00cebe933e3b26620eda39f57425aaba928e5
---
 gdb/auxv.c       |  2 ++
 gdb/inferior.c   | 23 ++++++++++++++++++++++-
 gdb/inferior.h   |  9 ++-------
 gdb/observable.c |  1 +
 gdb/observable.h |  3 +++
 5 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/gdb/auxv.c b/gdb/auxv.c
index 0ac74826aebb..73e84cf144db 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -614,4 +614,6 @@ This is information provided by the operating system at program startup."));
   gdb::observers::inferior_exit.attach (invalidate_auxv_cache_inf, "auxv");
   gdb::observers::inferior_appeared.attach (invalidate_auxv_cache_inf, "auxv");
   gdb::observers::executable_changed.attach (invalidate_auxv_cache, "auxv");
+  gdb::observers::inferior_target_stack_changed.attach
+    (invalidate_auxv_cache_inf, "auxv");
 }
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 23cbfd63bde0..23a68a87c728 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -84,6 +84,25 @@ inferior::inferior (int pid_)
 
 /* See inferior.h.  */
 
+void
+inferior::push_target (target_ops *t)
+{
+  m_target_stack.push (t);
+  gdb::observers::inferior_target_stack_changed.notify (this);
+}
+
+/* See inferior.h.  */
+
+void
+inferior::push_target (target_ops_up &&t)
+{
+  m_target_stack.push (t.get ());
+  t.release ();
+  gdb::observers::inferior_target_stack_changed.notify (this);
+}
+
+/* See inferior.h.  */
+
 int
 inferior::unpush_target (struct target_ops *t)
 {
@@ -100,7 +119,9 @@ inferior::unpush_target (struct target_ops *t)
 	proc_target->maybe_remove_resumed_with_pending_wait_status (thread);
     }
 
-  return m_target_stack.unpush (t);
+  bool ret = m_target_stack.unpush (t);
+  gdb::observers::inferior_target_stack_changed.notify (this);
+  return ret;
 }
 
 void
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 69525a2e053f..909e1819922d 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -352,15 +352,10 @@ class inferior : public refcounted_object,
   bool deletable () const { return refcount () == 0; }
 
   /* Push T in this inferior's target stack.  */
-  void push_target (struct target_ops *t)
-  { m_target_stack.push (t); }
+  void push_target (target_ops *t);
 
   /* An overload that deletes the target on failure.  */
-  void push_target (target_ops_up &&t)
-  {
-    m_target_stack.push (t.get ());
-    t.release ();
-  }
+  void push_target (target_ops_up &&t);
 
   /* Unpush T from this inferior's target stack.  */
   int unpush_target (struct target_ops *t);
diff --git a/gdb/observable.c b/gdb/observable.c
index 0bc8697f137a..dd17ec9ab769 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -44,6 +44,7 @@ DEFINE_OBSERVABLE (target_changed);
 DEFINE_OBSERVABLE (executable_changed);
 DEFINE_OBSERVABLE (inferior_created);
 DEFINE_OBSERVABLE (inferior_execd);
+DEFINE_OBSERVABLE (inferior_target_stack_changed);
 DEFINE_OBSERVABLE (record_changed);
 DEFINE_OBSERVABLE (solib_loaded);
 DEFINE_OBSERVABLE (solib_unloaded);
diff --git a/gdb/observable.h b/gdb/observable.h
index 1103c5c98a6b..23ea6d5f6a30 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -93,6 +93,9 @@ extern observable<inferior */* inferior */> inferior_created;
 /* The inferior INF has exec'ed a new executable file.  */
 extern observable<struct inferior */* inf */> inferior_execd;
 
+/* Inferior INF's target stack changed (a target was pushed or popped).  */
+extern observable<inferior */* inf */> inferior_target_stack_changed;
+
 /* The status of process record for inferior inferior in gdb has
    changed.  The process record is started if STARTED is true, and
    the process record is stopped if STARTED is false.

base-commit: d0a2cfbd3141dae38498fa077b01ae6bb394462b
-- 
2.38.1

[PATCH 2/3] gdb: break up core_target initialization

[Simon Marchi]

In the core target constructor, we currently have to jump through some
hoops to read the auxv data from the core, in order to choose an
appropriate gdbarch for the inferior.  The core BFD gives us a gdbarch,
but it might not tell the whole story.  By reading some auxv fields,
some architectures are able to choose a more specific gdbarch.  The
problem is that to read auxv from the core using the inferior's target
stack, the core target needs to be pushed on the inferior's target
stack.  But this work is done in the core target constructor, so it
can't be pushed at this point.  The current solution is to pass a
pointer to the core target to gdbarch_core_read_description, in
core_target::read_description.  That "hack" must then propagate to many
functions involved in selecting the architecture and reading auxv.

With this patch, I propose to break things up to avoid the problem.  The
core_target constructor will now do only trivial stuff that doesn't need
to call things outside core_target.  Then, we'll push the core_target on
the inferior's target stack.  And finally, complete the initialization
that potentially requires doing target calls.  The target calls to read
auxv at this point will just be regular target calls.

Change-Id: Icf072ccb777b260d67409ddf48fa1966ecdb2af3
---
 gdb/corelow.c  | 24 +++++++++++++++++++++---
 gdb/inferior.h |  1 +
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 293bc8d4f593..c8cd5b7a2570 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -72,6 +72,13 @@ class core_target final : public process_stratum_target
 public:
   core_target ();
 
+  /* Complete the initialization.
+
+     Called after construction, after pushing the target to the inferior's
+     target stack, so that arches are can do target calls, for instance to read
+     auxv.  */
+  void initialize ();
+
   const target_info &info () const override
   { return core_target_info; }
 
@@ -170,7 +177,11 @@ core_target::core_target ()
   /* Find a first arch based on the BFD.  We need the initial gdbarch so
      we can setup the hooks to find a target description.  */
   m_core_gdbarch = gdbarch_from_bfd (core_bfd);
+}
 
+void
+core_target::initialize ()
+{
   /* If the arch is able to read a target description from the core, it
      could yield a more specific gdbarch.  */
   const struct target_desc *tdesc = read_description ();
@@ -517,8 +528,15 @@ core_target_open (const char *arg, int from_tty)
 
   core_target *target = new core_target ();
 
-  /* Own the target until it is successfully pushed.  */
-  target_ops_up target_holder (target);
+  /* Push the target to the inferior's target stack.  Unpush it if something
+     goes wrong.  */
+  current_inferior ()->push_target (target);
+  target_unpush_up unpusher (target);
+
+  /* Finish initialization.  The work done in setup may need to some target
+     calls (read auxv data from the core), hence the need to do it after
+     pushing the target.  */
+  target->initialize ();
 
   validate_files ();
 
@@ -529,7 +547,7 @@ core_target_open (const char *arg, int from_tty)
   if (!current_program_space->exec_bfd ())
     set_gdbarch_from_file (core_bfd);
 
-  current_inferior ()->push_target (std::move (target_holder));
+  (void) unpusher.release ();
 
   switch_to_no_thread ();
 
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 909e1819922d..a4a0304052e6 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -50,6 +50,7 @@ struct thread_info;
 
 #include "progspace.h"
 #include "registry.h"
+#include "observable.h"
 
 #include "symfile-add-flags.h"
 #include "gdbsupport/refcounted-object.h"
-- 
2.38.1

[PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description

[Simon Marchi]

Following the previous patch ("gdb: change order of core_target
initialization"), it is no longer necessary for the core target to pass
a pointer to itself to gdbarch_core_read_description.  Implementations
of this method can just do regular calls to target_read_auxv, through
the inferior target stack.

This allows removing the target_ops parameter in
gdbarch_core_read_description and a bunch of functions called
downstream.  Some auxv-related functions received the auxv data as a
parameter, since they could not assume it could be read from the current
inferior, that is also no longer needed.

Regression tested.  I also tested manually against an AArch64 MTE test
case that Luis Machado sent me a while ago when we were working on this
auxv caching issue, the desired behavior of reporting the MTE tags when
opening the code still worked.

Change-Id: I1e00361209028e9b65dde085d383cf950a7b5e3a
---
 gdb/aarch64-fbsd-tdep.c   |  3 +--
 gdb/aarch64-linux-tdep.c  |  8 +++-----
 gdb/amd64-fbsd-tdep.c     |  4 +---
 gdb/amd64-linux-tdep.c    |  4 +---
 gdb/arc-linux-tdep.c      |  4 +---
 gdb/arm-fbsd-tdep.c       | 24 +++++-------------------
 gdb/arm-fbsd-tdep.h       | 15 +++++----------
 gdb/arm-linux-tdep.c      |  7 ++-----
 gdb/auxv.c                | 11 ++---------
 gdb/auxv.h                |  4 ----
 gdb/corelow.c             |  2 +-
 gdb/gdbarch-components.py |  2 +-
 gdb/gdbarch-gen.h         |  4 ++--
 gdb/gdbarch.c             |  4 ++--
 gdb/i386-fbsd-tdep.c      |  4 +---
 gdb/i386-linux-tdep.c     |  4 +---
 gdb/linux-tdep.c          | 24 ++++++++++--------------
 gdb/linux-tdep.h          | 14 ++++++--------
 gdb/mips-linux-tdep.c     |  4 +---
 gdb/ppc-linux-tdep.c      |  7 ++-----
 gdb/s390-linux-tdep.c     |  6 ++----
 21 files changed, 50 insertions(+), 109 deletions(-)

diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
index 39d193551059..b76ca24aacfd 100644
--- a/gdb/aarch64-fbsd-tdep.c
+++ b/gdb/aarch64-fbsd-tdep.c
@@ -195,8 +195,7 @@ aarch64_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Implement the "core_read_description" gdbarch method.  */
 
 static const struct target_desc *
-aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
-				    struct target_ops *target, bfd *abfd)
+aarch64_fbsd_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
 
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index a321aee036a0..fb9875f5701d 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -776,13 +776,11 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Implement the "core_read_description" gdbarch method.  */
 
 static const struct target_desc *
-aarch64_linux_core_read_description (struct gdbarch *gdbarch,
-				     struct target_ops *target, bfd *abfd)
+aarch64_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
-  CORE_ADDR hwcap2 = linux_get_hwcap2 (auxv, target, gdbarch);
+  CORE_ADDR hwcap = linux_get_hwcap (gdbarch);
+  CORE_ADDR hwcap2 = linux_get_hwcap2 (gdbarch);
 
   aarch64_features features;
   features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
diff --git a/gdb/amd64-fbsd-tdep.c b/gdb/amd64-fbsd-tdep.c
index 960bb0b5942f..90dad4cce98e 100644
--- a/gdb/amd64-fbsd-tdep.c
+++ b/gdb/amd64-fbsd-tdep.c
@@ -220,9 +220,7 @@ static const struct tramp_frame amd64_fbsd_sigframe =
 /* Implement the core_read_description gdbarch method.  */
 
 static const struct target_desc *
-amd64fbsd_core_read_description (struct gdbarch *gdbarch,
-				 struct target_ops *target,
-				 bfd *abfd)
+amd64fbsd_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   return amd64_target_description (i386fbsd_core_read_xcr0 (abfd), true);
 }
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 07c1669f91e0..ace20ed738bd 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1613,9 +1613,7 @@ amd64_linux_read_description (uint64_t xcr0_features_bit, bool is_x32)
 /* Get Linux/x86 target description from core dump.  */
 
 static const struct target_desc *
-amd64_linux_core_read_description (struct gdbarch *gdbarch,
-				  struct target_ops *target,
-				  bfd *abfd)
+amd64_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   /* Linux/x86-64.  */
   uint64_t xcr0 = i386_linux_core_read_xcr0 (abfd);
diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index da7f4758c195..805251d5f4fe 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -679,9 +679,7 @@ arc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Implement the `core_read_description` gdbarch method.  */
 
 static const struct target_desc *
-arc_linux_core_read_description (struct gdbarch *gdbarch,
-				 struct target_ops *target,
-				 bfd *abfd)
+arc_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   arc_arch_features features
     = arc_arch_features_create (abfd,
diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
index 75ee08eba506..4ea238de7c7f 100644
--- a/gdb/arm-fbsd-tdep.c
+++ b/gdb/arm-fbsd-tdep.c
@@ -215,10 +215,11 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* See arm-fbsd-tdep.h.  */
 
 const struct target_desc *
-arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
-				target_ops *target, gdbarch *gdbarch, bool tls)
+arm_fbsd_read_description_auxv (gdbarch *gdbarch, bool tls)
 {
   CORE_ADDR arm_hwcap = 0;
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
+  target_ops *target = current_inferior ()->top_target ();
 
   if (!auxv.has_value ()
       || target_auxv_search (*auxv, target, gdbarch, AT_FREEBSD_HWCAP,
@@ -239,29 +240,14 @@ arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
   return arm_read_description (ARM_FP_TYPE_NONE, tls);
 }
 
-/* See arm-fbsd-tdep.h.  */
-
-const struct target_desc *
-arm_fbsd_read_description_auxv (bool tls)
-{
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
-  return arm_fbsd_read_description_auxv (auxv,
-					 current_inferior ()->top_target (),
-					 current_inferior ()->gdbarch,
-					 tls);
-}
-
 /* Implement the "core_read_description" gdbarch method.  */
 
 static const struct target_desc *
-arm_fbsd_core_read_description (struct gdbarch *gdbarch,
-				struct target_ops *target,
-				bfd *abfd)
+arm_fbsd_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
 
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  return arm_fbsd_read_description_auxv (auxv, target, gdbarch, tls != nullptr);
+  return arm_fbsd_read_description_auxv (gdbarch, tls != nullptr);
 }
 
 /* Implement the get_thread_local_address gdbarch method.  */
diff --git a/gdb/arm-fbsd-tdep.h b/gdb/arm-fbsd-tdep.h
index b4137eb9a51a..d76da0d4d40b 100644
--- a/gdb/arm-fbsd-tdep.h
+++ b/gdb/arm-fbsd-tdep.h
@@ -22,6 +22,8 @@
 
 #include "regset.h"
 
+struct gdbarch;
+
 /* The general-purpose regset consists of 13 R registers, plus SP, LR,
    PC, and CPSR registers.  */
 #define ARM_FBSD_SIZEOF_GREGSET  (17 * 4)
@@ -43,17 +45,10 @@ extern const struct regset arm_fbsd_tls_regset;
 #define	HWCAP_VFPv3		0x00002000
 #define	HWCAP_VFPD32		0x00080000
 
-/* Lookup a target description based on the AT_HWCAP value in the auxv data
-   AUXV.  */
-
-extern const struct target_desc *
-  arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
-				  target_ops *target, gdbarch *gdbarch,
-				  bool tls);
-
-/* Same as the above, but read the auxv data from the current inferior.  */
+/* Lookup a target description based on the AT_HWCAP value in the current
+   inferior's auxv data.  */
 
 extern const struct target_desc *
-  arm_fbsd_read_description_auxv (bool tls);
+  arm_fbsd_read_description_auxv (gdbarch *gdbarch, bool tls);
 
 #endif /* ARM_FBSD_TDEP_H */
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 27aca0c39e6b..2f88bb036d15 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -728,12 +728,9 @@ arm_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Determine target description from core file.  */
 
 static const struct target_desc *
-arm_linux_core_read_description (struct gdbarch *gdbarch,
-				 struct target_ops *target,
-				 bfd *abfd)
+arm_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  CORE_ADDR arm_hwcap = linux_get_hwcap (auxv, target, gdbarch);
+  CORE_ADDR arm_hwcap = linux_get_hwcap (gdbarch);
 
   if (arm_hwcap & HWCAP_VFP)
     {
diff --git a/gdb/auxv.c b/gdb/auxv.c
index 73e84cf144db..c4362a22f870 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -363,7 +363,8 @@ target_read_auxv ()
   if (info == nullptr)
     {
       info = auxv_inferior_data.emplace (inf);
-      info->data = target_read_auxv_raw (inf->top_target ());
+      info->data
+	= target_read_alloc (inf->top_target (), TARGET_OBJECT_AUXV, NULL);
     }
 
   return info->data;
@@ -371,14 +372,6 @@ target_read_auxv ()
 
 /* See auxv.h.  */
 
-gdb::optional<gdb::byte_vector>
-target_read_auxv_raw (target_ops *ops)
-{
-  return target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
-}
-
-/* See auxv.h.  */
-
 int
 target_auxv_search (const gdb::byte_vector &auxv, target_ops *ops,
 		    gdbarch *gdbarch, CORE_ADDR match, CORE_ADDR *valp)
diff --git a/gdb/auxv.h b/gdb/auxv.h
index 788d187b27a0..abdba082e7bb 100644
--- a/gdb/auxv.h
+++ b/gdb/auxv.h
@@ -50,10 +50,6 @@ extern int svr4_auxv_parse (struct gdbarch *gdbarch, const gdb_byte **readptr,
 
 extern gdb::optional<gdb::byte_vector> target_read_auxv ();
 
-/* Read auxv data from OPS.  */
-
-extern gdb::optional<gdb::byte_vector> target_read_auxv_raw (target_ops *ops);
-
 /* Search AUXV for an entry with a_type matching MATCH.
 
    OPS and GDBARCH are the target and architecture to use to parse auxv entries.
diff --git a/gdb/corelow.c b/gdb/corelow.c
index c8cd5b7a2570..1fdfd80bbc91 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -1123,7 +1123,7 @@ core_target::read_description ()
     {
       const struct target_desc *result;
 
-      result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
+      result = gdbarch_core_read_description (m_core_gdbarch, core_bfd);
       if (result != NULL)
 	return result;
     }
diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index 9b688998a7bd..70381e5a8fce 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -1892,7 +1892,7 @@ Refresh overlay mapped state for section OSECT.
 Method(
     type="const struct target_desc *",
     name="core_read_description",
-    params=[("struct target_ops *", "target"), ("bfd *", "abfd")],
+    params=[("bfd *", "abfd")],
     predicate=True,
     invalid=True,
 )
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index a663316df166..4de2b31bc2e3 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1127,8 +1127,8 @@ extern void set_gdbarch_overlay_update (struct gdbarch *gdbarch, gdbarch_overlay
 
 extern bool gdbarch_core_read_description_p (struct gdbarch *gdbarch);
 
-typedef const struct target_desc * (gdbarch_core_read_description_ftype) (struct gdbarch *gdbarch, struct target_ops *target, bfd *abfd);
-extern const struct target_desc * gdbarch_core_read_description (struct gdbarch *gdbarch, struct target_ops *target, bfd *abfd);
+typedef const struct target_desc * (gdbarch_core_read_description_ftype) (struct gdbarch *gdbarch, bfd *abfd);
+extern const struct target_desc * gdbarch_core_read_description (struct gdbarch *gdbarch, bfd *abfd);
 extern void set_gdbarch_core_read_description (struct gdbarch *gdbarch, gdbarch_core_read_description_ftype *core_read_description);
 
 /* Set if the address in N_SO or N_FUN stabs may be zero. */
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 3227e9458801..1ed28515a3d9 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -4211,13 +4211,13 @@ gdbarch_core_read_description_p (struct gdbarch *gdbarch)
 }
 
 const struct target_desc *
-gdbarch_core_read_description (struct gdbarch *gdbarch, struct target_ops *target, bfd *abfd)
+gdbarch_core_read_description (struct gdbarch *gdbarch, bfd *abfd)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->core_read_description != NULL);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_core_read_description called\n");
-  return gdbarch->core_read_description (gdbarch, target, abfd);
+  return gdbarch->core_read_description (gdbarch, abfd);
 }
 
 void
diff --git a/gdb/i386-fbsd-tdep.c b/gdb/i386-fbsd-tdep.c
index eef124fca0ce..fb3a384d9442 100644
--- a/gdb/i386-fbsd-tdep.c
+++ b/gdb/i386-fbsd-tdep.c
@@ -281,9 +281,7 @@ i386fbsd_core_read_xcr0 (bfd *abfd)
 /* Implement the core_read_description gdbarch method.  */
 
 static const struct target_desc *
-i386fbsd_core_read_description (struct gdbarch *gdbarch,
-				struct target_ops *target,
-				bfd *abfd)
+i386fbsd_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true);
 }
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 5c2fed39868c..63a56b86a974 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -703,9 +703,7 @@ i386_linux_read_description (uint64_t xcr0)
 /* Get Linux/x86 target description from core dump.  */
 
 static const struct target_desc *
-i386_linux_core_read_description (struct gdbarch *gdbarch,
-				  struct target_ops *target,
-				  bfd *abfd)
+i386_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   /* Linux/i386.  */
   uint64_t xcr0 = i386_linux_core_read_xcr0 (abfd);
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index c30d9fb13f8c..e094f7a5735e 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2661,10 +2661,12 @@ linux_displaced_step_restore_all_in_ptid (inferior *parent_inf, ptid_t ptid)
 /* Helper for linux_get_hwcap and linux_get_hwcap2.  */
 
 static CORE_ADDR
-linux_get_hwcap_helper (const gdb::optional<gdb::byte_vector> &auxv,
-			target_ops *target, gdbarch *gdbarch, CORE_ADDR match)
+linux_get_hwcap_helper (gdbarch *gdbarch, CORE_ADDR match)
 {
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
+  target_ops *target = current_inferior ()->top_target ();
   CORE_ADDR field;
+
   if (!auxv.has_value ()
       || target_auxv_search (*auxv, target, gdbarch, match, &field) != 1)
     return 0;
@@ -2674,10 +2676,9 @@ linux_get_hwcap_helper (const gdb::optional<gdb::byte_vector> &auxv,
 /* See linux-tdep.h.  */
 
 CORE_ADDR
-linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
-		 target_ops *target, gdbarch *gdbarch)
+linux_get_hwcap (gdbarch *gdbarch)
 {
-  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP);
+  return linux_get_hwcap_helper (gdbarch, AT_HWCAP);
 }
 
 /* See linux-tdep.h.  */
@@ -2685,18 +2686,15 @@ linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
 CORE_ADDR
 linux_get_hwcap ()
 {
-  return linux_get_hwcap (target_read_auxv (),
-			  current_inferior ()->top_target (),
-			  current_inferior ()->gdbarch);
+  return linux_get_hwcap (current_inferior ()->gdbarch);
 }
 
 /* See linux-tdep.h.  */
 
 CORE_ADDR
-linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
-		  target_ops *target, gdbarch *gdbarch)
+linux_get_hwcap2 (gdbarch *gdbarch)
 {
-  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP2);
+  return linux_get_hwcap_helper (gdbarch, AT_HWCAP2);
 }
 
 /* See linux-tdep.h.  */
@@ -2704,9 +2702,7 @@ linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
 CORE_ADDR
 linux_get_hwcap2 ()
 {
-  return linux_get_hwcap2 (target_read_auxv (),
-			   current_inferior ()->top_target (),
-			   current_inferior ()->gdbarch);
+  return linux_get_hwcap2 (current_inferior ()->gdbarch);
 }
 
 /* Display whether the gcore command is using the
diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
index 95cc29c828c2..b7a52ccde235 100644
--- a/gdb/linux-tdep.h
+++ b/gdb/linux-tdep.h
@@ -90,23 +90,21 @@ extern void linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
 
 extern int linux_is_uclinux (void);
 
-/* Fetch the AT_HWCAP entry from auxv data AUXV.  Use TARGET and GDBARCH to
-   parse auxv entries.
+/* Fetch the AT_HWCAP entry from the current inferior's auxv data.  Use GDBARCH
+   to parse auxv entries.
 
    On error, 0 is returned.  */
-extern CORE_ADDR linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
-				  struct target_ops *target, gdbarch *gdbarch);
+extern CORE_ADDR linux_get_hwcap (gdbarch *gdbarch);
 
 /* Same as the above, but obtain all the inputs from the current inferior.  */
 
 extern CORE_ADDR linux_get_hwcap ();
 
-/* Fetch the AT_HWCAP2 entry from auxv data AUXV.  Use TARGET and GDBARCH to
-   parse auxv entries.
+/* Fetch the AT_HWCAP2 entry from the current inferior's auxv data.  Use GDBARCH
+   to parse auxv entries.
 
    On error, 0 is returned.  */
-extern CORE_ADDR linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
-				   struct target_ops *target, gdbarch *gdbarch);
+extern CORE_ADDR linux_get_hwcap2 (gdbarch *gdbarch);
 
 /* Same as the above, but obtain all the inputs from the current inferior.  */
 
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 1b3b5f88edbc..534d47fc1ae5 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -554,9 +554,7 @@ mips_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 }
 
 static const struct target_desc *
-mips_linux_core_read_description (struct gdbarch *gdbarch,
-				  struct target_ops *target,
-				  bfd *abfd)
+mips_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *section = bfd_get_section_by_name (abfd, ".reg");
   if (! section)
diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index 39d692b2764c..cec3a31d7746 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1566,9 +1566,7 @@ ppc_linux_write_pc (struct regcache *regcache, CORE_ADDR pc)
 }
 
 static const struct target_desc *
-ppc_linux_core_read_description (struct gdbarch *gdbarch,
-				 struct target_ops *target,
-				 bfd *abfd)
+ppc_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   struct ppc_linux_features features = ppc_linux_no_features;
   asection *altivec = bfd_get_section_by_name (abfd, ".reg-ppc-vmx");
@@ -1601,8 +1599,7 @@ ppc_linux_core_read_description (struct gdbarch *gdbarch,
   if (vsx)
     features.vsx = true;
 
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
+  CORE_ADDR hwcap = linux_get_hwcap (gdbarch);
 
   features.isa205 = ppc_linux_has_isa205 (hwcap);
 
diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 14d71134e0cd..d134332b7b16 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -328,12 +328,10 @@ s390_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Implement core_read_description gdbarch method.  */
 
 static const struct target_desc *
-s390_core_read_description (struct gdbarch *gdbarch,
-			    struct target_ops *target, bfd *abfd)
+s390_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *section = bfd_get_section_by_name (abfd, ".reg");
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
+  CORE_ADDR hwcap = linux_get_hwcap (gdbarch);
   bool high_gprs, v1, v2, te, vx, gs;
 
   if (!section)
-- 
2.38.1

Re: [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description

[John Baldwin]

On 11/28/22 6:50 PM, Simon Marchi via Gdb-patches wrote:
> Following the previous patch ("gdb: change order of core_target
> initialization"), it is no longer necessary for the core target to pass
> a pointer to itself to gdbarch_core_read_description.  Implementations
> of this method can just do regular calls to target_read_auxv, through
> the inferior target stack.
> 
> This allows removing the target_ops parameter in
> gdbarch_core_read_description and a bunch of functions called
> downstream.  Some auxv-related functions received the auxv data as a
> parameter, since they could not assume it could be read from the current
> inferior, that is also no longer needed.
> 
> Regression tested.  I also tested manually against an AArch64 MTE test
> case that Luis Machado sent me a while ago when we were working on this
> auxv caching issue, the desired behavior of reporting the MTE tags when
> opening the code still worked.
> 
> Change-Id: I1e00361209028e9b65dde085d383cf950a7b5e3a
> ---
>   gdb/aarch64-fbsd-tdep.c   |  3 +--
>   gdb/aarch64-linux-tdep.c  |  8 +++-----
>   gdb/amd64-fbsd-tdep.c     |  4 +---
>   gdb/amd64-linux-tdep.c    |  4 +---
>   gdb/arc-linux-tdep.c      |  4 +---
>   gdb/arm-fbsd-tdep.c       | 24 +++++-------------------
>   gdb/arm-fbsd-tdep.h       | 15 +++++----------
>   gdb/arm-linux-tdep.c      |  7 ++-----
>   gdb/auxv.c                | 11 ++---------
>   gdb/auxv.h                |  4 ----
>   gdb/corelow.c             |  2 +-
>   gdb/gdbarch-components.py |  2 +-
>   gdb/gdbarch-gen.h         |  4 ++--
>   gdb/gdbarch.c             |  4 ++--
>   gdb/i386-fbsd-tdep.c      |  4 +---
>   gdb/i386-linux-tdep.c     |  4 +---
>   gdb/linux-tdep.c          | 24 ++++++++++--------------
>   gdb/linux-tdep.h          | 14 ++++++--------
>   gdb/mips-linux-tdep.c     |  4 +---
>   gdb/ppc-linux-tdep.c      |  7 ++-----
>   gdb/s390-linux-tdep.c     |  6 ++----
>   21 files changed, 50 insertions(+), 109 deletions(-)
> 
> diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
> index 75ee08eba506..4ea238de7c7f 100644
> --- a/gdb/arm-fbsd-tdep.c
> +++ b/gdb/arm-fbsd-tdep.c
> @@ -215,10 +215,11 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
>   /* See arm-fbsd-tdep.h.  */
>   
>   const struct target_desc *
> -arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
> -				target_ops *target, gdbarch *gdbarch, bool tls)
> +arm_fbsd_read_description_auxv (gdbarch *gdbarch, bool tls)
>   {
>     CORE_ADDR arm_hwcap = 0;
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
> +  target_ops *target = current_inferior ()->top_target ();
>   
>     if (!auxv.has_value ()
>         || target_auxv_search (*auxv, target, gdbarch, AT_FREEBSD_HWCAP,
> @@ -239,29 +240,14 @@ arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
>     return arm_read_description (ARM_FP_TYPE_NONE, tls);
>   }
>   
> -/* See arm-fbsd-tdep.h.  */
> -
> -const struct target_desc *
> -arm_fbsd_read_description_auxv (bool tls)
> -{
> -  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
> -  return arm_fbsd_read_description_auxv (auxv,
> -					 current_inferior ()->top_target (),
> -					 current_inferior ()->gdbarch,
> -					 tls);
> -}
> -

This function is used by arm-fbsd-nat.c in the read_description target
method.  Probably that function just needs to be updated to pass in the
gdbarch explicitly rather than keeping this wrapper method though:

diff --git a/gdb/arm-fbsd-nat.c b/gdb/arm-fbsd-nat.c
index 340b8e0d710..0ed3104d12d 100644
--- a/gdb/arm-fbsd-nat.c
+++ b/gdb/arm-fbsd-nat.c
@@ -96,7 +96,7 @@ arm_fbsd_nat_target::read_description ()
  #ifdef PT_GETREGSET
    tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
  #endif
-  desc = arm_fbsd_read_description_auxv (tls);
+  desc = arm_fbsd_read_description_auxv (current_inferior ()->gdbarch, tls);
    if (desc == NULL)
      desc = this->beneath ()->read_description ();
    return desc;

-- 
John Baldwin

Re: [PATCH 2/3] gdb: break up core_target initialization

[John Baldwin]

On 11/28/22 6:50 PM, Simon Marchi via Gdb-patches wrote:
> In the core target constructor, we currently have to jump through some
> hoops to read the auxv data from the core, in order to choose an
> appropriate gdbarch for the inferior.  The core BFD gives us a gdbarch,
> but it might not tell the whole story.  By reading some auxv fields,
> some architectures are able to choose a more specific gdbarch.  The
> problem is that to read auxv from the core using the inferior's target
> stack, the core target needs to be pushed on the inferior's target
> stack.  But this work is done in the core target constructor, so it
> can't be pushed at this point.  The current solution is to pass a
> pointer to the core target to gdbarch_core_read_description, in
> core_target::read_description.  That "hack" must then propagate to many
> functions involved in selecting the architecture and reading auxv.
> 
> With this patch, I propose to break things up to avoid the problem.  The
> core_target constructor will now do only trivial stuff that doesn't need
> to call things outside core_target.  Then, we'll push the core_target on
> the inferior's target stack.  And finally, complete the initialization
> that potentially requires doing target calls.  The target calls to read
> auxv at this point will just be regular target calls.

I think this is a nice solution to the problem.
  
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 293bc8d4f593..c8cd5b7a2570 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -72,6 +72,13 @@ class core_target final : public process_stratum_target
>   public:
>     core_target ();
>   
> +  /* Complete the initialization.
> +
> +     Called after construction, after pushing the target to the inferior's
> +     target stack, so that arches are can do target calls, for instance to read
> +     auxv.  */
> +  void initialize ();

s/arches are can/arches can/

> @@ -170,7 +177,11 @@ core_target::core_target ()
>     /* Find a first arch based on the BFD.  We need the initial gdbarch so
>        we can setup the hooks to find a target description.  */
>     m_core_gdbarch = gdbarch_from_bfd (core_bfd);
> +}
>   
> +void
> +core_target::initialize ()
> +{
>     /* If the arch is able to read a target description from the core, it
>        could yield a more specific gdbarch.  */
>     const struct target_desc *tdesc = read_description ();

I do wonder if the comments above want expanding slightly as we are now in a
new function and over time it might move around in the file so that these
two comments aren't right next to each other?  Maybe something like like:

     /* Find a first arch based on the BFD.  We need the initial gdbarch so
        we can provide a barebones target able to read information such as
        auxv data.  The final gdbarch will be set in initialize.  */
     m_core_gdbarch = gdbarch_from_bfd (core_bfd);
}

void
core_target::initialize ()
{
     /* If the initial arch from the BFD is able to read a target description
        from the core, it could yield a more specific gdbarch.  */


-- 
John Baldwin

Re: [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description

[Simon Marchi]

On 11/29/22 14:16, John Baldwin wrote:
> On 11/28/22 6:50 PM, Simon Marchi via Gdb-patches wrote:
>> Following the previous patch ("gdb: change order of core_target
>> initialization"), it is no longer necessary for the core target to pass
>> a pointer to itself to gdbarch_core_read_description.  Implementations
>> of this method can just do regular calls to target_read_auxv, through
>> the inferior target stack.
>>
>> This allows removing the target_ops parameter in
>> gdbarch_core_read_description and a bunch of functions called
>> downstream.  Some auxv-related functions received the auxv data as a
>> parameter, since they could not assume it could be read from the current
>> inferior, that is also no longer needed.
>>
>> Regression tested.  I also tested manually against an AArch64 MTE test
>> case that Luis Machado sent me a while ago when we were working on this
>> auxv caching issue, the desired behavior of reporting the MTE tags when
>> opening the code still worked.
>>
>> Change-Id: I1e00361209028e9b65dde085d383cf950a7b5e3a
>> ---
>>   gdb/aarch64-fbsd-tdep.c   |  3 +--
>>   gdb/aarch64-linux-tdep.c  |  8 +++-----
>>   gdb/amd64-fbsd-tdep.c     |  4 +---
>>   gdb/amd64-linux-tdep.c    |  4 +---
>>   gdb/arc-linux-tdep.c      |  4 +---
>>   gdb/arm-fbsd-tdep.c       | 24 +++++-------------------
>>   gdb/arm-fbsd-tdep.h       | 15 +++++----------
>>   gdb/arm-linux-tdep.c      |  7 ++-----
>>   gdb/auxv.c                | 11 ++---------
>>   gdb/auxv.h                |  4 ----
>>   gdb/corelow.c             |  2 +-
>>   gdb/gdbarch-components.py |  2 +-
>>   gdb/gdbarch-gen.h         |  4 ++--
>>   gdb/gdbarch.c             |  4 ++--
>>   gdb/i386-fbsd-tdep.c      |  4 +---
>>   gdb/i386-linux-tdep.c     |  4 +---
>>   gdb/linux-tdep.c          | 24 ++++++++++--------------
>>   gdb/linux-tdep.h          | 14 ++++++--------
>>   gdb/mips-linux-tdep.c     |  4 +---
>>   gdb/ppc-linux-tdep.c      |  7 ++-----
>>   gdb/s390-linux-tdep.c     |  6 ++----
>>   21 files changed, 50 insertions(+), 109 deletions(-)
>>
>> diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
>> index 75ee08eba506..4ea238de7c7f 100644
>> --- a/gdb/arm-fbsd-tdep.c
>> +++ b/gdb/arm-fbsd-tdep.c
>> @@ -215,10 +215,11 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
>>   /* See arm-fbsd-tdep.h.  */
>>     const struct target_desc *
>> -arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
>> -                target_ops *target, gdbarch *gdbarch, bool tls)
>> +arm_fbsd_read_description_auxv (gdbarch *gdbarch, bool tls)
>>   {
>>     CORE_ADDR arm_hwcap = 0;
>> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
>> +  target_ops *target = current_inferior ()->top_target ();
>>       if (!auxv.has_value ()
>>         || target_auxv_search (*auxv, target, gdbarch, AT_FREEBSD_HWCAP,
>> @@ -239,29 +240,14 @@ arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
>>     return arm_read_description (ARM_FP_TYPE_NONE, tls);
>>   }
>>   -/* See arm-fbsd-tdep.h.  */
>> -
>> -const struct target_desc *
>> -arm_fbsd_read_description_auxv (bool tls)
>> -{
>> -  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
>> -  return arm_fbsd_read_description_auxv (auxv,
>> -                     current_inferior ()->top_target (),
>> -                     current_inferior ()->gdbarch,
>> -                     tls);
>> -}
>> -
> 
> This function is used by arm-fbsd-nat.c in the read_description target
> method.  Probably that function just needs to be updated to pass in the
> gdbarch explicitly rather than keeping this wrapper method though:
> 
> diff --git a/gdb/arm-fbsd-nat.c b/gdb/arm-fbsd-nat.c
> index 340b8e0d710..0ed3104d12d 100644
> --- a/gdb/arm-fbsd-nat.c
> +++ b/gdb/arm-fbsd-nat.c
> @@ -96,7 +96,7 @@ arm_fbsd_nat_target::read_description ()
>  #ifdef PT_GETREGSET
>    tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>  #endif
> -  desc = arm_fbsd_read_description_auxv (tls);
> +  desc = arm_fbsd_read_description_auxv (current_inferior ()->gdbarch, tls);
>    if (desc == NULL)
>      desc = this->beneath ()->read_description ();
>    return desc;

Woops, thanks for pointing it out.  Actually, I'll leave the
one-parameter version there and adapt it, so no change should be needed
in the nat file.

Thanks,

Simon

Re: [PATCH 2/3] gdb: break up core_target initialization

[Simon Marchi]

On 11/29/22 14:27, John Baldwin wrote:
> On 11/28/22 6:50 PM, Simon Marchi via Gdb-patches wrote:
>> In the core target constructor, we currently have to jump through some
>> hoops to read the auxv data from the core, in order to choose an
>> appropriate gdbarch for the inferior.  The core BFD gives us a gdbarch,
>> but it might not tell the whole story.  By reading some auxv fields,
>> some architectures are able to choose a more specific gdbarch.  The
>> problem is that to read auxv from the core using the inferior's target
>> stack, the core target needs to be pushed on the inferior's target
>> stack.  But this work is done in the core target constructor, so it
>> can't be pushed at this point.  The current solution is to pass a
>> pointer to the core target to gdbarch_core_read_description, in
>> core_target::read_description.  That "hack" must then propagate to many
>> functions involved in selecting the architecture and reading auxv.
>>
>> With this patch, I propose to break things up to avoid the problem.  The
>> core_target constructor will now do only trivial stuff that doesn't need
>> to call things outside core_target.  Then, we'll push the core_target on
>> the inferior's target stack.  And finally, complete the initialization
>> that potentially requires doing target calls.  The target calls to read
>> auxv at this point will just be regular target calls.
> 
> I think this is a nice solution to the problem.

Ack.

>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index 293bc8d4f593..c8cd5b7a2570 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -72,6 +72,13 @@ class core_target final : public process_stratum_target
>>   public:
>>     core_target ();
>>   +  /* Complete the initialization.
>> +
>> +     Called after construction, after pushing the target to the inferior's
>> +     target stack, so that arches are can do target calls, for instance to read
>> +     auxv.  */
>> +  void initialize ();
> 
> s/arches are can/arches can/

Fixed.

>> @@ -170,7 +177,11 @@ core_target::core_target ()
>>     /* Find a first arch based on the BFD.  We need the initial gdbarch so
>>        we can setup the hooks to find a target description.  */
>>     m_core_gdbarch = gdbarch_from_bfd (core_bfd);
>> +}
>>   +void
>> +core_target::initialize ()
>> +{
>>     /* If the arch is able to read a target description from the core, it
>>        could yield a more specific gdbarch.  */
>>     const struct target_desc *tdesc = read_description ();
> 
> I do wonder if the comments above want expanding slightly as we are now in a
> new function and over time it might move around in the file so that these
> two comments aren't right next to each other?  Maybe something like like:
> 
>     /* Find a first arch based on the BFD.  We need the initial gdbarch so
>        we can provide a barebones target able to read information such as
>        auxv data.  The final gdbarch will be set in initialize.  */

I would not say "will", because it's conditional.

>     m_core_gdbarch = gdbarch_from_bfd (core_bfd);
> }
> 
> void
> core_target::initialize ()
> {
>     /* If the initial arch from the BFD is able to read a target description
>        from the core, it could yield a more specific gdbarch.  */

What about:


core_target::core_target ()
{
  /* Find a first arch based on the BFD.  It will possibly be overriden by a
     more precise gdbarch in core_target::initialize  */
  m_core_gdbarch = gdbarch_from_bfd (core_bfd);
}

void
core_target::initialize ()
{
  /* If the initial arch (obtained from the BFD) is able to read a target
     description from the core, it could yield a more specific gdbarch.  */
  const struct target_desc *tdesc = read_description ();

Simon

Re: [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Add an inferior_target_stack_changed observable, and attach
Simon> invalidate_auxv_cache_inf to it.  Notify this observable in the
Simon> push_target and unpush_target methods of inferior.

This looks good to me, thanks.

It seems like overall it would be better to have the auxv cache just be
some member of 'inferior'.  Then these indirections and observers
wouldn't be needed -- the inferior could just manage it directly.

Tom

Re: [PATCH 2/3] gdb: break up core_target initialization

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> diff --git a/gdb/inferior.h b/gdb/inferior.h
Simon> index 909e1819922d..a4a0304052e6 100644
Simon> --- a/gdb/inferior.h
Simon> +++ b/gdb/inferior.h
Simon> @@ -50,6 +50,7 @@ struct thread_info;
 
Simon>  #include "progspace.h"
Simon>  #include "registry.h"
Simon> +#include "observable.h"

It seems strange for this hunk to be in this patch.
Is it needed at all?

Tom

Re: [PATCH 2/3] gdb: break up core_target initialization

[John Baldwin]

On 11/29/22 1:53 PM, Simon Marchi wrote:
> What about:
> 
> 
> core_target::core_target ()
> {
>    /* Find a first arch based on the BFD.  It will possibly be overriden by a
>       more precise gdbarch in core_target::initialize  */
>    m_core_gdbarch = gdbarch_from_bfd (core_bfd);
> }
> 
> void
> core_target::initialize ()
> {
>    /* If the initial arch (obtained from the BFD) is able to read a target
>       description from the core, it could yield a more specific gdbarch.  */
>    const struct target_desc *tdesc = read_description ();
> 
> Simon

This looks good to me, thanks!

-- 
John Baldwin

Re: [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache

[Simon Marchi]

On 11/30/22 10:44, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Add an inferior_target_stack_changed observable, and attach
> Simon> invalidate_auxv_cache_inf to it.  Notify this observable in the
> Simon> push_target and unpush_target methods of inferior.
> 
> This looks good to me, thanks.

Thanks, will push once the rest of the series is approved.

> It seems like overall it would be better to have the auxv cache just be
> some member of 'inferior'.  Then these indirections and observers
> wouldn't be needed -- the inferior could just manage it directly.

I kind of like observers, it gives the impression that things are
decoupled... but yeah in practice it just adds unnecessary layers of
indirection.

Simon

Re: [PATCH 2/3] gdb: break up core_target initialization

[Simon Marchi]

On 11/30/22 11:05, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> diff --git a/gdb/inferior.h b/gdb/inferior.h
> Simon> index 909e1819922d..a4a0304052e6 100644
> Simon> --- a/gdb/inferior.h
> Simon> +++ b/gdb/inferior.h
> Simon> @@ -50,6 +50,7 @@ struct thread_info;
>  
> Simon>  #include "progspace.h"
> Simon>  #include "registry.h"
> Simon> +#include "observable.h"
> 
> It seems strange for this hunk to be in this patch.
> Is it needed at all?

You are right, removed.

Simon

Re: [PATCH v1.1 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description

[Simon Marchi]

Here's a slightly updated patch.  I realized we could do further
simplifications in target_auxv_search and callers.


From 868f295627164596ea8c868e6ad07e86e44ee7a1 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Mon, 28 Nov 2022 16:09:25 -0500
Subject: [PATCH 3/3] gdb: remove target_ops parameter from
 gdbarch_core_read_description

Following the previous patch ("gdb: change order of core_target
initialization"), it is no longer necessary for the core target to pass
a pointer to itself to gdbarch_core_read_description.  Implementations
of this method can just do regular calls to target_read_auxv, through
the inferior target stack.

This allows removing the target_ops parameter in
gdbarch_core_read_description and a bunch of functions called
downstream.  Some auxv-related functions received the auxv data as a
parameter, since they could not assume it could be read from the current
inferior, that is also no longer needed.

Regression tested.  I also tested manually against an AArch64 MTE test
case that Luis Machado sent me a while ago when we were working on this
auxv caching issue, the desired behavior of reporting the MTE tags when
opening the code still worked.

Change-Id: I1e00361209028e9b65dde085d383cf950a7b5e3a
---
 gdb/aarch64-fbsd-tdep.c   |  3 +--
 gdb/aarch64-linux-tdep.c  |  8 +++-----
 gdb/amd64-fbsd-tdep.c     |  4 +---
 gdb/amd64-linux-tdep.c    |  4 +---
 gdb/arc-linux-tdep.c      |  4 +---
 gdb/arm-fbsd-tdep.c       | 20 +++++--------------
 gdb/arm-fbsd-tdep.h       | 12 ++++++------
 gdb/arm-linux-tdep.c      |  7 ++-----
 gdb/auxv.c                | 41 ++++++++++++++-------------------------
 gdb/auxv.h                | 14 ++++---------
 gdb/corelow.c             |  2 +-
 gdb/gdbarch-components.py |  2 +-
 gdb/gdbarch-gen.h         |  4 ++--
 gdb/gdbarch.c             |  4 ++--
 gdb/i386-fbsd-tdep.c      |  4 +---
 gdb/i386-linux-tdep.c     |  4 +---
 gdb/linux-tdep.c          | 25 +++++++++---------------
 gdb/linux-tdep.h          | 14 ++++++-------
 gdb/mips-linux-tdep.c     |  4 +---
 gdb/ppc-linux-tdep.c      |  7 ++-----
 gdb/s390-linux-tdep.c     |  6 ++----
 21 files changed, 67 insertions(+), 126 deletions(-)

diff --git a/gdb/aarch64-fbsd-tdep.c b/gdb/aarch64-fbsd-tdep.c
index 39d193551059..b76ca24aacfd 100644
--- a/gdb/aarch64-fbsd-tdep.c
+++ b/gdb/aarch64-fbsd-tdep.c
@@ -195,8 +195,7 @@ aarch64_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Implement the "core_read_description" gdbarch method.  */
 
 static const struct target_desc *
-aarch64_fbsd_core_read_description (struct gdbarch *gdbarch,
-				    struct target_ops *target, bfd *abfd)
+aarch64_fbsd_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
 
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index a321aee036a0..fb9875f5701d 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -776,13 +776,11 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Implement the "core_read_description" gdbarch method.  */
 
 static const struct target_desc *
-aarch64_linux_core_read_description (struct gdbarch *gdbarch,
-				     struct target_ops *target, bfd *abfd)
+aarch64_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
-  CORE_ADDR hwcap2 = linux_get_hwcap2 (auxv, target, gdbarch);
+  CORE_ADDR hwcap = linux_get_hwcap (gdbarch);
+  CORE_ADDR hwcap2 = linux_get_hwcap2 (gdbarch);
 
   aarch64_features features;
   features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
diff --git a/gdb/amd64-fbsd-tdep.c b/gdb/amd64-fbsd-tdep.c
index 960bb0b5942f..90dad4cce98e 100644
--- a/gdb/amd64-fbsd-tdep.c
+++ b/gdb/amd64-fbsd-tdep.c
@@ -220,9 +220,7 @@ static const struct tramp_frame amd64_fbsd_sigframe =
 /* Implement the core_read_description gdbarch method.  */
 
 static const struct target_desc *
-amd64fbsd_core_read_description (struct gdbarch *gdbarch,
-				 struct target_ops *target,
-				 bfd *abfd)
+amd64fbsd_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   return amd64_target_description (i386fbsd_core_read_xcr0 (abfd), true);
 }
diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 07c1669f91e0..ace20ed738bd 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1613,9 +1613,7 @@ amd64_linux_read_description (uint64_t xcr0_features_bit, bool is_x32)
 /* Get Linux/x86 target description from core dump.  */
 
 static const struct target_desc *
-amd64_linux_core_read_description (struct gdbarch *gdbarch,
-				  struct target_ops *target,
-				  bfd *abfd)
+amd64_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   /* Linux/x86-64.  */
   uint64_t xcr0 = i386_linux_core_read_xcr0 (abfd);
diff --git a/gdb/arc-linux-tdep.c b/gdb/arc-linux-tdep.c
index da7f4758c195..805251d5f4fe 100644
--- a/gdb/arc-linux-tdep.c
+++ b/gdb/arc-linux-tdep.c
@@ -679,9 +679,7 @@ arc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Implement the `core_read_description` gdbarch method.  */
 
 static const struct target_desc *
-arc_linux_core_read_description (struct gdbarch *gdbarch,
-				 struct target_ops *target,
-				 bfd *abfd)
+arc_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   arc_arch_features features
     = arc_arch_features_create (abfd,
diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
index 75ee08eba506..cd8b55997d7d 100644
--- a/gdb/arm-fbsd-tdep.c
+++ b/gdb/arm-fbsd-tdep.c
@@ -215,14 +215,11 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* See arm-fbsd-tdep.h.  */
 
 const struct target_desc *
-arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
-				target_ops *target, gdbarch *gdbarch, bool tls)
+arm_fbsd_read_description_auxv (gdbarch *gdbarch, bool tls)
 {
   CORE_ADDR arm_hwcap = 0;
 
-  if (!auxv.has_value ()
-      || target_auxv_search (*auxv, target, gdbarch, AT_FREEBSD_HWCAP,
-			     &arm_hwcap) != 1)
+  if (target_auxv_search (gdbarch, AT_FREEBSD_HWCAP, &arm_hwcap) != 1)
     return arm_read_description (ARM_FP_TYPE_NONE, tls);
 
   if (arm_hwcap & HWCAP_VFP)
@@ -244,24 +241,17 @@ arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
 const struct target_desc *
 arm_fbsd_read_description_auxv (bool tls)
 {
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
-  return arm_fbsd_read_description_auxv (auxv,
-					 current_inferior ()->top_target (),
-					 current_inferior ()->gdbarch,
-					 tls);
+  return arm_fbsd_read_description_auxv (current_inferior ()->gdbarch, tls);
 }
 
 /* Implement the "core_read_description" gdbarch method.  */
 
 static const struct target_desc *
-arm_fbsd_core_read_description (struct gdbarch *gdbarch,
-				struct target_ops *target,
-				bfd *abfd)
+arm_fbsd_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
 
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  return arm_fbsd_read_description_auxv (auxv, target, gdbarch, tls != nullptr);
+  return arm_fbsd_read_description_auxv (gdbarch, tls != nullptr);
 }
 
 /* Implement the get_thread_local_address gdbarch method.  */
diff --git a/gdb/arm-fbsd-tdep.h b/gdb/arm-fbsd-tdep.h
index b4137eb9a51a..00263fb420b5 100644
--- a/gdb/arm-fbsd-tdep.h
+++ b/gdb/arm-fbsd-tdep.h
@@ -22,6 +22,8 @@
 
 #include "regset.h"
 
+struct gdbarch;
+
 /* The general-purpose regset consists of 13 R registers, plus SP, LR,
    PC, and CPSR registers.  */
 #define ARM_FBSD_SIZEOF_GREGSET  (17 * 4)
@@ -43,15 +45,13 @@ extern const struct regset arm_fbsd_tls_regset;
 #define	HWCAP_VFPv3		0x00002000
 #define	HWCAP_VFPD32		0x00080000
 
-/* Lookup a target description based on the AT_HWCAP value in the auxv data
-   AUXV.  */
+/* Lookup a target description based on the AT_HWCAP value in the current
+   inferior's auxv data.  */
 
 extern const struct target_desc *
-  arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
-				  target_ops *target, gdbarch *gdbarch,
-				  bool tls);
+  arm_fbsd_read_description_auxv (gdbarch *gdbarch, bool tls);
 
-/* Same as the above, but read the auxv data from the current inferior.  */
+/* Same as the above, but get the GDBARCH from the current inferior.  */
 
 extern const struct target_desc *
   arm_fbsd_read_description_auxv (bool tls);
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 27aca0c39e6b..2f88bb036d15 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -728,12 +728,9 @@ arm_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Determine target description from core file.  */
 
 static const struct target_desc *
-arm_linux_core_read_description (struct gdbarch *gdbarch,
-				 struct target_ops *target,
-				 bfd *abfd)
+arm_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  CORE_ADDR arm_hwcap = linux_get_hwcap (auxv, target, gdbarch);
+  CORE_ADDR arm_hwcap = linux_get_hwcap (gdbarch);
 
   if (arm_hwcap & HWCAP_VFP)
     {
diff --git a/gdb/auxv.c b/gdb/auxv.c
index 73e84cf144db..62ad2bf84219 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -315,16 +315,16 @@ svr4_auxv_parse (struct gdbarch *gdbarch, const gdb_byte **readptr,
    Return 1 if an entry was read into *TYPEP and *VALP.  */
 
 static int
-parse_auxv (target_ops *ops, gdbarch *gdbarch, const gdb_byte **readptr,
+parse_auxv (gdbarch *gdbarch, const gdb_byte **readptr,
 	    const gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)
 {
   if (gdbarch_auxv_parse_p (gdbarch))
     return gdbarch_auxv_parse (gdbarch, readptr, endptr, typep, valp);
 
-  return ops->auxv_parse (readptr, endptr, typep, valp);
+  return current_inferior ()->top_target ()->auxv_parse
+    (readptr, endptr, typep, valp);
 }
 
-
 /*  Auxiliary Vector information structure.  This is used by GDB
     for caching purposes for each inferior.  This helps reduce the
     overhead of transfering data from a remote target to the local host.  */
@@ -363,7 +363,8 @@ target_read_auxv ()
   if (info == nullptr)
     {
       info = auxv_inferior_data.emplace (inf);
-      info->data = target_read_auxv_raw (inf->top_target ());
+      info->data
+	= target_read_alloc (inf->top_target (), TARGET_OBJECT_AUXV, NULL);
     }
 
   return info->data;
@@ -371,25 +372,20 @@ target_read_auxv ()
 
 /* See auxv.h.  */
 
-gdb::optional<gdb::byte_vector>
-target_read_auxv_raw (target_ops *ops)
-{
-  return target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
-}
-
-/* See auxv.h.  */
-
 int
-target_auxv_search (const gdb::byte_vector &auxv, target_ops *ops,
-		    gdbarch *gdbarch, CORE_ADDR match, CORE_ADDR *valp)
+target_auxv_search (gdbarch *gdbarch, CORE_ADDR match, CORE_ADDR *valp)
 {
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
+  if (!auxv.has_value ())
+    return 0;
+
   CORE_ADDR type, val;
-  const gdb_byte *data = auxv.data ();
+  const gdb_byte *data = auxv->data ();
   const gdb_byte *ptr = data;
-  size_t len = auxv.size ();
+  size_t len = auxv->size ();
 
   while (1)
-    switch (parse_auxv (ops, gdbarch, &ptr, data + len, &type, &val))
+    switch (parse_auxv (gdbarch, &ptr, data + len, &type, &val))
       {
       case 1:			/* Here's an entry, check it.  */
 	if (type == match)
@@ -410,13 +406,7 @@ target_auxv_search (const gdb::byte_vector &auxv, target_ops *ops,
 int
 target_auxv_search (CORE_ADDR match, CORE_ADDR *valp)
 {
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
-
-  if (!auxv.has_value ())
-    return -1;
-
-  return target_auxv_search (*auxv, current_inferior ()->top_target (),
-			     current_inferior ()->gdbarch, match, valp);
+  return target_auxv_search (current_inferior ()->gdbarch, match, valp);
 }
 
 /* Print the description of a single AUXV entry on the specified file.  */
@@ -573,8 +563,7 @@ fprint_target_auxv (struct ui_file *file)
   const gdb_byte *ptr = data;
   size_t len = auxv->size ();
 
-  while (parse_auxv (current_inferior ()->top_target (),
-		     current_inferior ()->gdbarch,
+  while (parse_auxv (current_inferior ()->gdbarch,
 		     &ptr, data + len, &type, &val) > 0)
     {
       gdbarch_print_auxv_entry (gdbarch, file, type, val);
diff --git a/gdb/auxv.h b/gdb/auxv.h
index 788d187b27a0..6b6ffedf1bdb 100644
--- a/gdb/auxv.h
+++ b/gdb/auxv.h
@@ -50,24 +50,18 @@ extern int svr4_auxv_parse (struct gdbarch *gdbarch, const gdb_byte **readptr,
 
 extern gdb::optional<gdb::byte_vector> target_read_auxv ();
 
-/* Read auxv data from OPS.  */
-
-extern gdb::optional<gdb::byte_vector> target_read_auxv_raw (target_ops *ops);
-
 /* Search AUXV for an entry with a_type matching MATCH.
 
-   OPS and GDBARCH are the target and architecture to use to parse auxv entries.
+   Use GDBARCH to parse auxv entries, instead of the current inferior's arch.
 
    Return zero if no such entry was found, or -1 if there was
    an error getting the information.  On success, return 1 after
    storing the entry's value field in *VALP.  */
 
-extern int target_auxv_search (const gdb::byte_vector &auxv,
-			       target_ops *ops, gdbarch *gdbarch,
-			       CORE_ADDR match, CORE_ADDR *valp);
+extern int target_auxv_search (gdbarch *gdbarch, CORE_ADDR match,
+			       CORE_ADDR *valp);
 
-/* Same as the above, but read the auxv data from the current inferior.  Use
-   the current inferior's top target and arch to parse auxv entries.  */
+/* Same as the above, but get the gdbarch from the current inferior.  */
 
 extern int target_auxv_search (CORE_ADDR match, CORE_ADDR *valp);
 
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 8cd6be436094..bdd23bc2a328 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -1123,7 +1123,7 @@ core_target::read_description ()
     {
       const struct target_desc *result;
 
-      result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
+      result = gdbarch_core_read_description (m_core_gdbarch, core_bfd);
       if (result != NULL)
 	return result;
     }
diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index e7230949aad5..0791f373d7e4 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -1892,7 +1892,7 @@ Refresh overlay mapped state for section OSECT.
 Method(
     type="const struct target_desc *",
     name="core_read_description",
-    params=[("struct target_ops *", "target"), ("bfd *", "abfd")],
+    params=[("bfd *", "abfd")],
     predicate=True,
     invalid=True,
 )
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index a663316df166..4de2b31bc2e3 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1127,8 +1127,8 @@ extern void set_gdbarch_overlay_update (struct gdbarch *gdbarch, gdbarch_overlay
 
 extern bool gdbarch_core_read_description_p (struct gdbarch *gdbarch);
 
-typedef const struct target_desc * (gdbarch_core_read_description_ftype) (struct gdbarch *gdbarch, struct target_ops *target, bfd *abfd);
-extern const struct target_desc * gdbarch_core_read_description (struct gdbarch *gdbarch, struct target_ops *target, bfd *abfd);
+typedef const struct target_desc * (gdbarch_core_read_description_ftype) (struct gdbarch *gdbarch, bfd *abfd);
+extern const struct target_desc * gdbarch_core_read_description (struct gdbarch *gdbarch, bfd *abfd);
 extern void set_gdbarch_core_read_description (struct gdbarch *gdbarch, gdbarch_core_read_description_ftype *core_read_description);
 
 /* Set if the address in N_SO or N_FUN stabs may be zero. */
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index ddb8dec1c72d..f6138a42219a 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -4211,13 +4211,13 @@ gdbarch_core_read_description_p (struct gdbarch *gdbarch)
 }
 
 const struct target_desc *
-gdbarch_core_read_description (struct gdbarch *gdbarch, struct target_ops *target, bfd *abfd)
+gdbarch_core_read_description (struct gdbarch *gdbarch, bfd *abfd)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->core_read_description != NULL);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_core_read_description called\n");
-  return gdbarch->core_read_description (gdbarch, target, abfd);
+  return gdbarch->core_read_description (gdbarch, abfd);
 }
 
 void
diff --git a/gdb/i386-fbsd-tdep.c b/gdb/i386-fbsd-tdep.c
index eef124fca0ce..fb3a384d9442 100644
--- a/gdb/i386-fbsd-tdep.c
+++ b/gdb/i386-fbsd-tdep.c
@@ -281,9 +281,7 @@ i386fbsd_core_read_xcr0 (bfd *abfd)
 /* Implement the core_read_description gdbarch method.  */
 
 static const struct target_desc *
-i386fbsd_core_read_description (struct gdbarch *gdbarch,
-				struct target_ops *target,
-				bfd *abfd)
+i386fbsd_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   return i386_target_description (i386fbsd_core_read_xcr0 (abfd), true);
 }
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index 5c2fed39868c..63a56b86a974 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -703,9 +703,7 @@ i386_linux_read_description (uint64_t xcr0)
 /* Get Linux/x86 target description from core dump.  */
 
 static const struct target_desc *
-i386_linux_core_read_description (struct gdbarch *gdbarch,
-				  struct target_ops *target,
-				  bfd *abfd)
+i386_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   /* Linux/i386.  */
   uint64_t xcr0 = i386_linux_core_read_xcr0 (abfd);
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index c30d9fb13f8c..99eaec431a5d 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2661,12 +2661,11 @@ linux_displaced_step_restore_all_in_ptid (inferior *parent_inf, ptid_t ptid)
 /* Helper for linux_get_hwcap and linux_get_hwcap2.  */
 
 static CORE_ADDR
-linux_get_hwcap_helper (const gdb::optional<gdb::byte_vector> &auxv,
-			target_ops *target, gdbarch *gdbarch, CORE_ADDR match)
+linux_get_hwcap_helper (gdbarch *gdbarch, CORE_ADDR match)
 {
   CORE_ADDR field;
-  if (!auxv.has_value ()
-      || target_auxv_search (*auxv, target, gdbarch, match, &field) != 1)
+
+  if (target_auxv_search (gdbarch, match, &field) != 1)
     return 0;
   return field;
 }
@@ -2674,10 +2673,9 @@ linux_get_hwcap_helper (const gdb::optional<gdb::byte_vector> &auxv,
 /* See linux-tdep.h.  */
 
 CORE_ADDR
-linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
-		 target_ops *target, gdbarch *gdbarch)
+linux_get_hwcap (gdbarch *gdbarch)
 {
-  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP);
+  return linux_get_hwcap_helper (gdbarch, AT_HWCAP);
 }
 
 /* See linux-tdep.h.  */
@@ -2685,18 +2683,15 @@ linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
 CORE_ADDR
 linux_get_hwcap ()
 {
-  return linux_get_hwcap (target_read_auxv (),
-			  current_inferior ()->top_target (),
-			  current_inferior ()->gdbarch);
+  return linux_get_hwcap (current_inferior ()->gdbarch);
 }
 
 /* See linux-tdep.h.  */
 
 CORE_ADDR
-linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
-		  target_ops *target, gdbarch *gdbarch)
+linux_get_hwcap2 (gdbarch *gdbarch)
 {
-  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP2);
+  return linux_get_hwcap_helper (gdbarch, AT_HWCAP2);
 }
 
 /* See linux-tdep.h.  */
@@ -2704,9 +2699,7 @@ linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
 CORE_ADDR
 linux_get_hwcap2 ()
 {
-  return linux_get_hwcap2 (target_read_auxv (),
-			   current_inferior ()->top_target (),
-			   current_inferior ()->gdbarch);
+  return linux_get_hwcap2 (current_inferior ()->gdbarch);
 }
 
 /* Display whether the gcore command is using the
diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
index 95cc29c828c2..b7a52ccde235 100644
--- a/gdb/linux-tdep.h
+++ b/gdb/linux-tdep.h
@@ -90,23 +90,21 @@ extern void linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
 
 extern int linux_is_uclinux (void);
 
-/* Fetch the AT_HWCAP entry from auxv data AUXV.  Use TARGET and GDBARCH to
-   parse auxv entries.
+/* Fetch the AT_HWCAP entry from the current inferior's auxv data.  Use GDBARCH
+   to parse auxv entries.
 
    On error, 0 is returned.  */
-extern CORE_ADDR linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
-				  struct target_ops *target, gdbarch *gdbarch);
+extern CORE_ADDR linux_get_hwcap (gdbarch *gdbarch);
 
 /* Same as the above, but obtain all the inputs from the current inferior.  */
 
 extern CORE_ADDR linux_get_hwcap ();
 
-/* Fetch the AT_HWCAP2 entry from auxv data AUXV.  Use TARGET and GDBARCH to
-   parse auxv entries.
+/* Fetch the AT_HWCAP2 entry from the current inferior's auxv data.  Use GDBARCH
+   to parse auxv entries.
 
    On error, 0 is returned.  */
-extern CORE_ADDR linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
-				   struct target_ops *target, gdbarch *gdbarch);
+extern CORE_ADDR linux_get_hwcap2 (gdbarch *gdbarch);
 
 /* Same as the above, but obtain all the inputs from the current inferior.  */
 
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 1b3b5f88edbc..534d47fc1ae5 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -554,9 +554,7 @@ mips_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
 }
 
 static const struct target_desc *
-mips_linux_core_read_description (struct gdbarch *gdbarch,
-				  struct target_ops *target,
-				  bfd *abfd)
+mips_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *section = bfd_get_section_by_name (abfd, ".reg");
   if (! section)
diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index 39d692b2764c..cec3a31d7746 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1566,9 +1566,7 @@ ppc_linux_write_pc (struct regcache *regcache, CORE_ADDR pc)
 }
 
 static const struct target_desc *
-ppc_linux_core_read_description (struct gdbarch *gdbarch,
-				 struct target_ops *target,
-				 bfd *abfd)
+ppc_linux_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   struct ppc_linux_features features = ppc_linux_no_features;
   asection *altivec = bfd_get_section_by_name (abfd, ".reg-ppc-vmx");
@@ -1601,8 +1599,7 @@ ppc_linux_core_read_description (struct gdbarch *gdbarch,
   if (vsx)
     features.vsx = true;
 
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
+  CORE_ADDR hwcap = linux_get_hwcap (gdbarch);
 
   features.isa205 = ppc_linux_has_isa205 (hwcap);
 
diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 14d71134e0cd..d134332b7b16 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -328,12 +328,10 @@ s390_iterate_over_regset_sections (struct gdbarch *gdbarch,
 /* Implement core_read_description gdbarch method.  */
 
 static const struct target_desc *
-s390_core_read_description (struct gdbarch *gdbarch,
-			    struct target_ops *target, bfd *abfd)
+s390_core_read_description (gdbarch *gdbarch, bfd *abfd)
 {
   asection *section = bfd_get_section_by_name (abfd, ".reg");
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
-  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
+  CORE_ADDR hwcap = linux_get_hwcap (gdbarch);
   bool high_gprs, v1, v2, te, vx, gs;
 
   if (!section)
-- 
2.38.1

Re: [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache

[Tom Tromey]

>> It seems like overall it would be better to have the auxv cache just be
>> some member of 'inferior'.  Then these indirections and observers
>> wouldn't be needed -- the inferior could just manage it directly.

Simon> I kind of like observers, it gives the impression that things are
Simon> decoupled... but yeah in practice it just adds unnecessary layers of
Simon> indirection.

I like them depending on the situation.  For me it's sort of like
attaching a registry entry.

If there are "optional" modules that need to attach data to a core data
structure (e.g., something arch-specific attaching to an objfile) then a
registry entry makes sense.  But for things that are inherit attributes
of an object, a registry entry would be sort of absurd.

I see observers the same way.  If two modules "should be" decoupled,
like if one is a core bit of gdb and another is some thing that might be
not compiled in, or only useful in some situations, then an observer is
a good way to convey state changes.

For auxv, IIUC, it's intended to always cache this data, which is
somewhat intrinsic to the inferior and the module must track some
aspects of the inferior's lifetime.  To me this says, just make some
auxv cache object as a field of inferior and let the inferior tell it
directly.

That said I wouldn't expect a change here since it's already written.
Just wouldn't object; and I guess it's worthwhile to try to articulate
the principle for new code.

thanks,
Tom