[analyzer][scan-build-py] subprocess output handling reviewed in clang module

[rizsotto.mailinglist]

Summary

In python subprocess.check_output is an easy way to collect child process output. Current implementation does deal with Popen class, which requires more code to get the desired output lines. Beside the simplifications, this patch also introduce a few more tests on active checker logic.

[dcoughlin]

This looks good to me. I've run this on a suite of open source projects and it reported no changes in the reference results. Thanks for simplifying and adding the extra tests!

tools/scan-build-py/libscanbuild/clang.py

90

What's the benefit of using a function attribute here rather than simply closing over a local variable?

[rizsotto.mailinglist]

hey Devin, thanks for looking to it.

tools/scan-build-py/libscanbuild/clang.py

90

there is no difference between the two. (it even can be written in the same order.) i guess, just wanted to decorate the name of the variable. i have no preference.

[Eugene.Zelenko]

Looks like patch was not committed.

[rizsotto.mailinglist]

commited at r282317

tools/scan-build-py/libscanbuild/analyze.py

     """ Validation done by the parser itself, but semantic check still
     needs to be done. This method is doing that. """
 
+    # Make plugins always a list. (It might be None when not specified.)
+    args.plugins = args.plugins if args.plugins else []
+
     if args.help_checkers_verbose:
         print_checkers(get_checkers(args.clang, args.plugins))
         parser.exit()

tools/scan-build-py/libscanbuild/clang.py

 
 __all__ = ['get_version', 'get_arguments', 'get_checkers']
 
+# regex for activated checker
+ACTIVE_CHECKER_PATTERN = re.compile(r'^-analyzer-checker=(.*)$')
 
-def get_version(cmd):
-    """ Returns the compiler version as string. """
 
-    lines = subprocess.check_output([cmd, '-v'], stderr=subprocess.STDOUT)
-    return lines.decode('ascii').splitlines()[0]
+def get_version(clang):
+    """ Returns the compiler version as string.
+
+    :param clang:   the compiler we are using
+    :return:        the version string printed to stderr """
+
+    output = subprocess.check_output([clang, '-v'], stderr=subprocess.STDOUT)
+    return output.decode('utf-8').splitlines()[0]
 
 
 def get_arguments(command, cwd):
     """ Capture Clang invocation.
 
-    This method returns the front-end invocation that would be executed as
-    a result of the given driver invocation. """
-
-    def lastline(stream):
-        last = None
-        for line in stream:
-            last = line
-        if last is None:
-            raise Exception("output not found")
-        return last
+    :param command: the compilation command
+    :param cwd:     the current working directory
+    :return:        the detailed front-end invocation command """
 
     cmd = command[:]
     cmd.insert(1, '-###')
     logging.debug('exec command in %s: %s', cwd, ' '.join(cmd))
-    child = subprocess.Popen(cmd,
-                             cwd=cwd,
-                             universal_newlines=True,
-                             stdout=subprocess.PIPE,
-                             stderr=subprocess.STDOUT)
-    line = lastline(child.stdout)
-    child.stdout.close()
-    child.wait()
-    if child.returncode == 0:
-        if re.search(r'clang(.*): error:', line):
-            raise Exception(line)
-        return decode(line)
-    else:
-        raise Exception(line)
+
+    output = subprocess.check_output(cmd, cwd=cwd, stderr=subprocess.STDOUT)
+    # The relevant information is in the last line of the output.
+    # Don't check if finding last line fails, would throw exception anyway.
+    last_line = output.decode('utf-8').splitlines()[-1]
+    if re.search(r'clang(.*): error:', last_line):
+        raise Exception(last_line)
+    return decode(last_line)
 
 
 def get_active_checkers(clang, plugins):
-    """ To get the default plugins we execute Clang to print how this
-    compilation would be called.
+    """ Get the active checker list.
 
-    For input file we specify stdin and pass only language information. """
+    :param clang:   the compiler we are using
+    :param plugins: list of plugins which was requested by the user
+    :return:        list of checker names which are active
 
-    def checkers(language):
+    To get the default checkers we execute Clang to print how this
+    compilation would be called. And take out the enabled checker from the
+    arguments. For input file we specify stdin and pass only language
+    information. """
+
+    def get_active_checkers_for(language):
         """ Returns a list of active checkers for the given language. """
 
-        load = [elem
+        load_args = [arg
                      for plugin in plugins
-                for elem in ['-Xclang', '-load', '-Xclang', plugin]]
-        cmd = [clang, '--analyze'] + load + ['-x', language, '-']
-        pattern = re.compile(r'^-analyzer-checker=(.*)$')
-        return [pattern.match(arg).group(1)
-                for arg in get_arguments(cmd, '.') if pattern.match(arg)]
+                     for arg in ['-Xclang', '-load', '-Xclang', plugin]]
+        cmd = [clang, '--analyze'] + load_args + ['-x', language, '-']
+        return [ACTIVE_CHECKER_PATTERN.match(arg).group(1)
+                for arg in get_arguments(cmd, '.')
+                if ACTIVE_CHECKER_PATTERN.match(arg)]
 
     result = set()
     for language in ['c', 'c++', 'objective-c', 'objective-c++']:
-        result.update(checkers(language))
-    return result
+        result.update(get_active_checkers_for(language))
+    return frozenset(result)
 
 
-def get_checkers(clang, plugins):
-    """ Get all the available checkers from default and from the plugins.
+def is_active(checkers):
+    """ Returns a method, which classifies the checker active or not,
+    based on the received checker name list. """
 
-    clang -- the compiler we are using
-    plugins -- list of plugins which was requested by the user
+    def predicate(checker):
+        """ Returns True if the given checker is active. """
 
-    This method returns a dictionary of all available checkers and status.
+        return any(pattern.match(checker) for pattern in predicate.patterns)
 
-    {<plugin name>: (<plugin description>, <is active by default>)} """
+    predicate.patterns = [re.compile(r'^' + a + r'(\.|$)') for a in checkers]
+    return predicate
 
-    plugins = plugins if plugins else []
 
-    def parse_checkers(stream):
-        """ Parse clang -analyzer-checker-help output.
 
-        Below the line 'CHECKERS:' are there the name description pairs.
-        Many of them are in one line, but some long named plugins has the
-        name and the description in separate lines.
+    Many of them are in one line, but some long named checker has the
 
-        The plugin name is always prefixed with two space character. The
-        name contains no whitespaces. Then followed by newline (if it's
-        too long) or other space characters comes the description of the
-        plugin. The description ends with a newline character. """
+    The checker name is always prefixed with two space character. The
+    checker. The description ends with a newline character.
 
-        # find checkers header
-        for line in stream:
-            if re.match(r'^CHECKERS:', line):
-                break
-        # find entries
-        state = None
-        for line in stream:
-            if state and not re.match(r'^\s\s\S', line):
-                yield (state, line.strip())
-                state = None
-            elif re.match(r'^\s\s\S+$', line.rstrip()):
-                state = line.strip()
-            else:
-                pattern = re.compile(r'^\s\s(?P<key>\S*)\s*(?P<value>.*)')
-                match = pattern.match(line.rstrip())
-                if match:
-                    current = match.groupdict()
-                    yield (current['key'], current['value'])
+    :param stream:  list of lines to parse
+    :return:        generator of tuples
 
-    def is_active(actives, entry):
-        """ Returns true if plugin name is matching the active plugin names.
+    (<checker name>, <checker description>) """
 
-        actives -- set of active plugin names (or prefixes).
-        entry -- the current plugin name to judge.
+    lines = iter(stream)
+    # find checkers header
+    for line in lines:
+        if re.match(r'^CHECKERS:', line):
+            break
+    # find entries
+    state = None
+    for line in lines:
+        if state and not re.match(r'^\s\s\S', line):
+            yield (state, line.strip())
+            state = None
+        elif re.match(r'^\s\s\S+$', line.rstrip()):
+            state = line.strip()
+        else:
+            pattern = re.compile(r'^\s\s(?P<key>\S*)\s*(?P<value>.*)')
+            match = pattern.match(line.rstrip())
+            if match:
+                current = match.groupdict()
+                yield (current['key'], current['value'])
 
-        The active plugin names are specific plugin names or prefix of some
-        names. One example for prefix, when it say 'unix' and it shall match
-        on 'unix.API', 'unix.Malloc' and 'unix.MallocSizeof'. """
 
-        return any(re.match(r'^' + a + r'(\.|$)', entry) for a in actives)
+def get_checkers(clang, plugins):
+    """ Get all the available checkers from default and from the plugins.
+
+    :param clang:   the compiler we are using
+    :param plugins: list of plugins which was requested by the user
+    :return:        a dictionary of all available checkers and its status
 
-    actives = get_active_checkers(clang, plugins)
+    {<checker name>: (<checker description>, <is active by default>)} """
 
     load = [elem for plugin in plugins for elem in ['-load', plugin]]
     cmd = [clang, '-cc1'] + load + ['-analyzer-checker-help']
 
     logging.debug('exec command: %s', ' '.join(cmd))
-    child = subprocess.Popen(cmd,
-                             universal_newlines=True,
-                             stdout=subprocess.PIPE,
-                             stderr=subprocess.STDOUT)
+    output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+    lines = output.decode('utf-8').splitlines()
+
+    is_active_checker = is_active(get_active_checkers(clang, plugins))
+
     checkers = {
-        k: (v, is_active(actives, k))
-        for k, v in parse_checkers(child.stdout)
+        name: (description, is_active_checker(name))
+        for name, description in parse_checkers(lines)
     }
-    child.stdout.close()
-    child.wait()
-    if child.returncode == 0 and len(checkers):
-        return checkers
-    else:
+    if not checkers:
         raise Exception('Could not query Clang for available checkers.')
+
+    return checkers

tools/scan-build-py/tests/unit/test_clang.py

 import libscanbuild.clang as sut
 import unittest
 import os.path
+import sys
 
 
-class GetClangArgumentsTest(unittest.TestCase):
+class ClangGetVersion(unittest.TestCase):
+    def test_get_version_is_not_empty(self):
+        self.assertTrue(sut.get_version('clang'))
+
+    def test_get_version_throws(self):
+        with self.assertRaises(OSError):
+            sut.get_version('notexists')
+
+
+class ClangGetArgumentsTest(unittest.TestCase):
     def test_get_clang_arguments(self):
         with libear.TemporaryDirectory() as tmpdir:
             filename = os.path.join(tmpdir, 'test.c')

             self.assertTrue('var="this is it"' in result)
 
     def test_get_clang_arguments_fails(self):
-        self.assertRaises(
-            Exception, sut.get_arguments,
-            ['clang', '-###', '-fsyntax-only', '-x', 'c', 'notexist.c'], '.')
+        with self.assertRaises(Exception):
+            sut.get_arguments(['clang', '-x', 'c', 'notexist.c'], '.')
+
+    def test_get_clang_arguments_fails_badly(self):
+        with self.assertRaises(OSError):
+            sut.get_arguments(['notexist'], '.')
 
 
-class GetCheckersTest(unittest.TestCase):
+class ClangGetCheckersTest(unittest.TestCase):
     def test_get_checkers(self):
         # this test is only to see is not crashing
         result = sut.get_checkers('clang', [])
         self.assertTrue(len(result))
+        # do check result types
+        string_type = unicode if sys.version_info < (3,) else str
+        for key, value in result.items():
+            self.assertEqual(string_type, type(key))
+            self.assertEqual(string_type, type(value[0]))
+            self.assertEqual(bool, type(value[1]))
 
     def test_get_active_checkers(self):
         # this test is only to see is not crashing
         result = sut.get_active_checkers('clang', [])
         self.assertTrue(len(result))
+        # do check result types
+        for value in result:
+            self.assertEqual(str, type(value))
+
+    def test_is_active(self):
+        test = sut.is_active(['a', 'b.b', 'c.c.c'])
+
+        self.assertTrue(test('a'))
+        self.assertTrue(test('a.b'))
+        self.assertTrue(test('b.b'))
+        self.assertTrue(test('b.b.c'))
+        self.assertTrue(test('c.c.c.p'))
+
+        self.assertFalse(test('ab'))
+        self.assertFalse(test('ba'))
+        self.assertFalse(test('bb'))
+        self.assertFalse(test('c.c'))
+        self.assertFalse(test('b'))
+        self.assertFalse(test('d'))
+
+    def test_parse_checkers(self):
+        lines = [
+            'OVERVIEW: Clang Static Analyzer Checkers List',
+            '',
+            'CHECKERS:',
+            '  checker.one       Checker One description',
+            '  checker.two',
+            '                    Checker Two description']
+        result = dict(sut.parse_checkers(lines))
+        self.assertTrue('checker.one' in result)
+        self.assertEqual('Checker One description', result.get('checker.one'))
+        self.assertTrue('checker.two' in result)
+        self.assertEqual('Checker Two description', result.get('checker.two'))