[patch] Implement reglocs for s390/s390x

Roland McGrath roland at hack.frob.com
Wed Oct 10 20:44:31 UTC 2012


> It is there for special handling of ./tests/run-readelf-vmcoreinfo.sh .
> So I put there 'offset == 0' which is still compatible with VMCOREINFO and
> removed the unclean previous fix.

Sorry if I'm being dense.  But I think we still need some more explanation
about this, and should make it clear in comments.  (Of course it's my fault
for not doing this five years ago, but I don't have time to puzzle through
it now myself.)  The comment shouldn't say it's a special case for a
particular note type.  If anything, say it's a special case for '\n'
format.  But I'd still like first to see some more thorough explanation
about what the special case actually does differently than the general
case.

Incidentally, it looks to me like a '\n' item with descsz==0 will crash
(assert) since Petr's change.  Can you verify that and fix it?  Probably
handle_core_items should just bail out at the top for descsz==0.

I think any readelf changes we settle on should be done in a separate
change before the backend additions.

> I have both fixed it and reworded it, 32-bit s390 has unusual 8 bytes
> alignment.  This fixed displaying the pr_fpvalid field.

I see.  

> --- a/backends/linux-core-note.c
> +++ b/backends/linux-core-note.c
> @@ -83,6 +83,9 @@ struct EBLHOOK(prstatus)
>    struct EBLHOOK(timeval) pr_cstime;
>    FIELD (ULONG, pr_reg[PRSTATUS_REGS_SIZE / sizeof (ULONG)]);
>    FIELD (INT, pr_fpvalid);
> +#ifdef ALIGN_STRUCT
> +  char alignment[] __attribute__ ((aligned (ALIGN_STRUCT)));
> +#endif
>  };

The aligned attribute should be on the pr_reg field, since that's
what actually has that alignment requirement.  What I'd do is:

#ifndef PR_REG_TYPE
# define PR_REG_TYPE		ULONG
#endif
#ifndef ALIGN_PR_REG_TYPE
# define ALIGN_PR_REG_TYPE	ALIGN_ULONG
#endif

and use PR_REG_TYPE rather than ULONG in the pr_reg field declaration.

Then in s390_corenote.c you can do:

#define ALIGN_PR_REG_TYPE	8

and be done.

> +#ifndef BITS
> +# define BITS 		32
> +# define BACKEND	s390_
> +#else
> +# define BITS 		64
> +# define BACKEND	s390x_
> +#endif

#ifdef BITS
# define BITS

is odd.  But I see you were following the model of the existing ones.
So I guess it's fine that way.

> +static const Ebl_Core_Item fpregset_items[] =
> +  {
> +    {
> +      .name = "fpc", .group = "register",
> +      .offset = 0,
> +      .count = 0, .type = ELF_T_WORD,
> +      .format = 'x',
> +    },
> +  };

It's weird to use .count = 0 to mean .count = 1.
I know arm_corenote.c does that, but I don't know why.
It's more normal just to omit .count entirely so it's implicitly zero.

> +      .name = "high_r" #n , .group = "register", .offset = (n) * 4,	\
> +      .count = 0, .type = ELF_T_WORD, .format = 'x',			\

Omit .count here too.

> +    HR (0), HR (1), HR (2), HR (3), HR (4), HR (5), HR (6), HR (7), HR (8),
> +    HR (9), HR (10), HR (11), HR (12), HR (13), HR (14), HR (15)

Split the line symmetrically so there are eight items on each line.


Thanks,
Roland


More information about the elfutils-devel mailing list