| Message ID | 4A5131F4.7070007@ramsay1.demon.co.uk (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Sun, Jul 5, 2009 at 4:06 PM, Ramsay Jones<ramsay@ramsay1.demon.co.uk> wrote: > > pre-process.c:609 in function expand(): > Â Â Â Â struct arg args[nargs]; That is a known limit of sparse. It does not handle dynamic sized array. In kernel that is not a good thing because kernel stack is very limited. > sparse: > Â Â Â Â cgcc -no-compile $(ALL_CFLAGS) $(SPARSE_FLAGS) $(SRC_C) I think libgit2 should make sparse to compile the file one by one. Do them all at once might overflow your memory. > > this results in the follwing warnings (on cygwin): > > .../byteorder.h:39:1: warning: multiple definitions for function '__ntohl' > .../byteorder.h:39:1: Â the previous one is here > .../byteorder.h:56:1: warning: multiple definitions for function '__ntohs' > .../byteorder.h:56:1: Â the previous one is here GNU C has special treatment for "extern inline". It does not generate the stand alone copy of the function. Sparse does not handle that very well yet. I think the easiest way to fix is just run those source file in sparse one by one. > > The third hunk of the diff to parse.c actually triggers the problem, > but is not the real cause of the regression. I think that is the right thing to do. The problem is that sparse currently does not have way to mark a function as inline only without the standalone copy. > > I think the problem is mainly caused by bind_symbol() binding the > global_scope to some symbols. Indeed, the global_scope/file_scope > seems to be a bit confused and confusing. Note that the global_scope Global scope is for store global visible symbols. File scopes is use for file local symbols like static functions. > is not changed at all once intialised. In particular, each translation > unit keeps adding to the, one and only, global_scope; which is > effectively the same as the builtin_scope! What is wrong with only one global scope? Chris > Hmm, the following diff shows a quick fix: > > --->8--- > diff --git a/scope.c b/scope.c > index 27e38bc..fb4c039 100644 > --- a/scope.c > +++ b/scope.c > @@ -49,6 +49,7 @@ void start_file_scope(void) > Â Â Â Â /* top-level stuff defaults to file scope, "extern" etc will choose global scope */ > Â Â Â Â function_scope = scope; > Â Â Â Â block_scope = scope; > + Â Â Â start_scope(&global_scope); > Â } > > Â void start_symbol_scope(void) > @@ -87,6 +88,7 @@ static void end_scope(struct scope **s) > Â void end_file_scope(void) > Â { > Â Â Â Â end_scope(&file_scope); > + Â Â Â end_scope(&global_scope); > Â } > > Â void new_file_scope(void) > --->8--- > > But this is not the correct fix. In fact it may have broken c2xml > and ctags. I haven't checked, but look at the main() code in c2xml.c > and ctags.c; again the semantics of global_scope vs. file_scope > seems confused and confusing. At least to me. ;-) As you said, that is not the right fix. Ctags needs to use global scope to look for symbol has been define but does not used in the file. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Can you try the following patch? I move the extern inline function to file scope. Again, I still think it would be better let sparse check one file at a time. Chris
Christopher Li wrote: > Can you try the following patch? I move the extern inline function > to file scope. Again, I still think it would be better let sparse > check one file at a time. > This solves the problem on cygwin. I almost didn't test it on Linux, since it looked like this should work just fine... Unfortunately, the problem persists on Linux and I don't have time tonight to try and debug it; I will look into it further, hopefully tomorrow evening. ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 8, 2009 at 12:32 PM, Ramsay Jones<ramsay@ramsay1.demon.co.uk> wrote: > Unfortunately, the problem persists on Linux and I don't have time > tonight to try and debug it; I will look into it further, hopefully > tomorrow evening. If you can give me a small test case on Linux that will be great. I can pick it up from there. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christopher Li wrote: > On Wed, Jul 8, 2009 at 12:32 PM, Ramsay Jones<ramsay@ramsay1.demon.co.uk> wrote: >> Unfortunately, the problem persists on Linux and I don't have time >> tonight to try and debug it; I will look into it further, hopefully >> tomorrow evening. > > If you can give me a small test case on Linux that will be great. > I can pick it up from there. > [sorry for the late reply; something came up!] I forgot to mention that, on Linux, enabling optimization is required to trip this, since the extern inline function definitions are #ifdef'ed out otherwise. That does not affect the following test case, however. I've added to the previous test.c file (Note that the declaration of g() does not include __inline__): $ cat -n test.c 1 2 extern __inline__ int f(int); 3 4 extern __inline__ int 5 f(int x) 6 { 7 return x; 8 } 9 10 11 extern int g(int); 12 13 extern __inline__ int 14 g(int x) 15 { 16 return x; 17 } 18 $ ./sparse test.c $ ./sparse test.c test.c test.c:14:1: warning: multiple definitions for function 'g' test.c:14:1: the previous one is here $ cp test.c test-again.c $ ./sparse test.c test-again.c test-again.c:14:1: warning: multiple definitions for function 'g' test.c:14:1: the previous one is here $ Actually, the following may be closer to the situation on Linux: $ cat -n test.c 1 2 extern __inline__ int f(int); 3 4 extern __inline__ int 5 f(int x) 6 { 7 return x; 8 } 9 10 11 extern int g(int); 12 13 #ifdef __OPTIMIZE__ 14 extern __inline__ int 15 g(int x) 16 { 17 return x; 18 } 19 #endif 20 $ ./sparse test.c test.c $ ./sparse -O2 test.c test.c test.c:15:1: warning: multiple definitions for function 'g' test.c:15:1: the previous one is here HTH ATB Ramsay Jones extern __inline__ int f(int); extern __inline__ int f(int x) { return x; } extern int g(int); extern __inline__ int g(int x) { return x; }
On Fri, Jul 10, 2009 at 1:53 PM, Ramsay Jones<ramsay@ramsay1.demon.co.uk> wrote: > Â Â 11 Â extern int g(int); > Â Â 12 > Â Â 13 Â extern __inline__ int > Â Â 14 Â g(int x) > Â Â 15 Â { > Â Â 16 Â Â Â Â Â return x; > Â Â 17 Â } > Â Â 18 Err, that is ugly. I just find out what happen there. The none inline version picks up the function definition from the inline version because they are both "extern". Sparse only has one global extern scope. Later even the inline version get remove from the filescope, the second file still conflict with the non-inline version of the function body, which steal from the inline version. I add two lines to distinguish the "extern inline" vs "extern". They are not the same_symbol any more. Because effectly the "extern inline" is not visiable in the global "extern" scope. Any one see problem using this approach? I attach the patch follows. Can you give it a try? Chris
Christopher Li wrote: > I attach the patch follows. Can you give it a try? > Yep, this works. Tested on cygwin and Linux. Thanks! ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scope.c b/scope.c index 27e38bc..fb4c039 100644 --- a/scope.c +++ b/scope.c @@ -49,6 +49,7 @@ void start_file_scope(void) /* top-level stuff defaults to file scope, "extern" etc will choose global scope */ function_scope = scope; block_scope = scope; + start_scope(&global_scope); } void start_symbol_scope(void) @@ -87,6 +88,7 @@ static void end_scope(struct scope **s) void end_file_scope(void) { end_scope(&file_scope); + end_scope(&global_scope); } void new_file_scope(void)
Hi Chris, The recent patches from Linus caught my eye, since I have my own versions of two of them (the array restrict and transparent_union patches) in my sparse repo. (I suspect for the same reason; try to reduce the number of sparse warnings on git. I also have a quick hack to sorta-kinda implement transparent_union on glibc/Linux; new-lib/cygwin doesn't need it. I did manage to get to zero warnings at one point). So, I decided to fetch from your repo, in order to upgrade to the latest sparse (I was still using Josh's repo, commit e3bff51f, as the basis of my version). Having done so, I noticed a regression when using sparse on libgit2. <offtopic for="Chris, Junio"> I note that both sparse and git have only one "serious" sparse error; but it is effectively the same one. If you run sparse over itself you will find: pre-process.c:609:25: error: bad constant expression and for git (on platforms for which THREADED_DELTA_SEARCH is defined): builtin-pack-objects.c:1606:32: error: bad constant expression These errors relate to using the "dynamic local arrays" gcc extension (aka C99 VLAs); viz: pre-process.c:609 in function expand(): struct arg args[nargs]; builtin-pack-objects.c:1606 in function ll_find_deltas(): struct thread_params p[delta_search_threads]; So, whether this is actually a problem, depends on the project policy regarding this extension... </offtopic> In libgit2, the sparse Makefile target includes all source files (via the SRC_C macro) in a single invocation of cgcc, thus: sparse: cgcc -no-compile $(ALL_CFLAGS) $(SPARSE_FLAGS) $(SRC_C) this results in the follwing warnings (on cygwin): .../byteorder.h:39:1: warning: multiple definitions for function '__ntohl' .../byteorder.h:39:1: the previous one is here .../byteorder.h:56:1: warning: multiple definitions for function '__ntohs' .../byteorder.h:56:1: the previous one is here repeated 10 times. (Hint: it is significant that the number of source files is 11 [-1 = 10]). It is much much worse on Linux, where the glibc header files have many more extern inline function declarations. Note that the previous definition of __ntohl is at, er... ;-) However, if you run cgcc on each file separately, then no warnings are issued! To make things a little clearer, try this: $ cat -n test.c 1 2 extern __inline__ int f(int); 3 4 extern __inline__ int 5 f(int x) 6 { 7 return x; 8 } 9 $ ./sparse test.c $ ./sparse test.c test.c test.c:5:1: warning: multiple definitions for function 'f' test.c:5:1: the previous one is here $ cp test.c test-again.c $ ./sparse test.c test-again.c test-again.c:5:1: warning: multiple definitions for function 'f' test.c:5:1: the previous one is here $ So, the previous definition was in the previous translation unit! I then used "git bisect" to try and find the culprit, and it fingered: commit 0ed9c1290e9fd37e6a320d16c621beba202d422e Author: Al Viro <viro@ZenIV.linux.org.uk> Date: Mon Feb 2 07:30:19 2009 +0000 fun with declarations and definitions ... The third hunk of the diff to parse.c actually triggers the problem, but is not the real cause of the regression. I think the problem is mainly caused by bind_symbol() binding the global_scope to some symbols. Indeed, the global_scope/file_scope seems to be a bit confused and confusing. Note that the global_scope is not changed at all once intialised. In particular, each translation unit keeps adding to the, one and only, global_scope; which is effectively the same as the builtin_scope! Hmm, the following diff shows a quick fix: --->8--- --->8--- But this is not the correct fix. In fact it may have broken c2xml and ctags. I haven't checked, but look at the main() code in c2xml.c and ctags.c; again the semantics of global_scope vs. file_scope seems confused and confusing. At least to me. ;-) Note the comment in the first hunk above. I suspect that the global_scope was intended to be used to check the semantics of local extern declarations, which are effectively hoisted to global scope, in order the check the type of the re-declarations (and maybe re-definitions). (but it would be a separate symbol table, used only for the semantic checks). Hmm, you would also need to compose the two types, check the resulting linkage... Take a look at this example: $ cat extern-test.c int f(void) { extern float g(int); return 0; } int g(void) { return -1; } /* ERROR, conflicting type for g() */ int h(void) { extern double g(void); return 0; } /* ditto */ int x; /* the extern decl. below conflicts with this x, not param */ int k(int x) { { extern float x; x=1; } /* ERROR, conflicting type for x */ return x; /* this is the parameter x */ } static int y; /* y has internal linkage */ extern int y; /* OK, y still has internal linkage */ static int y; /* OK, y still has internal linkage */ int y; /* ERROR, y has internal linkage */ extern int z; /* z has external linkage */ extern int z; /* OK, z still has external linkage */ int z; /* OK, z still has external linkage */ static int z; /* ERROR, z has external linkage */ int w; /* w has external linkage */ extern int w; /* OK, w still has external linkage */ int w; /* OK, w still has external linkage */ static int w; /* ERROR, w has external linkage */ In order to get sparse to issue the correct errors, a lot more code would be needed. I'm sure I could add the necessary code eventually, but it would be better for someone who understands the code better than me to implement this... I prefer to provide solutions, but in this case I will have to make do with just reporting the issue :( Random question: I noticed the preprocessor symbols were defined frequently; every call to preprocess() in fact: is this intended? HTH ATB Ramsay Jones P.S. I've attached extern-test.c so you can try it out yourself. I've tried it with 5 different compilers and the results are very uneven! Older versions of gcc (circa 3.4) don't get it completely correct, for example. int f(void) { extern float g(int); return 0; } int g(void) { return -1; } /* ERROR, conflicting type for g() */ int h(void) { extern double g(void); return 0; } /* ditto */ int x; /* the extern decl. below conflicts with this x, not param */ int k(int x) { { extern float x; x=1; } /* ERROR, conflicting type for x */ return x; /* this is the parameter x */ } static int y; /* y has internal linkage */ extern int y; /* OK, y still has internal linkage */ static int y; /* OK, y still has internal linkage */ int y; /* ERROR, y has internal linkage */ extern int z; /* z has external linkage */ extern int z; /* OK, z still has external linkage */ int z; /* OK, z still has external linkage */ static int z; /* ERROR, z has external linkage */ int w; /* w has external linkage */ extern int w; /* OK, w still has external linkage */ int w; /* OK, w still has external linkage */ static int w; /* ERROR, w has external linkage */ extern __inline__ int f(int); extern __inline__ int f(int x) { return x; }