Re: [PATCH 2/2] Makefile: run coccicheck on more source files
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 > > --- > > 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 > >
Re: [PATCH 2/2] Makefile: run coccicheck on more source files
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 >
Re: [PATCH 2/2] Makefile: run coccicheck on more source files
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 > --- > 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 >
[PATCH 2/2] Makefile: run coccicheck on more source files
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 --- 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