Re: [RFC] Refactor elf_prpsinfo{32,64} structures

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, November 16 2012, Pedro Alves wrote:

> On 16-11-2012 04:18, Sergio Durigan Junior wrote:
>
>> +/* Unsigned 64-bit integer aligned to 8 bytes.  */
>> +typedef uint64_t __attribute__ ((__aligned__ (8))) a8_uint64_t;
>
> So this has been moved to what should be host-independent code;
> not sure using attribute aligned is kosher.

All right.  Given your suggestion below, this can be removed.

>> +#define PRPSINFO64_FIELD_SIZE(field) sizeof (((prpsinfo64_t *) 0)->field)
>> +
>> +struct elf_prpsinfo64
>> +  {
>> +    int8_t pr_state;			/* Numeric process state.  */
>> +    char pr_sname;			/* Char for pr_state.  */
>> +    int8_t pr_zomb;			/* Zombie.  */
>> +    int8_t pr_nice;			/* Nice val.  */
>> +    a8_uint64_t pr_flag;		/* Flags.  */
>> +    uint32_t pr_uid;
>> +    uint32_t pr_gid;
>> +    int32_t 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.  */
>> +  };
>> +
>> +/* Process status and info.  In the end we do provide typedefs for them.  */
>> +typedef struct elf_prpsinfo32 prpsinfo32_t;
>> +typedef struct elf_prpsinfo64 prpsinfo64_t;
>
> So these are the external representations of these types, right?

Yes.

> The BFD way to handle this is I think to define the external types's
> fields as char arrays.  So you'd have something like:
>
> struct elf_external_prpsinfo64
>   {
>     char pr_state;			/* Numeric process state.  */
>     char pr_sname;			/* Char for pr_state.  */
>     char pr_zomb;			/* Zombie.  */
>     char pr_nice;			/* Nice val.  */
>     char gap[4];                        /* alignment gap */
>     char pr_flag[8];		/* Flags.  */
>     char pr_uid[8];
>     char pr_gid[8];
>     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.  */
>   } ATTRIBUTE_PACKED;
>
> Dunno if the gap is always present on all targets or not.  See below.

Hm, good to know.

>> +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;
>> +  char data[sizeof (prpsinfo32_t)];
>> +  char *p = 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);
>> +
>> +      memset (data, 0, sizeof (data));
>> +
>> +      memcpy (p, &prpsinfo->pr_state, PRPSINFO32_FIELD_SIZE (pr_state));
>> +      p += PRPSINFO32_FIELD_SIZE (pr_state);
>
> here this doesn't handle the case of different host and target
> endianness.

Yes...

> I suggest looking at how this is usually handled in the swapping routines,
> like e.g., elf_swap_ehdr_out (note how that's actually "templated" code - don't
> know if it makes sense to do something similar for these structures).
> Going the swap way, you'd write something like
>
>   H_PUT_32 (abfd, prpsinfo->pr_uid, dst->pr_uid);
>
> etc. which handles the memcpy+endianness in one go, and you don't have
> to care here for advancing a pointer, and caring for the alignment gap
> in 64-bit here, since that'd be embedded in the external struct itself instead.

Hm, interesting, I was wondering whether BFD had such convenience
functions (just like extract_unsigned_integer in GDB).

Well, so the bottom line is that if I use such, I could declare the
external structure above without the alignment gap, right?  That would
certainly make things easier.

I will rewrite the patch and resubmit it, thanks.

-- 
Sergio