Bug 1479298 - [Static-Analysis][Clang-Based] Implement the passing of configuration flags to checkers in the configuration file. r=sylvestre
authorAndi-Bogdan Postelnicu <bpostelnicu@mozilla.com>
Wed, 10 Oct 2018 08:01:24 +0000
changeset 498969 33692f367564122728279498617718bbc235e542
parent 498968 f624eab872101f974a3da00d4d19257a755ced1c
child 498970 3e9933f36e458a44212e316b554fbaaaacbfd72b
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssylvestre
bugs1479298
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1479298 - [Static-Analysis][Clang-Based] Implement the passing of configuration flags to checkers in the configuration file. r=sylvestre Differential Revision: https://phabricator.services.mozilla.com/D7997
python/mozbuild/mozbuild/mach_commands.py
tools/clang-tidy/config.yaml
tools/clang-tidy/test/readability-implicit-bool-conversion.cpp
--- a/python/mozbuild/mozbuild/mach_commands.py
+++ b/python/mozbuild/mozbuild/mach_commands.py
@@ -10,16 +10,17 @@ import itertools
 import json
 import logging
 import operator
 import os
 import re
 import subprocess
 import sys
 import tempfile
+import yaml
 
 from collections import OrderedDict
 
 import mozpack.path as mozpath
 
 from mach.decorators import (
     CommandArgument,
     CommandArgumentGroup,
@@ -1722,16 +1723,17 @@ class StaticAnalysis(MachCommandBase):
             if name_re.search(f['file']):
                 total = total + 1
 
         if not total:
             return 0
 
         cwd = self.topobjdir
         self._compilation_commands_path = self.topobjdir
+        self._clang_tidy_config = self._get_clang_tidy_config()
         args = self._get_clang_tidy_command(
             checks=checks, header_filter=header_filter, sources=source, jobs=jobs, fix=fix)
 
         monitor = StaticAnalysisMonitor(self.topsrcdir, self.topobjdir, total)
 
         footer = StaticAnalysisFooter(self.log_manager.terminal, monitor)
         with StaticAnalysisOutputManager(self.log_manager, monitor, footer) as output:
             rc = self.run_process(args=args, ensure_exit_code=False, line_handler=output.on_line, cwd=cwd)
@@ -1840,17 +1842,16 @@ class StaticAnalysis(MachCommandBase):
         f = tempfile.NamedTemporaryFile()
         for source in sources:
             f.write(source+'\n')
         f.flush()
         return (f, ['--changed-files-index', f.name])
 
     def _get_infer_config(self):
         '''Load the infer config file.'''
-        import yaml
         checkers = []
         tp_path = ''
         with open(mozpath.join(self.topsrcdir, 'tools',
                                'infer', 'config.yaml')) as f:
             try:
                 config = yaml.safe_load(f)
                 for item in config['infer_checkers']:
                     if item['publish']:
@@ -1870,32 +1871,50 @@ class StaticAnalysis(MachCommandBase):
         for checker in checks:
             checkers.append('--' + checker)
         with open(third_party_path) as f:
             for line in f:
                 excludes.append('--skip-analysis-in-path')
                 excludes.append(line.strip('\n'))
         return checkers, excludes
 
+    def _get_clang_tidy_config(self):
+        try:
+            file_handler = open(mozpath.join(self.topsrcdir, "tools", "clang-tidy", "config.yaml"))
+            config = yaml.safe_load(file_handler)
+        except Exception:
+                print('Looks like config.yaml is not valid, we are going to use default'
+                      ' values for the rest of the analysis.')
+                return None
+        return config
+
     def _get_clang_tidy_command(self, checks, header_filter, sources, jobs, fix):
 
         if checks == '-*':
             checks = self._get_checks()
 
         common_args = ['-clang-tidy-binary', self._clang_tidy_path,
                        '-clang-apply-replacements-binary', self._clang_apply_replacements,
                        '-checks=%s' % checks,
                        '-extra-arg=-DMOZ_CLANG_PLUGIN']
 
         # Flag header-filter is passed in order to limit the diagnostic messages only
         # to the specified header files. When no value is specified the default value
         # is considered to be the source in order to limit the diagnostic message to
         # the source files or folders.
         common_args += ['-header-filter=%s' % (header_filter
                                               if len(header_filter) else '|'.join(sources))]
+
+        # From our configuration file, config.yaml, we build the configuration list, for
+        # the checkers that are used. These configuration options are used to better fit
+        # the checkers to our code.
+        cfg = self._get_checks_config()
+        if cfg:
+            common_args += ['-config=%s' % yaml.dump(cfg)]
+
         if fix:
             common_args += '-fix'
 
         return [
             self.virtualenv_manager.python_path, self._run_clang_tidy_path, '-j',
             str(jobs), '-p', self._compilation_commands_path
         ] + common_args + sources
 
@@ -1959,22 +1978,20 @@ class StaticAnalysis(MachCommandBase):
         if rc != 0:
             self.log(logging.ERROR, 'ERROR: static-analysis', {},
                      'clang-tidy unable to locate package.')
             return self.TOOLS_FAILED_DOWNLOAD
 
         self._clang_tidy_base_path = mozpath.join(self.topsrcdir, "tools", "clang-tidy")
 
         # For each checker run it
-        f = open(mozpath.join(self._clang_tidy_base_path, "config.yaml"))
-        import yaml
-        config = yaml.safe_load(f)
+        self._clang_tidy_config = self._get_clang_tidy_config()
         platform, _ = self.platform
 
-        if platform not in config['platforms']:
+        if platform not in self._clang_tidy_config['platforms']:
             self.log(logging.ERROR, 'static-analysis', {},
                      "RUNNING: clang-tidy autotest for platform {} not supported.".format(platform))
             return self.TOOLS_UNSUPORTED_PLATFORM
 
         import concurrent.futures
         import multiprocessing
         import shutil
 
@@ -1987,21 +2004,21 @@ class StaticAnalysis(MachCommandBase):
         # List all available checkers
         cmd = [self._clang_tidy_path, '-list-checks', '-checks=*']
         clang_output = subprocess.check_output(
             cmd, stderr=subprocess.STDOUT).decode('utf-8')
         available_checks = clang_output.split('\n')[1:]
         self._clang_tidy_checks = [c.strip() for c in available_checks if c]
 
         # Build the dummy compile_commands.json
-        self._compilation_commands_path = self._create_temp_compilation_db(config)
+        self._compilation_commands_path = self._create_temp_compilation_db(self._clang_tidy_config)
         checkers_test_batch = []
         with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor:
             futures = []
-            for item in config['clang_checkers']:
+            for item in self._clang_tidy_config['clang_checkers']:
                 # Skip if any of the following statements is true:
                 # 1. Checker attribute 'publish' is False.
                 not_published = not bool(item.get('publish', True))
                 # 2. Checker has restricted-platforms and current platform is not of them.
                 ignored_platform = 'restricted-platforms' in item and platform not in item['restricted-platforms']
                 # 3. Checker name is mozilla-* or -*.
                 ignored_checker = item['name'] in ['mozilla-*', '-*']
                 # 4. List checker_names is passed and the current checker is not part of the
@@ -2265,28 +2282,51 @@ class StaticAnalysis(MachCommandBase):
         for _, header in enumerate(headers):
             header_group = header.groups()
             element = [header_group[3], header_group[4], header_group[5]]
             issues.append(element)
         return issues
 
     def _get_checks(self):
         checks = '-*'
-        import yaml
-        with open(mozpath.join(self.topsrcdir, "tools", "clang-tidy", "config.yaml")) as f:
-            try:
-                config = yaml.safe_load(f)
-                for item in config['clang_checkers']:
-                    if item.get('publish', True):
-                        checks += ',' + item['name']
-            except Exception:
-                print('Looks like config.yaml is not valid, so we are unable to '
-                      'determine default checkers, using \'-checks=-*,mozilla-*\'')
-                checks += ',mozilla-*'
-        return checks
+        try:
+            config = self._clang_tidy_config
+            for item in config['clang_checkers']:
+                if item.get('publish', True):
+                    checks += ',' + item['name']
+        except Exception:
+            print('Looks like config.yaml is not valid, so we are unable to '
+                    'determine default checkers, using \'-checks=-*,mozilla-*\'')
+            checks += ',mozilla-*'
+        finally:
+            return checks
+
+    def _get_checks_config(self):
+        config_list = []
+        checker_config = {}
+        try:
+            config = self._clang_tidy_config
+            for checker in config['clang_checkers']:
+                if checker.get('publish', True) and 'config' in checker:
+                    for checker_option in checker['config']:
+                        # Verify if the format of the Option is correct,
+                        # possibilities are:
+                        # 1. CheckerName.Option
+                        # 2. Option -> that will become CheckerName.Option
+                        if not checker_option['key'].startswith(checker['name']):
+                            checker_option['key'] = "{}.{}".format(
+                                checker['name'], checker_option['key'])
+                    config_list += checker['config']
+            checker_config['CheckOptions'] = config_list
+        except Exception:
+            print('Looks like config.yaml is not valid, so we are unable to '
+                    'determine configuration for checkers, so using default')
+            checker_config = None
+        finally:
+            return checker_config
 
     def _get_config_environment(self):
         ran_configure = False
         config = None
         builder = Build(self._mach_context)
 
         try:
             config = self.config_environment
--- a/tools/clang-tidy/config.yaml
+++ b/tools/clang-tidy/config.yaml
@@ -111,16 +111,23 @@ clang_checkers:
   - name: performance-unnecessary-copy-initialization
   - name: performance-unnecessary-value-param
   - name: readability-braces-around-statements
   # Note: this can be loosened up by using the ShortStatementLines option
   - name: readability-container-size-empty
   - name: readability-delete-null-pointer
   - name: readability-else-after-return
   - name: readability-implicit-bool-conversion
+    config:
+      - key: AllowIntegerConditions
+        # The check will allow conditional integer conversions.
+        value: 1
+      - key: AllowPointerConditions
+        # The check will allow conditional pointer conversions.
+        value: 1
   - name: readability-inconsistent-declaration-parameter-name
   - name: readability-misleading-indentation
   - name: readability-non-const-parameter
   - name: readability-redundant-control-flow
   - name: readability-redundant-smartptr-get
   - name: readability-redundant-string-cstr
   - name: readability-redundant-string-init
   - name: readability-static-accessed-through-instance
--- a/tools/clang-tidy/test/readability-implicit-bool-conversion.cpp
+++ b/tools/clang-tidy/test/readability-implicit-bool-conversion.cpp
@@ -43,9 +43,13 @@ void f() {
 
   int exp = (int)true;
 
   if (x == b) {}
   if (x != b) {}
 
   if (b == exp) {}
   if (exp == b) {}
+
+  char* ptr;
+  // Shouldn't trigger a checker warning since we are using AllowPointerConditions
+  if (ptr) {}
 }