Re: [PATCH/RFC 01/02 v2] Refactor PRPSINFO handling on Binutils

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Sergio,

I do not feel too confident in bfd/ but at least there may be more response to
my wrong comments.


> diff --git a/bfd/Makefile.in b/bfd/Makefile.in
> index 92d9d08..e12deb5 100644
> --- a/bfd/Makefile.in
> +++ b/bfd/Makefile.in
> @@ -1050,7 +1050,7 @@ BUILD_CFILES = \
>  CFILES = $(SOURCE_CFILES) $(BUILD_CFILES)
>  SOURCE_HFILES = \
>  	aout-target.h aoutf1.h aoutx.h coffcode.h coffswap.h ecoffswap.h \
> -	elf-bfd.h elf-hppa.h elf32-hppa.h \
> +	elf-bfd.h elf-psinfo.h elf-hppa.h elf32-hppa.h \
>  	elf64-hppa.h elfcode.h elfcore.h \
>  	freebsd.h genlink.h go32stub.h \
>  	libaout.h libbfd.h libcoff.h libecoff.h libhppa.h libieee.h \
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index b8d82b1..d314291 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -2234,11 +2234,15 @@ extern bfd_boolean bfd_elf_lookup_section_flags
>  extern Elf_Internal_Phdr * _bfd_elf_find_segment_containing_section
>    (bfd * abfd, asection * section);
>  
> +/* Forward declaration of prpsinfo.  See `elf-psinfo.h' for more details.  */
> +
> +struct elf_internal_prpsinfo;
> +
>  /* Exported interface for writing elf corefile notes. */
>  extern char *elfcore_write_note
>    (bfd *, char *, int *, const char *, int, const void *, int);
>  extern char *elfcore_write_prpsinfo
> -  (bfd *, char *, int *, const char *, const char *);
> +  (bfd *, char *, int *, const struct elf_internal_prpsinfo *);
>  extern char *elfcore_write_prstatus
>    (bfd *, char *, int *, long, int, const void *);
>  extern char * elfcore_write_pstatus
> diff --git a/bfd/elf-psinfo.h b/bfd/elf-psinfo.h
> new file mode 100644
> index 0000000..61b38e1
> --- /dev/null
> +++ b/bfd/elf-psinfo.h
> @@ -0,0 +1,215 @@
> +/* Declarations for PRPSINFO structures under ELF on GNU/Linux.
> +   Copyright 2012 Free Software Foundation, Inc.
> +
> +   This file is part of BFD, the Binary File Descriptor library.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
> +   MA 02110-1301, USA.  */
> +
> +#undef HAVE_PRPSINFO32_T
> +#define HAVE_PRPSINFO32_T
> +#undef HAVE_PRPSINFO32_T_PR_PID
> +#define HAVE_PRPSINFO32_T_PR_PID

This does not seem to be useful, these are auto-detected by configure and
elf-psinfo.h is included unconditionally.  Therefore they could be completely
removed.

But in reality they should not be removed as your elf-core definitions do not
cover very every target supported by bfd, only some of them.



> +
> +/* Maximum size of the arguments that can be stored in a PRPSINFO
> +   structure.  */
> +
> +#define ELF_PRARGSZ (80)
> +
> +/* Internal structure which holds information to be included in the
> +   PRPSINFO section of the corefile.
> +
> +   This is an "internal" structure in the sense that it should be used to
> +   pass information to BFD (via the `elfcore_write_prpsinfo', for example),
> +   so things like endianess shouldn't be an issue.  This structure will
> +   eventually be converted in one of the `elf_external_*' structures
> +   below.  */
> +
> +struct elf_internal_prpsinfo
> +  {
> +    char pr_state;			/* Numeric process state.  */
> +    char pr_sname;			/* Char for pr_state.  */
> +    char pr_zomb;			/* Zombie.  */
> +    char pr_nice;			/* Nice val.  */
> +    unsigned long pr_flag;		/* Flags.  */
> +    unsigned int pr_uid;
> +    unsigned int pr_gid;
> +    int pr_pid, pr_ppid, pr_pgrp, pr_sid;
> +    /* Lots missing */
> +    char pr_fname[16];			/* Filename of executable.  */
> +    char pr_psargs[ELF_PRARGSZ];	/* Initial part of arg list.  */
> +  };
> +
> +/* External 32-bit structure for PRPSINFO.  This structure is ABI-defined,
> +   thus we choose to use char arrays here in order to avoid dealing with
> +   different types in different architectures.
> +
> +   This structure will ultimately be written in the corefile's note section,
> +   as the PRPSINFO.  */
> +
> +struct elf_external_prpsinfo32
> +  {
> +    char pr_state;			/* Numeric process state.  */
> +    char pr_sname;			/* Char for pr_state.  */
> +    char pr_zomb;			/* Zombie.  */
> +    char pr_nice;			/* Nice val.  */
> +    char pr_flag[4];			/* Flags.  */
> +    char pr_uid[2];
> +    char pr_gid[2];
> +    char pr_pid[4];
> +    char pr_ppid[4];
> +    char pr_pgrp[4];
> +    char pr_sid[4];
> +    /* Lots missing */
> +    char pr_fname[16];			/* Filename of executable.  */
> +    char pr_psargs[ELF_PRARGSZ];	/* Initial part of arg list.  */
> +  };

As it does not match very every arch I believe you should just copy it into
elf*-*.c files containing the *_elf_write_core_note function for archs where
you have verified (by comparing some headers, not just by experiment) it does
match.


> +
> +/* External 32-bit PPC structure for PRPSINFO.

If it is arch-specific then it should not be in a generic file.

I believe elf32-ppc.c and elf64-ppc.c files are fine for it.


In the end I do not see a use for this header file.


> diff --git a/bfd/elf.c b/bfd/elf.c
> index a92dd5d..a39b88c 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -44,6 +44,7 @@ SECTION
>  #include "elf-bfd.h"
>  #include "libiberty.h"
>  #include "safe-ctype.h"
> +#include "elf-psinfo.h"
>  
>  #ifdef CORE_HEADER
>  #include CORE_HEADER
> @@ -8161,13 +8162,6 @@ elfcore_grok_arm_vfp (bfd *abfd, Elf_Internal_Note *note)
>    return elfcore_make_note_pseudosection (abfd, ".reg-arm-vfp", note);
>  }
>  
> -#if defined (HAVE_PRPSINFO_T)
> -typedef prpsinfo_t   elfcore_psinfo_t;
> -#if defined (HAVE_PRPSINFO32_T)		/* Sparc64 cross Sparc32 */
> -typedef prpsinfo32_t elfcore_psinfo32_t;
> -#endif
> -#endif
> -

This is arch-independent file (elf.c).  Some targets are unverified they match
your new ABI elfcore definition.  As they may provide correct system
prpsinfo_t defintion you should use it there, if available.


>  #if defined (HAVE_PSINFO_T)
>  typedef psinfo_t   elfcore_psinfo_t;
>  #if defined (HAVE_PSINFO32_T)		/* Sparc64 cross Sparc32 */
> @@ -8201,17 +8195,17 @@ _bfd_elfcore_strndup (bfd *abfd, char *start, size_t max)
>    return dups;
>  }
>  
> -#if defined (HAVE_PRPSINFO_T) || defined (HAVE_PSINFO_T)

All these changes are like the comment above.


>  static bfd_boolean
>  elfcore_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
>  {
> +#if defined (HAVE_PSINFO_T)
>    if (note->descsz == sizeof (elfcore_psinfo_t))
>      {
>        elfcore_psinfo_t psinfo;
>  
>        memcpy (&psinfo, note->descdata, sizeof (psinfo));
>  
> -#if defined (HAVE_PSINFO_T_PR_PID) || defined (HAVE_PRPSINFO_T_PR_PID)
> +#if defined (HAVE_PSINFO_T_PR_PID)
>        elf_tdata (abfd)->core_pid = psinfo.pr_pid;
>  #endif
>        elf_tdata (abfd)->core_program
> @@ -8222,7 +8216,7 @@ elfcore_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
>  	= _bfd_elfcore_strndup (abfd, psinfo.pr_psargs,
>  				sizeof (psinfo.pr_psargs));
>      }
> -#if defined (HAVE_PRPSINFO32_T) || defined (HAVE_PSINFO32_T)
> +#if defined (HAVE_PSINFO32_T)
>    else if (note->descsz == sizeof (elfcore_psinfo32_t))
>      {
>        /* 64-bit host, 32-bit corefile */
> @@ -8230,7 +8224,7 @@ elfcore_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
>  
>        memcpy (&psinfo, note->descdata, sizeof (psinfo));
>  
> -#if defined (HAVE_PSINFO32_T_PR_PID) || defined (HAVE_PRPSINFO32_T_PR_PID)
> +#if defined (HAVE_PSINFO32_T_PR_PID)
>        elf_tdata (abfd)->core_pid = psinfo.pr_pid;
>  #endif
>        elf_tdata (abfd)->core_program
> @@ -8261,10 +8255,14 @@ elfcore_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
>      if (0 < n && command[n - 1] == ' ')
>        command[n - 1] = '\0';
>    }
> +#else /* defined (HAVE_PSINFO_T) */
> +  /* Avoid compiler warning about "unused variables".  */
> +  (void) abfd;
> +  (void) note;
>  
>    return TRUE;
> +#endif
>  }
> -#endif /* defined (HAVE_PRPSINFO_T) || defined (HAVE_PSINFO_T) */
>  
>  #if defined (HAVE_PSTATUS_T)
>  static bfd_boolean
> @@ -9062,16 +9060,16 @@ char *
>  elfcore_write_prpsinfo (bfd  *abfd,
>  			char *buf,
>  			int  *bufsiz,
> -			const char *fname,
> -			const char *psargs)
> +			const struct elf_internal_prpsinfo *input)
>  {
>    const struct elf_backend_data *bed = get_elf_backend_data (abfd);
>  
>    if (bed->elf_backend_write_core_note != NULL)
>      {
>        char *ret;
> +
>        ret = (*bed->elf_backend_write_core_note) (abfd, buf, bufsiz,
> -						 NT_PRPSINFO, fname, psargs);
> +						 NT_PRPSINFO, input);
>        if (ret != NULL)
>  	return ret;
>      }
> @@ -9089,8 +9087,8 @@ elfcore_write_prpsinfo (bfd  *abfd,
>  #endif
>  
>        memset (&data, 0, sizeof (data));
> -      strncpy (data.pr_fname, fname, sizeof (data.pr_fname));
> -      strncpy (data.pr_psargs, psargs, sizeof (data.pr_psargs));
> +      strncpy (data.pr_fname, input->pr_fname, sizeof (data.pr_fname));
> +      strncpy (data.pr_psargs, input->pr_psargs, sizeof (data.pr_psargs));
>        return elfcore_write_note (abfd, buf, bufsiz,
>  				 "CORE", note_type, &data, sizeof (data));
>      }
> @@ -9101,13 +9099,13 @@ elfcore_write_prpsinfo (bfd  *abfd,
>        psinfo_t data;
>        int note_type = NT_PSINFO;
>  #else
> -      prpsinfo_t data;
> +      prpsinfo64_t data;
>        int note_type = NT_PRPSINFO;
>  #endif
>  
>        memset (&data, 0, sizeof (data));
> -      strncpy (data.pr_fname, fname, sizeof (data.pr_fname));
> -      strncpy (data.pr_psargs, psargs, sizeof (data.pr_psargs));
> +      strncpy (data.pr_fname, input->pr_fname, sizeof (data.pr_fname));
> +      strncpy (data.pr_psargs, input->pr_psargs, sizeof (data.pr_psargs));
>        return elfcore_write_note (abfd, buf, bufsiz,
>  				 "CORE", note_type, &data, sizeof (data));
>      }
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index fd7d26a..cacc895 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -30,6 +30,7 @@
>  #include "elf-nacl.h"
>  #include "elf-vxworks.h"
>  #include "elf/arm.h"
> +#include "elf-psinfo.h"
>  
>  /* Return the relocation section associated with NAME.  HTAB is the
>     bfd's elf32_arm_link_hash_entry.  */
> @@ -2004,17 +2005,19 @@ elf32_arm_nabi_write_core_note (bfd *abfd, char *buf, int *bufsiz,
>  
>      case NT_PRPSINFO:
>        {
> -	char data[124];
> +	const struct elf_internal_prpsinfo *prpsinfo;
> +	struct elf_external_prpsinfo32 data;
>  	va_list ap;
>  
>  	va_start (ap, note_type);
> -	memset (data, 0, sizeof (data));
> -	strncpy (data + 28, va_arg (ap, const char *), 16);
> -	strncpy (data + 44, va_arg (ap, const char *), 80);
> +	prpsinfo = va_arg (ap, const struct elf_internal_prpsinfo *);
>  	va_end (ap);
>  
> +	memset (&data, 0, sizeof (data));
> +	PRPSINFO32_COPY_FIELDS (abfd, prpsinfo, data);
> +
>  	return elfcore_write_note (abfd, buf, bufsiz,
> -				   "CORE", note_type, data, sizeof (data));
> +				   "CORE", note_type, &data, sizeof (data));
>        }
>  
>      case NT_PRSTATUS:
> diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c
> index a188cec..5b81baa 100644
> --- a/bfd/elf32-i386.c
> +++ b/bfd/elf32-i386.c
> @@ -31,12 +31,20 @@
>  #include "objalloc.h"
>  #include "hashtab.h"
>  #include "dwarf2.h"
> +#include "elf-bfd.h"
> +#include "elf-psinfo.h"
> +
> +#include <stdarg.h>
>  
>  /* 386 uses REL relocations instead of RELA.  */
>  #define USE_REL	1
>  
>  #include "elf/i386.h"
>  
> +#ifdef CORE_HEADER
> +#include CORE_HEADER
> +#endif

I believe new CORE_HEADER should not be introduced, they are rather deprecated
as they depend on system <procfs.h> which is not compatible for cross-build
core handling.  (But I may be completely wrong here.)


> +
>  static reloc_howto_type elf_howto_table[]=
>  {
>    HOWTO(R_386_NONE, 0, 0, 0, FALSE, 0, complain_overflow_bitfield,
> @@ -500,6 +508,67 @@ elf_i386_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
>  
>    return TRUE;
>  }
> +
> +static char *
> +elf_i386_write_core_note (bfd *abfd, char *buf, int *bufsiz,
> +			  int note_type, ...)
> +{
> +  const struct elf_backend_data *bed = get_elf_backend_data (abfd);
> +  va_list ap;
> +  const struct elf_internal_prpsinfo *prpsinfo;
> +  long pid;
> +  int cursig;
> +  const void *gregs;
> +  struct elf_external_prpsinfo32 data;
> +
> +  switch (note_type)
> +    {
> +    default:
> +      return NULL;
> +
> +    case NT_PRPSINFO:
> +      va_start (ap, note_type);
> +      prpsinfo = va_arg (ap, const struct elf_internal_prpsinfo *);
> +      va_end (ap);

Maybe to use a union pointer instead?  But nothing serious, fine with va_arg.


> +
> +      memset (&data, 0, sizeof (data));
> +      PRPSINFO32_COPY_FIELDS (abfd, prpsinfo, data);
> +
> +      return elfcore_write_note (abfd, buf, bufsiz, "CORE", note_type,
> +				 &data, sizeof (data));
> +      /* NOTREACHED */


Thanks,
Jan