Re: [PATCH 15/15] x86/headers: Remove <asm/sigcontext.h>

[Mikko Rapeli <mikko.rapeli@iki.fi>]

On Sun, Sep 06, 2015 at 08:41:45AM +0200, Ingo Molnar wrote:
> 
> * Mikko Rapeli <mikko.rapeli@iki.fi> wrote:
> 
> > On Sat, Sep 05, 2015 at 01:59:43PM +0200, Ingo Molnar wrote:
> > > 
> > > * Mikko Rapeli <mikko.rapeli@iki.fi> wrote:
> > > 
> > > > > diff --git a/arch/x86/include/uapi/asm/sigcontext32.h b/arch/x86/include/uapi/asm/sigcontext32.h
> > > > > index a92b0f0dc09e..8b870175befa 100644
> > > > > --- a/arch/x86/include/uapi/asm/sigcontext32.h
> > > > > +++ b/arch/x86/include/uapi/asm/sigcontext32.h
> > > > > @@ -3,6 +3,6 @@
> > > > >  
> > > > >  /* This is a legacy file - all the type definitions are in sigcontext.h: */
> > > > >  
> > > > > -#include <asm/sigcontext.h>
> > > > > +#include <uapi/asm/sigcontext.h>
> > > > 
> > > > This needs to be without uapi directory in path.
> > > 
> > > What do you mean?
> > 
> > There is not uapi in path in userspace so it fails to compile:
> 
> Ok, I see. So it's not common, but I don't think there's an outright prohibition 
> for uapi headers to refer to each other:
> 
>  arch/hexagon/include/uapi/asm/signal.h:#include <uapi/asm/registers.h>
>  arch/mips/include/uapi/asm/siginfo.h:#include <uapi/asm-generic/siginfo.h>
>  arch/x86/include/uapi/asm/sigcontext32.h:#include <uapi/asm/sigcontext.h>
> 
> There are a couple of solutions:
> 
>   - copy the uapi/ directory if you take the kernel headers as-is
> 
>   - adapt the headers to the old user-space layout when you import them.
>     (i.e. do a sed -i 's/<uapi/</' on them).
> 
>   - create a symbolic link from asm/uapi to asm/ in user-space.
> 
> The kernel side solutions are uglier:
> 
>   - We could create a symbolic link from asm/uapi/sigcontext32.h to 
>     asm/uapi/sigcontext.h, although I'm not sure what the policy for that is in 
>     the kernel repository - I think it's generally frowned upon.
> 
>   - We could keep asm/sigcontext.h that includes asm/uapi/sigcontext.h - a
>     poor man's symbolic link.
> 
> OTOH the last option isn't all that ugly.

Yes, AFAIK it's a common patterns in kernel.

> > This is the fix:
> > 
> > --- a/arch/x86/include/uapi/asm/sigcontext32.h
> > +++ b/arch/x86/include/uapi/asm/sigcontext32.h
> > @@ -3,6 +3,6 @@
> >  
> >  /* This is a legacy file - all the type definitions are in sigcontext.h: */
> >  
> > -#include <uapi/asm/sigcontext.h>
> > +#include <asm/sigcontext.h>
> 
> There's no asm/sigcontext.h file anymore if you apply my patches - but we could 
> reintroduce it to make the copy of UAPI headers to user-space work as-is.

Actually there is, in user space :)

$ git show -s --pretty=one HEAD
bea04c5252803fa53dcc5366ae32357a0ab07b44 x86/headers: Remove <asm/sigcontext.h>
$ make headers_install
...
$ ls usr/include/asm/sigcontext.h 
usr/include/asm/sigcontext.h
$ diff -up arch/x86/include/uapi/asm/sigcontext.h usr/include/asm/sigcontext.h
--- arch/x86/include/uapi/asm/sigcontext.h	2015-09-06 22:10:54.918303649 +0200
+++ usr/include/asm/sigcontext.h	2015-09-06 22:11:18.214835356 +0200
@@ -1,5 +1,5 @@
-#ifndef _UAPI_ASM_X86_SIGCONTEXT_H
-#define _UAPI_ASM_X86_SIGCONTEXT_H
+#ifndef _ASM_X86_SIGCONTEXT_H
+#define _ASM_X86_SIGCONTEXT_H
 
 /*
  * Linux signal context definitions. The sigcontext includes a complex hierarchy of CPU
@@ -14,7 +14,7 @@
  * to grow new quirks for quite some time. Promise!
  */
 
-#include <linux/compiler.h>
+
 #include <linux/types.h>
 
 #define FP_XSTATE_MAGIC1		0x46505853U
@@ -271,20 +271,12 @@ struct sigcontext_64 {
 /*
  * Create the real 'struct sigcontext' type:
  */
-#ifdef __KERNEL__
-# ifdef __i386__
-#  define sigcontext sigcontext_32
-# else
-#  define sigcontext sigcontext_64
-# endif
-#endif
 
 /*
  * The old user-space sigcontext definition, just in case user-space still
  * relies on it. The kernel definition (in asm/sigcontext.h) has unified
  * field names but otherwise the same layout.
  */
-#ifndef __KERNEL__
 
 #define _fpstate_ia32			_fpstate_32
 #define sigcontext_ia32			sigcontext_32
@@ -311,7 +303,7 @@ struct sigcontext {
 	__u32				eflags;
 	__u32				esp_at_signal;
 	__u16				ss, __ssh;
-	struct _fpstate __user		*fpstate;
+	struct _fpstate 	*fpstate;
 	__u32				oldmask;
 	__u32				cr2;
 };
@@ -343,13 +335,12 @@ struct sigcontext {
 	__u64				trapno;
 	__u64				oldmask;
 	__u64				cr2;
-	struct _fpstate __user		*fpstate;	/* Zero when no FPU context */
+	struct _fpstate 	*fpstate;	/* Zero when no FPU context */
 #  ifdef __ILP32__
 	__u32				__fpstate_pad;
 #  endif
 	__u64				reserved1[8];
 };
 # endif /* __x86_64__ */
-#endif /* !__KERNEL__ */
 
-#endif /* _UAPI_ASM_X86_SIGCONTEXT_H */
+#endif /* _ASM_X86_SIGCONTEXT_H */

So that's what changes when the file is exported for userspace together with
the path from which uapi is removed. So if, like you said kernel side has
problems with

--- a/arch/x86/include/uapi/asm/sigcontext32.h
+++ b/arch/x86/include/uapi/asm/sigcontext32.h
@@ -3,6 +3,6 @@
 /* This is a legacy file - all the type definitions are in sigcontext.h: */

-#include <uapi/asm/sigcontext.h>
+#include <asm/sigcontext.h>

 #endif /* _ASM_X86_SIGCONTEXT32_H */

Then a nice solution would be for the kernel side wrapper to do like you
said (on top of your changes on master):

--- /dev/null
+++ b/arch/x86/include/asm/sigcontext.h
@@ -0,0 +1,6 @@
+#ifndef _ASM_X86_SIGCONTEXT_H
+#define _ASM_X86_SIGCONTEXT_H
+
+#include <uapi/asm/sigcontext.h>
+
+#endif /* _ASM_X86_SIGCONTEXT_H */

And I guess logically this belongs to commit "x86/headers: Remove
<asm/sigcontext.h>".

-Mikko