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

[Pedro Alves <palves@redhat.com>]

On 01/04/2013 04:39 AM, Sergio Durigan Junior wrote:> +++ b/bfd/elf-psinfo.h
> @@ -0,0 +1,124 @@
> +/* Definitions for PRPSINFO structures under ELF on GNU/Linux.

What about other OSs, like e.g., Solaris?

> +/* External 64-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.
> +
> +   Differently from the 32-bit version, the PowerPC guys made our lives better
> +   and used the same size as the other architectures.

In the current revision, this comment appears out of the blue.
Better would be to instead add a comment further up, above the
32-bit version stating that these are used by most architectures,
although some define their own versions (like e.g., PPC).

> +
> +   This structure will ultimately be written in the corefile's note section,
> +   as the PRPSINFO.  */
> +
> +struct elf_external_prpsinfo64

> +#define PRPSINFO32_COPY_FIELDS(abfd, from, to) \
> +  do \

Personally, I'd prefer calling these SWAP instead
of COPY, as you're not really doing straight
copying, and "swap" is what bfd calls this idiom
about everywhere.

...

> +/* Process info.  In the end we do provide typedefs for them.  */
> +
> +typedef struct elf_external_prpsinfo32 prpsinfo32;
> +typedef struct elf_external_prpsinfo64 prpsinfo64;

What's the point of these typedefs?  To me, they just
obscure things, and are easily confused with the host
types...

> @@ -8165,7 +8166,7 @@ elfcore_grok_arm_vfp (bfd *abfd, Elf_Internal_Note *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;
> +typedef prpsinfo32 elfcore_psinfo32_t;

... like here.  HAVE_PRPSINFO32_T/prpsinfo32_t were
about a host type.  prpsinfo32 is always defined (it's a
bfd type).  This in fact looks suspiciously wrong, and a
potential breakage if in fact Sparc32 has a different
psinfo32_t (what OS was that?), or in the other spots,
some other arcane port/OS/arch having a different psinfo32_t
than the Linux one.  Either the #ifdef can be removed and
this made unconditional, and host independent (the best),
or the type should remain the host type (until all ports
convert to implement elf_backend_write_core_note&co
host-independent hooks).

>  #endif
>  #endif
>

> +   The reason why we have a different structure only for PPC is because
> +   on this architecture (and *only* here!) the size of 32-bit structure

This "only here" bit of the comment should go away, because
it'll generate confusion whenever some other arch needs
its own structure, making this outdated, because nobody
will remember to update it.


> @@ -415,14 +417,13 @@ elf_x86_64_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
>    return TRUE;
>  }
>  
> -#ifdef CORE_HEADER
>  static char *
>  elf_x86_64_write_core_note (bfd *abfd, char *buf, int *bufsiz,
>  			    int note_type, ...)
>  {
...
>      case NT_PRSTATUS:
> +#ifdef CORE_HEADER
>        va_start (ap, note_type);
>        pid = va_arg (ap, long);
>        cursig = va_arg (ap, int);
> @@ -498,10 +501,13 @@ elf_x86_64_write_core_note (bfd *abfd, char *buf, int *bufsiz,
>  	  return elfcore_write_note (abfd, buf, bufsiz, "CORE", note_type,
>  				     &prstat, sizeof (prstat));
>  	}
> +#else
> +      return NULL;
> +#endif /* CORE_HEADER */
>      }
>    /* NOTREACHED */
>  }
> -#endif
> +

Do you plan on following up with similar treatment for prstatus?

Thanks for doing this,
-- 
Pedro Alves