Re: [PATCH] aarch64: use <sys/user.h> defined register structures

Re: [PATCH] aarch64: use <sys/user.h> defined register structures

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

On Thu, 2014-06-12 at 14:37 +0200, Mark Wielaard wrote:
> On Tue, 2014-06-10 at 09:37 -0400, Kyle McMartin wrote:
> > On Tue, Jun 10, 2014 at 11:51:48AM +0200, Mark Wielaard wrote:
> > > On Mon, 2014-06-09 at 21:06 +0200, Kyle McMartin wrote:
> > > > glibc now supplies these (compatible) structs instead of including the
> > > > kernel's <asm/ptrace.h> header, so let's use them. Annoyingly this will
> > > > cause new elfutils to FTBFS on old glibc, and vice versa, but that seems
> > > > unavoidable in the growth of a new port, and the workaround of checking
> > > > for header defines and defining one to the other seems unpleasant as
> > > > well. Therefore, bite the bullet, and let packaging systems alter their
> > > > build requires accordingly.
> > > 
> > > That is indeed annoying, but using the glibc defined structs seems to be
> > > the right thing to do. Do you know which glibc version introduced them?
> > > 
> > Hrm, looks like it's rawhide churn which has caused this... I guess
> > it'll be in glibc 2.20, but isn't in a released version. I can sit on
> > this and resend this patch when it is, if you'd like?
> 
> I think that would be better. I don't think we'll do the next elfutils
> release before glibc 2.20 is released. But if we do, it would be
> somewhat inconvenient if we relied on an unreleased glibc version.

Meanwhile it became a bit inconvenient because some other distros also
switched to a glibc 2.20-prerelease setup, but stable aarch64 distros
are still on older glibc. I think we just have to add the configure
check. Which I added to your patch.  I tested it against old and new
glibc setups on aarch64. What do you think? Does that look OK?

Thanks,

Mark

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-aarch64-use-sys-user.h-defined-register-structures.patch --]
[-- Type: text/x-patch, Size: 4953 bytes --]

>From 5df2dc63e96808afb1602d4338e30dbca560a656 Mon Sep 17 00:00:00 2001
From: Kyle McMartin <kyle@redhat.com>
Date: Mon, 9 Jun 2014 21:06:26 +0200
Subject: [PATCH] aarch64: use <sys/user.h> defined register structures

glibc now supplies these (compatible) structs instead of including the
kernel's <asm/ptrace.h> header, so let's use them. Annoyingly this will
cause new elfutils to FTBFS on old glibc, and vice versa. So include a
new configure check for the new struct names and use the old ones if
they are not avilable.

Signed-off-by: Kyle McMartin <kyle@redhat.com>
Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 ChangeLog                  |    4 ++++
 backends/ChangeLog         |    9 +++++++++
 backends/aarch64_initreg.c |   11 ++++++++---
 backends/arm_initreg.c     |    6 +++++-
 configure.ac               |   13 +++++++++++++
 5 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b05f5bc..b470b3d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2014-07-18  Mark Wielaard  <mjw@redhat.com>
+
+	* configure.ac (AC_CHECK_TYPE): Test for struct user_regs_struct.
+
 2014-05-26  Mark Wielaard  <mjw@redhat.com>
 
 	* NEWS: New section 0.160. Add unstrip --force.
diff --git a/backends/ChangeLog b/backends/ChangeLog
index d29a80f..a335b20 100644
--- a/backends/ChangeLog
+++ b/backends/ChangeLog
@@ -1,3 +1,12 @@
+2014-07-18  Kyle McMartin  <kyle@redhat.com>
+	    Mark Wielaard  <mjw@redhat.com>
+
+	* aarch64_initreg.c: Check HAVE_SYS_USER_REGS.
+	(aarch64_set_initial_registers_tid): Use user_regs_struct and
+	user_fpsimd_struct.
+	* arm_initreg.c: Check HAVE_SYS_USER_REGS.
+	(arm_set_initial_registers_tid): Use user_regs_struct in compat mode.
+
 2014-07-04  Menanteau Guy  <menantea@linux.vnet.ibm.com>
 	    Mark Wielaard  <mjw@redhat.com>
 
diff --git a/backends/aarch64_initreg.c b/backends/aarch64_initreg.c
index 2492d56..9706205 100644
--- a/backends/aarch64_initreg.c
+++ b/backends/aarch64_initreg.c
@@ -1,5 +1,5 @@
 /* Fetch live process registers from TID.
-   Copyright (C) 2013 Red Hat, Inc.
+   Copyright (C) 2013, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -36,6 +36,11 @@
 # include <linux/uio.h>
 # include <sys/user.h>
 # include <sys/ptrace.h>
+/* Deal with old glibc defining user_pt_regs instead of user_regs_struct.  */
+# ifndef HAVE_SYS_USER_REGS
+#  define user_regs_struct user_pt_regs
+#  define user_fpsimd_struct user_fpsimd_state
+# endif
 #endif
 
 #define BACKEND aarch64_
@@ -51,7 +56,7 @@ aarch64_set_initial_registers_tid (pid_t tid __attribute__ ((unused)),
 #else /* __aarch64__ */
 
   /* General registers.  */
-  struct user_pt_regs gregs;
+  struct user_regs_struct gregs;
   struct iovec iovec;
   iovec.iov_base = &gregs;
   iovec.iov_len = sizeof (gregs);
@@ -69,7 +74,7 @@ aarch64_set_initial_registers_tid (pid_t tid __attribute__ ((unused)),
   /* ELR cannot be found.  */
 
   /* FP registers (only 64bits are used).  */
-  struct user_fpsimd_state fregs;
+  struct user_fpsimd_struct fregs;
   iovec.iov_base = &fregs;
   iovec.iov_len = sizeof (fregs);
   if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET, &iovec) != 0)
diff --git a/backends/arm_initreg.c b/backends/arm_initreg.c
index 5837383..a0a9be9 100644
--- a/backends/arm_initreg.c
+++ b/backends/arm_initreg.c
@@ -40,6 +40,10 @@
 # include <linux/uio.h>
 # include <sys/user.h>
 # include <sys/ptrace.h>
+/* Deal with old glibc defining user_pt_regs instead of user_regs_struct.  */
+# ifndef HAVE_SYS_USER_REGS
+#  define user_regs_struct user_pt_regs
+# endif
 #endif
 
 #define BACKEND arm_
@@ -67,7 +71,7 @@ arm_set_initial_registers_tid (pid_t tid __attribute__ ((unused)),
 #elif defined __aarch64__
   /* Compat mode: arm compatible code running on aarch64 */
   int i;
-  struct user_pt_regs gregs;
+  struct user_regs_struct gregs;
   struct iovec iovec;
   iovec.iov_base = &gregs;
   iovec.iov_len = sizeof (gregs);
diff --git a/configure.ac b/configure.ac
index 1d79597..f9c3c30 100644
--- a/configure.ac
+++ b/configure.ac
@@ -301,6 +301,19 @@ eu_version=$(( (eu_version + 999) / 1000 ))
 
 AC_CHECK_SIZEOF(long)
 
+# On aarch64 before glibc 2.20 we would get the kernel user_pt_regs instead
+# of the user_regs_struct from sys/user.h. They are structurally the same
+# but we get either one or the other.
+AC_CHECK_TYPE([struct user_regs_struct],
+              [sys_user_has_user_regs=yes], [sys_user_has_user_regs=no],
+              [[#include <sys/ptrace.h>]
+               [#include <sys/time.h>]
+               [#include <sys/user.h>]])
+if test "$sys_user_has_user_regs" = "yes"; then
+  AC_DEFINE(HAVE_SYS_USER_REGS, 1,
+            [Define to 1 if <sys/user.h> defines struct user_regs_struct])
+fi
+
 # On a 64-bit host where can can use $CC -m32, we'll run two sets of tests.
 # Likewise in a 32-bit build on a host where $CC -m64 works.
 utrace_BIARCH
-- 
1.7.1

Re: [PATCH] aarch64: use <sys/user.h> defined register structures

[Kyle McMartin]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Fri, Aug 01, 2014 at 12:42:37PM +0200, Mark Wielaard wrote:
> > From 5df2dc63e96808afb1602d4338e30dbca560a656 Mon Sep 17 00:00:00 2001
> > From: Kyle McMartin <kyle@redhat.com>
> > Date: Mon, 9 Jun 2014 21:06:26 +0200
> > Subject: [PATCH] aarch64: use <sys/user.h> defined register structures
> > 
> > glibc now supplies these (compatible) structs instead of including the
> > kernel's <asm/ptrace.h> header, so let's use them. Annoyingly this will
> > cause new elfutils to FTBFS on old glibc, and vice versa. So include a
> > new configure check for the new struct names and use the old ones if
> > they are not avilable.
> > 
> > Signed-off-by: Kyle McMartin <kyle@redhat.com>
> > Signed-off-by: Mark Wielaard <mjw@redhat.com>
> 
> It seems to do the right thing with old/new aarch64 glibc so I pushed
> this to master now.
> 

awesome, thanks very much mark, sorry for the radio silence.

--kyle

> Cheers,
> 
> Mark

Re: [PATCH] aarch64: use <sys/user.h> defined register structures

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

> From 5df2dc63e96808afb1602d4338e30dbca560a656 Mon Sep 17 00:00:00 2001
> From: Kyle McMartin <kyle@redhat.com>
> Date: Mon, 9 Jun 2014 21:06:26 +0200
> Subject: [PATCH] aarch64: use <sys/user.h> defined register structures
> 
> glibc now supplies these (compatible) structs instead of including the
> kernel's <asm/ptrace.h> header, so let's use them. Annoyingly this will
> cause new elfutils to FTBFS on old glibc, and vice versa. So include a
> new configure check for the new struct names and use the old ones if
> they are not avilable.
> 
> Signed-off-by: Kyle McMartin <kyle@redhat.com>
> Signed-off-by: Mark Wielaard <mjw@redhat.com>

It seems to do the right thing with old/new aarch64 glibc so I pushed
this to master now.

Cheers,

Mark

Re: [PATCH] aarch64: use <sys/user.h> defined register structures

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On Tue, 2014-06-10 at 09:37 -0400, Kyle McMartin wrote:
> On Tue, Jun 10, 2014 at 11:51:48AM +0200, Mark Wielaard wrote:
> > On Mon, 2014-06-09 at 21:06 +0200, Kyle McMartin wrote:
> > > glibc now supplies these (compatible) structs instead of including the
> > > kernel's <asm/ptrace.h> header, so let's use them. Annoyingly this will
> > > cause new elfutils to FTBFS on old glibc, and vice versa, but that seems
> > > unavoidable in the growth of a new port, and the workaround of checking
> > > for header defines and defining one to the other seems unpleasant as
> > > well. Therefore, bite the bullet, and let packaging systems alter their
> > > build requires accordingly.
> > 
> > That is indeed annoying, but using the glibc defined structs seems to be
> > the right thing to do. Do you know which glibc version introduced them?
> > 
> Hrm, looks like it's rawhide churn which has caused this... I guess
> it'll be in glibc 2.20, but isn't in a released version. I can sit on
> this and resend this patch when it is, if you'd like?

I think that would be better. I don't think we'll do the next elfutils
release before glibc 2.20 is released. But if we do, it would be
somewhat inconvenient if we relied on an unreleased glibc version.

I'll probably remember because I see the patch is in fedora rawhide now
and when releasing I make sure all fedora distro patches have been
properly upstreamed. But if you could remind me when glibc 2.20 is
released that would be appreciated.

Thanks,

Mark

Re: [PATCH] aarch64: use <sys/user.h> defined register structures

[Kyle McMartin]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

On Tue, Jun 10, 2014 at 11:51:48AM +0200, Mark Wielaard wrote:
> On Mon, 2014-06-09 at 21:06 +0200, Kyle McMartin wrote:
> > glibc now supplies these (compatible) structs instead of including the
> > kernel's <asm/ptrace.h> header, so let's use them. Annoyingly this will
> > cause new elfutils to FTBFS on old glibc, and vice versa, but that seems
> > unavoidable in the growth of a new port, and the workaround of checking
> > for header defines and defining one to the other seems unpleasant as
> > well. Therefore, bite the bullet, and let packaging systems alter their
> > build requires accordingly.
> 
> That is indeed annoying, but using the glibc defined structs seems to be
> the right thing to do. Do you know which glibc version introduced them?
> 

Hrm, looks like it's rawhide churn which has caused this... I guess
it'll be in glibc 2.20, but isn't in a released version. I can sit on
this and resend this patch when it is, if you'd like?

> BTW. For this trivial patch it doesn't really matter, but in general we
> like to have a Signed-off-by line for all patches to indicate
> contributors know about and agree with our CONTRIBUTING guidelines.
> Would you mind adding one?
> https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING
> 

Sure, I'll add this to the v2 for the next glibc release? Otherwise
adding
Signed-off-by: Kyle McMartin <kyle@redhat.com>
is fine by me.

regards, Kyle

> Thanks,
> 
> Mark
> 

Re: [PATCH] aarch64: use <sys/user.h> defined register structures

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On Mon, 2014-06-09 at 21:06 +0200, Kyle McMartin wrote:
> glibc now supplies these (compatible) structs instead of including the
> kernel's <asm/ptrace.h> header, so let's use them. Annoyingly this will
> cause new elfutils to FTBFS on old glibc, and vice versa, but that seems
> unavoidable in the growth of a new port, and the workaround of checking
> for header defines and defining one to the other seems unpleasant as
> well. Therefore, bite the bullet, and let packaging systems alter their
> build requires accordingly.

That is indeed annoying, but using the glibc defined structs seems to be
the right thing to do. Do you know which glibc version introduced them?

BTW. For this trivial patch it doesn't really matter, but in general we
like to have a Signed-off-by line for all patches to indicate
contributors know about and agree with our CONTRIBUTING guidelines.
Would you mind adding one?
https://git.fedorahosted.org/cgit/elfutils.git/tree/CONTRIBUTING

Thanks,

Mark

[PATCH] aarch64: use <sys/user.h> defined register structures

[Kyle McMartin]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

glibc now supplies these (compatible) structs instead of including the
kernel's <asm/ptrace.h> header, so let's use them. Annoyingly this will
cause new elfutils to FTBFS on old glibc, and vice versa, but that seems
unavoidable in the growth of a new port, and the workaround of checking
for header defines and defining one to the other seems unpleasant as
well. Therefore, bite the bullet, and let packaging systems alter their
build requires accordingly.

--- a/backends/aarch64_initreg.c
+++ b/backends/aarch64_initreg.c
@@ -51,7 +51,7 @@ aarch64_set_initial_registers_tid (pid_t tid __attribute__ ((unused)),
 #else /* __aarch64__ */
 
   /* General registers.  */
-  struct user_pt_regs gregs;
+  struct user_regs_struct gregs;
   struct iovec iovec;
   iovec.iov_base = &gregs;
   iovec.iov_len = sizeof (gregs);
@@ -69,7 +69,7 @@ aarch64_set_initial_registers_tid (pid_t tid __attribute__ ((unused)),
   /* ELR cannot be found.  */
 
   /* FP registers (only 64bits are used).  */
-  struct user_fpsimd_state fregs;
+  struct user_fpsimd_struct fregs;
   iovec.iov_base = &fregs;
   iovec.iov_len = sizeof (fregs);
   if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET, &iovec) != 0)
diff --git a/backends/arm_initreg.c b/backends/arm_initreg.c
index 5837383..1edf62b 100644
--- a/backends/arm_initreg.c
+++ b/backends/arm_initreg.c
@@ -67,7 +67,7 @@ arm_set_initial_registers_tid (pid_t tid __attribute__ ((unused)),
 #elif defined __aarch64__
   /* Compat mode: arm compatible code running on aarch64 */
   int i;
-  struct user_pt_regs gregs;
+  struct user_regs_struct gregs;
   struct iovec iovec;
   iovec.iov_base = &gregs;
   iovec.iov_len = sizeof (gregs);