This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [patch] Implement reglocs for s390/s390x
- From: Roland McGrath <roland at hack dot frob dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Tue, 09 Oct 2012 11:53:48 -0700
- Subject: 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