[code coverage] Only instrument Java class files related to affected …

…source files for per-CL

Right now Java per-CL coverage instrument everything as full code base coverage.
To only instrument class files related to affected files in a CL, we can:
1. Extract class files only related to affected source files in a jar.
2. Instrument these extracted class files.
3. Extract unaffected members from original jar.
4. Zip everything out to output path.

Bug: 1001387
Change-Id: Iaf6a75c7f8005b5d64eee22bc604f25d0c2fcd8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1797109
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Yun Liu <yliuyliu@google.com>
Cr-Commit-Position: refs/heads/master@{#697661}

build/android/docs/coverage.md

@@ -17,7 +17,7 @@ class files and runtime **.exec** files. Then we need to process them using the
 
   ```gn
   target_os = "android"
-  jacoco_coverage = true
+  use_jacoco_coverage = true
   ```
 
    Now when building, pre-instrumented files will be created in the build directory.

build/android/gyp/jacoco_instr.py

@@ -6,7 +6,7 @@
 
 """Instruments classes and jar files.
 
-This script corresponds to the 'jacoco_instr' action in the java build process.
+This script corresponds to the 'jacoco_instr' action in the Java build process.
 Depending on whether jacoco_instrument is set, the 'jacoco_instr' action will
 call the instrument command which accepts a jar and instruments it using
 jacococli.jar.
@@ -21,6 +21,7 @@
 import shutil
 import sys
 import tempfile
+import zipfile
 
 from util import build_utils
 
@@ -53,6 +54,9 @@ def _AddArguments(parser):
       help='File containing newline-separated .java paths')
   parser.add_argument(
       '--jacococli-jar', required=True, help='Path to jacococli.jar.')
+  parser.add_argument(
+      '--files-to-instrument',
+      help='Path to a file containing which source files are affected.')
 
 
 def _GetSourceDirsFromSourceFiles(source_files):
@@ -101,44 +105,133 @@ def _CreateSourcesJsonFile(source_dirs, input_path, sources_json_file,
     json.dump(data, f)
 
 
-def _RunInstrumentCommand(parser):
-  """Instruments jar files using Jacoco.
+def _GetAffectedClasses(jar_file, source_files):
+  """Gets affected classes by affected source files to a jar.
 
   Args:
-    parser: ArgumentParser object.
+    jar_file: The jar file to get all members.
+    source_files: The list of affected source files.
 
   Returns:
-    An exit code.
+    A tuple of affected classes and unaffected members.
   """
-  args = parser.parse_args()
+  with zipfile.ZipFile(jar_file) as f:
+    members = f.namelist()
 
-  temp_dir = tempfile.mkdtemp()
-  try:
-    cmd = [
-        'java', '-jar', args.jacococli_jar, 'instrument', args.input_path,
-        '--dest', temp_dir
-    ]
+  affected_classes = []
+  unaffected_members = []
 
-    build_utils.CheckOutput(cmd)
+  for member in members:
+    if not member.endswith('.class'):
+      unaffected_members.append(member)
+      continue
 
-    jars = os.listdir(temp_dir)
-    if len(jars) != 1:
-      print('Error: multiple output files in: %s' % (temp_dir))
-      return 1
+    is_affected = False
+    index = member.find('$')
+    if index == -1:
+      index = member.find('.class')
+    for source_file in source_files:
+      if source_file.endswith(member[:index] + '.java'):
+        affected_classes.append(member)
+        is_affected = True
+        break
+    if not is_affected:
+      unaffected_members.append(member)
+
+  return affected_classes, unaffected_members
+
+
+def _InstrumentWholeJar(instrument_cmd, input_path, output_path, temp_dir):
+  """Instruments input jar to output_path.
+
+  Args:
+    instrument_cmd: JaCoCo instrument command.
+    input_path: The input path to non-instrumented jar.
+    output_path: The output path to instrumented jar.
+    temp_dir: The temporary directory.
+  """
+  instrument_cmd.extend([input_path, '--dest', temp_dir])
+
+  build_utils.CheckOutput(instrument_cmd)
+
+  jars = os.listdir(temp_dir)
+  if len(jars) != 1:
+    raise Exception('Error: multiple output files: %s' % jars)
+
+  # Delete output_path first to avoid modifying input_path in the case where
+  # input_path is a hardlink to output_path. http://crbug.com/571642
+  if os.path.exists(output_path):
+    os.unlink(output_path)
+  shutil.move(os.path.join(temp_dir, jars[0]), output_path)
+
+
+def _InstrumentClassFiles(instrument_cmd, input_path, output_path, temp_dir,
+                          affected_source_files):
+  """Instruments affected class files from input jar.
+
+  Args:
+    instrument_cmd: JaCoCo instrument command.
+    input_path: The input path to non-instrumented jar.
+    output_path: The output path to instrumented jar.
+    temp_dir: The temporary directory.
+    affected_source_files: The affected source file paths to input jar.
+  """
+  affected_classes, unaffected_members = _GetAffectedClasses(
+      input_path, affected_source_files)
+
+  # Extract affected class files.
+  with zipfile.ZipFile(input_path) as f:
+    f.extractall(temp_dir, affected_classes)
+
+  instrumented_dir = os.path.join(temp_dir, 'instrumented')
+
+  # Instrument extracted class files.
+  instrument_cmd.extend([temp_dir, '--dest', instrumented_dir])
+  build_utils.CheckOutput(instrument_cmd)
+
+  # Extract unaffected members to instrumented_dir.
+  with zipfile.ZipFile(input_path) as f:
+    f.extractall(instrumented_dir, unaffected_members)
 
-    # Delete output_path first to avoid modifying input_path in the case where
-    # input_path is a hardlink to output_path. http://crbug.com/571642
-    if os.path.exists(args.output_path):
-      os.unlink(args.output_path)
-    shutil.move(os.path.join(temp_dir, jars[0]), args.output_path)
-  finally:
-    shutil.rmtree(temp_dir)
+  # Zip all files to output_path
+  build_utils.ZipDir(output_path, instrumented_dir)
+
+
+def _RunInstrumentCommand(parser):
+  """Instruments class or Jar files using JaCoCo.
+
+  Args:
+    parser: ArgumentParser object.
+
+  Returns:
+    An exit code.
+  """
+  args = parser.parse_args()
 
   source_files = []
   if args.java_sources_file:
     source_files.extend(build_utils.ReadSourcesList(args.java_sources_file))
-  source_dirs = _GetSourceDirsFromSourceFiles(source_files)
 
+  with build_utils.TempDir() as temp_dir:
+    instrument_cmd = ['java', '-jar', args.jacococli_jar, 'instrument']
+
+    if not args.files_to_instrument:
+      _InstrumentWholeJar(instrument_cmd, args.input_path, args.output_path,
+                          temp_dir)
+    else:
+      affected_files = build_utils.ReadSourcesList(args.files_to_instrument)
+      source_set = set(source_files)
+      affected_source_files = [f for f in affected_files if f in source_set]
+
+      # Copy input_path to output_path and return if no source file affected.
+      if not affected_source_files:
+        shutil.copyfile(args.input_path, args.output_path)
+        return 0
+      else:
+        _InstrumentClassFiles(instrument_cmd, args.input_path, args.output_path,
+                              temp_dir, affected_source_files)
+
+  source_dirs = _GetSourceDirsFromSourceFiles(source_files)
   # TODO(GYP): In GN, we are passed the list of sources, detecting source
   # directories, then walking them to re-establish the list of sources.
   # This can obviously be simplified!

build/config/android/config.gni

@@ -182,10 +182,6 @@ if (is_android || is_chromeos) {
     # Set to false to disable the Errorprone compiler
     use_errorprone_java_compiler = true
 
-    # Enables Jacoco Java code coverage. Instruments classes during build to
-    # produce .exec files during runtime
-    jacoco_coverage = false
-
     # Disables process isolation when building _incremental targets.
     # Required for Android M+ due to SELinux policies (stronger sandboxing).
     disable_incremental_isolated_processes = false

build/config/android/internal_rules.gni

@@ -6,6 +6,7 @@
 # Some projects (e.g. V8) do not have non-build directories DEPS'ed in.
 import("//build/config/android/config.gni")
 import("//build/config/android/copy_ex.gni")
+import("//build/config/coverage/coverage.gni")
 import("//build/config/dcheck_always_on.gni")
 import("//build/config/python.gni")
 import("//build/config/sanitizers/sanitizers.gni")
@@ -697,7 +698,7 @@ template("test_runner_script") {
       if (defined(invoker.proguard_enabled) && invoker.proguard_enabled) {
         executable_args += [ "--enable-java-deobfuscation" ]
       }
-      if (jacoco_coverage) {
+      if (use_jacoco_coverage) {
         # Set a default coverage output directory (can be overridden by user
         # passing the same flag).
         _rebased_coverage_dir =
@@ -729,7 +730,7 @@ template("test_runner_script") {
         "--robolectric-runtime-deps-dir",
         "@WrappedPath(${_rebased_robolectric_runtime_deps_dir})",
       ]
-      if (jacoco_coverage) {
+      if (use_jacoco_coverage) {
         # Set a default coverage output directory (can be overridden by user
         # passing the same flag).
         _rebased_coverage_dir =
@@ -1092,7 +1093,7 @@ if (enable_java_templates) {
       }
       args += [ "--classpath=@FileArg($_rebased_build_config:deps_info:java_runtime_classpath)" ]
 
-      if (jacoco_coverage) {
+      if (use_jacoco_coverage) {
         args += [
           "--classpath",
           rebase_path("//third_party/jacoco/lib/jacocoagent.jar",
@@ -1434,6 +1435,8 @@ if (enable_java_templates) {
         rebase_path(invoker.java_sources_file, root_build_dir),
         "--jacococli-jar",
         rebase_path(_jacococli_jar, root_build_dir),
+        "--files-to-instrument",
+        rebase_path(coverage_instrumentation_input_file, root_build_dir),
       ]
     }
   }
@@ -3084,7 +3087,7 @@ if (enable_java_templates) {
   #    Chromium-specific unless it is in a 'chromium' sub-directory).
   #  jacoco_never_instrument: Optional. If provided, whether to forbid
   #    instrumentation with the Jacoco coverage processor. If not provided,
-  #    this is controlled by the global jacoco_coverage build arg variable
+  #    this is controlled by the global use_jacoco_coverage build arg variable
   #    and only used for non-test Chromium code.
   #  include_android_sdk: Optional. Whether or not the android SDK dep
   #    should be added to deps. Defaults to true for non-system libraries
@@ -3304,7 +3307,7 @@ if (enable_java_templates) {
 
     if (defined(_final_jar_path)) {
       _jacoco_instrument =
-          jacoco_coverage && _chromium_code && _java_files != [] &&
+          use_jacoco_coverage && _chromium_code && _java_files != [] &&
           !_is_java_binary && !_is_annotation_processor &&
           (!defined(invoker.testonly) || !invoker.testonly)
       if (defined(invoker.jacoco_never_instrument)) {

build/config/coverage/coverage.gni

@@ -5,15 +5,20 @@
 import("//build/toolchain/toolchain.gni")
 
 # There are two ways to enable code coverage instrumentation:
-# 1. When |use_clang_coverage| is true and |coverage_instrumentation_input_file|
-#    is empty, all source files are instrumented.
-# 2. When |use_clang_coverage| is true and |coverage_instrumentation_input_file|
-#    is NOT empty and points to a text file on the file system, ONLY source
-#    files specified in the input file are instrumented.
+# 1. When |use_clang_coverage| or |use_jacoco_coverage| is true and
+#    |coverage_instrumentation_input_file| is empty, all source files or
+#    Java class files are instrumented.
+# 2. When |use_clang_coverage| or |use_jacoco_coverage| is true and
+#    |coverage_instrumentation_input_file| is NOT empty and points to
+#    a text file on the file system, ONLY source files specified in the
+#    input file or Java class files related to source files are instrumented.
 declare_args() {
   # Enable Clang's Source-based Code Coverage.
   use_clang_coverage = false
 
+  # Enables JaCoCo Java code coverage.
+  use_jacoco_coverage = false
+
   # The path to the coverage instrumentation input file should be a source root
   # absolute path (e.g. //out/Release/coverage_instrumentation_input.txt), and
   # the file consists of multiple lines where each line represents a path to a

tools/mb/mb.py

@@ -399,34 +399,32 @@ def CmdRun(self):
       return self._RunLocallyIsolated(self.args.path, self.args.target)
 
   def CmdZip(self):
-      ret = self.CmdIsolate()
-      if ret:
-          return ret
+    ret = self.CmdIsolate()
+    if ret:
+      return ret
 
-      zip_dir = None
-      try:
-          zip_dir = self.TempDir()
-          remap_cmd = [
-            self.executable,
-            self.PathJoin(self.chromium_src_dir, 'tools', 'swarming_client',
-                          'isolate.py'),
-            'remap',
-            '--collapse_symlinks',
-            '-s', self.PathJoin(self.args.path, self.args.target + '.isolated'),
-            '-o', zip_dir
-          ]
-          self.Run(remap_cmd)
-
-          zip_path = self.args.zip_path
-          with zipfile.ZipFile(zip_path, 'w', zipfile.ZIP_DEFLATED,
-                               allowZip64=True) as fp:
-              for root, _, files in os.walk(zip_dir):
-                  for filename in files:
-                      path = self.PathJoin(root, filename)
-                      fp.write(path, self.RelPath(path, zip_dir))
-      finally:
-          if zip_dir:
-              self.RemoveDirectory(zip_dir)
+    zip_dir = None
+    try:
+      zip_dir = self.TempDir()
+      remap_cmd = [
+          self.executable,
+          self.PathJoin(self.chromium_src_dir, 'tools', 'swarming_client',
+                        'isolate.py'), 'remap', '--collapse_symlinks', '-s',
+          self.PathJoin(self.args.path, self.args.target + '.isolated'), '-o',
+          zip_dir
+      ]
+      self.Run(remap_cmd)
+
+      zip_path = self.args.zip_path
+      with zipfile.ZipFile(
+          zip_path, 'w', zipfile.ZIP_DEFLATED, allowZip64=True) as fp:
+        for root, _, files in os.walk(zip_dir):
+          for filename in files:
+            path = self.PathJoin(root, filename)
+            fp.write(path, self.RelPath(path, zip_dir))
+    finally:
+      if zip_dir:
+        self.RemoveDirectory(zip_dir)
 
   @staticmethod
   def _AddBaseSoftware(cmd):
@@ -806,8 +804,8 @@ def FlattenMixins(self, mixins, vals, visited):
         vals['cros_passthrough'] = mixin_vals['cros_passthrough']
       if 'args_file' in mixin_vals:
         if vals['args_file']:
-            raise MBErr('args_file specified multiple times in mixins '
-                        'for %s on %s' % (self.args.builder, self.args.master))
+          raise MBErr('args_file specified multiple times in mixins '
+                      'for %s on %s' % (self.args.builder, self.args.master))
         vals['args_file'] = mixin_vals['args_file']
       if 'gn_args' in mixin_vals:
         if vals['gn_args']:
@@ -888,10 +886,10 @@ def RunGNGenAllIsolates(self, vals):
     ret, output, _ = self.Run(self.GNCmd('ls', build_dir),
                               force_verbose=False)
     if ret:
-        # If `gn ls` failed, we should exit early rather than trying to
-        # generate isolates.
-        self.Print('GN ls failed: %d' % ret)
-        return ret
+      # If `gn ls` failed, we should exit early rather than trying to
+      # generate isolates.
+      self.Print('GN ls failed: %d' % ret)
+      return ret
 
     # Create a reverse map from isolate label to isolate dict.
     isolate_map = self.ReadIsolateMap()
@@ -1307,7 +1305,7 @@ def GetIsolateCommand(self, target, vals):
     msan = 'is_msan=true' in vals['gn_args']
     tsan = 'is_tsan=true' in vals['gn_args']
     cfi_diag = 'use_cfi_diag=true' in vals['gn_args']
-    java_coverage = 'jacoco_coverage=true' in vals['gn_args']
+    java_coverage = 'use_jacoco_coverage=true' in vals['gn_args']
 
     test_type = isolate_map[target]['type']
     use_python3 = isolate_map[target].get('use_python3', False)