[v2] Update the core file architecture if a target description is present

Message ID 20210622013933.3750993-1-luis.machado@linaro.org
State New
Headers show
Series
  • [v2] Update the core file architecture if a target description is present
Related show

Commit Message

Eli Zaretskii via Gdb-patches June 22, 2021, 1:39 a.m.
Updates on v2:

- Update the constructor to read the target description from the core file.

--

At the moment, the core target has its own gdbarch (m_core_gdbarch), and that
gets set from the core_bfd on the core target's constructor.

That gdbarch doesn't contain a target description because it is constructed
before we get a chance to fetch the target description.

As a result, some hooks that depend on the target description being set are
not set, and that leads to problems. One of the examples is
gdbarch_report_signal_info, which is used to show AArch64 tag violation
information.

Fix this by reading the target description before fetching the core file's
gdbarch.

gdb/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

	* corelow.c (core_target::core_target) Update to read target
	description.
---
 gdb/corelow.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.25.1

Comments

Eli Zaretskii via Gdb-patches June 22, 2021, 9:23 p.m. | #1
On 2021-06-21 9:39 p.m., Luis Machado via Gdb-patches wrote:
> Updates on v2:

> 

> - Update the constructor to read the target description from the core file.

> 

> --

> 

> At the moment, the core target has its own gdbarch (m_core_gdbarch), and that

> gets set from the core_bfd on the core target's constructor.

> 

> That gdbarch doesn't contain a target description because it is constructed

> before we get a chance to fetch the target description.

> 

> As a result, some hooks that depend on the target description being set are

> not set, and that leads to problems. One of the examples is

> gdbarch_report_signal_info, which is used to show AArch64 tag violation

> information.

> 

> Fix this by reading the target description before fetching the core file's

> gdbarch.



> 

> gdb/ChangeLog:

> 

> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

> 

> 	* corelow.c (core_target::core_target) Update to read target

> 	description.

> ---

>  gdb/corelow.c | 6 +++++-

>  1 file changed, 5 insertions(+), 1 deletion(-)

> 

> diff --git a/gdb/corelow.c b/gdb/corelow.c

> index a1943ab2ea6..71174172c22 100644

> --- a/gdb/corelow.c

> +++ b/gdb/corelow.c

> @@ -154,7 +154,11 @@ class core_target final : public process_stratum_target

>  

>  core_target::core_target ()

>  {

> -  m_core_gdbarch = gdbarch_from_bfd (core_bfd);

> +  struct gdbarch_info info;

> +  gdbarch_info_init (&info);

> +  info.abfd = core_bfd;

> +  info.target_desc = read_description ();

> +  m_core_gdbarch = gdbarch_find_by_info (info);


I looked at this in a bit more details and noticed something while
stepping in core_target::core_target and read_description.

When you call read_description, is m_core_gdbarch is nullptr, its
initial value.  If an XML target description is included in the
.gdb-tdesc section, all is well, it describes the exact target
description to use.  But my x86-64 core doesn't have that.  Do your
cores have it?

Otherwise, we reach:

  if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch))
    {
      const struct target_desc *result;

      result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);
      if (result != NULL)
	return result;
    }

This is skipped over, since m_core_gdbarch is nullptr.
gdbarch_core_read_description is meant returns a precise target
description based on some arch-specific criteria.  But since we don't
have a gdbarch, it's not called.  So we end up at:

  return this->beneath ()->read_description ();

which hits dummy_target::read_description and returns nullptr.  We
likely just end up with the same behavior as we have today, with a
gdbarch only derived from the BFD (gdbarch_from_bfd) and not from the
target description we could derive from the core.

It's a bit of a particular case, because in order to get
gdbarch_core_read_description called, we need to at least know
approximately which architecture we are working with.  That would be the
arch we can get from gdbarch_from_bfd.  Once we have it, we can ask it,
"are you able to generate a target description from that core file?",
and then possibly derive a more specific / precise gdbarch from it.

So I wonder if it would make sense to do:

  /* Find a first arch based on the BFD.  */
  m_core_gdbarch = gdbarch_from_bfd (core_bfd);

  /* If the arch is able to read a target description from the core, it
     could yield a more specific gdbarch.  */
  struct gdbarch_info info;
  gdbarch_info_init (&info);
  info.abfd = core_bfd;
  info.target_desc = read_description ();
  if (info.target_desc != nullptr)
    m_core_gdbarch = gdbarch_find_by_info (info);

Does that make sense?

I'm curious to know how it works for your AArch64 cores, because if you
sent this patch version, it must be because it does fix your problem.

Simon
Eli Zaretskii via Gdb-patches June 23, 2021, 1:22 a.m. | #2
On 6/22/21 6:23 PM, Simon Marchi wrote:
> On 2021-06-21 9:39 p.m., Luis Machado via Gdb-patches wrote:

>> Updates on v2:

>>

>> - Update the constructor to read the target description from the core file.

>>

>> --

>>

>> At the moment, the core target has its own gdbarch (m_core_gdbarch), and that

>> gets set from the core_bfd on the core target's constructor.

>>

>> That gdbarch doesn't contain a target description because it is constructed

>> before we get a chance to fetch the target description.

>>

>> As a result, some hooks that depend on the target description being set are

>> not set, and that leads to problems. One of the examples is

>> gdbarch_report_signal_info, which is used to show AArch64 tag violation

>> information.

>>

>> Fix this by reading the target description before fetching the core file's

>> gdbarch.

> 

> 

>>

>> gdb/ChangeLog:

>>

>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

>>

>> 	* corelow.c (core_target::core_target) Update to read target

>> 	description.

>> ---

>>   gdb/corelow.c | 6 +++++-

>>   1 file changed, 5 insertions(+), 1 deletion(-)

>>

>> diff --git a/gdb/corelow.c b/gdb/corelow.c

>> index a1943ab2ea6..71174172c22 100644

>> --- a/gdb/corelow.c

>> +++ b/gdb/corelow.c

>> @@ -154,7 +154,11 @@ class core_target final : public process_stratum_target

>>   

>>   core_target::core_target ()

>>   {

>> -  m_core_gdbarch = gdbarch_from_bfd (core_bfd);

>> +  struct gdbarch_info info;

>> +  gdbarch_info_init (&info);

>> +  info.abfd = core_bfd;

>> +  info.target_desc = read_description ();

>> +  m_core_gdbarch = gdbarch_find_by_info (info);

> 

> I looked at this in a bit more details and noticed something while

> stepping in core_target::core_target and read_description.

> 

> When you call read_description, is m_core_gdbarch is nullptr, its

> initial value.  If an XML target description is included in the

> .gdb-tdesc section, all is well, it describes the exact target

> description to use.  But my x86-64 core doesn't have that.  Do your

> cores have it?


They do, since they are actually generated by GDB at the moment, so the 
XML is in the core file. I expect core files that are generated by the 
Linux Kernel to not contain that information. So we will need to derive 
the architecture from other contents of the core file. But there is no 
support for MTE core files from the Linux Kernel's side yet, so that 
wasn't tested yet.

I could artifically remove the XML from the GDB-generated core file and 
give it a go just to make sure.

> 

> Otherwise, we reach:

> 

>    if (m_core_gdbarch && gdbarch_core_read_description_p (m_core_gdbarch))

>      {

>        const struct target_desc *result;

> 

>        result = gdbarch_core_read_description (m_core_gdbarch, this, core_bfd);

>        if (result != NULL)

> 	return result;

>      }

> 

> This is skipped over, since m_core_gdbarch is nullptr.

> gdbarch_core_read_description is meant returns a precise target

> description based on some arch-specific criteria.  But since we don't

> have a gdbarch, it's not called.  So we end up at:

> 

>    return this->beneath ()->read_description ();

> 

> which hits dummy_target::read_description and returns nullptr.  We

> likely just end up with the same behavior as we have today, with a

> gdbarch only derived from the BFD (gdbarch_from_bfd) and not from the

> target description we could derive from the core.

> 

> It's a bit of a particular case, because in order to get

> gdbarch_core_read_description called, we need to at least know

> approximately which architecture we are working with.  That would be the

> arch we can get from gdbarch_from_bfd.  Once we have it, we can ask it,

> "are you able to generate a target description from that core file?",

> and then possibly derive a more specific / precise gdbarch from it.


Ugh. Yeah, that seems to be one artifact of such an early 
initialization. I crafted the code so that even with a missing 
info.target_desc, gdbarch_find_by_info (...) would still return a valid 
core gdbarch.

But you are right that in order to fetch the target description via 
read_description, we need to at least have a minimum core gdbarch.

> 

> So I wonder if it would make sense to do:

> 

>    /* Find a first arch based on the BFD.  */

>    m_core_gdbarch = gdbarch_from_bfd (core_bfd);

> 

>    /* If the arch is able to read a target description from the core, it

>       could yield a more specific gdbarch.  */

>    struct gdbarch_info info;

>    gdbarch_info_init (&info);

>    info.abfd = core_bfd;

>    info.target_desc = read_description ();

>    if (info.target_desc != nullptr)

>      m_core_gdbarch = gdbarch_find_by_info (info);

> 

> Does that make sense?


It does. Let me patch that up.

Thanks for catching that.

> 

> I'm curious to know how it works for your AArch64 cores, because if you

> sent this patch version, it must be because it does fix your problem.


It works basically because it is a GDB-generated core file with XML 
target description data in it.

> 

> Simon

>

Patch

diff --git a/gdb/corelow.c b/gdb/corelow.c
index a1943ab2ea6..71174172c22 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -154,7 +154,11 @@  class core_target final : public process_stratum_target
 
 core_target::core_target ()
 {
-  m_core_gdbarch = gdbarch_from_bfd (core_bfd);
+  struct gdbarch_info info;
+  gdbarch_info_init (&info);
+  info.abfd = core_bfd;
+  info.target_desc = read_description ();
+  m_core_gdbarch = gdbarch_find_by_info (info);
 
   if (!m_core_gdbarch
       || !gdbarch_iterate_over_regset_sections_p (m_core_gdbarch))