[2/3] gdb: break up core_target initialization
Commit Message
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(-)
Comments
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. */
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
>>>>> "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
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!
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
@@ -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 ();
@@ -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"