patchUsing sparse in a CI job

I suspect that the Makefile sparse target is not easy to use in a CI job, since the 'sparse' program (via cgcc -no-compile) does not exit with a non-zero value, even when issuing errors and warnings.

The way I use sparse, this is not an issue. I run sparse over the master branch, check that the output file has the no/expected errors and warnings, then just diff the output files from the next and pu branches. something like:

  $ make sparse >sp-out 2>&1 # on master branch
  $ vim sp-out # check for errors/warnings
  ...
  $ make sparse >nsp-out 2>&1 # on next branch
  $ diff sp-out nsp-out # any new errors/warnings?
  ...
  $ make sparse >psp-out 2>&1 # on pu branch
  $ diff nsp-out psp-out # any new errors/warnings?

At the moment, on Linux, the sp-out file is free from any sparse errors or warnings. So are next and pu:

  $ grep error sp-out
  $ grep warning sp-out
  $ diff sp-out nsp-out
  391a392
  >     SP fuzz-commit-graph.c
  $ diff nsp-out psp-out
  170a171,180
  >     SP trace2.c
  >     SP trace2/tr2_cfg.c
  >     SP trace2/tr2_dst.c
  >     SP trace2/tr2_sid.c
  >     SP trace2/tr2_tbuf.c
  >     SP trace2/tr2_tgt_event.c
  >     SP trace2/tr2_tgt_normal.c
  >     SP trace2/tr2_tgt_perf.c
  >     SP trace2/tr2_tls.c
  >     SP trace2/tr2_verb.c
  298a309
  >     SP builtin/stash.c
  375a387
  >     SP t/helper/test-trace2.c
  376a389
  >     SP t/helper/test-xml-encode.c
  $ 

However, if we go back a few days, then the pu branch was not clean:

  $ diff nsp-out psp-out
  18a20,24
  >     SP change-table.c
  > change-table.h:53:24: error: dubious one-bit signed bitfield
  > change-table.h:54:25: error: dubious one-bit signed bitfield
  > change-table.h:55:25: error: dubious one-bit signed bitfield
  > change-table.h:56:26: error: dubious one-bit signed bitfield
  69a76
  >     GEN command-list.h
  93a101,106
  >     SP metacommit.c
  > change-table.h:53:24: error: dubious one-bit signed bitfield
  > change-table.h:54:25: error: dubious one-bit signed bitfield
  > change-table.h:55:25: error: dubious one-bit signed bitfield
  > change-table.h:56:26: error: dubious one-bit signed bitfield
  >     SP metacommit-parser.c
  170a184,193
  >     SP trace2.c
  >     SP trace2/tr2_cfg.c
  >     SP trace2/tr2_dst.c
  >     SP trace2/tr2_sid.c
  >     SP trace2/tr2_tbuf.c
  >     SP trace2/tr2_tgt_event.c
  >     SP trace2/tr2_tgt_normal.c
  >     SP trace2/tr2_tgt_perf.c
  >     SP trace2/tr2_tls.c
  >     SP trace2/tr2_verb.c
  213a237
  >     SP builtin/change.c
  298a323
  >     SP builtin/stash.c
  375a401
  >     SP t/helper/test-trace2.c
  376a403,405
  >     SP t/helper/test-xml-encode.c
  > t/helper/test-xml-encode.c:29:40: warning: Using plain integer as NULL pointer
  > t/helper/test-xml-encode.c:37:40: warning: Using plain integer as NULL pointer
  $ 
 
Note that 'make' does not exit at the first 'error', since the command exit
code is zero (or not non-zero! :) ):
 
  $ make change-table.sp
      SP change-table.c
  change-table.h:53:24: error: dubious one-bit signed bitfield
  change-table.h:54:25: error: dubious one-bit signed bitfield
  change-table.h:55:25: error: dubious one-bit signed bitfield
  change-table.h:56:26: error: dubious one-bit signed bitfield
  $ echo $?
  0
  $ 

We can change that by passing '-Wsparse-error' to 'sparse':

  $ make SPARSE_FLAGS=-Wsparse-error change-table.sp
      SP change-table.c
  change-table.h:53:24: error: dubious one-bit signed bitfield
  change-table.h:54:25: error: dubious one-bit signed bitfield
  change-table.h:55:25: error: dubious one-bit signed bitfield
  change-table.h:56:26: error: dubious one-bit signed bitfield
  Makefile:2729: recipe for target 'change-table.sp' failed
  make: *** [change-table.sp] Error 1
  $ echo $?
  2
  $ 

Note that '-Wsparse-error' not only returns a non-zero exit code (1), but
it also changes a 'warning' into an 'error' (see above):
  
  $ make SPARSE_FLAGS=-Wsparse-error t/helper/test-xml-encode.sp
      SP t/helper/test-xml-encode.c
  t/helper/test-xml-encode.c:29:40: error: Using plain integer as NULL pointer
  t/helper/test-xml-encode.c:37:40: error: Using plain integer as NULL pointer
  Makefile:2729: recipe for target 't/helper/test-xml-encode.sp' failed
  make: *** [t/helper/test-xml-encode.sp] Error 1
  $ echo $?
  2
  $ 

Unfortunately, using SPARSE_FLAGS from the 'make' command-line is not ideal, since it was not really intended to be used that way. This will cause problems for those files which have target specific settings for SPARSE_FLAGS. For example:

  $ make pack-revindex.sp
      SP pack-revindex.c
  $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp
      SP pack-revindex.c
  pack-revindex.c:65:23: error: memset with byte count of 262144
  Makefile:2729: recipe for target 'pack-revindex.sp' failed
  make: *** [pack-revindex.sp] Error 1
  $ echo $?
  2
  $ 

So, passing SPARSE_FLAGS on the command-line, effectively nullifies the target specific settings (making them useless).

This patch splits the duties of SPARSE_FLAGS, by introducing a separate SP_EXTRA_FLAGS variable, for use with the target specific settings. In addition, we use a conditional assignment (?=) of the default (empty) value of SPARSE_FLAGS, to allow setting the value of this variable from the environment as well as the command-line. So, after this patch:

  $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp
      SP pack-revindex.c
  $ echo $?
  0
  $ 

  $ SPARSE_FLAGS=-Wsparse-error make pack-revindex.sp
      SP pack-revindex.c
  $ echo $?
  0
  $ 

Now, we should be able to run the sparse Makefile target in a CI job, and still find all sparse errors and warnings (now marked as errors also), using something like this:

  $ SPARSE_FLAGS=-Wsparse-error make -k sparse >sp-out 2>&1

Note that the '-k' argument to 'make' is now required. ;-)

ATB, Ramsay Jones

Ramsay Jones (1):
  Makefile: improve SPARSE_FLAGS customisation

 Makefile | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

On Fri, Feb 01, 2019 at 09:01:20PM +0000, Ramsay Jones wrote:

...
 

Yes, I know :( The fact that, by default, sparse doesn't fail on errors is wanted (otherwise it would break the kernel compile). But that the only way to return an error is to use -Wsparse-error (which is supposed to replace GCC's -Werror) is a real problem.

-- Luc

On 01/02/2019 22:40, Luc Van Oostenryck wrote:

Given that I only use sparse as a checker, I actually don't mind the current behaviour. That would be different if I was using sparsec/sparsei etc., as a compiler, of course! ;-)

[If I were to suggest any change at all to -Wsparse-error it would be: do not change the 'warning' to an 'error' (yes, the actual text label of the message), exit(1) if any errors or warnings, but *if* only warnings have been issued, then print an "treating warnings as errors [-Wsparse-error]" message.]

ATB, Ramsay Jones

On Fri, Feb 01, 2019 at 09:01:20PM +0000, Ramsay Jones wrote:

We had the same issue with Coccinelle in the static analysis build job on Travis CI, which was happily succeeding even when Coccinelle noticed undesired code patterns entering the codebase. We've dealt with that by looking at the patch files generated by 'make coccicheck': if they were empty, then all is well, if they are not, then dump their contents into the log and 'exit 1'. See 0860a7641b (travis-ci: fail if Coccinelle static analysis found something to transform, 2018-07-23).

I think we could do something like that with sparse as well.

On 'master' I get:

  $ grep error sp-out 
  $ grep warning sp-out 
  connect.c:652:40: warning: incorrect type in argument 2 (invalid types)
  pack-revindex.c:65:23: warning: memset with byte count of 262144
  unix-socket.c:83:26: warning: incorrect type in argument 2 (invalid types)
  unix-socket.c:108:23: warning: incorrect type in argument 2 (invalid types)
  daemon.c:1041:36: warning: incorrect type in argument 2 (invalid types)
  daemon.c:1184:67: warning: incorrect type in argument 2 (invalid types)
  imap-send.c:1022:42: warning: incorrect type in argument 2 (invalid types)
  credential-cache--daemon.c:180:37: warning: incorrect type in argument 2 (invalid types)
  $ sparse --version
  v0.5.0

On 02/02/2019 00:41, SZEDER Gábor wrote:

[snip]

Yeah, that version of sparse is a bit too old.

If memory serves (it may not), all of the 'argument 2 (invalid types)' errors are caused by the glibc headers using a 'transparent union' to define the 'struct sockaddr' type. sparse could not handle that until commit 7698bae699 (aka. v0.5.0-5-g7698bae). The only remaining warning was addressed by commit bcfe020ed9 (aka. v0.5.1-rc1-22-gbcfe020) in sparse and commit 54360a1956 in git.

So, it seems you need at least v0.5.2 of sparse on your Linux system (which can't be too recent, or you would need v0.6.0).

ATB, Ramsay Jones

On Sun, Feb 03, 2019 at 01:49:37AM +0000, Ramsay Jones wrote:

The latest Linux image available on Travis CI is based on Ubuntu 16.04 LTS, which contains sparse 0.5.0, while their default image is based on 14.04, with sparse 0.4.5-rc1. Even the latest Ubuntu LTS is only at 0.5.1.

  https://travis-ci.org/szeder/git/jobs/488113660#L487
  https://travis-ci.org/szeder/git/jobs/488113661#L208

On 03/02/2019 12:12, SZEDER Gábor wrote:

Indeed.

[This is why I build sparse from source! :-D ]

With a bit of luck, v0.6.1 will be available soon.

ATB, Ramsay Jones