This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Implement reglocs for s390/s390x


> TBH I do not know what S390_HIGH_GPRS are for.  From what I read they are
> not present on s390 iron, they are also not present for s390x processes,
> they are present only for s390 processes on s390x iron/kernel.  But gcc
> -m31 still does not use the upper halves of r0-r15 there.  I could ask at
> IBM but so far I ignore S390_HIGH_GPRS in this and further patches.  They
> are now just printed by readelf, nothing more.

You should check with some s390 folks.  My understanding is that 31-bit
processes on 64-bit kernels are free to use the full 64-bit registers.
There is an HWCAP bit that says this is available.  glibc has
IFUNC-selected assembly implementations of mem* functions that use these.
So it is important that debuggers and such can see these values.

I don't know what the story is on DWARF register number mapping for those.
i.e., if there are separate DWARF register numbers assigned for the high
halves or what.

> I also do not understand much what is there the exception <nitems == 1> in
> readelf.c but it crashes without the fix for s390 core file S390_LAST_BREAK.

I don't recall enough any more about this.  Let's get some clarity before
changing it.  I did it for some reason.  So let's figure out what core file
input produces different -n output with and without this code, and then add
some comments and go from there on exactly how it should be.

> 2012-10-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* Makefile.am (s390_SRCS): Add s390_corenote.c and s390x_corenote.c.
> 	* s390_corenote.c: new file.
> 	* s390_init.c (s390_init): Install s390x_core_note and s390_core_note.
> 	* s390x_corenote.c: new file.

Capitalize "New file".

> +static const Ebl_Register_Location prstatus_regs[] =
> +  {
> +#define GR(at, n, dwreg)						\
> +    { .offset = at * BITS/8, .regno = dwreg, .count = n, .bits = BITS }
> +
> +    GR (0,  1, 64),		/* pswm */
> +    GR (1,  1, 65),		/* pswa */
> +    GR (2, 16,  0),		/* r0-r15 */

Put a blank line here.

> +    /* ar0-r15 */
> +    { .offset = 18 * BITS/8, .regno = 48, .count = 16, .bits = 32 }
> +    /* ar15 end is at "offset" (BITS == 32 ? 18 + 16 == 34 : 18 + 16 / 2 == 26).
> +       orig_r2 is at "offset" (BITS == 32 ? 34 : 26). */

I'm not sure I understand this comment or why it's here.

> +#define PRSTATUS_REGS_SIZE	(BITS / 8 * (BITS == 32 ? 37 : 27))

I don't understand this difference.  What are the 10 extra words for 32-bit?

> +static const Ebl_Register_Location fpregset_regs[] =
> +  {
> +#define FPR(at, n, dwreg)						\
> +    { .offset = at * 64/8, .regno = dwreg, .count = n, .bits = 64 }
> +
> +    /* fpc is at 0.  */

So there is no DWARF number for fpc?  Then say so in the comment.
And there should be an extra Ebl_Core_Item describing fpc.

> +#define PRSTATUS_REGSET_ITEMS					\
> +  {								\
> +    .name = "orig_r2", .type = TYPE_LONG, .format = 'd',	\
> +    .offset = offsetof (struct EBLHOOK(prstatus),		\
> +    pr_reg[BITS == 32 ? 34 : 26]), .group = "register"		\

Bad indentation here.  It can be:

#define PRSTATUS_REGSET_ITEMS						      \
  {									      \
    .name = "orig_r2", .type = TYPE_LONG, .format = 'd',		      \
    .offset = offsetof (struct EBLHOOK(prstatus),			      \
			pr_reg[BITS == 32 ? 34 : 26]),			      \
    .group = "register"							      \
  }

> +static const Ebl_Core_Item no_items[0];

Bletch!  You don't need this.  
Just use EXTRA_REGSET instead of EXTRA_REGSET_ITEMS.

> +static const Ebl_Register_Location high_regs[] =
> +  {
> +    /* Upper halves of r0-r15 are stored here.
> +       FIXME: It is currently not combined with the r0-r15 lower halves.  */
> +    { .offset = 0, .regno = 0, .count = 16, .bits = 32 },
> +  };

This can't be right, because those DWARF register numbers are already
taken.  If they don't have their own register numbers, then there are two
possibilities:

1. We define a first-class concept for split registers, so something
   generically knows how to combine upper and lower halves.
2. We just treat these as non-register values, using Ebl_Core_Item so
   readelf will print them separately but nothing will know how they
   correspond to any DWARF-described registers.

#2 is easy.  #1 would need to be designed.

> +static const Ebl_Core_Item last_break_items[] =
> +  {
> +    {
> +      .name = "last_break", .group = "last_break",
> +      .offset = BITS == 32 ? 4 : 0,
> +      .count = 0, .type = BITS == 32 ? ELF_T_WORD : ELF_T_XWORD,
> +      .format = 'x',
> +    },
> +  };
> +
> +static const Ebl_Core_Item system_call_items[] =
> +  {
> +    {
> +      .name = "system_call", .group = "system_call",
> +      .offset = 0,
> +      .count = 0, .type = ELF_T_WORD,
> +      .format = 'x',
> +    },
> +  };

The groups are pretty useless if everything has its own group.
I'd put these two together in a group called "system" or something.

> +static const Ebl_Register_Location no_regs[0];

Bletch again.  Just add an EXTRA_ITEMS macro to linux-core-note.c.

> +  if (eh->class == ELFCLASS64)
> +    {
> +      __typeof (s390_core_note) s390x_core_note;
> +      eh->core_note = s390x_core_note;

It would be better to avoid a local declaration like this.
But if it's easiest to have it, at least put extern on it.
It would look better to do this declaration at top level and
have a one-line if body.


Thanks,
Roland

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]