Re: [PATCH 2/8] bfd/binutils: support for gdb target descriptions in the core file

Andrew Burgess
2020-12-07T12:49:05Z

* Luis Machado <luis.machado@linaro.org> [2020-12-02 15:21:49 -0300]:

> On 12/2/20 2:39 PM, Andrew Burgess wrote:
> > This commit lays the ground work for allowing GDB to write its target
> > description into a generated core file.
> >
> > The goal of this work is to allow a user to connect to a remote
> > target, capture a core file from within GDB, then pass the executable
> > and core file to another user and have the user be able to examine the
> > state of the machine without needing to connect to a running target.
> >
> > Different remote targets can have different register sets and this
> > information is communicated from the target to GDB in the target
> > description.
> >
> > It is possible for a user to extract the target description from GDB
> > and pass this along with the core file so that when the core file is
> > used the target description can be fed back into GDB, however this is
> > not a great user experience.
> >
> > It would be nicer, I think, if GDB could write the target description
> > directly into the core file, and then make use of this description
> > when loading a core file.
> >
> > This commit performs the binutils/bfd side of this task, adding the
> > boiler plate functions to access the target description from within a
> > core file note, and reserving a new number for a note containing the
> > target description.
> >
> > Later commits will extend GDB to make use of this.
> >
> > bfd/ChangeLog:
> >
> > * elf-bfd.h (elfcore_write_gdb_tdesc): Declare new function.
> > * elf.c (elfcore_grok_gdb_tdesc): New function.
> > (elfcore_grok_note): Handle NT_GDB_TDESC.
> > (elfcore_write_gdb_tdesc): New function.
> > (elfcore_write_register_note): Handle NT_GDB_TDESC.
> >
> > binutils/ChangeLog:
> >
> > * readelf.c (get_note_type): Handle NT_GDB_TDESC.
> >
> > include/ChangeLog:
> >
> > * elf/common.h (NT_GDB_TDESC): Define.
> > ---
> > bfd/ChangeLog | 9 +++++++++
> > bfd/elf-bfd.h | 2 ++
> > bfd/elf.c | 23 +++++++++++++++++++++++
> > binutils/ChangeLog | 5 +++++
> > binutils/readelf.c | 2 ++
> > include/ChangeLog | 5 +++++
> > include/elf/common.h | 2 ++
> > 7 files changed, 48 insertions(+)
> >
> > diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> > index e9c890f6f16..5ef69ab5b13 100644
> > --- a/bfd/elf-bfd.h
> > +++ b/bfd/elf-bfd.h
> > @@ -2796,6 +2796,8 @@ extern char *elfcore_write_aarch_pauth
> > (bfd *, char *, int *, const void *, int);
> > extern char *elfcore_write_arc_v2
> > (bfd *, char *, int *, const void *, int);
> > +extern char *elfcore_write_gdb_tdesc
> > + (bfd *, char *, int *, const void *, int);
> > extern char *elfcore_write_lwpstatus
> > (bfd *, char *, int *, long, int, const void *);
> > extern char *elfcore_write_register_note
> > diff --git a/bfd/elf.c b/bfd/elf.c
> > index 419c5f4420c..bea5ab12773 100644
> > --- a/bfd/elf.c
> > +++ b/bfd/elf.c
> > @@ -9909,6 +9909,12 @@ elfcore_grok_arc_v2 (bfd *abfd, Elf_Internal_Note *note)
> > return elfcore_make_note_pseudosection (abfd, ".reg-arc-v2", note);
> > }
> > +static bfd_boolean
> > +elfcore_grok_gdb_tdesc (bfd *abfd, Elf_Internal_Note *note)
> > +{
> > + return elfcore_make_note_pseudosection (abfd, ".gdb-tdesc", note);
> > +}
> > +
> > #if defined (HAVE_PRPSINFO_T)
> > typedef prpsinfo_t elfcore_psinfo_t;
> > #if defined (HAVE_PRPSINFO32_T) /* Sparc64 cross Sparc32 */
> > @@ -10566,6 +10572,9 @@ elfcore_grok_note (bfd *abfd, Elf_Internal_Note *note)
> > else
> > return TRUE;
> > + case NT_GDB_TDESC:
> > + return elfcore_grok_gdb_tdesc (abfd, note);
> > +
> > case NT_PRPSINFO:
> > case NT_PSINFO:
> > if (bed->elf_backend_grok_psinfo)
> > @@ -11947,6 +11956,18 @@ elfcore_write_arc_v2 (bfd *abfd,
> > note_name, NT_ARC_V2, arc_v2, size);
> > }
> > +char *
> > +elfcore_write_gdb_tdesc (bfd *abfd,
> > + char *buf,
> > + int *bufsiz,
> > + const void *tdesc,
> > + int size)
> > +{
> > + const char *note_name = "CORE";
> > + return elfcore_write_note (abfd, buf, bufsiz,
> > + note_name, NT_GDB_TDESC, tdesc, size);
> > +}
> > +
> > char *
> > elfcore_write_register_note (bfd *abfd,
> > char *buf,
> > @@ -12031,6 +12052,8 @@ elfcore_write_register_note (bfd *abfd,
> > return elfcore_write_aarch_pauth (abfd, buf, bufsiz, data, size);
> > if (strcmp (section, ".reg-arc-v2") == 0)
> > return elfcore_write_arc_v2 (abfd, buf, bufsiz, data, size);
> > + if (strcmp (section, ".gdb-tdesc") == 0)
> > + return elfcore_write_gdb_tdesc (abfd, buf, bufsiz, data, size);
> > return NULL;
> > }
> > diff --git a/binutils/readelf.c b/binutils/readelf.c
> > index 57e0f1de459..5b3871d3e5f 100644
> > --- a/binutils/readelf.c
> > +++ b/binutils/readelf.c
> > @@ -18246,6 +18246,8 @@ get_note_type (Filedata * filedata, unsigned e_type)
> > return _("NT_PRPSINFO (prpsinfo structure)");
> > case NT_TASKSTRUCT:
> > return _("NT_TASKSTRUCT (task structure)");
> > + case NT_GDB_TDESC:
> > + return _("NT_GDB_TDESC (GDB XML target description)");
> > case NT_PRXFPREG:
> > return _("NT_PRXFPREG (user_xfpregs structure)");
> > case NT_PPC_VMX:
> > diff --git a/include/elf/common.h b/include/elf/common.h
> > index 95a852f0cf5..1dbf0b11983 100644
> > --- a/include/elf/common.h
> > +++ b/include/elf/common.h
> > @@ -666,6 +666,8 @@
> > #define NT_SIGINFO 0x53494749 /* Fields of siginfo_t. */
> > #define NT_FILE 0x46494c45 /* Description of mapped files. */
> > +#define NT_GDB_TDESC 0x54444553 /* Contains copy of GDB's target description XML. */
> > +
>
> How about a generic name without the tool's name on it? Other tools may want
> to use it as well, and that can be a little confusing. NT_XML_TDESC,
> maybe?

I originally did name the note NT_XML_TDESC, however, I moved away
from this because NT_GDB_TDESC describes what this actually is, and I
don't think switching back is a good choice.

If some other tool can process these descriptions that this is because
that tool has made the choice to accept GDB format target
descriptions, so I don't think having GDB in the name would be a
problem.

Further, if some other tool comes up with its own target description
specification then it's quite possible that it would also use XML then
we have to find a new name that (a) doesn't include the tool name, but
(b) doesn't clash with NT_XML_TDESC.

>
> For the constant, I find the magic number amusing, but I don't think it does
> any good when you're trying to find out why that particular magic number was
> picked, and what the next number should be.
>
> Should we go with the basic increasing ID instead? I think this would be 7,
> right after NT_AUXV.

The number certainly wasn't picked for amusement.

I didn't pick 7 for fear of clashing with a number that someone else
might already be using, if I understand Jim correctly then it seems
this was a good call.

The ASCII encoded string provided a very semi random large number
that was unlikely to clash, and was inline with how at least some
other notes were numbered.

I really have no strong feelings one way or the other on what the note
number should be, if someone wants to propose a scheme, then I'm more
than happy to adopt that.

I'll wait to see how the numbering conversation pans out and adapt
this patch to fit in.

Thanks,
Andrew