diff mbox series

[2/2] Makefile: run coccicheck on more source files

Message ID f62b0c7d1774cefc66e519430515eeb64acad1e0.1568101393.git.liu.denton@gmail.com (mailing list archive)
State New, archived
Headers show
Series Makefile: run coccicheck on all non-upstream sources | expand

Commit Message

Denton Liu Sept. 10, 2019, 7:44 a.m. UTC 
Before, when running the "coccicheck" target, only the source files
which were being compiled would have been checked by Coccinelle.
However, just because we aren't compiling a source file doesn't mean we
have to exclude it from analysis. This will allow us to catch more
mistakes, in particular ones that affect Windows-only sources since
Coccinelle currently runs only on Linux.

Make the "coccicheck" target run on all C sources except for those that
are taken from some upstream. We don't want to patch these files since
we want them to be as close to upstream as possible so that it'll be
easier to pull in upstream updates.

This results in the following sources now being checked:

* block-sha1/sha1.c
* compat/access.c
* compat/basename.c
* compat/fileno.c
* compat/gmtime.c
* compat/hstrerror.c
* compat/memmem.c
* compat/mingw.c
* compat/mkdir.c
* compat/mkdtemp.c
* compat/mmap.c
* compat/msvc.c
* compat/pread.c
* compat/precompose_utf8.c
* compat/qsort.c
* compat/setenv.c
* compat/sha1-chunked.c
* compat/snprintf.c
* compat/stat.c
* compat/strcasestr.c
* compat/strdup.c
* compat/strtoimax.c
* compat/strtoumax.c
* compat/unsetenv.c
* compat/win32/dirent.c
* compat/win32/path-utils.c
* compat/win32/pthread.c
* compat/win32/syslog.c
* compat/win32/trace2_win32_process_info.c
* compat/win32mmap.c
* compat/winansi.c
* ppc/sha1.c

This also results in the following source now being excluded:

* compat/obstack.c

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Makefile | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

SZEDER Gábor Sept. 10, 2019, 1:28 p.m. UTC | #1 
On Tue, Sep 10, 2019 at 12:44:31AM -0700, Denton Liu wrote:
> Before, when running the "coccicheck" target, only the source files
> which were being compiled would have been checked by Coccinelle.
> However, just because we aren't compiling a source file doesn't mean we
> have to exclude it from analysis. This will allow us to catch more
> mistakes, in particular ones that affect Windows-only sources since
> Coccinelle currently runs only on Linux.
> 
> Make the "coccicheck" target run on all C sources except for those that
> are taken from some upstream. We don't want to patch these files since
> we want them to be as close to upstream as possible so that it'll be
> easier to pull in upstream updates.
> 
> This results in the following sources now being checked:
> 
> * block-sha1/sha1.c
> * compat/access.c
> * compat/basename.c
> * compat/fileno.c
> * compat/gmtime.c
> * compat/hstrerror.c
> * compat/memmem.c
> * compat/mingw.c

When applying 'array.cocci' to 'compat/mingw.c' Coccinelle 1.0.4 (the
version shipped in Ubuntu 16.04, i.e. what is used in our static
analysis CI job) suggests to use the COPY_ARRAY macro instead of
memcpy():

diff -u -p a/compat/mingw.c b/compat/mingw.c
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1265,7 +1265,7 @@ static wchar_t *make_environment_block(c
 		}
 
 		ALLOC_ARRAY(result, size);
-		memcpy(result, wenv, size * sizeof(*wenv));
+		COPY_ARRAY(result, wenv, size);
 		FreeEnvironmentStringsW(wenv);
 		return result;
 	}
@@ -1309,7 +1309,7 @@ static wchar_t *make_environment_block(c
 			continue;
 
 		size = wcslen(array[i]) + 1;
-		memcpy(p, array[i], size * sizeof(*p));
+		COPY_ARRAY(p, array[i], size);
 		p += size;
 	}
 	*p = L'\0';

This patch series should apply these suggestions in preparatory
patches to make sure that our CI builds still pass.

> * compat/mkdir.c
> * compat/mkdtemp.c
> * compat/mmap.c
> * compat/msvc.c
> * compat/pread.c
> * compat/precompose_utf8.c
> * compat/qsort.c
> * compat/setenv.c
> * compat/sha1-chunked.c
> * compat/snprintf.c
> * compat/stat.c
> * compat/strcasestr.c
> * compat/strdup.c
> * compat/strtoimax.c
> * compat/strtoumax.c
> * compat/unsetenv.c
> * compat/win32/dirent.c
> * compat/win32/path-utils.c
> * compat/win32/pthread.c
> * compat/win32/syslog.c
> * compat/win32/trace2_win32_process_info.c
> * compat/win32mmap.c
> * compat/winansi.c
> * ppc/sha1.c
> 
> This also results in the following source now being excluded:
> 
> * compat/obstack.c

Why mention this file here, and, more importantly, why mention _only_
this file here?  According to the previous patch, there should be a
few more excluded files...

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  Makefile | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 708df2c289..d468b7c9c4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2802,12 +2802,8 @@ check: command-list.h
>  		exit 1; \
>  	fi
>  
> -C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
> -ifdef DC_SHA1_SUBMODULE
> -COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))
> -else
> -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))
> -endif
> +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
> +COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))
>  
>  %.cocci.patch: %.cocci $(COCCI_SOURCES)
>  	@echo '    ' SPATCH $<; \
> -- 
> 2.23.0.248.g3a9dd8fb08
>
SZEDER Gábor Sept. 10, 2019, 2:18 p.m. UTC | #2 
On Tue, Sep 10, 2019 at 12:44:31AM -0700, Denton Liu wrote:
> Make the "coccicheck" target run on all C sources except for those that
> are taken from some upstream. We don't want to patch these files since
> we want them to be as close to upstream as possible so that it'll be
> easier to pull in upstream updates.

> diff --git a/Makefile b/Makefile
> index 708df2c289..d468b7c9c4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2802,12 +2802,8 @@ check: command-list.h
>  		exit 1; \
>  	fi
>  
> -C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
> -ifdef DC_SHA1_SUBMODULE
> -COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))
> -else
> -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))
> -endif
> +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))

Hrm, so this uses FIND_SOURCE_FILES, which first attempts to run 'git
ls-files' and if that fails it falls back to run 'find'.

Unfortunately, the output of the two slightly differ: 'git ls-files'
prints 'abspath.c advice.c alias.c ...' why 'find' prints
'./upload-pack.c ./unpack-trees.c ./gpg-interface.c ...'.  Now, while
the order of files doesn't matter, the './' prefix does, because:

> +COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))

Here the paths/patterns in UPSTREAM_SOURCES don't have that './'
prefix, and thus won't match and won't filter out any of the upstream
files that they are supposed to.  IOW, if someone runs 'make
coccicheck' on a system without Git installed, then Coccinelle will
check all upstream sources as well, and will e.g. suggest using
COPY_ARRAY in 'compat/regex/regexec.c'.

Now, running 'make coccicheck' on a git.git clone without Git
installed might look like quite a pathological case on the first
sight, but I would argue that it is not that pathological: e.g.
consider someone running the recent Coccinelle version in a small-ish
Docker image containing just enough to run 'make coccicheck', but not
Git.  (yeah, you guessed right, I am that someone ;)

I don't know how to convince 'find' to omit that './' prefix from each
listed file, and in a portable way at that.  Piping its output through
'sed' or even 'cut' easily takes care of it, though.

>  %.cocci.patch: %.cocci $(COCCI_SOURCES)
>  	@echo '    ' SPATCH $<; \
> -- 
> 2.23.0.248.g3a9dd8fb08
>
Denton Liu Sept. 10, 2019, 4:07 p.m. UTC | #3 
On Tue, Sep 10, 2019 at 03:28:13PM +0200, SZEDER Gábor wrote:
> On Tue, Sep 10, 2019 at 12:44:31AM -0700, Denton Liu wrote:
> > Before, when running the "coccicheck" target, only the source files
> > which were being compiled would have been checked by Coccinelle.
> > However, just because we aren't compiling a source file doesn't mean we
> > have to exclude it from analysis. This will allow us to catch more
> > mistakes, in particular ones that affect Windows-only sources since
> > Coccinelle currently runs only on Linux.
> > 
> > Make the "coccicheck" target run on all C sources except for those that
> > are taken from some upstream. We don't want to patch these files since
> > we want them to be as close to upstream as possible so that it'll be
> > easier to pull in upstream updates.
> > 
> > This results in the following sources now being checked:
> > 
> > * block-sha1/sha1.c
> > * compat/access.c
> > * compat/basename.c
> > * compat/fileno.c
> > * compat/gmtime.c
> > * compat/hstrerror.c
> > * compat/memmem.c
> > * compat/mingw.c
> 
> When applying 'array.cocci' to 'compat/mingw.c' Coccinelle 1.0.4 (the
> version shipped in Ubuntu 16.04, i.e. what is used in our static
> analysis CI job) suggests to use the COPY_ARRAY macro instead of
> memcpy():
> 
> diff -u -p a/compat/mingw.c b/compat/mingw.c
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1265,7 +1265,7 @@ static wchar_t *make_environment_block(c
>  		}
>  
>  		ALLOC_ARRAY(result, size);
> -		memcpy(result, wenv, size * sizeof(*wenv));
> +		COPY_ARRAY(result, wenv, size);
>  		FreeEnvironmentStringsW(wenv);
>  		return result;
>  	}
> @@ -1309,7 +1309,7 @@ static wchar_t *make_environment_block(c
>  			continue;
>  
>  		size = wcslen(array[i]) + 1;
> -		memcpy(p, array[i], size * sizeof(*p));
> +		COPY_ARRAY(p, array[i], size);
>  		p += size;
>  	}
>  	*p = L'\0';
> 
> This patch series should apply these suggestions in preparatory
> patches to make sure that our CI builds still pass.

This patch series depends on the "dl/compat-cleanup" branch, which
should've already made these changes.

> 
> > * compat/mkdir.c
> > * compat/mkdtemp.c
> > * compat/mmap.c
> > * compat/msvc.c
> > * compat/pread.c
> > * compat/precompose_utf8.c
> > * compat/qsort.c
> > * compat/setenv.c
> > * compat/sha1-chunked.c
> > * compat/snprintf.c
> > * compat/stat.c
> > * compat/strcasestr.c
> > * compat/strdup.c
> > * compat/strtoimax.c
> > * compat/strtoumax.c
> > * compat/unsetenv.c
> > * compat/win32/dirent.c
> > * compat/win32/path-utils.c
> > * compat/win32/pthread.c
> > * compat/win32/syslog.c
> > * compat/win32/trace2_win32_process_info.c
> > * compat/win32mmap.c
> > * compat/winansi.c
> > * ppc/sha1.c
> > 
> > This also results in the following source now being excluded:
> > 
> > * compat/obstack.c
> 
> Why mention this file here, and, more importantly, why mention _only_
> this file here?  According to the previous patch, there should be a
> few more excluded files...

I guess this commit message is pretty weak since this only really
applies to my system. I'll mention in a reroll that this was for a
standard build with no additional flags on Arch Linux to be specific.

I got these lists by dumping the contents of $(COCCI_SOURCES) before and
after applying the patch and, with my "standard" build, it seems like
the only upstream file that was being checked before was
compat/obstack.c.

I don't have build options like, for example, NO_REGEX enabled so the
regex stuff wasn't being built in the first place but for users that
might want to run coccicheck on such a platform (do they even exist?),
this would benefit them.

> 
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> >  Makefile | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 708df2c289..d468b7c9c4 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2802,12 +2802,8 @@ check: command-list.h
> >  		exit 1; \
> >  	fi
> >  
> > -C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
> > -ifdef DC_SHA1_SUBMODULE
> > -COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))
> > -else
> > -COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))
> > -endif
> > +FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
> > +COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))
> >  
> >  %.cocci.patch: %.cocci $(COCCI_SOURCES)
> >  	@echo '    ' SPATCH $<; \
> > -- 
> > 2.23.0.248.g3a9dd8fb08
> >
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 708df2c289..d468b7c9c4 100644
--- a/Makefile
+++ b/Makefile
@@ -2802,12 +2802,8 @@  check: command-list.h
 		exit 1; \
 	fi
 
-C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
-ifdef DC_SHA1_SUBMODULE
-COCCI_SOURCES = $(filter-out sha1collisiondetection/%,$(C_SOURCES))
-else
-COCCI_SOURCES = $(filter-out sha1dc/%,$(C_SOURCES))
-endif
+FIND_C_SOURCES = $(filter %.c,$(shell $(FIND_SOURCE_FILES)))
+COCCI_SOURCES = $(filter-out $(UPSTREAM_SOURCES),$(FIND_C_SOURCES))
 
 %.cocci.patch: %.cocci $(COCCI_SOURCES)
 	@echo '    ' SPATCH $<; \