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

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

[Luis Machado]

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 updating the core file gdbarch after fetching the target
description.

gdb/ChangeLog:

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

	* corelow.c: Include "observable.h".
	(class core_target) <set_core_gdbarch>: New method.
	(core_on_new_gdbarch): New function.
	(_initialize_corelow): Register with observer "architecture_changed".
---
 gdb/corelow.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index a1943ab2ea6..452b4dd4f9a 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -50,6 +50,7 @@
 #include <unordered_set>
 #include "gdbcmd.h"
 #include "xml-tdesc.h"
+#include "observable.h"
 
 #ifndef O_LARGEFILE
 #define O_LARGEFILE 0
@@ -107,6 +108,12 @@ class core_target final : public process_stratum_target
     return m_core_gdbarch;
   }
 
+  /* Setter, see variable definition.  */
+  void set_core_gdbarch (gdbarch *arch)
+  {
+    m_core_gdbarch = arch;
+  }
+
   /* See definition.  */
   void get_core_register_section (struct regcache *regcache,
 				  const struct regset *regset,
@@ -1185,6 +1192,30 @@ maintenance_print_core_file_backed_mappings (const char *args, int from_tty)
     targ->info_proc_mappings (targ->core_gdbarch ());
 }
 
+static void
+core_on_new_gdbarch (struct gdbarch *newarch)
+{
+  /* The target architecture has changed.  Attempt to update the core
+     target's architecture with NEWARCH's target description.  */
+  struct gdbarch_info info;
+
+  gdbarch_info_init (&info);
+  info.abfd = core_bfd;
+  info.target_desc = gdbarch_target_desc (newarch);
+
+  struct gdbarch *new_core_arch = gdbarch_find_by_info (info);
+
+  /* If we've found a valid architecture, update the core target.  */
+  if (new_core_arch != nullptr)
+    {
+      /* Update the core file architecture.  */
+      struct core_target *target = get_current_core_target ();
+
+      if (target != nullptr)
+	target->set_core_gdbarch (new_core_arch);
+    }
+}
+
 void _initialize_corelow ();
 void
 _initialize_corelow ()
@@ -1194,4 +1225,8 @@ _initialize_corelow ()
 	   maintenance_print_core_file_backed_mappings,
 	   _("Print core file's file-backed mappings."),
 	   &maintenanceprintlist);
+
+  /* Register observer to listen to architecture changes.  */
+  gdb::observers::architecture_changed.attach (core_on_new_gdbarch,
+					       "py-unwind");
 }
-- 
2.25.1

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

[Luis Machado]

On 5/18/21 5:20 PM, Luis Machado wrote:
> 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 updating the core file gdbarch after fetching the target
> description.
> 
> gdb/ChangeLog:
> 
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 
> 	* corelow.c: Include "observable.h".
> 	(class core_target) <set_core_gdbarch>: New method.
> 	(core_on_new_gdbarch): New function.
> 	(_initialize_corelow): Register with observer "architecture_changed".
> ---
>   gdb/corelow.c | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
> 
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index a1943ab2ea6..452b4dd4f9a 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -50,6 +50,7 @@
>   #include <unordered_set>
>   #include "gdbcmd.h"
>   #include "xml-tdesc.h"
> +#include "observable.h"
>   
>   #ifndef O_LARGEFILE
>   #define O_LARGEFILE 0
> @@ -107,6 +108,12 @@ class core_target final : public process_stratum_target
>       return m_core_gdbarch;
>     }
>   
> +  /* Setter, see variable definition.  */
> +  void set_core_gdbarch (gdbarch *arch)
> +  {
> +    m_core_gdbarch = arch;
> +  }
> +
>     /* See definition.  */
>     void get_core_register_section (struct regcache *regcache,
>   				  const struct regset *regset,
> @@ -1185,6 +1192,30 @@ maintenance_print_core_file_backed_mappings (const char *args, int from_tty)
>       targ->info_proc_mappings (targ->core_gdbarch ());
>   }
>   
> +static void
> +core_on_new_gdbarch (struct gdbarch *newarch)
> +{
> +  /* The target architecture has changed.  Attempt to update the core
> +     target's architecture with NEWARCH's target description.  */
> +  struct gdbarch_info info;
> +
> +  gdbarch_info_init (&info);
> +  info.abfd = core_bfd;
> +  info.target_desc = gdbarch_target_desc (newarch);
> +
> +  struct gdbarch *new_core_arch = gdbarch_find_by_info (info);
> +
> +  /* If we've found a valid architecture, update the core target.  */
> +  if (new_core_arch != nullptr)
> +    {
> +      /* Update the core file architecture.  */
> +      struct core_target *target = get_current_core_target ();
> +
> +      if (target != nullptr)
> +	target->set_core_gdbarch (new_core_arch);
> +    }
> +}
> +
>   void _initialize_corelow ();
>   void
>   _initialize_corelow ()
> @@ -1194,4 +1225,8 @@ _initialize_corelow ()
>   	   maintenance_print_core_file_backed_mappings,
>   	   _("Print core file's file-backed mappings."),
>   	   &maintenanceprintlist);
> +
> +  /* Register observer to listen to architecture changes.  */
> +  gdb::observers::architecture_changed.attach (core_on_new_gdbarch,
> +					       "py-unwind");
>   }
> 

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

[Luis Machado]

On 5/18/21 5:20 PM, Luis Machado wrote:
> 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 updating the core file gdbarch after fetching the target
> description.
> 
> gdb/ChangeLog:
> 
> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
> 
> 	* corelow.c: Include "observable.h".
> 	(class core_target) <set_core_gdbarch>: New method.
> 	(core_on_new_gdbarch): New function.
> 	(_initialize_corelow): Register with observer "architecture_changed".
> ---
>   gdb/corelow.c | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
> 
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index a1943ab2ea6..452b4dd4f9a 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -50,6 +50,7 @@
>   #include <unordered_set>
>   #include "gdbcmd.h"
>   #include "xml-tdesc.h"
> +#include "observable.h"
>   
>   #ifndef O_LARGEFILE
>   #define O_LARGEFILE 0
> @@ -107,6 +108,12 @@ class core_target final : public process_stratum_target
>       return m_core_gdbarch;
>     }
>   
> +  /* Setter, see variable definition.  */
> +  void set_core_gdbarch (gdbarch *arch)
> +  {
> +    m_core_gdbarch = arch;
> +  }
> +
>     /* See definition.  */
>     void get_core_register_section (struct regcache *regcache,
>   				  const struct regset *regset,
> @@ -1185,6 +1192,30 @@ maintenance_print_core_file_backed_mappings (const char *args, int from_tty)
>       targ->info_proc_mappings (targ->core_gdbarch ());
>   }
>   
> +static void
> +core_on_new_gdbarch (struct gdbarch *newarch)
> +{
> +  /* The target architecture has changed.  Attempt to update the core
> +     target's architecture with NEWARCH's target description.  */
> +  struct gdbarch_info info;
> +
> +  gdbarch_info_init (&info);
> +  info.abfd = core_bfd;
> +  info.target_desc = gdbarch_target_desc (newarch);
> +
> +  struct gdbarch *new_core_arch = gdbarch_find_by_info (info);
> +
> +  /* If we've found a valid architecture, update the core target.  */
> +  if (new_core_arch != nullptr)
> +    {
> +      /* Update the core file architecture.  */
> +      struct core_target *target = get_current_core_target ();
> +
> +      if (target != nullptr)
> +	target->set_core_gdbarch (new_core_arch);
> +    }
> +}
> +
>   void _initialize_corelow ();
>   void
>   _initialize_corelow ()
> @@ -1194,4 +1225,8 @@ _initialize_corelow ()
>   	   maintenance_print_core_file_backed_mappings,
>   	   _("Print core file's file-backed mappings."),
>   	   &maintenanceprintlist);
> +
> +  /* Register observer to listen to architecture changes.  */
> +  gdb::observers::architecture_changed.attach (core_on_new_gdbarch,
> +					       "py-unwind");
>   }
> 

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

[Luis Machado]

On 6/10/21 8:48 AM, Luis Machado wrote:
> 
> 
> On 5/18/21 5:20 PM, Luis Machado wrote:
>> 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 updating the core file gdbarch after fetching the target
>> description.
>>
>> gdb/ChangeLog:
>>
>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>
>>
>>     * corelow.c: Include "observable.h".
>>     (class core_target) <set_core_gdbarch>: New method.
>>     (core_on_new_gdbarch): New function.
>>     (_initialize_corelow): Register with observer "architecture_changed".
>> ---
>>   gdb/corelow.c | 35 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>
>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index a1943ab2ea6..452b4dd4f9a 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -50,6 +50,7 @@
>>   #include <unordered_set>
>>   #include "gdbcmd.h"
>>   #include "xml-tdesc.h"
>> +#include "observable.h"
>>   #ifndef O_LARGEFILE
>>   #define O_LARGEFILE 0
>> @@ -107,6 +108,12 @@ class core_target final : public 
>> process_stratum_target
>>       return m_core_gdbarch;
>>     }
>> +  /* Setter, see variable definition.  */
>> +  void set_core_gdbarch (gdbarch *arch)
>> +  {
>> +    m_core_gdbarch = arch;
>> +  }
>> +
>>     /* See definition.  */
>>     void get_core_register_section (struct regcache *regcache,
>>                     const struct regset *regset,
>> @@ -1185,6 +1192,30 @@ maintenance_print_core_file_backed_mappings 
>> (const char *args, int from_tty)
>>       targ->info_proc_mappings (targ->core_gdbarch ());
>>   }
>> +static void
>> +core_on_new_gdbarch (struct gdbarch *newarch)
>> +{
>> +  /* The target architecture has changed.  Attempt to update the core
>> +     target's architecture with NEWARCH's target description.  */
>> +  struct gdbarch_info info;
>> +
>> +  gdbarch_info_init (&info);
>> +  info.abfd = core_bfd;
>> +  info.target_desc = gdbarch_target_desc (newarch);
>> +
>> +  struct gdbarch *new_core_arch = gdbarch_find_by_info (info);
>> +
>> +  /* If we've found a valid architecture, update the core target.  */
>> +  if (new_core_arch != nullptr)
>> +    {
>> +      /* Update the core file architecture.  */
>> +      struct core_target *target = get_current_core_target ();
>> +
>> +      if (target != nullptr)
>> +    target->set_core_gdbarch (new_core_arch);
>> +    }
>> +}
>> +
>>   void _initialize_corelow ();
>>   void
>>   _initialize_corelow ()
>> @@ -1194,4 +1225,8 @@ _initialize_corelow ()
>>          maintenance_print_core_file_backed_mappings,
>>          _("Print core file's file-backed mappings."),
>>          &maintenanceprintlist);
>> +
>> +  /* Register observer to listen to architecture changes.  */
>> +  gdb::observers::architecture_changed.attach (core_on_new_gdbarch,
>> +                           "py-unwind");
>>   }
>>

Re: [PATCH] Update the core file architecture if a target description is present

[Simon Marchi]

On 2021-05-18 4:20 p.m., Luis Machado via Gdb-patches wrote:
> 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 updating the core file gdbarch after fetching the target
> description.

It's a bit strange that the core target first determines a gdbarch from
the core bfd, in the constructor.  But then it's the target that is
responsible for providing the target description, also found in a
section of the core bfd, that is ultimately used to determine a new
gdbarch.  Couldn't the target determine the right gdbarch from the
start, in the constructor, from the target description stored in the
core?

Perhaps the answer is obvious, but I don't have a core file with such a
section to test and I'm not sure how to generate one.

> +static void
> +core_on_new_gdbarch (struct gdbarch *newarch)
> +{
> +  /* The target architecture has changed.  Attempt to update the core
> +     target's architecture with NEWARCH's target description.  */
> +  struct gdbarch_info info;
> +
> +  gdbarch_info_init (&info);
> +  info.abfd = core_bfd;
> +  info.target_desc = gdbarch_target_desc (newarch);
> +
> +  struct gdbarch *new_core_arch = gdbarch_find_by_info (info);
> +
> +  /* If we've found a valid architecture, update the core target.  */
> +  if (new_core_arch != nullptr)
> +    {
> +      /* Update the core file architecture.  */
> +      struct core_target *target = get_current_core_target ();
> +
> +      if (target != nullptr)
> +	target->set_core_gdbarch (new_core_arch);
> +    }
> +}

Just remember that core_on_new_gdbarch will be called every time
set_target_arch is set.  That is pretty much every time an exec file is
loaded in an inferior, and every time an inferior is ran.  For this
reason, I think it would be good to put the quick and cheap check "does
the current inferior even have a core target as its process target"
first.  That would just mean doing this first:

  core_target *target = get_current_core_target ();
  if (target == nullptr)
    {
      /* Current inferior does not use a core target.  */
      return;
    }

You can then re-use target in the following code.

> +
>  void _initialize_corelow ();
>  void
>  _initialize_corelow ()
> @@ -1194,4 +1225,8 @@ _initialize_corelow ()
>  	   maintenance_print_core_file_backed_mappings,
>  	   _("Print core file's file-backed mappings."),
>  	   &maintenanceprintlist);
> +
> +  /* Register observer to listen to architecture changes.  */
> +  gdb::observers::architecture_changed.attach (core_on_new_gdbarch,
> +					       "py-unwind");

Replace "py-unwind" with something appropriate, core, core-target,
corelow, etc.

Simon

Re: [PATCH] Update the core file architecture if a target description is present

[Luis Machado]

On 6/17/21 12:26 AM, Simon Marchi wrote:
> On 2021-05-18 4:20 p.m., Luis Machado via Gdb-patches wrote:
>> 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 updating the core file gdbarch after fetching the target
>> description.
> 
> It's a bit strange that the core target first determines a gdbarch from
> the core bfd, in the constructor.  But then it's the target that is
> responsible for providing the target description, also found in a
> section of the core bfd, that is ultimately used to determine a new
> gdbarch.  Couldn't the target determine the right gdbarch from the
> start, in the constructor, from the target description stored in the
> core?

I think trying to read the target description from the core target's 
constructor works. Initially I was under the impression that attempting 
to initialize things on the core target's constructor was a bad idea 
given how early it runs. But, after giving this a try, it does work and 
is simpler.

If the core file contains the target description section, we can read it 
from there. Otherwise, if it doesn't, then we can call 
gdbarch_core_read_description.

Thanks. I'll send v2 shortly.

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

[Luis Machado]

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);
 
   if (!m_core_gdbarch
       || !gdbarch_iterate_over_regset_sections_p (m_core_gdbarch))
-- 
2.25.1

Re: [PATCH, v2] Update the core file architecture if a target description is present

[Simon Marchi]

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

Re: [PATCH, v2] Update the core file architecture if a target description is present

[Luis Machado]

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, v3] Update the core file architecture if a target description is present

[Luis Machado]

Updates on v3:

- Fetch an initial gdbarch before attempting to fetch a target description.

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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/gdb/corelow.c b/gdb/corelow.c
index a1943ab2ea6..b762eaa0f2f 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -154,8 +154,23 @@ class core_target final : public process_stratum_target
 
 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);
 
+  /* 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 ();
+
+  if (tdesc != nullptr)
+    {
+      struct gdbarch_info info;
+      gdbarch_info_init (&info);
+      info.abfd = core_bfd;
+      info.target_desc = tdesc;
+      m_core_gdbarch = gdbarch_find_by_info (info);
+    }
+
   if (!m_core_gdbarch
       || !gdbarch_iterate_over_regset_sections_p (m_core_gdbarch))
     error (_("\"%s\": Core file format not supported"),
-- 
2.25.1

Re: [PATCH, v3] Update the core file architecture if a target description is present

[Simon Marchi]

On 2021-06-22 10:42 p.m., Luis Machado via Gdb-patches wrote:
> Updates on v3:
> 
> - Fetch an initial gdbarch before attempting to fetch a target description.
> 
> 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.

LGTM, thanks.

I don't really have an idea of a test for this, do you?

Simon

Re: [PATCH, v3] Update the core file architecture if a target description is present

[Luis Machado]

On 6/23/21 1:37 PM, Simon Marchi wrote:
> On 2021-06-22 10:42 p.m., Luis Machado via Gdb-patches wrote:
>> Updates on v3:
>>
>> - Fetch an initial gdbarch before attempting to fetch a target description.
>>
>> 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.
> 
> LGTM, thanks.
> 
> I don't really have an idea of a test for this, do you?

Off the top of my head the only test that exercises this is 
aarch64-specific (as described above) and is included with the MTE core 
file support patch.

A generic test would need to verify that the core gdbarch matches the 
inferior's gdbarch, but I'm not sure how that could be accomplished.

> 
> Simon
>