[PATCH] make sparse headers self-compilable...

[PATCH] make sparse headers self-compilable...

[Kamil Dudka]

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

Hello,

attached is another fix for SPARSE headers improving their sanity. I am
also attaching a simple test for the dependency tracking of headers. It's
not enough generic and portable and therefore not really useful. Maybe 
someone skilled in writing makefiles might want to include something like
that to the SPARSE Makefile as part of the 'check' target.

Kamil

[-- Attachment #2: 0001-make-sparse-headers-self-compilable.patch --]
[-- Type: text/x-diff, Size: 2053 bytes --]

From 8ffbb46652f69e662ce39c135f1867599c0f5477 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Fri, 7 Aug 2009 21:48:44 +0200
Subject: [PATCH] make sparse headers self-compilable...

... and thus possible to include them in arbitrary order and without any
external dependencies.

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 compile.h |    2 ++
 flow.h    |    2 ++
 ptrlist.h |    2 ++
 scope.h   |    2 ++
 storage.h |    2 ++
 5 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/compile.h b/compile.h
index 0db8fdb..6c5260d 100644
--- a/compile.h
+++ b/compile.h
@@ -1,6 +1,8 @@
 #ifndef COMPILE_H
 #define COMPILE_H
 
+#include "symbol.h"
+
 extern void emit_one_symbol(struct symbol *);
 extern void emit_unit_begin(const char *);
 extern void emit_unit_end(void);
diff --git a/flow.h b/flow.h
index 9f2e165..241f081 100644
--- a/flow.h
+++ b/flow.h
@@ -1,6 +1,8 @@
 #ifndef FLOW_H
 #define FLOW_H
 
+#include "symbol.h"
+
 extern unsigned long bb_generation;
 
 #define REPEAT_CSE		1
diff --git a/ptrlist.h b/ptrlist.h
index dae0906..85b23fc 100644
--- a/ptrlist.h
+++ b/ptrlist.h
@@ -7,6 +7,8 @@
  * (C) Copyright Linus Torvalds 2003-2005
  */
 
+#include <stdlib.h>
+
 #define container(ptr, type, member) \
 	(type *)((void *)(ptr) - offsetof(type, member))
 
diff --git a/scope.h b/scope.h
index 5f1f232..aff7b6b 100644
--- a/scope.h
+++ b/scope.h
@@ -9,6 +9,8 @@
  *  Licensed under the Open Software License version 1.1
  */
 
+#include "symbol.h"
+
 struct scope {
 	struct token *token;		/* Scope start information */
 	struct symbol_list *symbols;	/* List of symbols in this scope */
diff --git a/storage.h b/storage.h
index 610cbfd..0d3bbc5 100644
--- a/storage.h
+++ b/storage.h
@@ -1,6 +1,8 @@
 #ifndef STORAGE_H
 #define STORAGE_H
 
+#include "lib.h"
+
 /*
  * The "storage" that underlies an incoming/outgoing pseudo. It's
  * basically the backing store for a pseudo, and may be a real hardware
-- 
1.6.3.3


[-- Attachment #3: Makefile.chk --]
[-- Type: text/x-makefile, Size: 432 bytes --]

# all SPARSE headers (except ident-list.h which is not an ordinary header)
HEADERS ?= \
	allocate.h \
	bitmap.h \
	compat.h \
	compile.h \
	dissect.h \
	expression.h \
	flow.h \
	lib.h \
	linearize.h \
	parse.h \
	ptrlist.h \
	scope.h \
	storage.h \
	symbol.h \
	target.h \
	token.h \

SINK ?= /dev/null

.PHONY: check $(HEADERS)

# check whether each header is self-compilable
check: $(HEADERS)
$(HEADERS):
	$(CC) -o $(SINK) -c $@

Re: [PATCH] make sparse headers self-compilable...

[Kamil Dudka]

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

On Friday 07 of August 2009 22:27:08 Kamil Dudka wrote:
> attached is another fix for SPARSE headers improving their sanity. I am
> also attaching a simple test for the dependency tracking of headers. It's
> not enough generic and portable and therefore not really useful. Maybe
> someone skilled in writing makefiles might want to include something like
> that to the SPARSE Makefile as part of the 'check' target.

Attached are amended versions of the patch and the test. I didn't spot
some warnings before.

Kamil

diff --git a/storage.h b/storage.h
index 0d3bbc5..e049e23 100644
--- a/storage.h
+++ b/storage.h
@@ -1,6 +1,7 @@
 #ifndef STORAGE_H
 #define STORAGE_H

+#include "allocate.h"
 #include "lib.h"

 /*
diff -up Makefile.chk.orig Makefile.chk
--- Makefile.chk.orig
+++ Makefile.chk
@@ -19,9 +19,11 @@ HEADERS ?= \

 SINK ?= /dev/null

+CFLAGS += -Wall -Werror
+
 .PHONY: check $(HEADERS)

 # check whether each header is self-compilable
 check: $(HEADERS)
 $(HEADERS):
-   $(CC) -o $(SINK) -c $@
+   $(CC) $(CFLAGS) -o $(SINK) -c $@

[-- Attachment #2: Makefile.chk --]
[-- Type: text/x-makefile, Size: 467 bytes --]

# all SPARSE headers (except ident-list.h which is not an ordinary header)
HEADERS ?= \
	allocate.h \
	bitmap.h \
	compat.h \
	compile.h \
	dissect.h \
	expression.h \
	flow.h \
	lib.h \
	linearize.h \
	parse.h \
	ptrlist.h \
	scope.h \
	storage.h \
	symbol.h \
	target.h \
	token.h \

SINK ?= /dev/null

CFLAGS += -Wall -Werror

.PHONY: check $(HEADERS)

# check whether each header is self-compilable
check: $(HEADERS)
$(HEADERS):
	$(CC) $(CFLAGS) -o $(SINK) -c $@

[-- Attachment #3: 0001-make-sparse-headers-self-compilable.patch --]
[-- Type: text/x-diff, Size: 2076 bytes --]

From 619088669e2333a8c5921d99befa8d72b322004d Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Sat, 8 Aug 2009 12:58:41 +0200
Subject: [PATCH] make sparse headers self-compilable...

... and thus possible to include them in arbitrary order and without any
external dependencies.

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 compile.h |    2 ++
 flow.h    |    2 ++
 ptrlist.h |    2 ++
 scope.h   |    2 ++
 storage.h |    3 +++
 5 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/compile.h b/compile.h
index 0db8fdb..6c5260d 100644
--- a/compile.h
+++ b/compile.h
@@ -1,6 +1,8 @@
 #ifndef COMPILE_H
 #define COMPILE_H
 
+#include "symbol.h"
+
 extern void emit_one_symbol(struct symbol *);
 extern void emit_unit_begin(const char *);
 extern void emit_unit_end(void);
diff --git a/flow.h b/flow.h
index 9f2e165..241f081 100644
--- a/flow.h
+++ b/flow.h
@@ -1,6 +1,8 @@
 #ifndef FLOW_H
 #define FLOW_H
 
+#include "symbol.h"
+
 extern unsigned long bb_generation;
 
 #define REPEAT_CSE		1
diff --git a/ptrlist.h b/ptrlist.h
index dae0906..85b23fc 100644
--- a/ptrlist.h
+++ b/ptrlist.h
@@ -7,6 +7,8 @@
  * (C) Copyright Linus Torvalds 2003-2005
  */
 
+#include <stdlib.h>
+
 #define container(ptr, type, member) \
 	(type *)((void *)(ptr) - offsetof(type, member))
 
diff --git a/scope.h b/scope.h
index 5f1f232..aff7b6b 100644
--- a/scope.h
+++ b/scope.h
@@ -9,6 +9,8 @@
  *  Licensed under the Open Software License version 1.1
  */
 
+#include "symbol.h"
+
 struct scope {
 	struct token *token;		/* Scope start information */
 	struct symbol_list *symbols;	/* List of symbols in this scope */
diff --git a/storage.h b/storage.h
index 610cbfd..e049e23 100644
--- a/storage.h
+++ b/storage.h
@@ -1,6 +1,9 @@
 #ifndef STORAGE_H
 #define STORAGE_H
 
+#include "allocate.h"
+#include "lib.h"
+
 /*
  * The "storage" that underlies an incoming/outgoing pseudo. It's
  * basically the backing store for a pseudo, and may be a real hardware
-- 
1.6.4

Re: [PATCH] make sparse headers self-compilable...

[Christopher Li]

On Sat, Aug 8, 2009 at 4:10 AM, Kamil Dudka<kdudka@redhat.com> wrote:
> On Friday 07 of August 2009 22:27:08 Kamil Dudka wrote:
>> attached is another fix for SPARSE headers improving their sanity. I am
>> also attaching a simple test for the dependency tracking of headers. It's
>> not enough generic and portable and therefore not really useful. Maybe
>> someone skilled in writing makefiles might want to include something like
>> that to the SPARSE Makefile as part of the 'check' target.

What is the significant of making every header file self compilable?
Unlike the kernel header files exported to user space, which usually
have self contained meaning. Most of these header file have tight interaction
with each other. I don't think it make sense for other sparse application
to just use one of the header file.

Enforcing each header file to be self compilable will result in a lot
of unnecessary include. Gcc needs to include "symbol.h" many times
just to skip over it. Take a look at pre-process.c. It is not exactly
trivial. It needs to scan the token to find out the end of the if
scope. In this case, it might be better just let other header
file depend on "symbol.h".

I want to heard what other people think about it too. I am a little bit
reluctant on this one.

Chris

Re: [PATCH] make sparse headers self-compilable...

[Kamil Dudka]

On Tuesday 11 of August 2009 11:40:45 Christopher Li wrote:
> What is the significant of making every header file self compilable?
> Unlike the kernel header files exported to user space, which usually
> have self contained meaning. Most of these header file have tight
> interaction with each other. I don't think it make sense for other sparse
> application to just use one of the header file.
>
> Enforcing each header file to be self compilable will result in a lot
> of unnecessary include. Gcc needs to include "symbol.h" many times
> just to skip over it. Take a look at pre-process.c. It is not exactly
> trivial. It needs to scan the token to find out the end of the if
> scope. In this case, it might be better just let other header
> file depend on "symbol.h".

Then an ordinary SPARSE user needs to try various permutation of the includes 
to pass his hello-world through compiler. It could be really annoying. I'd 
like to have all includes in alphabetical order because it's then easier to 
maintenance.

If you really want to have external dependencies of SPARSE headers (no matter 
what reason for), the dependencies should be exactly documented somewhere - 
maybe in the headers themselves?

What about replacing my '#include "allocate.h"' with comment '// ATTENTION: 
you have to include allocate.h before storage.h'?

> I want to heard what other people think about it too. I am a little bit
> reluctant on this one.

No problem with me as it has easy workaround.

Kamil

Re: [PATCH] make sparse headers self-compilable...

[Kamil Dudka]

On Tuesday 11 of August 2009 11:55:19 Kamil Dudka wrote:
> On Tuesday 11 of August 2009 11:40:45 Christopher Li wrote:
> > What is the significant of making every header file self compilable?
> > Unlike the kernel header files exported to user space, which usually
> > have self contained meaning. Most of these header file have tight
> > interaction with each other. I don't think it make sense for other sparse
> > application to just use one of the header file.
> >
> > Enforcing each header file to be self compilable will result in a lot
> > of unnecessary include. Gcc needs to include "symbol.h" many times
> > just to skip over it. Take a look at pre-process.c. It is not exactly
> > trivial. It needs to scan the token to find out the end of the if
> > scope. In this case, it might be better just let other header
> > file depend on "symbol.h".

I agree processing of multiple-includes could be painful for the SPARSE 
preprocessor. But you don't solve it by avoiding multiple-includes in the 
SPARSE code itself. If you want to pass the SPARSE code through SPARSE, it's 
small enough to not bother us with performance.

But you have absolutely no control of the foreign code processed by SPARSE
and there the performance impact could be significant. I think this is the
way to go:

http://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html

Kamil

Re: [PATCH] make sparse headers self-compilable...

[Hannes Eder]

On Tue, Aug 11, 2009 at 11:40, Christopher Li<sparse@chrisli.org> wrote:
> On Sat, Aug 8, 2009 at 4:10 AM, Kamil Dudka<kdudka@redhat.com> wrote:
>> On Friday 07 of August 2009 22:27:08 Kamil Dudka wrote:
>>> attached is another fix for SPARSE headers improving their sanity. I am
>>> also attaching a simple test for the dependency tracking of headers. It's
>>> not enough generic and portable and therefore not really useful. Maybe
>>> someone skilled in writing makefiles might want to include something like
>>> that to the SPARSE Makefile as part of the 'check' target.
>
> What is the significant of making every header file self compilable?
> Unlike the kernel header files exported to user space, which usually
> have self contained meaning. Most of these header file have tight interaction
> with each other. I don't think it make sense for other sparse application
> to just use one of the header file.
>
> Enforcing each header file to be self compilable will result in a lot
> of unnecessary include. Gcc needs to include "symbol.h" many times
> just to skip over it. Take a look at pre-process.c. It is not exactly
> trivial. It needs to scan the token to find out the end of the if
> scope. In this case, it might be better just let other header
> file depend on "symbol.h".
>
> I want to heard what other people think about it too. I am a little bit
> reluctant on this one.

What about just using some forward decls instead of including the header files?

e.g. for compile.h it is enough to have a "struct symbol;" there
instead of the "#include <symbol.h>" to make it self-compilable.

Cheers,
-Hannes

Re: [PATCH] make sparse headers self-compilable...

[Kamil Dudka]

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

On Tuesday 11 of August 2009 14:48:39 Hannes Eder wrote:
> What about just using some forward decls instead of including the header
> files?
>
> e.g. for compile.h it is enough to have a "struct symbol;" there
> instead of the "#include <symbol.h>" to make it self-compilable.

Thanks for the hint! It's definitely better, though not sufficient in all 
cases. The amended patch is attached.

Kamil

[-- Attachment #2: 0001-make-sparse-headers-self-compilable.patch --]
[-- Type: text/x-diff, Size: 2218 bytes --]

From 93455c6fe7dc84e1519bec70a9fd8625a879d791 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Tue, 11 Aug 2009 14:56:53 +0200
Subject: [PATCH] make sparse headers self-compilable...

... and thus possible to include them in arbitrary order and without any
external dependencies.

Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 compile.h |    2 ++
 flow.h    |    5 +++++
 ptrlist.h |    2 ++
 scope.h   |    2 ++
 storage.h |    3 +++
 5 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/compile.h b/compile.h
index 0db8fdb..177363a 100644
--- a/compile.h
+++ b/compile.h
@@ -1,6 +1,8 @@
 #ifndef COMPILE_H
 #define COMPILE_H
 
+struct symbol;
+
 extern void emit_one_symbol(struct symbol *);
 extern void emit_unit_begin(const char *);
 extern void emit_unit_end(void);
diff --git a/flow.h b/flow.h
index fbc9505..370aadd 100644
--- a/flow.h
+++ b/flow.h
@@ -1,11 +1,16 @@
 #ifndef FLOW_H
 #define FLOW_H
 
+#include "lib.h"
+
 extern unsigned long bb_generation;
 
 #define REPEAT_CSE		1
 #define REPEAT_SYMBOL_CLEANUP	2
 
+struct entrypoint;
+struct instruction;
+
 extern int simplify_flow(struct entrypoint *ep);
 
 extern void simplify_symbol_usage(struct entrypoint *ep);
diff --git a/ptrlist.h b/ptrlist.h
index dae0906..fbfc080 100644
--- a/ptrlist.h
+++ b/ptrlist.h
@@ -1,6 +1,8 @@
 #ifndef PTR_LIST_H
 #define PTR_LIST_H
 
+#include <stdlib.h>
+
 /*
  * Generic pointer list manipulation code. 
  *
diff --git a/scope.h b/scope.h
index 5f1f232..0fab286 100644
--- a/scope.h
+++ b/scope.h
@@ -9,6 +9,8 @@
  *  Licensed under the Open Software License version 1.1
  */
 
+struct symbol;
+
 struct scope {
 	struct token *token;		/* Scope start information */
 	struct symbol_list *symbols;	/* List of symbols in this scope */
diff --git a/storage.h b/storage.h
index 610cbfd..e049e23 100644
--- a/storage.h
+++ b/storage.h
@@ -1,6 +1,9 @@
 #ifndef STORAGE_H
 #define STORAGE_H
 
+#include "allocate.h"
+#include "lib.h"
+
 /*
  * The "storage" that underlies an incoming/outgoing pseudo. It's
  * basically the backing store for a pseudo, and may be a real hardware
-- 
1.6.4

Re: [PATCH] make sparse headers self-compilable...

[Christopher Li]

On Tue, Aug 11, 2009 at 3:40 AM, Kamil Dudka<kdudka@redhat.com> wrote:
> I agree processing of multiple-includes could be painful for the SPARSE
> preprocessor. But you don't solve it by avoiding multiple-includes in the
> SPARSE code itself. If you want to pass the SPARSE code through SPARSE, it's
> small enough to not bother us with performance.

I am worry about adding too much duplicated include slow down the
gcc compile of sparse itself. I know I am paranoid :-)

>
> But you have absolutely no control of the foreign code processed by SPARSE
> and there the performance impact could be significant. I think this is the
> way to go:
>
> http://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html

BTW, sparse does similar things already. That is what the stream->protect
was for.

Chris

Re: [PATCH] make sparse headers self-compilable...

[Christopher Li]

On Tue, Aug 11, 2009 at 5:59 AM, Kamil Dudka<kdudka@redhat.com> wrote:
> Thanks for the hint! It's definitely better, though not sufficient in all
> cases. The amended patch is attached.

Will apply.

Thanks

Chris

Re: [PATCH] make sparse headers self-compilable...

[Kamil Dudka]

On Tuesday 11 of August 2009 23:08:24 Christopher Li wrote:
> On Tue, Aug 11, 2009 at 3:40 AM, Kamil Dudka<kdudka@redhat.com> wrote:
> > I agree processing of multiple-includes could be painful for the SPARSE
> > preprocessor. But you don't solve it by avoiding multiple-includes in the
> > SPARSE code itself. If you want to pass the SPARSE code through SPARSE,
> > it's small enough to not bother us with performance.
>
> I am worry about adding too much duplicated include slow down the
> gcc compile of sparse itself. I know I am paranoid :-)

Tested just now (ccache turned off)...

With the original (not amended) patch:
$ make clean && time make -j9
real    0m0.966s
user    0m4.654s
sys     0m1.471s

With the amended patch:
$ make clean && time make -j9
real    0m0.989s
user    0m4.580s
sys     0m1.504s

Without any of my patches applied:
$ make clean && time make -j9
real    0m0.980s
user    0m4.596s
sys     0m1.495s

Note that the results fluctuated approx 10% during runs.

My conclusion: absolutely no impact on gcc compile time.

Kamil