Re: [PATCH] aarch64 linux core dump support

[Will Newton <will.newton@linaro.org>]

On 14 May 2014 22:19, Omair Javaid <omair.javaid@linaro.org> wrote:

Hi Omair,

> 2014-05-15  Omair Javaid  <omair.javaid@linaro.org>
>
> bfd/
>         * elfxx-aarch64.c (stdarg.h): Include.
>         (_bfd_aarch64_elf_grok_prstatus): Updated.
>         (_bfd_aarch64_elf_grok_psinfo): New function.
>         (_bfd_aarch64_elf_write_core_note): New function.
>         * elfxx-aarch64.h (elf_backend_grok_psinfo): Define.
>         (elf_backend_write_core_note): Define.
> ---
>  bfd/elfxx-aarch64.c | 102 +++++++++++++++++++++++++++++++++++++++++++---------
>  bfd/elfxx-aarch64.h |  12 +++++--
>  2 files changed, 96 insertions(+), 18 deletions(-)
>
> diff --git a/bfd/elfxx-aarch64.c b/bfd/elfxx-aarch64.c
> index 7db6295..23e2ef1 100644
> --- a/bfd/elfxx-aarch64.c
> +++ b/bfd/elfxx-aarch64.c
> @@ -20,6 +20,7 @@
>
>  #include "sysdep.h"
>  #include "elfxx-aarch64.h"
> +#include <stdarg.h>
>
>  #define MASK(n) ((1u << (n)) - 1)
>
> @@ -498,25 +499,94 @@ _bfd_aarch64_elf_grok_prstatus (bfd *abfd, Elf_Internal_Note *note)
>    switch (note->descsz)
>      {
>        default:
> -       return FALSE;
> +        return FALSE;
> +
> +      /* sizeof(struct elf_prstatus) on Linux/aarch64.  */
> +      case 392:
> +        elf_tdata (abfd)->core->signal = bfd_get_16 (abfd, note->descdata + 12);
> +        elf_tdata (abfd)->core->lwpid = bfd_get_32 (abfd, note->descdata + 32);
> +        offset = 112;
> +        size = 272;
> +        break;
> +    }

Does anything actually change in this function? As far as I can tell
it just gets reformatted.

If the patch is just adding two functions and hooking them in then it
would make it simpler to review if the formatting changes were left
for a separate patch.

>
> -      case 392:                /* sizeof(struct elf_prstatus) on Linux/arm64.  */
> -       /* pr_cursig */
> -       elf_tdata (abfd)->core->signal
> -         = bfd_get_16 (abfd, note->descdata + 12);
> +  /* Make a ".reg/999" section.  */
> +  return _bfd_elfcore_make_pseudosection (abfd, ".reg",
> +                          size, note->descpos + offset);
> +}
>
> -       /* pr_pid */
> -       elf_tdata (abfd)->core->lwpid
> -         = bfd_get_32 (abfd, note->descdata + 32);
> +bfd_boolean
> +_bfd_aarch64_elf_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
> +{
> +  switch (note->descsz)
> +    {
> +      default:
> +        return FALSE;
> +
> +      case 136:                /* sizeof(struct elf_prpsinfo) on Linux/x86_64 */

Should be aarch64?

> +        elf_tdata (abfd)->core->pid = bfd_get_32 (abfd, note->descdata + 24);
> +        elf_tdata (abfd)->core->program =
> +        _bfd_elfcore_strndup (abfd, note->descdata + 40, 16);
> +        elf_tdata (abfd)->core->command =
> +        _bfd_elfcore_strndup (abfd, note->descdata + 56, 80);
> +    }
>
> -       /* pr_reg */
> -       offset = 112;
> -       size = 272;
> +  /* Note that for some reason, a spurious space is tacked
> +     onto the end of the args in some (at least one anyway)
> +     implementations, so strip it off if it exists.  */

I wonder if we (or anybody else) still needs this code. Bizarrely it
was added with no commentary in this commit:

commit eaa57a10aa324a06af1ac84ac8ffde8dc1b2bc87
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Tue Oct 27 00:00:50 1998 +0000

    (bfd_elf_hash): Optimize the hash function a bit.

And has been copy and pasted for ever more since.

> -       break;
> -    }
> +  {
> +    char *command = elf_tdata (abfd)->core->command;
> +    int n = strlen (command);

I suppose we should really include string.h for this and memcpy.

>
> -  /* Make a ".reg/999" section.  */
> -  return _bfd_elfcore_make_pseudosection (abfd, ".reg",
> -                                         size, note->descpos + offset);
> +    if (0 < n && command[n - 1] == ' ')
> +      command[n - 1] = '\0';
> +  }
> +
> +  return TRUE;
> +}
> +
> +char *
> +_bfd_aarch64_elf_write_core_note (bfd *abfd, char *buf, int *bufsiz, int note_type,
> +                                 ...)
> +{
> +  switch (note_type)
> +    {
> +      default:
> +        return NULL;
> +
> +      case NT_PRPSINFO:
> +        {
> +          char data[136];
> +          va_list ap;
> +          va_start (ap, note_type);
> +          memset (data, 0, sizeof (data));
> +          strncpy (data + 40, va_arg (ap, const char *), 16);
> +          strncpy (data + 56, va_arg (ap, const char *), 80);
> +          va_end (ap);
> +          return elfcore_write_note (abfd, buf, bufsiz,
> +                             "CORE", note_type, data, sizeof (data));
> +      }
> +
> +      case NT_PRSTATUS:
> +        {
> +          char data[392];
> +          va_list ap;
> +          long pid;
> +          int cursig;
> +          const void *greg;
> +
> +          va_start (ap, note_type);
> +          memset (data, 0, sizeof (data));
> +          pid = va_arg (ap, long);
> +          bfd_put_32 (abfd, pid, data + 32);
> +          cursig = va_arg (ap, int);
> +          bfd_put_16 (abfd, cursig, data + 12);
> +          greg = va_arg (ap, const void *);
> +          memcpy (data + 112, greg, 272);
> +          va_end (ap);
> +          return elfcore_write_note (abfd, buf, bufsiz,
> +                                    "CORE", note_type, data, sizeof (data));
> +      }
> +    }
>  }
> diff --git a/bfd/elfxx-aarch64.h b/bfd/elfxx-aarch64.h
> index 5ca3b7f..1d61d96 100644
> --- a/bfd/elfxx-aarch64.h
> +++ b/bfd/elfxx-aarch64.h
> @@ -42,6 +42,14 @@ _bfd_aarch64_elf_add_symbol_hook (bfd *, struct bfd_link_info *,
>  extern bfd_boolean
>  _bfd_aarch64_elf_grok_prstatus (bfd *, Elf_Internal_Note *);
>
> +extern bfd_boolean
> +_bfd_aarch64_elf_grok_psinfo (bfd *, Elf_Internal_Note *);
> +
> +extern char *
> +_bfd_aarch64_elf_write_core_note (bfd *abfd, char *buf, int *bufsiz,
> +                                       int note_type, ...);
>
> -#define elf_backend_add_symbol_hook    _bfd_aarch64_elf_add_symbol_hook
> -#define elf_backend_grok_prstatus      _bfd_aarch64_elf_grok_prstatus
> +#define elf_backend_add_symbol_hook  _bfd_aarch64_elf_add_symbol_hook
> +#define elf_backend_grok_prstatus    _bfd_aarch64_elf_grok_prstatus
> +#define elf_backend_grok_psinfo      _bfd_aarch64_elf_grok_psinfo
> +#define elf_backend_write_core_note  _bfd_aarch64_elf_write_core_note

There also seem to be some unrelated formatting changes here.

-- 
Will Newton
Toolchain Working Group, Linaro