Clang Static Analysis
Hey folks, I just wanted to let you all know that I made some updates to my OS X tinderbox (yuffie). It's has been updated to use clang 3.8. You can find the static analysis results at https://people.freedesktop.org/~jeremyhu/analyzer/yuffie --Jeremy smime.p7s Description: S/MIME cryptographic signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis updated with tinderbox
On Apr 30, 2011, at 00:05, Dirk Wallenstein wrote: (Ups, new-mail-notification must have failed when visiting my old OS) I don't know clang but it looks like the command can be prefixed to any autogen.sh command, so that there is no need to edit existing modulesets. It seems that it would be possible to simply prefix it to the command without going through the template. cmd = self.scan_build_template(buildscript) + cmd Thanks for the suggestions. I've incorporated feedback from here and sent the patch to gnome's bugzilla for further comments and eventual integration. https://bugzilla.gnome.org/show_bug.cgi?id=648990 --Jeremy ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis updated with tinderbox
On Thu, Apr 28, 2011 at 01:04:27AM -0700, Jeremy Huddleston wrote: I'm in the process of updating my tinderboxes to run clang static analysis and upload the results. Currently only yuffie (XQuartz) has data available, and I'll be updating my linux tinerboxes in the next few days once yuffie is going smoothly. Hopefully cjb can integrate this a bit nicer into the tinderbox.x.org site, but for now, you can see it on my p.fd.o webspace: http://people.freedesktop.org/~jeremyhu/analyzer/ I've started going through the list (some patches already sent to xorg-devel for font-util and iceauth), but help is always welcome. There are some tests wherewe know better. For those, hopefully the _X_UNUSED and _X_NONNULL macros will help tag things appropriately. For the other cases (rand() security concerns), we can use ifdef-foo to silence the analyzer: #ifndef __clang_analyzer__ // Code not to be analyzed #endif The changes to jhbuild are minor (comments welcome before I send it off to gnome bugzilla). Thanks, Jeremy From 8784873bb86f92cab7d0341892f5db4343eb68a0 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston jerem...@apple.com Date: Thu, 28 Apr 2011 00:55:13 -0700 Subject: [PATCH] Support running scan-build (Clang Static Analyzer) with autotools projects http://clang-analyzer.llvm.org Signed-off-by: Jeremy Huddleston jerem...@apple.com --- jhbuild/config.py |3 ++- jhbuild/defaults.jhbuildrc|6 ++ jhbuild/modtypes/autotools.py | 12 +--- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/jhbuild/config.py b/jhbuild/config.py index f13e303..3e20436 100644 --- a/jhbuild/config.py +++ b/jhbuild/config.py @@ -58,7 +58,8 @@ _known_keys = [ 'moduleset', 'modules', 'skip', 'tags', 'prefix', 'jhbuildbot_mastercfg', 'use_local_modulesets', 'ignore_suggests', 'modulesets_dir', 'mirror_policy', 'module_mirror_policy', 'dvcs_mirror_dir', 'build_targets', -'cmakeargs', 'module_cmakeargs' ] +'cmakeargs', 'module_cmakeargs', +'scan_build', 'module_scan_build', 'scan_buildargs', 'scan_build_outputdir' ] env_prepends = {} def prependpath(envvar, path): diff --git a/jhbuild/defaults.jhbuildrc b/jhbuild/defaults.jhbuildrc index 7abe832..f9c34b7 100644 --- a/jhbuild/defaults.jhbuildrc +++ b/jhbuild/defaults.jhbuildrc @@ -89,6 +89,12 @@ interact = True # whether to interact with the user. quiet_mode= False # whether to display running commands output progress_bar = True # whether to display a progress bar when running in quiet mode +# Run clang static analyzer (scan-build), scan_build_outputdir has subdirectories for each module id +scan_build= False +module_scan_build = {} +scan_buildargs = '-v' +scan_build_outputdir = '/tmp/jhbuild_scan_build' + # checkout modes. For VCS directories, it specifies how the checkout # is done. We can also specify checkout modes for specific modules checkout_mode = 'update' diff --git a/jhbuild/modtypes/autotools.py b/jhbuild/modtypes/autotools.py index 215df91..ff7a068 100644 --- a/jhbuild/modtypes/autotools.py +++ b/jhbuild/modtypes/autotools.py @@ -121,13 +121,14 @@ class AutogenModule(Package, DownloadableModule): if self.autogen_template: template = self.autogen_template If this is configured, %(scan_build) won't be added. Would it make sense to add it independently, after this branching? else: -template = (%(srcdir)s/%(autogen-sh)s --prefix %(prefix)s +template = (%(scan_build)s%(srcdir)s/%(autogen-sh)s --prefix %(prefix)s --libdir %(libdir)s %(autogenargs)s ) autogenargs = self.autogenargs + ' ' + self.config.module_autogenargs.get( self.name, self.config.autogenargs) -vars = {'prefix': buildscript.config.prefix, +vars = {'scan_build': self.scan_build_template(buildscript), +'prefix': buildscript.config.prefix, 'autogen-sh': self.autogen_sh, 'autogenargs': autogenargs} @@ -210,13 +211,18 @@ class AutogenModule(Package, DownloadableModule): buildscript.set_action(_('Building'), self) makeargs = self.makeargs + ' ' + self.config.module_makeargs.get( self.name, self.config.makeargs) -cmd = '%s %s' % (os.environ.get('MAKE', 'make'), makeargs) +cmd = '%s%s %s' % (self.scan_build_template(buildscript), os.environ.get('MAKE', 'make'), makeargs) buildscript.execute(cmd, cwd = self.get_builddir(buildscript), extra_env = self.extra_env) do_build.depends = [PHASE_CONFIGURE] do_build.error_phases = [PHASE_FORCE_CHECKOUT, PHASE_CONFIGURE, PHASE_CLEAN, PHASE_DISTCLEAN] +def scan_build_template
Re: clang static analysis updated with tinderbox
On Apr 29, 2011, at 1:35 AM, Dirk Wallenstein wrote: --- a/jhbuild/modtypes/autotools.py +++ b/jhbuild/modtypes/autotools.py @@ -121,13 +121,14 @@ class AutogenModule(Package, DownloadableModule): if self.autogen_template: template = self.autogen_template If this is configured, %(scan_build) won't be added. Would it make sense to add it independently, after this branching? I would expect that %(scan_build) would be provided in self.autogen_template in the modules xml file if it was desired. Is that not custom? I'm not really sure what should take precedence here. +def scan_build_template(self, buildscript): +if self.name in buildscript.config.module_scan_build or buildscript.config.scan_build: I would say the module specific setting should override the global one, something like: if buildscript.config.module_scan_build.get(self.name, buildscript.config.scan_build): Great, thanks. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
clang static analysis updated with tinderbox
I'm in the process of updating my tinderboxes to run clang static analysis and upload the results. Currently only yuffie (XQuartz) has data available, and I'll be updating my linux tinerboxes in the next few days once yuffie is going smoothly. Hopefully cjb can integrate this a bit nicer into the tinderbox.x.org site, but for now, you can see it on my p.fd.o webspace: http://people.freedesktop.org/~jeremyhu/analyzer/ I've started going through the list (some patches already sent to xorg-devel for font-util and iceauth), but help is always welcome. There are some tests wherewe know better. For those, hopefully the _X_UNUSED and _X_NONNULL macros will help tag things appropriately. For the other cases (rand() security concerns), we can use ifdef-foo to silence the analyzer: #ifndef __clang_analyzer__ // Code not to be analyzed #endif The changes to jhbuild are minor (comments welcome before I send it off to gnome bugzilla). Thanks, Jeremy From 8784873bb86f92cab7d0341892f5db4343eb68a0 Mon Sep 17 00:00:00 2001 From: Jeremy Huddleston jerem...@apple.com Date: Thu, 28 Apr 2011 00:55:13 -0700 Subject: [PATCH] Support running scan-build (Clang Static Analyzer) with autotools projects http://clang-analyzer.llvm.org Signed-off-by: Jeremy Huddleston jerem...@apple.com --- jhbuild/config.py |3 ++- jhbuild/defaults.jhbuildrc|6 ++ jhbuild/modtypes/autotools.py | 12 +--- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/jhbuild/config.py b/jhbuild/config.py index f13e303..3e20436 100644 --- a/jhbuild/config.py +++ b/jhbuild/config.py @@ -58,7 +58,8 @@ _known_keys = [ 'moduleset', 'modules', 'skip', 'tags', 'prefix', 'jhbuildbot_mastercfg', 'use_local_modulesets', 'ignore_suggests', 'modulesets_dir', 'mirror_policy', 'module_mirror_policy', 'dvcs_mirror_dir', 'build_targets', -'cmakeargs', 'module_cmakeargs' ] +'cmakeargs', 'module_cmakeargs', +'scan_build', 'module_scan_build', 'scan_buildargs', 'scan_build_outputdir' ] env_prepends = {} def prependpath(envvar, path): diff --git a/jhbuild/defaults.jhbuildrc b/jhbuild/defaults.jhbuildrc index 7abe832..f9c34b7 100644 --- a/jhbuild/defaults.jhbuildrc +++ b/jhbuild/defaults.jhbuildrc @@ -89,6 +89,12 @@ interact = True # whether to interact with the user. quiet_mode= False # whether to display running commands output progress_bar = True # whether to display a progress bar when running in quiet mode +# Run clang static analyzer (scan-build), scan_build_outputdir has subdirectories for each module id +scan_build= False +module_scan_build = {} +scan_buildargs = '-v' +scan_build_outputdir = '/tmp/jhbuild_scan_build' + # checkout modes. For VCS directories, it specifies how the checkout # is done. We can also specify checkout modes for specific modules checkout_mode = 'update' diff --git a/jhbuild/modtypes/autotools.py b/jhbuild/modtypes/autotools.py index 215df91..ff7a068 100644 --- a/jhbuild/modtypes/autotools.py +++ b/jhbuild/modtypes/autotools.py @@ -121,13 +121,14 @@ class AutogenModule(Package, DownloadableModule): if self.autogen_template: template = self.autogen_template else: -template = (%(srcdir)s/%(autogen-sh)s --prefix %(prefix)s +template = (%(scan_build)s%(srcdir)s/%(autogen-sh)s --prefix %(prefix)s --libdir %(libdir)s %(autogenargs)s ) autogenargs = self.autogenargs + ' ' + self.config.module_autogenargs.get( self.name, self.config.autogenargs) -vars = {'prefix': buildscript.config.prefix, +vars = {'scan_build': self.scan_build_template(buildscript), +'prefix': buildscript.config.prefix, 'autogen-sh': self.autogen_sh, 'autogenargs': autogenargs} @@ -210,13 +211,18 @@ class AutogenModule(Package, DownloadableModule): buildscript.set_action(_('Building'), self) makeargs = self.makeargs + ' ' + self.config.module_makeargs.get( self.name, self.config.makeargs) -cmd = '%s %s' % (os.environ.get('MAKE', 'make'), makeargs) +cmd = '%s%s %s' % (self.scan_build_template(buildscript), os.environ.get('MAKE', 'make'), makeargs) buildscript.execute(cmd, cwd = self.get_builddir(buildscript), extra_env = self.extra_env) do_build.depends = [PHASE_CONFIGURE] do_build.error_phases = [PHASE_FORCE_CHECKOUT, PHASE_CONFIGURE, PHASE_CLEAN, PHASE_DISTCLEAN] +def scan_build_template(self, buildscript): +if self.name in buildscript.config.module_scan_build or buildscript.config.scan_build: +return 'scan-build %s -o %s/%s ' % (buildscript.config.scan_buildargs, buildscript.config.scan_build_outputdir, self.name) +return '' + def skip_check(self
Re: clang static analysis
On May 21, 2010, at 22:18, Jamey Sharp wrote: What about this case though? http://people.freedesktop.org/~jeremyhu/clang/2010-05-20-1/report-bKYbbq.html Is clang having some kind of alias analysis trouble there? pDrawable isn't being passed to any functions, but ((WindowPtr) pDrawable)-borderClip is... That one is interesting. I'm not sure. One way to quiet it might be to just s/pixmapClip/pClip/ in that line... I'll file a bug report against the SA for this one... at minimum, it should maybe be more verbose. I'll let you know what I hear. --Jeremy ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis
On May 21, 2010, at 22:18, Jamey Sharp wrote: On Fri, May 21, 2010 at 11:19 AM, Jeremy Huddleston jerem...@apple.com wrote: The analyzer is correct. It sees the call to miFillPolyHelper on line 1849 and assumes that pGC can change because it is not const ... My guess is that applying const correctly in many places will help the SA avoid false positives like this. Ooh, interesting. OK. miFillPolyHelper can't take a const pGC though, because eventually it passes it to ChangeGC (although with the invariant that it will be changed back before returning). I think in this case, miFillPolyHelper should take a const pGC and pass it casted to non-const to ChangeGC (since it knows it will be preserved by another handshaking method). ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis
On Sat, 22 May 2010 09:16:48 -0700, Jeremy Huddleston jerem...@apple.com wrote: I think in this case, miFillPolyHelper should take a const pGC and pass it casted to non-const to ChangeGC (since it knows it will be preserved by another handshaking method). Yuck. I'd rather not see the server cluttered by casts in the name of adding 'const' everywhere. It's just not that useful. -- keith.pack...@intel.com pgpQ8ES4Oq2XZ.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis
On Thu, May 20, 2010 at 11:59 PM, Jeremy Huddleston jerem...@apple.com wrote: In case anyone's interested, I've posted an updated log from building the XQuartz server with clang SA: http://people.freedesktop.org/~jeremyhu/clang/2010-05-20-1/ The first one I examined (Assigned value is garbage or undefined, mi/miwideline.c line 1918) assumes in steps 10 and 11 that pGC-lineStyle == LineOnOffDash and pGC-capStyle == CapProjecting, but took the false branch for that condition at step 8. Is it often that wrong? (It makes the same mistake for line 1804, and the Branch condition evaluates to a garbage value error is based on a similarly contradictory assumption.) A tiny bit of abstract interpretation would go a long way here... Assigned value is garbage or undefined at dix/dixfonts.c line 1231 is correct, at least, except the real bug is that I didn't delete the oldfid variable when I deleted the code that cared what value it had. Patch for that shortly. I suspect quite a few dead-store warnings would go away if we kill the REGION_* macros. One particularly entertaining example, from http://people.freedesktop.org/~jeremyhu/clang/2010-05-20-1/report-7FMXPb.html /* This prevents warnings about pScreen not being used. */ pGC-pScreen = pScreen = pGC-pScreen; clang says Although the value stored to 'pScreen' is used in the enclosing expression, the value is never actually read from 'pScreen'. Jamey ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis
The analyzer is correct. It sees the call to miFillPolyHelper on line 1849 and assumes that pGC can change: miFillPolyHelper (pDrawable, pGC, pixel, spanData, y, h, left, right, nleft, nright); because it is not const: static void miFillPolyHelper (DrawablePtr pDrawable, GCPtr pGC, unsigned long pixel, SpanDataPtr spanData, int y, int overall_height, PolyEdgePtr left, PolyEdgePtr right, int left_count, int right_count) My guess is that applying const correctly in many places will help the SA avoid false positives like this. On May 21, 2010, at 10:32, Jamey Sharp wrote: On Thu, May 20, 2010 at 11:59 PM, Jeremy Huddleston jerem...@apple.com wrote: In case anyone's interested, I've posted an updated log from building the XQuartz server with clang SA: http://people.freedesktop.org/~jeremyhu/clang/2010-05-20-1/ The first one I examined (Assigned value is garbage or undefined, mi/miwideline.c line 1918) assumes in steps 10 and 11 that pGC-lineStyle == LineOnOffDash and pGC-capStyle == CapProjecting, but took the false branch for that condition at step 8. Is it often that wrong? (It makes the same mistake for line 1804, and the Branch condition evaluates to a garbage value error is based on a similarly contradictory assumption.) A tiny bit of abstract interpretation would go a long way here... Assigned value is garbage or undefined at dix/dixfonts.c line 1231 is correct, at least, except the real bug is that I didn't delete the oldfid variable when I deleted the code that cared what value it had. Patch for that shortly. I suspect quite a few dead-store warnings would go away if we kill the REGION_* macros. One particularly entertaining example, from http://people.freedesktop.org/~jeremyhu/clang/2010-05-20-1/report-7FMXPb.html /* This prevents warnings about pScreen not being used. */ pGC-pScreen = pScreen = pGC-pScreen; clang says Although the value stored to 'pScreen' is used in the enclosing expression, the value is never actually read from 'pScreen'. Jamey ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis
On Fri, May 21, 2010 at 11:19 AM, Jeremy Huddleston jerem...@apple.com wrote: The analyzer is correct. It sees the call to miFillPolyHelper on line 1849 and assumes that pGC can change because it is not const ... My guess is that applying const correctly in many places will help the SA avoid false positives like this. Ooh, interesting. OK. miFillPolyHelper can't take a const pGC though, because eventually it passes it to ChangeGC (although with the invariant that it will be changed back before returning). What about this case though? http://people.freedesktop.org/~jeremyhu/clang/2010-05-20-1/report-bKYbbq.html Is clang having some kind of alias analysis trouble there? pDrawable isn't being passed to any functions, but ((WindowPtr) pDrawable)-borderClip is... Say, looks like this putative null-pointer dereference should be fixable by making AtomError _X_NORETURN: http://people.freedesktop.org/~jeremyhu/clang/2010-05-20-1/report-0TXsTP.html I'll send that patch along shortly. Jamey ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis of xorg-server
On Mar 23, 10 06:45:00 -0700, Alan Coopersmith wrote: Matthias Hopf wrote: On Mar 22, 10 17:50:38 -0700, Jeremy Huddleston wrote: Actually, it was a valid error. The assignment was doing |= rather than =, and the current value was garbage. ? |= looks correct. Jeremy's right though - the struct is allocated on the stack, uninitialized, 2 lines previous, and no value was set first so the |= is doing cn.changedControls = (uninitalized) | XkbControlsEnabledMask; Eeek. Yes, overlooked that. Matthias -- Matthias Hopf mh...@suse.de ____ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ m...@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R D www.mshopf.de ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis of xorg-server
On Mar 22, 10 17:50:38 -0700, Jeremy Huddleston wrote: Actually, it was a valid error. The assignment was doing |= rather than =, and the current value was garbage. ? |= looks correct. Alan's comment is perfectly correct. 1L 31 is undefined on architectures that define long to have 32 bit. As most architectures use 2's complement for negative values this shouldn't have any negative side effect, though. Thanks for the work on clang Matthias -- Matthias Hopf mh...@suse.de ____ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ m...@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R D www.mshopf.de ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis of xorg-server
Matthias Hopf wrote: On Mar 22, 10 17:50:38 -0700, Jeremy Huddleston wrote: Actually, it was a valid error. The assignment was doing |= rather than =, and the current value was garbage. ? |= looks correct. Jeremy's right though - the struct is allocated on the stack, uninitialized, 2 lines previous, and no value was set first so the |= is doing cn.changedControls = (uninitalized) | XkbControlsEnabledMask; -- -Alan Coopersmith- alan.coopersm...@sun.com Oracle Solaris Platform Engineering: X Window System ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
clang static analysis of xorg-server
I ran the static analyzer on a build of the current git master of xorg-server. Here are the results: http://people.freedesktop.org/~jeremyhu/clang/2010-03-22-1/ There are quite a number of hidden issues. Would people find it useful to have this data part of tinderbox.x.org? If so, I'll look into seeing how easily it would be to integrate clang into jhbuild. --Jeremy smime.p7s Description: S/MIME cryptographic signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis of xorg-server
On Mar 22, 10 09:58:59 -0700, Jeremy Huddleston wrote: I ran the static analyzer on a build of the current git master of xorg-server. Here are the results: http://people.freedesktop.org/~jeremyhu/clang/2010-03-22-1/ There are quite a number of hidden issues. Would people find it useful to have this data part of tinderbox.x.org? If so, I'll look into seeing how easily it would be to integrate clang into jhbuild. While 'rand is a poor random generator' and 'Dead assignment' are questionable, at least some of the NULL pointer accesses seem real. It might be interesting to have this on tinderbox, however, there should be a possibility to mark false positives (like the use of rand() in not security critical code). Any ideas? Thanks for the test Matthias -- Matthias Hopf mh...@suse.de ____ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ m...@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R D www.mshopf.de ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: clang static analysis of xorg-server
Jeremy Huddleston wrote: I aree ... some of the dead store issues are more stylistic or future proofing. The real ones we should consider are the logic errors... garbage assignment and null dereference. Some of the errors seem outright bogus (http://people.freedesktop.org/~jeremyhu/clang/2010-03-22-1/report-K6CxIC.html#EndPath), so if you see any others like that, let me know. I'm building the latest version of clang right now, and if those oddities are still present, I'll report them to the developers. That might be pointing out that the definition of XkbControlsEnabledMask is technically undefined on a 32-bit system, since it should be (1UL 31) not (1L 31) - we've gotten that error in various tools before. -- -Alan Coopersmith- alan.coopersm...@sun.com Oracle Solaris Platform Engineering: X Window System ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel