[COMMITTED] backends: Swap sys/ptrace.h and asm/ptrace.h include order on s390.

[COMMITTED] backends: Swap sys/ptrace.h and asm/ptrace.h include order on s390.

[Mark Wielaard]

glibc 2.26 changed the sys/ptrace.h header so that it cannot be included
after asm/ptrace.h. We still need to include the kernel asm/ptrace.h for
the ptrace_area definition. Including it after sys/ptrace.h works against
both old and new glibc.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 backends/ChangeLog      | 4 ++++
 backends/s390_initreg.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/backends/ChangeLog b/backends/ChangeLog
index c5f61e8..d628245 100644
--- a/backends/ChangeLog
+++ b/backends/ChangeLog
@@ -1,3 +1,7 @@
+2017-06-17  Mark Wielaard  <mark@klomp.org>
+
+	* s390_initreg.c: Swap sys/ptrace.h and asm/ptrace.h include order.
+
 2017-06-15  Andreas Schwab  <schwab@linux-m68k.org>
 
 	* ppc_symbol.c (ppc_machine_flag_check): New function.
diff --git a/backends/s390_initreg.c b/backends/s390_initreg.c
index 011305c..23bf8ed 100644
--- a/backends/s390_initreg.c
+++ b/backends/s390_initreg.c
@@ -34,8 +34,8 @@
 #include <assert.h>
 #if defined(__s390__) && defined(__linux__)
 # include <sys/user.h>
-# include <asm/ptrace.h>
 # include <sys/ptrace.h>
+# include <asm/ptrace.h>
 #endif
 
 #define BACKEND s390_
-- 
1.8.3.1

Re: [COMMITTED] backends: Swap sys/ptrace.h and asm/ptrace.h include order on s390.

[Dmitry V. Levin]

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

On Mon, Jul 17, 2017 at 05:44:54PM +0200, Mark Wielaard wrote:
> glibc 2.26 changed the sys/ptrace.h header so that it cannot be included
> after asm/ptrace.h. We still need to include the kernel asm/ptrace.h for
> the ptrace_area definition. Including it after sys/ptrace.h works against
> both old and new glibc.

If it's a glibc regression, shouldn't it be fixed on glibc side before
2.26 is out?


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [COMMITTED] backends: Swap sys/ptrace.h and asm/ptrace.h include order on s390.

[Mark Wielaard]

On Mon, 2017-07-17 at 19:29 +0300, Dmitry V. Levin wrote:
> On Mon, Jul 17, 2017 at 05:44:54PM +0200, Mark Wielaard wrote:
> > glibc 2.26 changed the sys/ptrace.h header so that it cannot be included
> > after asm/ptrace.h. We still need to include the kernel asm/ptrace.h for
> > the ptrace_area definition. Including it after sys/ptrace.h works against
> > both old and new glibc.
> 
> If it's a glibc regression, shouldn't it be fixed on glibc side before
> 2.26 is out?

I asked and it was done deliberately. See glibc 2.26 NEWS under
Deprecated and removed features, and other changes affecting
compatibility.

For the functionality we needed it was always necessary to include the
kernel asm/ptrace.h also (and it still is with 2.26) on s390. The only
regression (for us) is that the order of the includes needs to be
sys/ptrace.h first, then asm/ptrace.h. I double checked that the
functionality needed still works (the run-backtrace-native.sh testcase
works on both old s390 and s390x and new glibc s390x versions).

Cheers,

Mark

Re: [COMMITTED] backends: Swap sys/ptrace.h and asm/ptrace.h include order on s390.

[Dmitry V. Levin]

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

On Mon, Jul 17, 2017 at 06:41:47PM +0200, Mark Wielaard wrote:
> On Mon, 2017-07-17 at 19:29 +0300, Dmitry V. Levin wrote:
> > On Mon, Jul 17, 2017 at 05:44:54PM +0200, Mark Wielaard wrote:
> > > glibc 2.26 changed the sys/ptrace.h header so that it cannot be included
> > > after asm/ptrace.h. We still need to include the kernel asm/ptrace.h for
> > > the ptrace_area definition. Including it after sys/ptrace.h works against
> > > both old and new glibc.
> > 
> > If it's a glibc regression, shouldn't it be fixed on glibc side before
> > 2.26 is out?
> 
> I asked and it was done deliberately. See glibc 2.26 NEWS under
> Deprecated and removed features, and other changes affecting
> compatibility.

There are exactly two commits in glibc since 2.25 that changed
sysdeps/unix/sysv/linux/s390/sys/ptrace.h:
3f67d1a7021ed3184830511636a0867faec730fe and
b08a6a0dea63742313ed3d9577c1e2d83436b196.

I reviewed and approved both of these commits assuming that they brought
no regressions.  If sys/ptrace.h from glibc 2.25 could be included before
or after linux/ptrace.h, this shouldn't have changed in glibc 2.26.

In other words, I think you've spotted a regression that I missed during
b08a6a0dea63742313ed3d9577c1e2d83436b196 review and that has to be fixed
in glibc before 2.26 is released.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [COMMITTED] backends: Swap sys/ptrace.h and asm/ptrace.h include order on s390.

[Mark Wielaard]

On Tue, Jul 18, 2017 at 01:24:46AM +0300, Dmitry V. Levin wrote:
> I reviewed and approved both of these commits assuming that they brought
> no regressions.  If sys/ptrace.h from glibc 2.25 could be included before
> or after linux/ptrace.h, this shouldn't have changed in glibc 2.26.
> 
> In other words, I think you've spotted a regression that I missed during
> b08a6a0dea63742313ed3d9577c1e2d83436b196 review and that has to be fixed
> in glibc before 2.26 is released.

Aha. So if you look at that patch you see that the same include pattern
that got us in trouble was removed from sysdeps/s390/fpu/fesetenv.c and
the new order is used in the newly added testcase
sysdeps/unix/sysv/linux/s390/tst-ptrace-singleblock.c.

On fedora s390x rawhide (which has a glibc 2.16 pre-release)
including asm/ptrace.h before sys/ptrace.h will cause errors:
https://kojipkgs.fedoraproject.org//work/tasks/4924/20574924/build.log

Cheers,

Mark