diff mbox

S390: Sync ptrace.h with kernel. [BZ #21539]

Message ID 20170718102039.GA20971@altlinux.org
State New, archived
Headers

Commit Message

Dmitry V. Levin July 18, 2017, 10:20 a.m. UTC 
  On Mon, Jun 19, 2017 at 03:10:57PM +0200, Stefan Liebler wrote:
[...]
> --- a/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
> +++ b/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
> @@ -89,25 +89,9 @@ enum __ptrace_request
>    PTRACE_SINGLESTEP = 9,
>  #define PT_STEP PTRACE_SINGLESTEP
>  
> -  /* Get all general purpose registers used by a processes.
> -     This is not supported on all machines.  */
> -   PTRACE_GETREGS = 12,
> -#define PT_GETREGS PTRACE_GETREGS
> -
> -  /* Set all general purpose registers used by a processes.
> -     This is not supported on all machines.  */
> -   PTRACE_SETREGS = 13,
> -#define PT_SETREGS PTRACE_SETREGS
> -
> -  /* Get all floating point registers used by a processes.
> -     This is not supported on all machines.  */
> -   PTRACE_GETFPREGS = 14,
> -#define PT_GETFPREGS PTRACE_GETFPREGS
> -
> -  /* Set all floating point registers used by a processes.
> -     This is not supported on all machines.  */
> -   PTRACE_SETFPREGS = 15,
> -#define PT_SETFPREGS PTRACE_SETFPREGS
> +  /* Execute process until next taken branch.  */
> +  PTRACE_SINGLEBLOCK = 12,
> +#define PT_STEPBLOCK PTRACE_SINGLEBLOCK
>  
>    /* Attach to a process that is already running. */
>    PTRACE_ATTACH = 16,
> @@ -167,8 +151,26 @@ enum __ptrace_request
>    PTRACE_SETSIGMASK = 0x420b,
>  #define PTRACE_SETSIGMASK PTRACE_SETSIGMASK
>  
> -  PTRACE_SECCOMP_GET_FILTER = 0x420c
> +  PTRACE_SECCOMP_GET_FILTER = 0x420c,
>  #define PTRACE_SECCOMP_GET_FILTER PTRACE_SECCOMP_GET_FILTER
> +
> +  PTRACE_PEEKUSR_AREA = 0x5000,
> +#define PTRACE_PEEKUSR_AREA PTRACE_PEEKUSR_AREA
> +
> +  PTRACE_POKEUSR_AREA = 0x5001,
> +#define PTRACE_POKEUSR_AREA PTRACE_POKEUSR_AREA
> +
> +  PTRACE_GET_LAST_BREAK = 0x5006,
> +#define PTRACE_GET_LAST_BREAK PTRACE_GET_LAST_BREAK
> +
> +  PTRACE_ENABLE_TE = 0x5009,
> +#define PTRACE_ENABLE_TE PTRACE_ENABLE_TE
> +
> +  PTRACE_DISABLE_TE = 0x5010,
> +#define PTRACE_DISABLE_TE PTRACE_DISABLE_TE
> +
> +  PTRACE_TE_ABORT_RAND = 0x5011
> +#define PTRACE_TE_ABORT_RAND PTRACE_TE_ABORT_RAND
>  };

Mark Wielaard has spotted [1] a regression that I missed during review.
After this change, this test case fails to compile with the following
diagnostics:

$ gcc -c -xc -o/dev/null - <<'EOF'
#include <asm/ptrace.h>
#include <sys/ptrace.h>
EOF
In file included from <stdin>:1:0:
/usr/include/sys/ptrace.h:93:3: error: expected identifier before numeric constant
   PTRACE_SINGLEBLOCK = 12,
   ^
In file included from <stdin>:2:0:
/usr/include/sys/ptrace.h:158:0: warning: "PTRACE_PEEKUSR_AREA" redefined
 #define PTRACE_PEEKUSR_AREA PTRACE_PEEKUSR_AREA
 ^
In file included from <stdin>:1:0:
/usr/include/asm/ptrace.h:392:0: note: this is the location of the previous definition
 #define PTRACE_PEEKUSR_AREA           0x5000
 ^
In file included from <stdin>:2:0:
/usr/include/sys/ptrace.h:161:0: warning: "PTRACE_POKEUSR_AREA" redefined
 #define PTRACE_POKEUSR_AREA PTRACE_POKEUSR_AREA
 ^
In file included from <stdin>:1:0:
/usr/include/asm/ptrace.h:393:0: note: this is the location of the previous definition
 #define PTRACE_POKEUSR_AREA           0x5001
 ^
In file included from <stdin>:2:0:
/usr/include/sys/ptrace.h:164:0: warning: "PTRACE_GET_LAST_BREAK" redefined
 #define PTRACE_GET_LAST_BREAK PTRACE_GET_LAST_BREAK
 ^
In file included from <stdin>:1:0:
/usr/include/asm/ptrace.h:398:0: note: this is the location of the previous definition
 #define PTRACE_GET_LAST_BREAK       0x5006
 ^
In file included from <stdin>:2:0:
/usr/include/sys/ptrace.h:167:0: warning: "PTRACE_ENABLE_TE" redefined
 #define PTRACE_ENABLE_TE PTRACE_ENABLE_TE
 ^
In file included from <stdin>:1:0:
/usr/include/asm/ptrace.h:401:0: note: this is the location of the previous definition
 #define PTRACE_ENABLE_TE       0x5009
 ^
In file included from <stdin>:2:0:
/usr/include/sys/ptrace.h:170:0: warning: "PTRACE_DISABLE_TE" redefined
 #define PTRACE_DISABLE_TE PTRACE_DISABLE_TE
 ^
In file included from <stdin>:1:0:
/usr/include/asm/ptrace.h:402:0: note: this is the location of the previous definition
 #define PTRACE_DISABLE_TE       0x5010
 ^
In file included from <stdin>:2:0:
/usr/include/sys/ptrace.h:173:0: warning: "PTRACE_TE_ABORT_RAND" redefined
 #define PTRACE_TE_ABORT_RAND PTRACE_TE_ABORT_RAND
 ^
In file included from <stdin>:1:0:
/usr/include/asm/ptrace.h:403:0: note: this is the location of the previous definition
 #define PTRACE_TE_ABORT_RAND       0x5011
 ^

The following change fixes this and similar compilation issues that arise
when sys/ptrace.h is included after linux/ptrace.h:


The list was produced by the following command:
$ sed -n '/USER\|DEVEL/d;s/^[[:space:]]*\(PTRACE_[^=[:space:]]\+\)[[:space:]]*=.*/#undef \1/p' sysdeps/unix/sysv/linux/s390/sys/ptrace.h

PTRACE_PEEKUSER and PTRACE_POKEUSER were excluded because they are not
defined by Linux headers, and PTRACE_SEIZE_DEVEL was excluded because
it's obsolete and should be removed from sys/ptrace.h anyway.

[1] https://sourceware.org/ml/elfutils-devel/2017-q3/msg00018.html

Comments

Carlos O'Donell July 18, 2017, 1:31 p.m. UTC | #1 
On 07/18/2017 06:20 AM, Dmitry V. Levin wrote:
> Mark Wielaard has spotted [1] a regression that I missed during review.
> After this change, this test case fails to compile with the following
> diagnostics:
> 
> $ gcc -c -xc -o/dev/null - <<'EOF'
> #include <asm/ptrace.h>
> #include <sys/ptrace.h>
> EOF

This is a linux/glibc header coordination issue.

https://sourceware.org/glibc/wiki/Synchronizing_Headers

> The following change fixes this and similar compilation issues that arise
> when sys/ptrace.h is included after linux/ptrace.h:
 
This is a known conflict, and needs to be fixed properly using libc-compat.h
on the kernel side and the appropriate defines on the glibc side.
Dmitry V. Levin July 18, 2017, 1:39 p.m. UTC | #2 
On Tue, Jul 18, 2017 at 09:31:49AM -0400, Carlos O'Donell wrote:
> On 07/18/2017 06:20 AM, Dmitry V. Levin wrote:
> > Mark Wielaard has spotted [1] a regression that I missed during review.
> > After this change, this test case fails to compile with the following
> > diagnostics:
> > 
> > $ gcc -c -xc -o/dev/null - <<'EOF'
> > #include <asm/ptrace.h>
> > #include <sys/ptrace.h>
> > EOF
> 
> This is a linux/glibc header coordination issue.

Not really, this is a new issue since glibc-2.25.

> https://sourceware.org/glibc/wiki/Synchronizing_Headers

This project doesn't appear to be alive, unfortunately.

> > The following change fixes this and similar compilation issues that arise
> > when sys/ptrace.h is included after linux/ptrace.h:
>  
> This is a known conflict, and needs to be fixed properly using libc-compat.h
> on the kernel side and the appropriate defines on the glibc side.

No, there was no conflict between asm/ptrace.h and sys/ptrace.h on s390
in glibc-2.25, and we should avoid introducing new conflicts.
Stefan Liebler July 18, 2017, 1:41 p.m. UTC | #3 
On 07/18/2017 12:20 PM, Dmitry V. Levin wrote:
> On Mon, Jun 19, 2017 at 03:10:57PM +0200, Stefan Liebler wrote:
> [...]
>> --- a/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
>> +++ b/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
>> @@ -89,25 +89,9 @@ enum __ptrace_request
>>     PTRACE_SINGLESTEP = 9,
>>   #define PT_STEP PTRACE_SINGLESTEP
>>   
>> -  /* Get all general purpose registers used by a processes.
>> -     This is not supported on all machines.  */
>> -   PTRACE_GETREGS = 12,
>> -#define PT_GETREGS PTRACE_GETREGS
>> -
>> -  /* Set all general purpose registers used by a processes.
>> -     This is not supported on all machines.  */
>> -   PTRACE_SETREGS = 13,
>> -#define PT_SETREGS PTRACE_SETREGS
>> -
>> -  /* Get all floating point registers used by a processes.
>> -     This is not supported on all machines.  */
>> -   PTRACE_GETFPREGS = 14,
>> -#define PT_GETFPREGS PTRACE_GETFPREGS
>> -
>> -  /* Set all floating point registers used by a processes.
>> -     This is not supported on all machines.  */
>> -   PTRACE_SETFPREGS = 15,
>> -#define PT_SETFPREGS PTRACE_SETFPREGS
>> +  /* Execute process until next taken branch.  */
>> +  PTRACE_SINGLEBLOCK = 12,
>> +#define PT_STEPBLOCK PTRACE_SINGLEBLOCK
>>   
>>     /* Attach to a process that is already running. */
>>     PTRACE_ATTACH = 16,
>> @@ -167,8 +151,26 @@ enum __ptrace_request
>>     PTRACE_SETSIGMASK = 0x420b,
>>   #define PTRACE_SETSIGMASK PTRACE_SETSIGMASK
>>   
>> -  PTRACE_SECCOMP_GET_FILTER = 0x420c
>> +  PTRACE_SECCOMP_GET_FILTER = 0x420c,
>>   #define PTRACE_SECCOMP_GET_FILTER PTRACE_SECCOMP_GET_FILTER
>> +
>> +  PTRACE_PEEKUSR_AREA = 0x5000,
>> +#define PTRACE_PEEKUSR_AREA PTRACE_PEEKUSR_AREA
>> +
>> +  PTRACE_POKEUSR_AREA = 0x5001,
>> +#define PTRACE_POKEUSR_AREA PTRACE_POKEUSR_AREA
>> +
>> +  PTRACE_GET_LAST_BREAK = 0x5006,
>> +#define PTRACE_GET_LAST_BREAK PTRACE_GET_LAST_BREAK
>> +
>> +  PTRACE_ENABLE_TE = 0x5009,
>> +#define PTRACE_ENABLE_TE PTRACE_ENABLE_TE
>> +
>> +  PTRACE_DISABLE_TE = 0x5010,
>> +#define PTRACE_DISABLE_TE PTRACE_DISABLE_TE
>> +
>> +  PTRACE_TE_ABORT_RAND = 0x5011
>> +#define PTRACE_TE_ABORT_RAND PTRACE_TE_ABORT_RAND
>>   };
> 
> Mark Wielaard has spotted [1] a regression that I missed during review.
> After this change, this test case fails to compile with the following
> diagnostics:
> 
> $ gcc -c -xc -o/dev/null - <<'EOF'
> #include <asm/ptrace.h>
> #include <sys/ptrace.h>
> EOF
Is this order required?
I've tried it on x86_64 RHEL 7.3:
In file included from /usr/include/asm/ptrace.h:5:0,
                  from <stdin>:1:
/usr/include/sys/ptrace.h:74:4: error: expected identifier before 
numeric constant
     PTRACE_GETREGS = 12,
     ^

> In file included from <stdin>:1:0:
> /usr/include/sys/ptrace.h:93:3: error: expected identifier before numeric constant
>     PTRACE_SINGLEBLOCK = 12,
>     ^
> In file included from <stdin>:2:0:
> /usr/include/sys/ptrace.h:158:0: warning: "PTRACE_PEEKUSR_AREA" redefined
>   #define PTRACE_PEEKUSR_AREA PTRACE_PEEKUSR_AREA
>   ^
> In file included from <stdin>:1:0:
> /usr/include/asm/ptrace.h:392:0: note: this is the location of the previous definition
>   #define PTRACE_PEEKUSR_AREA           0x5000
>   ^
> In file included from <stdin>:2:0:
> /usr/include/sys/ptrace.h:161:0: warning: "PTRACE_POKEUSR_AREA" redefined
>   #define PTRACE_POKEUSR_AREA PTRACE_POKEUSR_AREA
>   ^
> In file included from <stdin>:1:0:
> /usr/include/asm/ptrace.h:393:0: note: this is the location of the previous definition
>   #define PTRACE_POKEUSR_AREA           0x5001
>   ^
> In file included from <stdin>:2:0:
> /usr/include/sys/ptrace.h:164:0: warning: "PTRACE_GET_LAST_BREAK" redefined
>   #define PTRACE_GET_LAST_BREAK PTRACE_GET_LAST_BREAK
>   ^
> In file included from <stdin>:1:0:
> /usr/include/asm/ptrace.h:398:0: note: this is the location of the previous definition
>   #define PTRACE_GET_LAST_BREAK       0x5006
>   ^
> In file included from <stdin>:2:0:
> /usr/include/sys/ptrace.h:167:0: warning: "PTRACE_ENABLE_TE" redefined
>   #define PTRACE_ENABLE_TE PTRACE_ENABLE_TE
>   ^
> In file included from <stdin>:1:0:
> /usr/include/asm/ptrace.h:401:0: note: this is the location of the previous definition
>   #define PTRACE_ENABLE_TE       0x5009
>   ^
> In file included from <stdin>:2:0:
> /usr/include/sys/ptrace.h:170:0: warning: "PTRACE_DISABLE_TE" redefined
>   #define PTRACE_DISABLE_TE PTRACE_DISABLE_TE
>   ^
> In file included from <stdin>:1:0:
> /usr/include/asm/ptrace.h:402:0: note: this is the location of the previous definition
>   #define PTRACE_DISABLE_TE       0x5010
>   ^
> In file included from <stdin>:2:0:
> /usr/include/sys/ptrace.h:173:0: warning: "PTRACE_TE_ABORT_RAND" redefined
>   #define PTRACE_TE_ABORT_RAND PTRACE_TE_ABORT_RAND
>   ^
> In file included from <stdin>:1:0:
> /usr/include/asm/ptrace.h:403:0: note: this is the location of the previous definition
>   #define PTRACE_TE_ABORT_RAND       0x5011
>   ^
> 
> The following change fixes this and similar compilation issues that arise
> when sys/ptrace.h is included after linux/ptrace.h:
> 
> --- a/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
> +++ b/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
> @@ -24,24 +24,61 @@
>   #include <bits/types.h>
>   
>   __BEGIN_DECLS
> -#ifdef _LINUX_PTRACE_H
> +#if defined _LINUX_PTRACE_H || defined _S390_PTRACE_H
Shall we also add "|| defined _UAPI_LINUX_PTRACE_H" here?
Some of the defines like PTRACE_EVENT_* | PTRACE_O_* are defined in the 
corresponding <kernel-src>/include/uapi/linux/ptrace.h header.

>   /* Kludge to stop stuff gdb & strace compiles from getting upset
>    */
>   #undef PTRACE_TRACEME
>   #undef PTRACE_PEEKTEXT
>   #undef PTRACE_PEEKDATA
> -#undef PTRACE_PEEKUSR
>   #undef PTRACE_POKETEXT
>   #undef PTRACE_POKEDATA
> -#undef PTRACE_POKEUSR
>   #undef PTRACE_CONT
>   #undef PTRACE_KILL
>   #undef PTRACE_SINGLESTEP
> -
> +#undef PTRACE_SINGLEBLOCK
>   #undef PTRACE_ATTACH
>   #undef PTRACE_DETACH
> -
>   #undef PTRACE_SYSCALL
> +#undef PTRACE_SETOPTIONS
> +#undef PTRACE_GETEVENTMSG
> +#undef PTRACE_GETSIGINFO
> +#undef PTRACE_SETSIGINFO
> +#undef PTRACE_GETREGSET
> +#undef PTRACE_SETREGSET
> +#undef PTRACE_SEIZE
> +#undef PTRACE_INTERRUPT
> +#undef PTRACE_LISTEN
> +#undef PTRACE_PEEKSIGINFO
> +#undef PTRACE_GETSIGMASK
> +#undef PTRACE_SETSIGMASK
> +#undef PTRACE_SECCOMP_GET_FILTER
> +#undef PTRACE_PEEKUSR_AREA
> +#undef PTRACE_POKEUSR_AREA
> +#undef PTRACE_GET_LAST_BREAK
> +#undef PTRACE_ENABLE_TE
> +#undef PTRACE_DISABLE_TE
> +#undef PTRACE_TE_ABORT_RAND
> +#undef PTRACE_O_TRACESYSGOOD
> +#undef PTRACE_O_TRACEFORK
> +#undef PTRACE_O_TRACEVFORK
> +#undef PTRACE_O_TRACECLONE
> +#undef PTRACE_O_TRACEEXEC
> +#undef PTRACE_O_TRACEVFORKDONE
> +#undef PTRACE_O_TRACEEXIT
> +#undef PTRACE_O_TRACESECCOMP
> +#undef PTRACE_O_EXITKILL
> +#undef PTRACE_O_SUSPEND_SECCOMP
> +#undef PTRACE_O_MASK
> +#undef PTRACE_EVENT_FORK
> +#undef PTRACE_EVENT_VFORK
> +#undef PTRACE_EVENT_CLONE
> +#undef PTRACE_EVENT_EXEC
> +#undef PTRACE_EVENT_VFORK_DONE
> +#undef PTRACE_EVENT_EXIT
> +#undef PTRACE_EVENT_SECCOMP
> +#undef PTRACE_EVENT_STOP
> +#undef PTRACE_PEEKSIGINFO_SHARED
> +
>   #endif
>   /* Type of the REQUEST argument to `ptrace.'  */
>   enum __ptrace_request
> 
> The list was produced by the following command:
> $ sed -n '/USER\|DEVEL/d;s/^[[:space:]]*\(PTRACE_[^=[:space:]]\+\)[[:space:]]*=.*/#undef \1/p' sysdeps/unix/sysv/linux/s390/sys/ptrace.h
> 
> PTRACE_PEEKUSER and PTRACE_POKEUSER were excluded because they are not
> defined by Linux headers, and PTRACE_SEIZE_DEVEL was excluded because
> it's obsolete and should be removed from sys/ptrace.h anyway.
> 
> [1] https://sourceware.org/ml/elfutils-devel/2017-q3/msg00018.html
> 
>
Carlos O'Donell July 18, 2017, 2:11 p.m. UTC | #4 
On 07/18/2017 09:39 AM, Dmitry V. Levin wrote:
> On Tue, Jul 18, 2017 at 09:31:49AM -0400, Carlos O'Donell wrote:
>> On 07/18/2017 06:20 AM, Dmitry V. Levin wrote:
>>> Mark Wielaard has spotted [1] a regression that I missed during review.
>>> After this change, this test case fails to compile with the following
>>> diagnostics:
>>>
>>> $ gcc -c -xc -o/dev/null - <<'EOF'
>>> #include <asm/ptrace.h>
>>> #include <sys/ptrace.h>
>>> EOF
>>
>> This is a linux/glibc header coordination issue.
> 
> Not really, this is a new issue since glibc-2.25.

It is both a new issue and a header coordination issue, the two are not
mutually exclusive.

>> https://sourceware.org/glibc/wiki/Synchronizing_Headers
> 
> This project doesn't appear to be alive, unfortunately.

The project is alive. We are the project. We need to send patches upstream
to the Linux kernel to keep fixing inclusion ordering issues where we need
them fixed.

When 2.27 opens I have a pile of header inclusion ordering fixes I want to
propose along with tests for them, but I haven't had a chance to submit. So
we can discuss this in a few weeks.

>>> The following change fixes this and similar compilation issues that arise
>>> when sys/ptrace.h is included after linux/ptrace.h:
>>  
>> This is a known conflict, and needs to be fixed properly using libc-compat.h
>> on the kernel side and the appropriate defines on the glibc side.
> 
> No, there was no conflict between asm/ptrace.h and sys/ptrace.h on s390
> in glibc-2.25, and we should avoid introducing new conflicts.
 
I have not verified that inclusion worked on both orders, if it did, then
this is indeed a regression.

However, I would like to take a step back:

* Why is this issue a blocker? What software does it stop from building?

* Can we delay the fix until after the release and fix it properly?

Mark, Is this a problem for Valgrind?
Carlos O'Donell July 18, 2017, 2:12 p.m. UTC | #5 
On 07/18/2017 09:41 AM, Stefan Liebler wrote:
>> $ gcc -c -xc -o/dev/null - <<'EOF'
>> #include <asm/ptrace.h>
>> #include <sys/ptrace.h>
>> EOF
> Is this order required?

Per the synchronization headers strategy for glibc we try to support *both*
orders for important headers where possible.

https://sourceware.org/glibc/wiki/Synchronizing_Headers

> I've tried it on x86_64 RHEL 7.3:
> In file included from /usr/include/asm/ptrace.h:5:0,
>                  from <stdin>:1:
> /usr/include/sys/ptrace.h:74:4: error: expected identifier before numeric constant
>     PTRACE_GETREGS = 12,
>     ^

This is a known issue. It is known that linux/ptrace.h and sys/ptrace.h do not really
work well together. We need to fix it.
Dmitry V. Levin July 18, 2017, 2:15 p.m. UTC | #6 
On Tue, Jul 18, 2017 at 03:41:41PM +0200, Stefan Liebler wrote:
> On 07/18/2017 12:20 PM, Dmitry V. Levin wrote:
> > On Mon, Jun 19, 2017 at 03:10:57PM +0200, Stefan Liebler wrote:
[...]
> > Mark Wielaard has spotted [1] a regression that I missed during review.
> > After this change, this test case fails to compile with the following
> > diagnostics:
> > 
> > $ gcc -c -xc -o/dev/null - <<'EOF'
> > #include <asm/ptrace.h>
> > #include <sys/ptrace.h>
> > EOF
> Is this order required?

No, the reversed order also works.

> I've tried it on x86_64 RHEL 7.3:
> In file included from /usr/include/asm/ptrace.h:5:0,
>                   from <stdin>:1:
> /usr/include/sys/ptrace.h:74:4: error: expected identifier before 
> numeric constant
>      PTRACE_GETREGS = 12,
>      ^

Interesting.  So it means that it didn't work with some old linux headers.
However, it used to work with recent linux headers and glibc-2.25.

> > In file included from <stdin>:1:0:
> > /usr/include/sys/ptrace.h:93:3: error: expected identifier before numeric constant
> >     PTRACE_SINGLEBLOCK = 12,
> >     ^
> > In file included from <stdin>:2:0:
> > /usr/include/sys/ptrace.h:158:0: warning: "PTRACE_PEEKUSR_AREA" redefined
> >   #define PTRACE_PEEKUSR_AREA PTRACE_PEEKUSR_AREA
> >   ^
> > In file included from <stdin>:1:0:
> > /usr/include/asm/ptrace.h:392:0: note: this is the location of the previous definition
> >   #define PTRACE_PEEKUSR_AREA           0x5000
> >   ^
> > In file included from <stdin>:2:0:
> > /usr/include/sys/ptrace.h:161:0: warning: "PTRACE_POKEUSR_AREA" redefined
> >   #define PTRACE_POKEUSR_AREA PTRACE_POKEUSR_AREA
> >   ^
> > In file included from <stdin>:1:0:
> > /usr/include/asm/ptrace.h:393:0: note: this is the location of the previous definition
> >   #define PTRACE_POKEUSR_AREA           0x5001
> >   ^
> > In file included from <stdin>:2:0:
> > /usr/include/sys/ptrace.h:164:0: warning: "PTRACE_GET_LAST_BREAK" redefined
> >   #define PTRACE_GET_LAST_BREAK PTRACE_GET_LAST_BREAK
> >   ^
> > In file included from <stdin>:1:0:
> > /usr/include/asm/ptrace.h:398:0: note: this is the location of the previous definition
> >   #define PTRACE_GET_LAST_BREAK       0x5006
> >   ^
> > In file included from <stdin>:2:0:
> > /usr/include/sys/ptrace.h:167:0: warning: "PTRACE_ENABLE_TE" redefined
> >   #define PTRACE_ENABLE_TE PTRACE_ENABLE_TE
> >   ^
> > In file included from <stdin>:1:0:
> > /usr/include/asm/ptrace.h:401:0: note: this is the location of the previous definition
> >   #define PTRACE_ENABLE_TE       0x5009
> >   ^
> > In file included from <stdin>:2:0:
> > /usr/include/sys/ptrace.h:170:0: warning: "PTRACE_DISABLE_TE" redefined
> >   #define PTRACE_DISABLE_TE PTRACE_DISABLE_TE
> >   ^
> > In file included from <stdin>:1:0:
> > /usr/include/asm/ptrace.h:402:0: note: this is the location of the previous definition
> >   #define PTRACE_DISABLE_TE       0x5010
> >   ^
> > In file included from <stdin>:2:0:
> > /usr/include/sys/ptrace.h:173:0: warning: "PTRACE_TE_ABORT_RAND" redefined
> >   #define PTRACE_TE_ABORT_RAND PTRACE_TE_ABORT_RAND
> >   ^
> > In file included from <stdin>:1:0:
> > /usr/include/asm/ptrace.h:403:0: note: this is the location of the previous definition
> >   #define PTRACE_TE_ABORT_RAND       0x5011
> >   ^
> > 
> > The following change fixes this and similar compilation issues that arise
> > when sys/ptrace.h is included after linux/ptrace.h:
> > 
> > --- a/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
> > +++ b/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
> > @@ -24,24 +24,61 @@
> >   #include <bits/types.h>
> >   
> >   __BEGIN_DECLS
> > -#ifdef _LINUX_PTRACE_H
> > +#if defined _LINUX_PTRACE_H || defined _S390_PTRACE_H
> Shall we also add "|| defined _UAPI_LINUX_PTRACE_H" here?

Isn't _UAPI prefix automatically stripped during linux headers export
procedure?
Mark Wielaard July 18, 2017, 2:28 p.m. UTC | #7 
On Tue, 2017-07-18 at 10:11 -0400, Carlos O'Donell wrote:
> On 07/18/2017 09:39 AM, Dmitry V. Levin wrote:
> > On Tue, Jul 18, 2017 at 09:31:49AM -0400, Carlos O'Donell wrote:
> >> On 07/18/2017 06:20 AM, Dmitry V. Levin wrote:
> >>> The following change fixes this and similar compilation issues that arise
> >>> when sys/ptrace.h is included after linux/ptrace.h:
> >>  
> >> This is a known conflict, and needs to be fixed properly using libc-compat.h
> >> on the kernel side and the appropriate defines on the glibc side.
> > 
> > No, there was no conflict between asm/ptrace.h and sys/ptrace.h on s390
> > in glibc-2.25, and we should avoid introducing new conflicts.
>  
> I have not verified that inclusion worked on both orders, if it did, then
> this is indeed a regression.

It used to work either way. We used asm/ptrace.h then sys/ptrace.h on
s390[x] in elfutils. That broke with 2.26 in fedora rawhide. So we
changed it to sys/ptrace.h then asm/ptrace.h. I verified that order
works on both old and new glibc/kernel headers (aka on RHEL7 s390[x] and
fedora rawhide s390x).

> Mark, Is this a problem for Valgrind?

It really shouldn't, but I haven't tried building valgrind on s390x
against glibc git yet. In valgrind we try to avoid including glibc and
kernel headers (valgrind comes with its own mini-libc that we statically
link into the tools and its own kernel headers for the syscall
wrappers).

Cheers,

Mark
Mark Wielaard July 18, 2017, 2:40 p.m. UTC | #8 
On Tue, 2017-07-18 at 16:28 +0200, Mark Wielaard wrote:
> On Tue, 2017-07-18 at 10:11 -0400, Carlos O'Donell wrote:
> > On 07/18/2017 09:39 AM, Dmitry V. Levin wrote:
> > > On Tue, Jul 18, 2017 at 09:31:49AM -0400, Carlos O'Donell wrote:
> > >> On 07/18/2017 06:20 AM, Dmitry V. Levin wrote:
> > >>> The following change fixes this and similar compilation issues that arise
> > >>> when sys/ptrace.h is included after linux/ptrace.h:
> > >>  
> > >> This is a known conflict, and needs to be fixed properly using libc-compat.h
> > >> on the kernel side and the appropriate defines on the glibc side.
> > > 
> > > No, there was no conflict between asm/ptrace.h and sys/ptrace.h on s390
> > > in glibc-2.25, and we should avoid introducing new conflicts.
> >  
> > I have not verified that inclusion worked on both orders, if it did, then
> > this is indeed a regression.
> 
> It used to work either way. We used asm/ptrace.h then sys/ptrace.h on
> s390[x] in elfutils. That broke with 2.26 in fedora rawhide. So we
> changed it to sys/ptrace.h then asm/ptrace.h. I verified that order
> works on both old and new glibc/kernel headers (aka on RHEL7 s390[x] and
> fedora rawhide s390x).

One more note. This really is only an issue for s390x since asm/ptrace.h
provides ptrace_area which is needed to use PTRACE_PEEKUSR_AREA. On
other architectures you only need sys/ptrace.h so it doesn't really
matter whether you can include them both.

See also the glibc testcase
sysdeps/unix/sysv/linux/s390/tst-ptrace-singleblock.c which also needs
to include both.

Cheers,

Mark
Stefan Liebler July 19, 2017, 8:33 a.m. UTC | #9 
On 07/18/2017 04:15 PM, Dmitry V. Levin wrote:
> On Tue, Jul 18, 2017 at 03:41:41PM +0200, Stefan Liebler wrote:
>> On 07/18/2017 12:20 PM, Dmitry V. Levin wrote:
>>> On Mon, Jun 19, 2017 at 03:10:57PM +0200, Stefan Liebler wrote:
> [...]
>>> Mark Wielaard has spotted [1] a regression that I missed during review.
>>> After this change, this test case fails to compile with the following
>>> diagnostics:
>>>
>>> $ gcc -c -xc -o/dev/null - <<'EOF'
>>> #include <asm/ptrace.h>
>>> #include <sys/ptrace.h>
>>> EOF
>> Is this order required?
> 
> No, the reversed order also works.
> 
>> I've tried it on x86_64 RHEL 7.3:
>> In file included from /usr/include/asm/ptrace.h:5:0,
>>                    from <stdin>:1:
>> /usr/include/sys/ptrace.h:74:4: error: expected identifier before
>> numeric constant
>>       PTRACE_GETREGS = 12,
>>       ^
> 
> Interesting.  So it means that it didn't work with some old linux headers.
> However, it used to work with recent linux headers and glibc-2.25.
> 
>>> In file included from <stdin>:1:0:
>>> /usr/include/sys/ptrace.h:93:3: error: expected identifier before numeric constant
>>>      PTRACE_SINGLEBLOCK = 12,
>>>      ^
>>> In file included from <stdin>:2:0:
>>> /usr/include/sys/ptrace.h:158:0: warning: "PTRACE_PEEKUSR_AREA" redefined
>>>    #define PTRACE_PEEKUSR_AREA PTRACE_PEEKUSR_AREA
>>>    ^
>>> In file included from <stdin>:1:0:
>>> /usr/include/asm/ptrace.h:392:0: note: this is the location of the previous definition
>>>    #define PTRACE_PEEKUSR_AREA           0x5000
>>>    ^
>>> In file included from <stdin>:2:0:
>>> /usr/include/sys/ptrace.h:161:0: warning: "PTRACE_POKEUSR_AREA" redefined
>>>    #define PTRACE_POKEUSR_AREA PTRACE_POKEUSR_AREA
>>>    ^
>>> In file included from <stdin>:1:0:
>>> /usr/include/asm/ptrace.h:393:0: note: this is the location of the previous definition
>>>    #define PTRACE_POKEUSR_AREA           0x5001
>>>    ^
>>> In file included from <stdin>:2:0:
>>> /usr/include/sys/ptrace.h:164:0: warning: "PTRACE_GET_LAST_BREAK" redefined
>>>    #define PTRACE_GET_LAST_BREAK PTRACE_GET_LAST_BREAK
>>>    ^
>>> In file included from <stdin>:1:0:
>>> /usr/include/asm/ptrace.h:398:0: note: this is the location of the previous definition
>>>    #define PTRACE_GET_LAST_BREAK       0x5006
>>>    ^
>>> In file included from <stdin>:2:0:
>>> /usr/include/sys/ptrace.h:167:0: warning: "PTRACE_ENABLE_TE" redefined
>>>    #define PTRACE_ENABLE_TE PTRACE_ENABLE_TE
>>>    ^
>>> In file included from <stdin>:1:0:
>>> /usr/include/asm/ptrace.h:401:0: note: this is the location of the previous definition
>>>    #define PTRACE_ENABLE_TE       0x5009
>>>    ^
>>> In file included from <stdin>:2:0:
>>> /usr/include/sys/ptrace.h:170:0: warning: "PTRACE_DISABLE_TE" redefined
>>>    #define PTRACE_DISABLE_TE PTRACE_DISABLE_TE
>>>    ^
>>> In file included from <stdin>:1:0:
>>> /usr/include/asm/ptrace.h:402:0: note: this is the location of the previous definition
>>>    #define PTRACE_DISABLE_TE       0x5010
>>>    ^
>>> In file included from <stdin>:2:0:
>>> /usr/include/sys/ptrace.h:173:0: warning: "PTRACE_TE_ABORT_RAND" redefined
>>>    #define PTRACE_TE_ABORT_RAND PTRACE_TE_ABORT_RAND
>>>    ^
>>> In file included from <stdin>:1:0:
>>> /usr/include/asm/ptrace.h:403:0: note: this is the location of the previous definition
>>>    #define PTRACE_TE_ABORT_RAND       0x5011
>>>    ^
>>>
>>> The following change fixes this and similar compilation issues that arise
>>> when sys/ptrace.h is included after linux/ptrace.h:
>>>
>>> --- a/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
>>> +++ b/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
>>> @@ -24,24 +24,61 @@
>>>    #include <bits/types.h>
>>>    
>>>    __BEGIN_DECLS
>>> -#ifdef _LINUX_PTRACE_H
>>> +#if defined _LINUX_PTRACE_H || defined _S390_PTRACE_H
>> Shall we also add "|| defined _UAPI_LINUX_PTRACE_H" here?
> 
> Isn't _UAPI prefix automatically stripped during linux headers export
> procedure?
> 
> 
Yes you are right. I've viewed the file in the kernel-sources and missed 
this fact.
Stefan Liebler July 20, 2017, 7:38 a.m. UTC | #10 
On 07/18/2017 04:11 PM, Carlos O'Donell wrote:
> On 07/18/2017 09:39 AM, Dmitry V. Levin wrote:
>> On Tue, Jul 18, 2017 at 09:31:49AM -0400, Carlos O'Donell wrote:
>>> On 07/18/2017 06:20 AM, Dmitry V. Levin wrote:
>>>> Mark Wielaard has spotted [1] a regression that I missed during review.
>>>> After this change, this test case fails to compile with the following
>>>> diagnostics:
>>>>
>>>> $ gcc -c -xc -o/dev/null - <<'EOF'
>>>> #include <asm/ptrace.h>
>>>> #include <sys/ptrace.h>
>>>> EOF
>>>
>>> This is a linux/glibc header coordination issue.
>>
>> Not really, this is a new issue since glibc-2.25.
> 
> It is both a new issue and a header coordination issue, the two are not
> mutually exclusive.
> 
>>> https://sourceware.org/glibc/wiki/Synchronizing_Headers
>>
>> This project doesn't appear to be alive, unfortunately.
> 
> The project is alive. We are the project. We need to send patches upstream
> to the Linux kernel to keep fixing inclusion ordering issues where we need
> them fixed.
> 
> When 2.27 opens I have a pile of header inclusion ordering fixes I want to
> propose along with tests for them, but I haven't had a chance to submit. So
> we can discuss this in a few weeks.
> 
>>>> The following change fixes this and similar compilation issues that arise
>>>> when sys/ptrace.h is included after linux/ptrace.h:
>>>   
>>> This is a known conflict, and needs to be fixed properly using libc-compat.h
>>> on the kernel side and the appropriate defines on the glibc side.
>>
>> No, there was no conflict between asm/ptrace.h and sys/ptrace.h on s390
>> in glibc-2.25, and we should avoid introducing new conflicts.
> 
> I have not verified that inclusion worked on both orders, if it did, then
> this is indeed a regression.
Before my commit "S390: Sync ptrace.h with kernel. [BZ #21539]",
both orders compiled without a failure:

#include <asm/ptrace.h>
#include <sys/ptrace.h>
=> okay (but fails after my commit)


#include <sys/ptrace.h>
#include <asm/ptrace.h>
=> okay


However if somebody used linux/ptrace.h instead of asm/ptrace.h:

#include <linux/ptrace.h>
#include <sys/ptrace.h>
=> fail


#include <sys/ptrace.h>
#include <linux/ptrace.h>
=> okay


With Dimitry's patch, all four cases are okay.

> 
> However, I would like to take a step back:
> 
> * Why is this issue a blocker? What software does it stop from building?
> 
> * Can we delay the fix until after the release and fix it properly?
> 
> Mark, Is this a problem for Valgrind?
> 

Carlos, shall we commit Dimitry's patch before the release?
Then we don't have this regression compared to glibc 2.25 release.

Bye.
Stefan
Carlos O'Donell July 20, 2017, 8:06 a.m. UTC | #11 
On 07/20/2017 03:38 AM, Stefan Liebler wrote:
> Carlos, shall we commit Dimitry's patch before the release?
> Then we don't have this regression compared to glibc 2.25 release.

I'm OK with Dmitry's patch going in for 2.26 to fix the regression.

Could you please work with Dmitry to get this committed?

I'll work on fixing this coordination issue with the kernel when we
reopen for development.
Stefan Liebler July 20, 2017, 8:32 a.m. UTC | #12 
On 07/20/2017 10:06 AM, Carlos O'Donell wrote:
> On 07/20/2017 03:38 AM, Stefan Liebler wrote:
>> Carlos, shall we commit Dimitry's patch before the release?
>> Then we don't have this regression compared to glibc 2.25 release.
> 
> I'm OK with Dmitry's patch going in for 2.26 to fix the regression.
Okay.
> > Could you please work with Dmitry to get this committed?
Okay.
Dimitry, I have one small note to your patch:
Can you add a space before the undefs and add the comment like this:
(according to 
https://sourceware.org/glibc/wiki/Style_and_Conventions#Nested_C_Preprocessor_Directives)

#if defined _LINUX_PTRACE_H || defined _S390_PTRACE_H
/* Kludge to stop stuff gdb & strace compiles from getting upset
  */
# undef PTRACE_TRACEME
...
#endif /* defined _LINUX_PTRACE_H || defined _S390_PTRACE_H  */

Then the patch is okay.
Can you please commit the patch?
Otherwise, post the final patch, then I'll commit it.

> 
> I'll work on fixing this coordination issue with the kernel when we
> reopen for development.
> 
Okay.


Thanks.
Stefan
Dmitry V. Levin July 23, 2017, 11:58 p.m. UTC | #13 
On Thu, Jul 20, 2017 at 10:32:22AM +0200, Stefan Liebler wrote:
> On 07/20/2017 10:06 AM, Carlos O'Donell wrote:
> > On 07/20/2017 03:38 AM, Stefan Liebler wrote:
> >> Carlos, shall we commit Dimitry's patch before the release?
> >> Then we don't have this regression compared to glibc 2.25 release.
> > 
> > I'm OK with Dmitry's patch going in for 2.26 to fix the regression.
> Okay.
> > > Could you please work with Dmitry to get this committed?
> Okay.
> Dimitry, I have one small note to your patch:
> Can you add a space before the undefs and add the comment like this:
> (according to 
> https://sourceware.org/glibc/wiki/Style_and_Conventions#Nested_C_Preprocessor_Directives)
> 
> #if defined _LINUX_PTRACE_H || defined _S390_PTRACE_H
> /* Kludge to stop stuff gdb & strace compiles from getting upset
>   */
> # undef PTRACE_TRACEME
> ...
> #endif /* defined _LINUX_PTRACE_H || defined _S390_PTRACE_H  */
> 
> Then the patch is okay.
> Can you please commit the patch?

OK, committed.
Stefan Liebler July 24, 2017, 7:01 a.m. UTC | #14 
On 07/24/2017 01:58 AM, Dmitry V. Levin wrote:
> On Thu, Jul 20, 2017 at 10:32:22AM +0200, Stefan Liebler wrote:
>> On 07/20/2017 10:06 AM, Carlos O'Donell wrote:
>>> On 07/20/2017 03:38 AM, Stefan Liebler wrote:
>>>> Carlos, shall we commit Dimitry's patch before the release?
>>>> Then we don't have this regression compared to glibc 2.25 release.
>>>
>>> I'm OK with Dmitry's patch going in for 2.26 to fix the regression.
>> Okay.
>>>> Could you please work with Dmitry to get this committed?
>> Okay.
>> Dimitry, I have one small note to your patch:
>> Can you add a space before the undefs and add the comment like this:
>> (according to
>> https://sourceware.org/glibc/wiki/Style_and_Conventions#Nested_C_Preprocessor_Directives)
>>
>> #if defined _LINUX_PTRACE_H || defined _S390_PTRACE_H
>> /* Kludge to stop stuff gdb & strace compiles from getting upset
>>    */
>> # undef PTRACE_TRACEME
>> ...
>> #endif /* defined _LINUX_PTRACE_H || defined _S390_PTRACE_H  */
>>
>> Then the patch is okay.
>> Can you please commit the patch?
> 
> OK, committed.
> 
> 
Thanks.
Stefan
diff mbox

Patch

--- a/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
+++ b/sysdeps/unix/sysv/linux/s390/sys/ptrace.h
@@ -24,24 +24,61 @@ 
 #include <bits/types.h>
 
 __BEGIN_DECLS
-#ifdef _LINUX_PTRACE_H
+#if defined _LINUX_PTRACE_H || defined _S390_PTRACE_H
 /* Kludge to stop stuff gdb & strace compiles from getting upset
  */
 #undef PTRACE_TRACEME
 #undef PTRACE_PEEKTEXT
 #undef PTRACE_PEEKDATA
-#undef PTRACE_PEEKUSR
 #undef PTRACE_POKETEXT
 #undef PTRACE_POKEDATA
-#undef PTRACE_POKEUSR
 #undef PTRACE_CONT
 #undef PTRACE_KILL
 #undef PTRACE_SINGLESTEP
-
+#undef PTRACE_SINGLEBLOCK
 #undef PTRACE_ATTACH
 #undef PTRACE_DETACH
-
 #undef PTRACE_SYSCALL
+#undef PTRACE_SETOPTIONS
+#undef PTRACE_GETEVENTMSG
+#undef PTRACE_GETSIGINFO
+#undef PTRACE_SETSIGINFO
+#undef PTRACE_GETREGSET
+#undef PTRACE_SETREGSET
+#undef PTRACE_SEIZE
+#undef PTRACE_INTERRUPT
+#undef PTRACE_LISTEN
+#undef PTRACE_PEEKSIGINFO
+#undef PTRACE_GETSIGMASK
+#undef PTRACE_SETSIGMASK
+#undef PTRACE_SECCOMP_GET_FILTER
+#undef PTRACE_PEEKUSR_AREA
+#undef PTRACE_POKEUSR_AREA
+#undef PTRACE_GET_LAST_BREAK
+#undef PTRACE_ENABLE_TE
+#undef PTRACE_DISABLE_TE
+#undef PTRACE_TE_ABORT_RAND
+#undef PTRACE_O_TRACESYSGOOD
+#undef PTRACE_O_TRACEFORK
+#undef PTRACE_O_TRACEVFORK
+#undef PTRACE_O_TRACECLONE
+#undef PTRACE_O_TRACEEXEC
+#undef PTRACE_O_TRACEVFORKDONE
+#undef PTRACE_O_TRACEEXIT
+#undef PTRACE_O_TRACESECCOMP
+#undef PTRACE_O_EXITKILL
+#undef PTRACE_O_SUSPEND_SECCOMP
+#undef PTRACE_O_MASK
+#undef PTRACE_EVENT_FORK
+#undef PTRACE_EVENT_VFORK
+#undef PTRACE_EVENT_CLONE
+#undef PTRACE_EVENT_EXEC
+#undef PTRACE_EVENT_VFORK_DONE
+#undef PTRACE_EVENT_EXIT
+#undef PTRACE_EVENT_SECCOMP
+#undef PTRACE_EVENT_STOP
+#undef PTRACE_PEEKSIGINFO_SHARED
+
 #endif
 /* Type of the REQUEST argument to `ptrace.'  */
 enum __ptrace_request