* [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache @ 2022-11-29 2:50 Simon Marchi 2022-11-29 2:50 ` [PATCH 2/3] gdb: break up core_target initialization Simon Marchi ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Simon Marchi @ 2022-11-29 2:50 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] gdb: break up core_target initialization 2022-11-29 2:50 [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache Simon Marchi @ 2022-11-29 2:50 ` Simon Marchi 2022-11-29 19:27 ` John Baldwin 2022-11-30 16:05 ` Tom Tromey 2022-11-29 2:50 ` [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description Simon Marchi 2022-11-30 15:44 ` [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache Tom Tromey 2 siblings, 2 replies; 14+ messages in thread From: Simon Marchi @ 2022-11-29 2:50 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] gdb: break up core_target initialization 2022-11-29 2:50 ` [PATCH 2/3] gdb: break up core_target initialization Simon Marchi @ 2022-11-29 19:27 ` John Baldwin 2022-11-29 21:53 ` Simon Marchi 2022-11-30 16:05 ` Tom Tromey 1 sibling, 1 reply; 14+ messages in thread From: John Baldwin @ 2022-11-29 19:27 UTC (permalink / raw) To: Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] gdb: break up core_target initialization 2022-11-29 19:27 ` John Baldwin @ 2022-11-29 21:53 ` Simon Marchi 2022-11-30 17:29 ` John Baldwin 0 siblings, 1 reply; 14+ messages in thread From: Simon Marchi @ 2022-11-29 21:53 UTC (permalink / raw) To: John Baldwin, gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] gdb: break up core_target initialization 2022-11-29 21:53 ` Simon Marchi @ 2022-11-30 17:29 ` John Baldwin 0 siblings, 0 replies; 14+ messages in thread From: John Baldwin @ 2022-11-30 17:29 UTC (permalink / raw) To: Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] gdb: break up core_target initialization 2022-11-29 2:50 ` [PATCH 2/3] gdb: break up core_target initialization Simon Marchi 2022-11-29 19:27 ` John Baldwin @ 2022-11-30 16:05 ` Tom Tromey 2022-12-02 19:38 ` Simon Marchi 1 sibling, 1 reply; 14+ messages in thread From: Tom Tromey @ 2022-11-30 16:05 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] gdb: break up core_target initialization 2022-11-30 16:05 ` Tom Tromey @ 2022-12-02 19:38 ` Simon Marchi 0 siblings, 0 replies; 14+ messages in thread From: Simon Marchi @ 2022-12-02 19:38 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description 2022-11-29 2:50 [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache Simon Marchi 2022-11-29 2:50 ` [PATCH 2/3] gdb: break up core_target initialization Simon Marchi @ 2022-11-29 2:50 ` Simon Marchi 2022-11-29 19:16 ` John Baldwin 2022-12-02 20:51 ` [PATCH v1.1 " Simon Marchi 2022-11-30 15:44 ` [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache Tom Tromey 2 siblings, 2 replies; 14+ messages in thread From: Simon Marchi @ 2022-11-29 2:50 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description 2022-11-29 2:50 ` [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description Simon Marchi @ 2022-11-29 19:16 ` John Baldwin 2022-11-29 21:45 ` Simon Marchi 2022-12-02 20:51 ` [PATCH v1.1 " Simon Marchi 1 sibling, 1 reply; 14+ messages in thread From: John Baldwin @ 2022-11-29 19:16 UTC (permalink / raw) To: Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description 2022-11-29 19:16 ` John Baldwin @ 2022-11-29 21:45 ` Simon Marchi 0 siblings, 0 replies; 14+ messages in thread From: Simon Marchi @ 2022-11-29 21:45 UTC (permalink / raw) To: John Baldwin, gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1.1 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description 2022-11-29 2:50 ` [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description Simon Marchi 2022-11-29 19:16 ` John Baldwin @ 2022-12-02 20:51 ` Simon Marchi 1 sibling, 0 replies; 14+ messages in thread From: Simon Marchi @ 2022-12-02 20:51 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache 2022-11-29 2:50 [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache Simon Marchi 2022-11-29 2:50 ` [PATCH 2/3] gdb: break up core_target initialization Simon Marchi 2022-11-29 2:50 ` [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description Simon Marchi @ 2022-11-30 15:44 ` Tom Tromey 2022-12-02 19:36 ` Simon Marchi 2 siblings, 1 reply; 14+ messages in thread From: Tom Tromey @ 2022-11-30 15:44 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache 2022-11-30 15:44 ` [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache Tom Tromey @ 2022-12-02 19:36 ` Simon Marchi 2022-12-02 20:59 ` Tom Tromey 0 siblings, 1 reply; 14+ messages in thread From: Simon Marchi @ 2022-12-02 19:36 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache 2022-12-02 19:36 ` Simon Marchi @ 2022-12-02 20:59 ` Tom Tromey 0 siblings, 0 replies; 14+ messages in thread From: Tom Tromey @ 2022-12-02 20:59 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Tom Tromey, Simon Marchi >> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-12-02 20:59 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-29 2:50 [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache Simon Marchi 2022-11-29 2:50 ` [PATCH 2/3] gdb: break up core_target initialization Simon Marchi 2022-11-29 19:27 ` John Baldwin 2022-11-29 21:53 ` Simon Marchi 2022-11-30 17:29 ` John Baldwin 2022-11-30 16:05 ` Tom Tromey 2022-12-02 19:38 ` Simon Marchi 2022-11-29 2:50 ` [PATCH 3/3] gdb: remove target_ops parameter from gdbarch_core_read_description Simon Marchi 2022-11-29 19:16 ` John Baldwin 2022-11-29 21:45 ` Simon Marchi 2022-12-02 20:51 ` [PATCH v1.1 " Simon Marchi 2022-11-30 15:44 ` [PATCH 1/3] gdb: add inferior_target_stack_changed observer, use it to clear auxv cache Tom Tromey 2022-12-02 19:36 ` Simon Marchi 2022-12-02 20:59 ` 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).