Skip to contentSkip to conversation

[2.5] [Cool stuff] "checking" mode for kernel builds

DISCUSSION Carl-Daniel Hailfinger 8 participants 13 messages May 27, 2003

lore
Share & tools
lore
#0 lore raw

Linus,

while eagerly waiting for the release of that tool, I have a few comments about the patch itself.

Quoted 13 lines

# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/07 [email protected] 1.1063.14.3
# Support a "checking" mode for kernel builds, that runs a

# user-supplied source checker on all C files before compiling
# them.
# 
# I'll release the actual checker once I've cleaned it up a
# bit more (yay, all the copyright paperwork completed!)
# --------------------------------------------
#
[...]

Review line 185

Quoted 4 lines
(quoted diff) +1 -0
@@ -172,6 +182,7 @@
 DEPMOD		= /sbin/depmod
 KALLSYMS	= scripts/kallsyms
 PERL		= perl
+CHECK		= /home/torvalds/parser/check

Hardcoded path

Review line 69

Quoted 8 lines
(quoted diff) +1 -0
 MODFLAGS	= -DMODULE
 CFLAGS_MODULE   = $(MODFLAGS)
 AFLAGS_MODULE   = $(MODFLAGS)
@@ -66,6 +66,12 @@
 	 $(subdir-ym) $(always)
 	@:
 
+# Linus's kernel sanity checking tool

IIRC my english lessons it should be
+# Linus' kernel sanity checking tool

Review line 77

Quoted 8 lines
(quoted diff) +5 -0
+ifneq ($(KBUILD_CHECKSRC),0)
+quiet_cmd_checksrc = CHECK   $<
+      cmd_checksrc = $(CHECK) $(CFLAGS) $< ;
+endif
+ 
 # Module versioning
 # ------------------------------------------------------
 

Regards,
Carl-Daniel

#1 lore raw
Quoted 5 lines
Quoted 2 lines

+# Linus's kernel sanity checking tool

IIRC my english lessons it should be
+# Linus' kernel sanity checking tool

It's just like if you want to quote a command like "chroot," which is confusing and nasty and ugly, whereas using "chroot", while not really correct, is The Right Thing To Do. We computer scientists have our own, *more precise* form(s) of our language(s). ^_^

#2 lore raw
In reply to Carl-Daniel Hailfinger
Quoted 3 lines · Carl-Daniel Hailfinger

while eagerly waiting for the release of that tool, I have a few comments about the patch itself.

Well, I guess I can just tell people about it. It's unfinished enough that I'm a bit embarrassed about some of it, but I've gotten the permission from Transmeta to make it open source, and it's actually been available for some time on "bk://kernel.bkbits.net/torvalds/sparse". I just didn't tell about it in public (although a few people have known about it).

Be gentle with it. It does many things wrong, including (but limited to) enums, but it does a lot of things right too.

The biggest problem (apart from the things it does wrong) is that some parts of the kenel re-use the same structure to hold both user- and kernel pointers. That's a big no-no for the static semantic parser, since it's literally a _static_ type-checker, and doesn't know about dynamic differences.

The main offender is the networking layer that sometimes keeps user pointers and sometimes kenrel pointers in a "struct iovec". David has promised to look into this, and seemed confident that he can fix it easily.

Most people who have used the tool actually like how it forces you to make it very _explicit_ whether you're using a user pointer or not. But I have to admit that I've grown tired of trying to look at all the uses and making sure which sparse warnings are valid and which aren't.

Linus

#3 lore raw

[Linus: Sorry that your e-mail to me bounced. Was a provider screwup, should be fixed now.]
[CC:ed kerneljanitors, who might be interested using it.
CC:ed Dan Carpenter, who mantains a set of tools similar to sparse, but
as a gcc patch at http://kbugs.org ]

Quoted 8 lines · Linus Torvalds

Well, I guess I can just tell people about it. It's unfinished enough that I'm a bit embarrassed about some of it, but I've gotten the permission from Transmeta to make it open source, and it's actually been available for some time on "bk://kernel.bkbits.net/torvalds/sparse". I just didn't tell about it in public (although a few people have known about it).

Be gentle with it. It does many things wrong, including (but limited to) enums, but it does a lot of things right too.

Playing with it right now.

Quoted 11 lines

The biggest problem (apart from the things it does wrong) is that some parts of the kenel re-use the same structure to hold both user- and kernel pointers. That's a big no-no for the static semantic parser, since it's literally a _static_ type-checker, and doesn't know about dynamic differences.

The main offender is the networking layer that sometimes keeps user pointers and sometimes kernel pointers in a "struct iovec". David has promised to look into this, and seemed confident that he can fix it easily.

Quoted 4 lines

Most people who have used the tool actually like how it forces you to make it very _explicit_ whether you're using a user pointer or not. But I have to admit that I've grown tired of trying to look at all the uses and making sure which sparse warnings are valid and which aren't.

Dan? IIRC you have tools to tackle this issue in a distributed manner.

Regards,
Carl-Daniel

#5 lore raw
In reply to Carl-Daniel Hailfinger
Quoted 4 lines · Carl-Daniel Hailfinger
Quoted 1 line

+# Linus's kernel sanity checking tool

IIRC my english lessons it should be
+# Linus' kernel sanity checking tool

BTW, it just follows the sound, and at least I say something sounding like `Linusez kernel' -- which would be written "Linus's kernel."

[in some cases, e.g. many plurals, you _do_ merge the two uses of S, e.g., "The Smiths' car"]

-Miles

Signature · 2 lines
I'm beginning to think that life is just one long Yoko Ono album; no rhyme
or reason, just a lot of incoherent shrieks and then it's over.  --Ian Wolff
#6 lore raw
In reply to Carl-Daniel Hailfinger
Quoted 3 lines · Carl-Daniel Hailfinger
Quoted 1 line

+CHECK = /home/torvalds/parser/check

Hardcoded path

Well, the checker its share of those problems:

const char *gcc_includepath[] = {
    "/usr/lib/gcc-lib/i386-redhat-linux/3.2.1/include",
    "/usr/lib/gcc-lib/i386-redhat-linux/3.2.2/include",
    NULL
};
#7 lore raw
Quoted 8 lines · Aaron Lehmann

Well, the checker its share of those problems:

const char *gcc_includepath[] = {
    "/usr/lib/gcc-lib/i386-redhat-linux/3.2.1/include",
    "/usr/lib/gcc-lib/i386-redhat-linux/3.2.2/include",
    NULL
};

Any takers? Some Makefile magic plus some hacky thing like

gcc -print-file-name=include

(Yeah, that's not righ either, it just happens to work. I don't know what the proper way of making gcc expose its local paths is).

So I'm doing what works for me, and open source (in this case the OSL) means that you can fix it and send me patches if you want to ;)

Btw, taling about it embarrassed me so much that I just fixed the enum confusion, so now a few less of the checker warnings are bogus. It still misparses some assembly (notably anything that is at the top level, not inside a function, and the kinds of asms used to rename variables).

And it doesn't handle pragmas, in particular "pragma pack" is hard to do right (well, from a _kernel_ checking standpoint it doesn't matter, but I do want it to actually generate a good parse tree too, which means that I try to get things like structure member offsets etc _right_. And as I currently just ignore - and warn about - "pragma pack", my type evaluation doesn't get the offsets/alignments right).

But _most_ of the warnings are because of type differences. Even those are sometimes bogus, though, so don't assume it's right.

Oh, before I forget: it also refuses to parse some gcc constructs that I personally don't like, like the gcc extension to make certain things lvalues even though they really aren't (ie casts and the ?: operator).

Even though I myself have been known to use those gcc extensions, I think they are _wrong_, since they don't actually buy you anything except for a dubious syntactic shorthand. It's the wrong kind of language extension.

Other - more worthwhile - extensions I do actually support.

Linus

#8 lore raw
Quoted 6 lines · Linus Torvalds

Any takers? Some Makefile magic plus some hacky thing like

gcc -print-file-name=include

(Yeah, that's not righ either, it just happens to work. I don't know what the proper way of making gcc expose its local paths is).

gcc -v will tell you the final include path, but only when you actually compile something. I'd probably make the makefile hackery parse the ouput of echo | gcc -v -E -. Yeah, it's ugly.

The output between "#include <...> search starts here:" and "End of search list." seems like the combination of what you want for gcc_includepath and sys_includepath. I assume the output is ordered. I might send a patch if I'm bored tonight.

#9 lore raw
Quoted 5 lines · Aaron Lehmann

The output between "#include <...> search starts here:" and "End of search list." seems like the combination of what you want for gcc_includepath and sys_includepath. I assume the output is ordered. I might send a patch if I'm bored tonight.

If it comes to parsing "gcc -v" output, I have to say that I personally find that to be just crazy, and I'd much rather just see the (incorrect but working) '-print-file-name=include' hack.

Note that to some degree the _better_ approach is to just make sparse-specific header files available to sparse itself, instead of making the checker try to look too much like gcc including using gcc's internal header files.

The kernel in particular only needs a very few files for the compiler, notably <stdarg.h>.

Of course, for debugging I especially initially used a lot of non-kernel files, and while my own personal interest is purely in the front-end, I'm actually hoping that some crack-smoking crazy person would actually write a back-end for it too, just for fun.

My own "test-parse.c" has it's own little back-end that outputs a very strange kind of pseudo-assembler, and that was absolutely _critical_ to finding a lot of parsing and evaluation bugs.

I guess I'm just a hopeless retard, but I initially tried to print out the parse tree with all the type information, and I couldn't make sense of it visually. Making a stupid back-end that outputs something that looks almost like real assembly language was a huge advantage for me, because it meant that I could mentally parse the output much better, and I found an incredible number of type evaluation bugs that way.

But that's clearly all my back-end is useful for.

Linus

#4 lore raw
In reply to Carl-Daniel Hailfinger
Quoted 8 lines · Carl-Daniel Hailfinger
Quoted 4 lines

Most people who have used the tool actually like how it forces you to make it very _explicit_ whether you're using a user pointer or not. But I have to admit that I've grown tired of trying to look at all the uses and making sure which sparse warnings are valid and which aren't.

Dan? IIRC you have tools to tackle this issue in a distributed manner.

Tracking bugs from one version to the next works pretty well for kbugs.org. Anyone can moderate the bugs as real or not. I haven't used sparse script yet, but I'll do that tomorrow. I'd be happy to post the results on kbugs.org. :)

regards,
dan carpenter

#10 lore raw
In reply to Linus Torvalds
Quoted 11 lines · Linus Torvalds
Quoted 5 lines · Aaron Lehmann

The output between "#include <...> search starts here:" and "End of search list." seems like the combination of what you want for gcc_includepath and sys_includepath. I assume the output is ordered. I might send a patch if I'm bored tonight.

If it comes to parsing "gcc -v" output, I have to say that I personally find that to be just crazy, and I'd much rather just see the (incorrect but working) '-print-file-name=include' hack.

How's this for a hack that makes it work a bit better?

# This is a BitKeeper generated patch for the following project:
# Project Name: TSCT - The Silly C Tokenizer
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:

#	           ChangeSet	1.344   -> 1.345  
#	       pre-process.c	1.62    -> 1.63   
#	            Makefile	1.19    -> 1.20   

#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/27 [email protected] 1.345
# This update (hopefully) will allow 'check' to find the appropriate, gcc-local

# include headers (dependent, of course, upon the system it was built on)
# 
# It's still a hack, but at least it will work in a few more places.
# --------------------------------------------
#
diff -Nru a/Makefile b/Makefile
--- a/Makefile	Tue May 27 05:10:56 2003
+++ b/Makefile	Tue May 27 05:10:56 2003
@@ -32,7 +32,7 @@
 expression.o: $(LIB_H)
 lib.o: $(LIB_H)
 parse.o: $(LIB_H)
-pre-process.o: $(LIB_H)
+pre-process.o: $(LIB_H) pre-process.h
 scope.o: $(LIB_H)
 show-parse.o: $(LIB_H)
 symbol.o: $(LIB_H)
@@ -40,5 +40,8 @@
 test-parsing.o: $(LIB_H)
 tokenize.o: $(LIB_H)

+pre-process.h:
+ echo "#define GCC_INTERNAL_INCLUDE \"gcc -print-file-name=include\"" > pre-process.h
+
clean:
- rm -f *.[oasi] core core.[0-9]* $(PROGRAMS)
+ rm -f *.[oasi] core core.[0-9]* $(PROGRAMS) pre-process.h

pre-process.c +2 -0
diff -Nru a/pre-process.c b/pre-process.c
@@ -18,6 +18,7 @@
 #include <fcntl.h>
 #include <limits.h>
 
+#include "pre-process.h"
 #include "lib.h"
 #include "parse.h"
 #include "token.h"
@@ -47,6 +48,7 @@
 const char *gcc_includepath[] = {
 	"/usr/lib/gcc-lib/i386-redhat-linux/3.2.1/include",
 	"/usr/lib/gcc-lib/i386-redhat-linux/3.2.2/include",
+	GCC_INTERNAL_INCLUDE,
 	NULL
 };
 
Signature · 2 lines
Ryan Anderson
  sometimes Pug Majere
#12 lore raw
In reply to Carl-Daniel Hailfinger

Carl-Daniel Hailfinger's quote:

| > +# Linus's kernel sanity checking tool
|
| IIRC my english lessons it should be
| +# Linus' kernel sanity checking tool

only if there is more than one linus. which we know is not only not the case but indeed not possible.

Signature · 4 lines
dep

http://www.linuxandmain.com -- outside the box, barely within
the envelope, and no animated paperclip anywhere.
#11 lore raw
In reply to Ryan Anderson
Quoted 2 lines · Ryan Anderson

How's this for a hack that makes it work a bit better?

Well, since it clearly isn't any worse than what I have now, I'll just say "hell yes!", and apply it.

In fact, I'll just make the old gcc paths go away entirely, as this should make them be non-issues.

Linus