[ovs-dev] [PATCH v2] Makefile.am: Add clang static analysis support

Ben Pfaff blp at ovn.org
Sun Jul 3 00:11:36 UTC 2016


On Sat, Jul 02, 2016 at 10:02:37PM +0000, Bodireddy, Bhanuprakash wrote:
> 
> >-----Original Message-----
> >From: Ben Pfaff [mailto:blp at ovn.org]
> >Sent: Saturday, July 2, 2016 9:31 PM
> >To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy at intel.com>
> >Cc: dev at openvswitch.org
> >Subject: Re: [ovs-dev] [PATCH v2] Makefile.am: Add clang static analysis
> >support
> >
> >On Sat, Jul 02, 2016 at 08:14:02PM +0000, Bodireddy, Bhanuprakash wrote:
> >> >-----Original Message-----
> >> >From: Ben Pfaff [mailto:blp at ovn.org]
> >> >Sent: Saturday, July 2, 2016 6:14 PM
> >> >To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy at intel.com>
> >> >Cc: dev at openvswitch.org
> >> >Subject: Re: [ovs-dev] [PATCH v2] Makefile.am: Add clang static
> >> >analysis support
> >> >
> >> >On Tue, Jun 28, 2016 at 05:09:13PM +0100, Bhanuprakash Bodireddy wrote:
> >> >> Clang Static Analyzer is a source code analysis tool to find bugs.
> >> >> This patch adds make target to trigger static analysis using below
> >commands.
> >> >>
> >> >> ./boot.sh
> >> >> ./configure --with-dpdk(in case of DPDK datapath) make
> >> >> clang-analyze scan-view --host=<ip address> --port <PORT>
> >> >>   $OVS_DIR>/clang-analyzer-results/yyyy-mm-dd-114251-1027-1>
> >> >>   --allow-all-hosts
> >> >>
> >> >> Results can be viewed on browser: http://<ip address>:<PORT>/
> >> >>
> >> >> v1->v2:
> >> >> * Change the output directory to tests/clang-analyzer-results
> >> >> * Remove '-j' make option, This might potentially hang some system
> >> >>   while spawning infinite jobs.
> >> >>
> >> >> Signed-off-by: Bhanuprakash Bodireddy
> >> >> <bhanuprakash.bodireddy at intel.com>
> >> >
> >> >I'd tend to write this a little differently, maybe like this:
> >> >
> >> >clang-analyze: clean
> >> >	@which clang scan-build >/dev/null 2>&1 || \
> >> >	  (echo "Unable to find clang/scan-build, Install
> >> >clang,clang-analyzer packages"; exit 1)
> >> >	@$(MKDIR_P) "$(srcdir)/tests/clang-analyzer-results"
> >> >	@scan-build -o $(srcdir)/tests/clang-analyzer-results --use-
> >> >analyzer=/usr/bin/clang $(MAKE)
> >> >.PHONY: clang-analyze
> >>
> >> This is fine for me.
> >>
> >> >
> >> >But it doesn't work for me anyway.  When I run it from a build tree
> >> >configured to use clang, I get the following:
> >> >
> >> >    make  all-recursive
> >> >    make[2]: Entering directory '/home/blp/nicira/ovs/_clang'
> >> >    Making all in datapath
> >> >    make[3]: Entering directory '/home/blp/nicira/ovs/_clang/datapath'
> >> >    make[4]: Entering directory '/home/blp/nicira/ovs/_clang/datapath'
> >> >    make[4]: Leaving directory '/home/blp/nicira/ovs/_clang/datapath'
> >> >    make[3]: Leaving directory '/home/blp/nicira/ovs/_clang/datapath'
> >> >    make[3]: Entering directory '/home/blp/nicira/ovs/_clang'
> >> >      CC       lib/aes128.lo
> >> >    gcc: error: unrecognized command line option '-Wthread-safety'
> >> >    gcc: error: unrecognized command line option '-Qunused-arguments'
> >> >    gcc: error: unrecognized command line option '-fno-caret-diagnostics'
> >> >    Makefile:4179: recipe for target 'lib/aes128.lo' failed
> >> >    make[3]: *** [lib/aes128.lo] Error 1
> >> >    make[3]: Leaving directory '/home/blp/nicira/ovs/_clang'
> >> >    Makefile:4831: recipe for target 'all-recursive' failed
> >> >    make[2]: *** [all-recursive] Error 1
> >> >    make[2]: Leaving directory '/home/blp/nicira/ovs/_clang'
> >> >    Makefile:2749: recipe for target 'all' failed
> >> >    make[1]: *** [all] Error 2
> >> >    make[1]: Leaving directory '/home/blp/nicira/ovs/_clang'
> >> >    scan-build: Removing directory
> >> >'/home/blp/nicira/ovs/tests/clang-analyzer-
> >> >results/2016-07-02-101251-14653-1' because it contains no reports.
> >> >    scan-build: No bugs found.
> >> >    Makefile:5845: recipe for target 'clang-analyze' failed
> >> >    make: *** [clang-analyze] Error 1
> >> >
> >> >Alternatively, if I run it from a build tree configured to use GCC, I
> >> >get the
> >> >following:
> >> >
> >> >    make  all-recursive
> >> >    make[2]: Entering directory '/home/blp/nicira/ovs/_build'
> >> >    Making all in datapath
> >> >    make[3]: Entering directory '/home/blp/nicira/ovs/_build/datapath'
> >> >    make[4]: Entering directory '/home/blp/nicira/ovs/_build/datapath'
> >> >    make[4]: Leaving directory '/home/blp/nicira/ovs/_build/datapath'
> >> >    make[3]: Leaving directory '/home/blp/nicira/ovs/_build/datapath'
> >> >    make[3]: Entering directory '/home/blp/nicira/ovs/_build'
> >> >      CC       lib/aes128.lo
> >> >      CC       lib/backtrace.lo
> >> >      CC       lib/bfd.lo
> >> >    In file included from ../lib/bfd.h:24:0,
> >> >                     from ../lib/bfd.c:16:
> >> >    ../lib/packets.h: In function 'eth_addr_invert':
> >> >    ../lib/packets.h:237:5: error: 'for' loop initial declarations
> >> >are only allowed in
> >> >C99 or C11 mode
> >> >    ../lib/packets.h:237:5: note: use option -std=c99, -std=gnu99,
> >> >-std=c11 or -
> >> >std=gnu11 to compile your code
> >>
> >> I have tested this on F22 and didn't see this issue.  But when I tested it now
> >on Ubuntu 14.04 LTS I could see the issue you reported here.
> >> Adding CFLAGS="-std=gnu99" to make should fix this issue and I tested
> >> it now on Ubuntu 14.04
> >>
> >> Can you try the below patch and see if you can generate clang analysis
> >report properly this time?
> >
> >This change seems rather arbitrary.  Why doesn't the analysis phase use the C
> >compiler flags that have already been found to be correct for compilation?
> 
> I tested the slightly tweaked patch updated below (added '--use-cc' flag) for both clang and gcc. 
> I would let the user specify the CFLAGS at the configuration phase especially with gcc compiler on some distributions.
> What do you think of this approach?
> 
> Clang:
> ./configure CC=clang --with-dpdk=$DPDK_BUILD
> make clang-analyze
> 
> gcc:
> ./configure CC=gcc --with-dpdk=$DPDK_BUILD CFLAGS="-std=gnu99"
> make clang-analyze
> 
> +clang-analyze: clean
> +       @which clang scan-build >/dev/null 2>&1 || \
> +               (echo "Unable to find clang/scan-build, Install clang,clang-analyzer packages"; exit 1)
> +       @$(MKDIR_P) "$(srcdir)/tests/clang-analyzer-results"
> +       @scan-build -o $(srcdir)/tests/clang-analyzer-results --use-cc=$(CC) --use-analyzer=/usr/bin/clang $(MAKE)
> +.PHONY: clang-analyze

The point I'm making is that the configure process already chooses a
correct set of CFLAGS and already allows the user to override them, so
why doesn't the clang-analyzer invocation use them?



More information about the dev mailing list