[PATCH] x86: break mutual header inclusion

[PATCH] x86: break mutual header inclusion

[Vegard Nossum]

Hi,

What do you think about this? The new file (vm86_mask.h) could actually be
embedded completely in processor-flags.h, but I went for what I believe is
the safer approach.

This has been compile tested on x86_32 and x86_64 defconfig.


Vegard


From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Tue, 27 May 2008 21:38:35 +0200
Subject: [PATCH] x86: break mutual header inclusion

This breaks up the mutual inclusion between headers ptrace.h and vm86.h
by moving what's needed by both files into a new (minimal) file
vm86_mask.h.

We also move #include lines to the top.

Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 include/asm-x86/ptrace.h    |   10 +++-------
 include/asm-x86/vm86.h      |    9 ++-------
 include/asm-x86/vm86_mask.h |   12 ++++++++++++
 3 files changed, 17 insertions(+), 14 deletions(-)
 create mode 100644 include/asm-x86/vm86_mask.h

diff --git a/include/asm-x86/ptrace.h b/include/asm-x86/ptrace.h
index 9f922b0..7ba799d 100644
--- a/include/asm-x86/ptrace.h
+++ b/include/asm-x86/ptrace.h
@@ -2,8 +2,10 @@
 #define _ASM_X86_PTRACE_H
 
 #include <linux/compiler.h>	/* For __user */
+#include <asm/ds.h>		/* the DS BTS struct is used for ptrace too */
+#include <asm/segment.h>
 #include <asm/ptrace-abi.h>
-
+#include <asm/vm86_mask.h>
 
 #ifndef __ASSEMBLY__
 
@@ -55,9 +57,6 @@ struct pt_regs {
 	unsigned long ss;
 };
 
-#include <asm/vm86.h>
-#include <asm/segment.h>
-
 #endif /* __KERNEL__ */
 
 #else /* __i386__ */
@@ -127,9 +126,6 @@ struct pt_regs {
 
 #ifdef __KERNEL__
 
-/* the DS BTS struct is used for ptrace as well */
-#include <asm/ds.h>
-
 struct task_struct;
 
 extern void ptrace_bts_take_timestamp(struct task_struct *, enum bts_qualifier);
diff --git a/include/asm-x86/vm86.h b/include/asm-x86/vm86.h
index 074b357..ad8d143 100644
--- a/include/asm-x86/vm86.h
+++ b/include/asm-x86/vm86.h
@@ -13,12 +13,8 @@
  */
 
 #include <asm/processor-flags.h>
-
-#ifdef CONFIG_VM86
-#define X86_VM_MASK	X86_EFLAGS_VM
-#else
-#define X86_VM_MASK	0 /* No VM86 support */
-#endif
+#include <asm/ptrace.h>
+#include <asm/vm86_mask.h>
 
 #define BIOSSEG		0x0f000
 
@@ -141,7 +137,6 @@ struct vm86plus_struct {
  * at the end of the structure. Look at ptrace.h to see the "normal"
  * setup. For user space layout see 'struct vm86_regs' above.
  */
-#include <asm/ptrace.h>
 
 struct kernel_vm86_regs {
 /*
diff --git a/include/asm-x86/vm86_mask.h b/include/asm-x86/vm86_mask.h
new file mode 100644
index 0000000..5f18288
--- /dev/null
+++ b/include/asm-x86/vm86_mask.h
@@ -0,0 +1,12 @@
+#ifndef ASM_X86_VM86_MASK
+#define ASM_X86_VM86_MASK
+
+#include <asm/processor-flags.h>
+
+#ifdef CONFIG_VM86
+#define X86_VM_MASK	X86_EFLAGS_VM
+#else
+#define X86_VM_MASK	0 /* No VM86 support */
+#endif
+
+#endif
-- 
1.5.4.1

Re: [PATCH] x86: break mutual header inclusion

[Adrian Bunk]

On Tue, May 27, 2008 at 10:49:26PM +0200, Vegard Nossum wrote:
> Hi,
> 
> What do you think about this? The new file (vm86_mask.h) could actually be
> embedded completely in processor-flags.h, but I went for what I believe is
> the safer approach.

Breaking the mutual header inclusion is appreciated, but some comments 
are below.

> This has been compile tested on x86_32 and x86_64 defconfig.
> 
> Vegard
>...
> --- a/include/asm-x86/ptrace.h
> +++ b/include/asm-x86/ptrace.h
> @@ -2,8 +2,10 @@
>  #define _ASM_X86_PTRACE_H
>  
>  #include <linux/compiler.h>	/* For __user */
> +#include <asm/ds.h>		/* the DS BTS struct is used for ptrace too */
> +#include <asm/segment.h>
>  #include <asm/ptrace-abi.h>
> -
> +#include <asm/vm86_mask.h>
>  
>  #ifndef __ASSEMBLY__
>  
> @@ -55,9 +57,6 @@ struct pt_regs {
>  	unsigned long ss;
>  };
>  
> -#include <asm/vm86.h>
> -#include <asm/segment.h>
> -
>  #endif /* __KERNEL__ */
>  
>  #else /* __i386__ */
> @@ -127,9 +126,6 @@ struct pt_regs {
>  
>  #ifdef __KERNEL__
>  
> -/* the DS BTS struct is used for ptrace as well */
> -#include <asm/ds.h>
> -
>  struct task_struct;
>  
>  extern void ptrace_bts_take_timestamp(struct task_struct *, enum bts_qualifier);

Moving #include's out of an #ifdef __KERNEL__ can (and does here) break
our userspace headers.

Running "make headers_check" after touching anything under include/ is
recommended since it catches these problems.

>...
> --- /dev/null
> +++ b/include/asm-x86/vm86_mask.h
> @@ -0,0 +1,12 @@
> +#ifndef ASM_X86_VM86_MASK
> +#define ASM_X86_VM86_MASK
> +
> +#include <asm/processor-flags.h>
> +
> +#ifdef CONFIG_VM86
> +#define X86_VM_MASK	X86_EFLAGS_VM
> +#else
> +#define X86_VM_MASK	0 /* No VM86 support */
> +#endif
> +
> +#endif

Do we need a new header for this or can it go into processor-flags.h ?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] x86: break mutual header inclusion

[Vegard Nossum]

Hi!

On Tue, May 27, 2008 at 11:38 PM, Adrian Bunk <bunk@kernel.org> wrote:
> On Tue, May 27, 2008 at 10:49:26PM +0200, Vegard Nossum wrote:
>> Hi,
>>
>> What do you think about this? The new file (vm86_mask.h) could actually be
>> embedded completely in processor-flags.h, but I went for what I believe is
>> the safer approach.
>
> Breaking the mutual header inclusion is appreciated, but some comments
> are below.

[...]

>> -/* the DS BTS struct is used for ptrace as well */
>> -#include <asm/ds.h>
>> -
>>  struct task_struct;
>>
>>  extern void ptrace_bts_take_timestamp(struct task_struct *, enum bts_qualifier);
>
> Moving #include's out of an #ifdef __KERNEL__ can (and does here) break
> our userspace headers.
>
> Running "make headers_check" after touching anything under include/ is
> recommended since it catches these problems.
>

Ah, that's true. My mistake.

  CHECK   include/asm/ptrace.h
[...]/usr/include/asm/ptrace.h requires asm/ds.h, which does not exist
in exported headers

What do you reckon is better here, to put #ifdef __KERNEL__ around the
whole of ds.h and export it, or put #ifdef __KERNEL__ around the
#include in ptrace.h? It feels wasteful to put it around the whole
ds.h, since what is the point in exporting an empty file? On the other
hand, it makes much more sense to put it there than in the "caller" in
order to avoid the duplication of #ifdef __KERNEL__. So unless you
prefer otherwise, I will choose the former (to export ds.h as well).

I hope you agree with the idea of moving the #include line to the top
of the file, however?

Thanks for the tip, that is very useful. I didn't realize I should use
that and I'm sorry for wasting your time with the first attempt :-(

>>...
>> --- /dev/null
>> +++ b/include/asm-x86/vm86_mask.h
>> @@ -0,0 +1,12 @@
>> +#ifndef ASM_X86_VM86_MASK
>> +#define ASM_X86_VM86_MASK
>> +
>> +#include <asm/processor-flags.h>
>> +
>> +#ifdef CONFIG_VM86
>> +#define X86_VM_MASK  X86_EFLAGS_VM
>> +#else
>> +#define X86_VM_MASK  0 /* No VM86 support */
>> +#endif
>> +
>> +#endif
>
> Do we need a new header for this or can it go into processor-flags.h ?

It can go in there, yes. I will do that in the re-worked (hopefully
correct) patch.

This business of breaking cycles is a bit nasty. There are very many
traps to fall into, very many setups to break, and very little support
from the tools to help you get it right (though with 'make
headers_check' I am a little better off).

Thanks for the help :-)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH] x86: break mutual header inclusion

[Adrian Bunk]

On Wed, May 28, 2008 at 12:01:02AM +0200, Vegard Nossum wrote:
> Hi!
> 
> On Tue, May 27, 2008 at 11:38 PM, Adrian Bunk <bunk@kernel.org> wrote:
> > On Tue, May 27, 2008 at 10:49:26PM +0200, Vegard Nossum wrote:
> >> Hi,
> >>
> >> What do you think about this? The new file (vm86_mask.h) could actually be
> >> embedded completely in processor-flags.h, but I went for what I believe is
> >> the safer approach.
> >
> > Breaking the mutual header inclusion is appreciated, but some comments
> > are below.
> 
> [...]
> 
> >> -/* the DS BTS struct is used for ptrace as well */
> >> -#include <asm/ds.h>
> >> -
> >>  struct task_struct;
> >>
> >>  extern void ptrace_bts_take_timestamp(struct task_struct *, enum bts_qualifier);
> >
> > Moving #include's out of an #ifdef __KERNEL__ can (and does here) break
> > our userspace headers.
> >
> > Running "make headers_check" after touching anything under include/ is
> > recommended since it catches these problems.
> >
> 
> Ah, that's true. My mistake.
> 
>   CHECK   include/asm/ptrace.h
> [...]/usr/include/asm/ptrace.h requires asm/ds.h, which does not exist
> in exported headers
> 
> What do you reckon is better here, to put #ifdef __KERNEL__ around the
> whole of ds.h and export it, or put #ifdef __KERNEL__ around the
> #include in ptrace.h? It feels wasteful to put it around the whole
> ds.h, since what is the point in exporting an empty file? On the other
> hand, it makes much more sense to put it there than in the "caller" in
> order to avoid the duplication of #ifdef __KERNEL__. So unless you
> prefer otherwise, I will choose the former (to export ds.h as well).

Our userspace headers are an API, and offering empty previsously 
unexported headers to userspace sounds like the worse choice to me.

Additionally, asm/segment.h (and asm/vm86_mask.h) have the same problem.

> I hope you agree with the idea of moving the #include line to the top
> of the file, however?

For me it looks good.

> Thanks for the tip, that is very useful. I didn't realize I should use
> that and I'm sorry for wasting your time with the first attempt :-(

No problem, and I don't consider it a waste of time.

>...
> Thanks for the help :-)
> 
> Vegard

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed