goma_link: Add more unit tests, split from integration tests

This splits the unit tests and integration tests for goma_link.
goma_link_unit_tests.py now contains unit tests that run quickly and
don't depend on the host system being set up with the necessary
external dependencies and environment. Integration tests that test
actual linking of actual programs are now in
goma_link_integration_tests.py. I have also expanded unit test
coverage to check computing index parameters, codegen parameters,
final link parameters, and the list of files to be codegenned,
which is where most of the complexity of goma_link is.

Bug: 877722
Change-Id: I18d5094e5044383cab2b4e43b9aff0086cfd2e35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2140813
Commit-Queue: Bob Haarman <inglorion@chromium.org>
Reviewed-by: George Burgess <gbiv@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757973}

tools/clang/scripts/goma_link.py

@@ -313,10 +313,26 @@ class GomaLinkBase(object):
     if os.path.basename(args.linker).startswith('pnacl-'):
       return None
 
-    if 'clang' in os.path.basename(args.linker):
-      compiler = args.linker
+    rsp_expanded = list(self.expand_args_rsps(args.linker_args))
+    expanded_args = list(self.expand_thin_archives(rsp_expanded))
+
+    return self.analyze_expanded_args(expanded_args, args.output, args.linker,
+                                      gen_dir, common_dir, use_common_objects)
+
+  def analyze_expanded_args(self, args, output, linker, gen_dir, common_dir,
+                            use_common_objects):
+    """
+    Helper function for analyze_args. This is called by analyze_args after
+    expanding rsp files and determining which files are bitcode files, and
+    produces codegen_params, final_params, and index_params.
+
+    This function interacts with the filesystem through os.path.exists,
+    is_bitcode_file, and ensure_file.
+    """
+    if 'clang' in os.path.basename(linker):
+      compiler = linker
     else:
-      compiler_dir = os.path.dirname(args.linker)
+      compiler_dir = os.path.dirname(linker)
       if compiler_dir:
         compiler_dir += '/'
       else:
@@ -356,14 +372,6 @@ class GomaLinkBase(object):
           return ['-mllvm', match.group(2)]
         else:
           return ['-mllvm']
-      match = re.match('(?:-Wl,)?--lto-O(.*)', param)
-      if match:
-        optlevel[0] = match.group(1)
-        return None
-      match = re.match('[-/]opt:.*lldlto=([^:]*)', param, re.IGNORECASE)
-      if match:
-        optlevel[0] = match.group(1)
-        return None
       if (param.startswith('-f') and not param.startswith('-flto')
           and not param.startswith('-fsanitize')
           and not param.startswith('-fthinlto')
@@ -373,25 +381,58 @@ class GomaLinkBase(object):
         return [param]
       return None
 
+    def extract_opt_level(param):
+      """
+      If param is a parameter that specifies the LTO optimization level,
+      returns the level. If not, returns None.
+      """
+      match = re.match('(?:-Wl,)?--lto-O(.+)', param)
+      if match:
+        return match.group(1)
+      match = re.match('[-/]opt:.*lldlto=([^:]*)', param, re.IGNORECASE)
+      if match:
+        return match.group(1)
+      return None
+
     def process_param(param):
       """
       Common code for processing a single parameter from the either the
       command line or an rsp file.
       """
-      if in_mllvm[0]:
-        if param.startswith('-Wl,'):
-          codegen_params.append(param[4:])
-        else:
-          codegen_params.append(param)
-        in_mllvm[0] = False
-      else:
+
+      def helper():
+        """
+        This exists so that we can use return instead of
+        nested if statements to use the first matching case.
+        """
+        # After -mllvm, just pass on the param.
+        if in_mllvm[0]:
+          if param.startswith('-Wl,'):
+            codegen_params.append(param[4:])
+          else:
+            codegen_params.append(param)
+          in_mllvm[0] = False
+          return
+
+        # Check for params that specify LTO optimization level.
+        o = extract_opt_level(param)
+        if o is not None:
+          optlevel[0] = o
+          return
+
+        # Check for params that affect code generation.
         cg_param = transform_codegen_param(param)
         if cg_param:
           codegen_params.extend(cg_param)
+          # No return here, we still want to check for -mllvm.
+
+        # Check for -mllvm.
         match = MLLVM_RE.match(param)
         if match and not match.group(2):
           # Next parameter will be the thing to pass to LLVM.
           in_mllvm[0] = True
+
+      helper()
       if self.GROUP_RE.match(param):
         return
       index_params.append(param)
@@ -411,17 +452,14 @@ class GomaLinkBase(object):
         final_params.append(param)
 
     index_params.append(self.WL + self.PREFIX_REPLACE + ';' + obj_dir)
-
-    rsp_expanded = list(self.expand_args_rsps(args.linker_args))
-    expanded_args = list(self.expand_thin_archives(rsp_expanded))
     i = 0
-    while i < len(expanded_args):
-      x = expanded_args[i]
+    while i < len(args):
+      x = args[i]
       if not self.GROUP_RE.match(x):
-        outfile, next_i = self.process_output_param(expanded_args, i)
+        outfile, next_i = self.process_output_param(args, i)
         if outfile is not None:
-          index_params.extend(expanded_args[i:next_i])
-          final_params.extend(expanded_args[i:next_i])
+          index_params.extend(args[i:next_i])
+          final_params.extend(args[i:next_i])
           i = next_i - 1
         else:
           process_param(x)
@@ -438,15 +476,15 @@ class GomaLinkBase(object):
       for tup in codegen:
         final_params.append(tup[0])
     else:
-      splitfile = gen_dir + '/' + args.output + '.split' + self.OBJ_SUFFIX
+      splitfile = gen_dir + '/' + output + '.split' + self.OBJ_SUFFIX
       final_params.append(splitfile)
       index_params.append(self.WL + self.OBJ_PATH + splitfile)
-      used_obj_file = gen_dir + '/' + args.output + '.objs'
+      used_obj_file = gen_dir + '/' + output + '.objs'
       final_params.append('@' + used_obj_file)
 
     return AnalyzeArgsResult(
-        output=args.output,
-        linker=args.linker,
+        output=output,
+        linker=linker,
         compiler=compiler,
         splitfile=splitfile,
         index_params=index_params,

tools/clang/scripts/goma_link_tests.py → tools/clang/scripts/goma_link_integration_tests.py

@@ -3,30 +3,28 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-# Unit tests for goma_link.
+# Integration tests for goma_link.
 #
 # Usage:
 #
 # Ensure that gomacc, llvm-objdump, and llvm-dwarfdump are in your PATH.
 # Then run:
 #
-# python third_party/pycoverage run tools/clang/scripts/goma_link_tests.py
+#   tools/clang/scripts/goma_link_integration_tests.py
 #
-# An HTML coverage report can be generated afterwards by running:
-# python third_party/pycoverage html
-#
-# The report will be available as htmlcov/index.html
+# See also goma_link_unit_tests.py, which contains unit tests and
+# instructions for generating coverage information.
 
 import goma_ld
 import goma_link
 
 import os
 import re
-import shutil
 import subprocess
-import tempfile
 import unittest
 
+from goma_link_test_utils import named_directory, working_directory
+
 # Path constants.
 CHROMIUM_DIR = os.path.abspath(
     os.path.join(os.path.dirname(__file__), '..', '..', '..'))
@@ -47,36 +45,6 @@ def _create_inputs(path):
     f.write('int bar() {\n  return 9;\n}\n')
 
 
-# tempfile.NamedDirectory is in Python 3.8. This is for compatibility with
-# older Python versions.
-class NamedDirectory(object):
-  def __init__(self, *args, **kwargs):
-    self.name = tempfile.mkdtemp(*args, **kwargs)
-
-  def __enter__(self):
-    return self.name
-
-  def __exit__(self, exnty, *args, **kwargs):
-    shutil.rmtree(self.name)
-    return exnty is None
-
-
-# Changes working directory to the specified directory, runs enclosed code,
-# and changes back to the previous directory.
-class WorkingDirectory(object):
-  def __init__(self, newcwd):
-    self.oldcwd = os.getcwd()
-    os.chdir(newcwd)
-    self.newcwd = os.getcwd()
-
-  def __enter__(self):
-    return self.newcwd
-
-  def __exit__(self, exnty, *args, **kwargs):
-    os.chdir(self.oldcwd)
-    return exnty is None
-
-
 class GomaLinkUnixWhitelistMain(goma_ld.GomaLinkUnix):
   """
   Same as goma_ld.GomaLinkUnix, but whitelists "main".
@@ -97,37 +65,6 @@ class GomaLinkWindowsWhitelistMain(goma_link.GomaLinkWindows):
     self.WHITELISTED_TARGETS = {'main.exe'}
 
 
-class GomaLinkUnitTest(unittest.TestCase):
-  """
-  Unit tests for goma_link.
-  """
-
-  def test_ensure_file_no_dir(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
-      self.assertFalse(os.path.exists('test'))
-      goma_link.ensure_file('test')
-      self.assertTrue(os.path.exists('test'))
-
-  def test_ensure_file_existing(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
-      self.assertFalse(os.path.exists('foo/test'))
-      goma_link.ensure_file('foo/test')
-      self.assertTrue(os.path.exists('foo/test'))
-      os.utime('foo/test', (0, 0))
-      statresult = os.stat('foo/test')
-      goma_link.ensure_file('foo/test')
-      self.assertTrue(os.path.exists('foo/test'))
-      newstatresult = os.stat('foo/test')
-      self.assertEqual(newstatresult.st_mtime, statresult.st_mtime)
-
-  def test_ensure_file_error(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
-      self.assertFalse(os.path.exists('test'))
-      goma_link.ensure_file('test')
-      self.assertTrue(os.path.exists('test'))
-      self.assertRaises(OSError, goma_link.ensure_file, 'test/impossible')
-
-
 class GomaLinkIntegrationTest(unittest.TestCase):
   def clangcl(self):
     return os.path.join(LLVM_BIN_DIR, 'clang-cl' + goma_link.exe_suffix())
@@ -136,7 +73,7 @@ class GomaLinkIntegrationTest(unittest.TestCase):
     return os.path.join(LLVM_BIN_DIR, 'lld-link' + goma_link.exe_suffix())
 
   def test_distributed_lto_common_objs(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       os.makedirs('obj')
       subprocess.check_call([
@@ -187,7 +124,7 @@ class GomaLinkIntegrationTest(unittest.TestCase):
       self.assertTrue(b'call' in disasm or b'jmp' in disasm)
 
   def test_distributed_lto_whitelisted(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       os.makedirs('obj')
       subprocess.check_call([
@@ -245,7 +182,7 @@ class GomaLdIntegrationTest(unittest.TestCase):
     return os.path.join(LLVM_BIN_DIR, 'clang++' + goma_link.exe_suffix())
 
   def test_nonlto(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       subprocess.check_call(
           [self.clangxx(), '-c', '-Os', 'main.cpp', '-o', 'main.o'])
@@ -267,7 +204,7 @@ class GomaLdIntegrationTest(unittest.TestCase):
       self.assertIn(b'foo', main_disasm)
 
   def test_fallback_lto(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       subprocess.check_call([
           self.clangxx(), '-c', '-Os', '-flto=thin', 'main.cpp', '-o', 'main.o'
@@ -291,7 +228,7 @@ class GomaLdIntegrationTest(unittest.TestCase):
       self.assertNotIn(b'foo', main_disasm)
 
   def test_distributed_lto(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       subprocess.check_call([
           self.clangxx(), '-c', '-Os', '-flto=thin', 'main.cpp', '-o', 'main.o'
@@ -319,7 +256,7 @@ class GomaLdIntegrationTest(unittest.TestCase):
       self.assertNotIn(b'foo', main_disasm)
 
   def test_distributed_lto_thin_archive_same_dir(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       subprocess.check_call([
           self.clangxx(), '-c', '-Os', '-flto=thin', 'main.cpp', '-o', 'main.o'
@@ -351,7 +288,7 @@ class GomaLdIntegrationTest(unittest.TestCase):
       self.assertNotIn(b'foo', main_disasm)
 
   def test_distributed_lto_thin_archive_subdir(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       os.makedirs('obj')
       subprocess.check_call([
@@ -389,37 +326,36 @@ class GomaLdIntegrationTest(unittest.TestCase):
       self.assertNotIn(b'foo', main_disasm)
 
   def test_debug_params(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       os.makedirs('obj')
       subprocess.check_call([
-          self.clangxx(), '-c', '-g', '-gsplit-dwarf', '-flto=thin',
-          'main.cpp', '-o', 'obj/main.o',
+          self.clangxx(), '-c', '-g', '-gsplit-dwarf', '-flto=thin', 'main.cpp',
+          '-o', 'obj/main.o'
       ])
       subprocess.check_call([
-          self.clangxx(), '-c', '-g', '-gsplit-dwarf', '-flto=thin',
-          'foo.cpp', '-o', 'obj/foo.o'
+          self.clangxx(), '-c', '-g', '-gsplit-dwarf', '-flto=thin', 'foo.cpp',
+          '-o', 'obj/foo.o'
       ])
       with open('main.rsp', 'w') as f:
-        f.write('obj/main.o\n'
-                'obj/foo.o\n')
+        f.write('obj/main.o\n' 'obj/foo.o\n')
       rc = GomaLinkUnixWhitelistMain().main([
           'goma_ld.py',
-          self.clangxx(), '-fuse-ld=lld', '-flto=thin',
-          '-g', '-gsplit-dwarf', '-Wl,--lto-O2', '-o', 'main', '@main.rsp',
+          self.clangxx(), '-fuse-ld=lld', '-flto=thin', '-g', '-gsplit-dwarf',
+          '-Wl,--lto-O2', '-o', 'main', '@main.rsp'
       ])
       # Should succeed.
       self.assertEqual(rc, 0)
       # Check debug info present, refers to .dwo file, and does not
       # contain full debug info for foo.cpp.
       dbginfo = subprocess.check_output(
-          ['llvm-dwarfdump', '-debug-info', 'main']
-      ).decode('utf-8', 'backslashreplace')
+          ['llvm-dwarfdump', '-debug-info', 'main']).decode(
+              'utf-8', 'backslashreplace')
       self.assertRegexpMatches(dbginfo, '\\bDW_AT_GNU_dwo_name\\b.*\\.dwo"')
       self.assertNotRegexpMatches(dbginfo, '\\bDW_AT_name\\b.*foo\\.cpp"')
 
   def test_distributed_lto_params(self):
-    with NamedDirectory() as d, WorkingDirectory(d):
+    with named_directory() as d, working_directory(d):
       _create_inputs(d)
       os.makedirs('obj')
       subprocess.check_call([

tools/clang/scripts/goma_link_test_utils.py

+# Copyright (c) 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+#
+# Utility classes for testing goma_link.
+
+import contextlib
+import os
+import shutil
+import tempfile
+
+
+# tempfile.NamedDirectory is in Python 3.8. This is for compatibility with
+# older Python versions.
+@contextlib.contextmanager
+def named_directory(*args, **kwargs):
+  name = tempfile.mkdtemp(*args, **kwargs)
+  try:
+    yield name
+  finally:
+    shutil.rmtree(name)
+
+
+@contextlib.contextmanager
+def working_directory(newcwd):
+  """
+  Changes working directory to the specified directory, runs enclosed code,
+  and changes back to the previous directory.
+  """
+  oldcwd = os.getcwd()
+  os.chdir(newcwd)
+  try:
+    # Use os.getcwd() instead of newcwd so that we have a path that works
+    # inside the block.
+    yield os.getcwd()
+  finally:
+    os.chdir(oldcwd)

tools/clang/scripts/goma_link_unit_tests.py

+#! /usr/bin/env python3
+# Copyright (c) 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+# Unit tests for goma_link.
+#
+# Usage:
+#
+#   tools/clang/scripts/goma_link_unit_tests.py
+#
+# A coverage report combining these tests with the integration tests
+# in goma_link_integration_tests.py can be generated by running:
+#
+#   env COVERAGE_FILE=.coverage.unit python3 third_party/pycoverage run \
+#     tools/clang/scripts/goma_link_unit_tests.py
+#   env COVERAGE_FILE=.coverage.integration python3 third_party/pycoverage \
+#     run tools/clang/scripts/goma_link_integration_tests.py
+#   python3 third_party/pycoverage combine
+#   python3 third_party/pycoverage html
+#
+# The report will be available as htmlcov/index.html
+
+import goma_ld
+import goma_link
+
+import os
+import unittest
+from unittest import mock
+
+from goma_link_test_utils import named_directory, working_directory
+
+
+class FakeFs(object):
+  """
+  Context manager that mocks the functions through which goma_link
+  interacts with the filesystem.
+  """
+
+  def __init__(self, bitcode_files=None, other_files=None):
+    self.bitcode_files = set(bitcode_files or [])
+    self.other_files = set(other_files or [])
+
+    def ensure_file(path):
+      self.other_files.add(path)
+
+    def exists(path):
+      return path in self.bitcode_files or path in self.other_files
+
+    def is_bitcode_file(path):
+      return path in self.bitcode_files
+
+    self.mock_ensure_file = mock.patch('goma_link.ensure_file', ensure_file)
+    self.mock_exists = mock.patch('os.path.exists', exists)
+    self.mock_is_bitcode_file = mock.patch('goma_link.is_bitcode_file',
+                                           is_bitcode_file)
+
+  def __enter__(self):
+    self.mock_ensure_file.start()
+    self.mock_exists.start()
+    self.mock_is_bitcode_file.start()
+    return self
+
+  def __exit__(self, exnty, *args, **kwargs):
+    self.mock_is_bitcode_file.stop()
+    self.mock_exists.stop()
+    self.mock_ensure_file.stop()
+    return exnty is None
+
+
+class GomaLinkUnitTest(unittest.TestCase):
+  """
+  Unit tests for goma_link.
+  """
+
+  def test_analyze_expanded_args_nocodegen(self):
+    with FakeFs(other_files=['foo.o', 'bar.o']):
+      self.assertIsNone(goma_ld.GomaLinkUnix().analyze_expanded_args(
+          ['clang', 'foo.o', 'bar.o', '-o', 'foo'], 'foo', 'clang', 'lto.foo',
+          'common', False))
+
+  def test_analyze_expanded_args_one_codegen(self):
+    with FakeFs(bitcode_files=['foo.o'], other_files=['bar.o']):
+      result = goma_ld.GomaLinkUnix().analyze_expanded_args(
+          ['clang', 'foo.o', 'bar.o', '-o', 'foo'], 'foo', 'clang', 'lto.foo',
+          'common', False)
+      self.assertIsNotNone(result)
+      self.assertNotEqual(len(result.codegen), 0)
+      self.assertEqual(result.codegen[0][1], 'foo.o')
+      self.assertEqual(len(result.codegen), 1)
+      self.assertIn('foo.o', result.index_params)
+      self.assertIn('bar.o', result.index_params)
+      self.assertIn('bar.o', result.final_params)
+      # foo.o should not be in final_params because it will be added via
+      # the used object file.
+      self.assertNotIn('foo.o', result.final_params)
+
+  def test_analyze_expanded_args_params(self):
+    with FakeFs(bitcode_files=['foo.o']):
+      result = goma_ld.GomaLinkUnix().analyze_expanded_args([
+          'clang', '-O2', '-flto=thin', '-fsplit-lto-unit',
+          '-fwhole-program-vtables', '-fsanitize=cfi', '-g', '-gsplit-dwarf',
+          '-mllvm', '-generate-type-units', 'foo.o', '-o', 'foo'
+      ], 'foo', 'clang', 'lto.foo', 'common', False)
+      self.assertIsNotNone(result)
+      self.assertIn('-Wl,-plugin-opt=obj-path=lto.foo/foo.split.o',
+                    result.index_params)
+      self.assertIn('-O2', result.index_params)
+      self.assertIn('-g', result.index_params)
+      self.assertIn('-gsplit-dwarf', result.index_params)
+      self.assertIn('-mllvm -generate-type-units',
+                    ' '.join(result.index_params))
+      self.assertIn('-flto=thin', result.index_params)
+      self.assertIn('-fwhole-program-vtables', result.index_params)
+      self.assertIn('-fsanitize=cfi', result.index_params)
+
+      self.assertIn('-O2', result.codegen_params)
+      self.assertIn('-g', result.codegen_params)
+      self.assertIn('-gsplit-dwarf', result.codegen_params)
+      self.assertIn('-mllvm -generate-type-units',
+                    ' '.join(result.codegen_params))
+      self.assertNotIn('-flto=thin', result.codegen_params)
+      self.assertNotIn('-fwhole-program-vtables', result.codegen_params)
+      self.assertNotIn('-fsanitize=cfi', result.codegen_params)
+
+      self.assertNotIn('-flto=thin', result.final_params)
+
+  def test_ensure_file_no_dir(self):
+    with named_directory() as d, working_directory(d):
+      self.assertFalse(os.path.exists('test'))
+      goma_link.ensure_file('test')
+      self.assertTrue(os.path.exists('test'))
+
+  def test_ensure_file_existing(self):
+    with named_directory() as d, working_directory(d):
+      self.assertFalse(os.path.exists('foo/test'))
+      goma_link.ensure_file('foo/test')
+      self.assertTrue(os.path.exists('foo/test'))
+      os.utime('foo/test', (0, 0))
+      statresult = os.stat('foo/test')
+      goma_link.ensure_file('foo/test')
+      self.assertTrue(os.path.exists('foo/test'))
+      newstatresult = os.stat('foo/test')
+      self.assertEqual(newstatresult.st_mtime, statresult.st_mtime)
+
+  def test_ensure_file_error(self):
+    with named_directory() as d, working_directory(d):
+      self.assertFalse(os.path.exists('test'))
+      goma_link.ensure_file('test')
+      self.assertTrue(os.path.exists('test'))
+      self.assertRaises(OSError, goma_link.ensure_file, 'test/impossible')
+
+
+if __name__ == '__main__':
+  unittest.main()