make sparse headers self-compilable...

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

From 8ffbb46652f69e662ce39c135f1867599c0f5477 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <[email protected]>
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.

---
 compile.h |    2 ++
 flow.h    |    2 ++
 ptrlist.h |    2 ++
 scope.h   |    2 ++
 storage.h |    2 ++
 5 files changed, 10 insertions(+), 0 deletions(-)

index 0db8fdb..6c5260d 100644
@@ -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);

index 9f2e165..241f081 100644
@@ -1,6 +1,8 @@
 #ifndef FLOW_H
 #define FLOW_H
 
+#include "symbol.h"
+
 extern unsigned long bb_generation;
 
 #define REPEAT_CSE		1

index dae0906..85b23fc 100644
@@ -7,6 +7,8 @@
  * (C) Copyright Linus Torvalds 2003-2005
  */
 
+#include <stdlib.h>
+
 #define container(ptr, type, member) \
 	(type *)((void *)(ptr) - offsetof(type, member))
 

index 5f1f232..aff7b6b 100644
@@ -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 */

index 610cbfd..0d3bbc5 100644
@@ -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

# 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 $@

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

Kamil

index 0d3bbc5..e049e23 100644
@@ -1,6 +1,7 @@
 #ifndef STORAGE_H
 #define STORAGE_H
 
+#include "allocate.h"
 #include "lib.h"
 
 /*

diff -up 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 $@

# 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 $@

From 619088669e2333a8c5921d99befa8d72b322004d Mon Sep 17 00:00:00 2001
From: Kamil Dudka <[email protected]>
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.

---
 compile.h |    2 ++
 flow.h    |    2 ++
 ptrlist.h |    2 ++
 scope.h   |    2 ++
 storage.h |    3 +++
 5 files changed, 11 insertions(+), 0 deletions(-)

index 0db8fdb..6c5260d 100644
@@ -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);

index 9f2e165..241f081 100644
@@ -1,6 +1,8 @@
 #ifndef FLOW_H
 #define FLOW_H
 
+#include "symbol.h"
+
 extern unsigned long bb_generation;
 
 #define REPEAT_CSE		1

index dae0906..85b23fc 100644
@@ -7,6 +7,8 @@
  * (C) Copyright Linus Torvalds 2003-2005
  */
 
+#include <stdlib.h>
+
 #define container(ptr, type, member) \
 	(type *)((void *)(ptr) - offsetof(type, member))
 

index 5f1f232..aff7b6b 100644
@@ -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 */

index 610cbfd..e049e23 100644
@@ -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

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

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'?

No problem with me as it has easy workaround.

Kamil

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

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

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

Chris

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

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

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

Kamil

From 93455c6fe7dc84e1519bec70a9fd8625a879d791 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <[email protected]>
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.

---
 compile.h |    2 ++
 flow.h    |    5 +++++
 ptrlist.h |    2 ++
 scope.h   |    2 ++
 storage.h |    3 +++
 5 files changed, 14 insertions(+), 0 deletions(-)

index 0db8fdb..177363a 100644
@@ -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);

index fbc9505..370aadd 100644
@@ -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);

index dae0906..fbfc080 100644
@@ -1,6 +1,8 @@
 #ifndef PTR_LIST_H
 #define PTR_LIST_H
 
+#include <stdlib.h>
+
 /*
  * Generic pointer list manipulation code. 
  *

index 5f1f232..0fab286 100644
@@ -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 */

index 610cbfd..e049e23 100644
@@ -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

Will apply.

Thanks

Chris