[foozzie] Refactoring - several code clean-ups

This simplifies the lengthy main method by extracting some code and
by replacing the scattered returns with exceptions.

We introduce two exceptions for early bail-out. This enables helper
methods on multiple layers. The early bail-out on time-out is
moved to the point where it is detected.

Previously on timeout and crash we also printed out the step number.
Clusterfuzz doesn't parse this, it was only for statistical purposes,
and the latest version of the experimental workbench only parses
crashes and timeouts, not the step in which they happened. Hence,
this CL removes those step numbers.

Except the change described in the last paragraph, this CL doesn't
intend to change behavior.

No-Try: true
Bug: chromium:1100114
Change-Id: Ie8c18f183e4fc538577f3eb49aaf6df1acd1e4e1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2270547Reviewed-by: Liviu Rau <liviurau@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68576}

tools/clusterfuzz/v8_commands.py

@@ -54,6 +54,22 @@ def _startup_files(options):
   return files
 
 
+class BaseException(Exception):
+  """Used to abort the comparison workflow and print the given message."""
+  def __init__(self, message):
+    self.message = message
+
+
+class PassException(BaseException):
+  """Represents an early abort making the overall run pass."""
+  pass
+
+
+class FailException(BaseException):
+  """Represents an early abort making the overall run fail."""
+  pass
+
+
 class Command(object):
   """Represents a configuration for running V8 multiple times with certain
   flags and files.
@@ -88,22 +104,15 @@ class Command(object):
 
 
 class Output(object):
-  def __init__(self, exit_code, timed_out, stdout, pid):
+  def __init__(self, exit_code, stdout, pid):
     self.exit_code = exit_code
-    self.timed_out = timed_out
     self.stdout = stdout
     self.pid = pid
 
   def HasCrashed(self):
-    # Timed out tests will have exit_code -signal.SIGTERM.
-    if self.timed_out:
-      return False
     return (self.exit_code < 0 and
             self.exit_code != -signal.SIGABRT)
 
-  def HasTimedOut(self):
-    return self.timed_out
-
 
 def Execute(args, cwd, timeout=None):
   popen_args = [c for c in args if c != ""]
@@ -136,9 +145,11 @@ def Execute(args, cwd, timeout=None):
   stdout, _ = process.communicate()
   timer.cancel()
 
+  if timeout_event.is_set():
+    raise PassException('# V8 correctness - T-I-M-E-O-U-T')
+
   return Output(
       process.returncode,
-      timeout_event.is_set(),
       stdout,
       process.pid,
   )

tools/clusterfuzz/v8_foozzie.py

@@ -22,7 +22,7 @@ import traceback
 
 from collections import namedtuple
 
-import v8_commands
+from v8_commands import Command, FailException, PassException
 import v8_suppressions
 
 PYTHON3 = sys.version_info >= (3, 0)
@@ -274,33 +274,19 @@ def content_bailout(content, ignore_fun):
   """Print failure state and return if ignore_fun matches content."""
   bug = (ignore_fun(content) or '').strip()
   if bug:
-    print(FAILURE_HEADER_TEMPLATE % dict(
+    raise FailException(FAILURE_HEADER_TEMPLATE % dict(
         configs='', source_key='', suppression=bug))
-    return True
-  return False
-
-
-def timeout_bailout(output, step_number):
-  """Print info and return if in timeout pass state."""
-  if output.HasTimedOut():
-    # Dashed output, so that no other clusterfuzz tools can match the
-    # words timeout or crash.
-    print('# V8 correctness - T-I-M-E-O-U-T %d' % step_number)
-    return True
-  return False
 
 
 def fail_bailout(output, ignore_by_output_fun):
   """Print failure state and return if ignore_by_output_fun matches output."""
   bug = (ignore_by_output_fun(output.stdout) or '').strip()
   if bug:
-    print(FAILURE_HEADER_TEMPLATE % dict(
+    raise FailException(FAILURE_HEADER_TEMPLATE % dict(
         configs='', source_key='', suppression=bug))
-    return True
-  return False
 
 
-def print_difference(
+def format_difference(
     options, source_key, first_command, second_command,
     first_config_output, second_config_output, difference, source=None):
   # The first three entries will be parsed by clusterfuzz. Format changes
@@ -332,9 +318,9 @@ def print_difference(
       difference=difference,
   ))
   if PYTHON3:
-    print(text)
+    return text
   else:
-    print(text.encode('utf-8', 'replace'))
+    return text.encode('utf-8', 'replace')
 
 
 def cluster_failures(source, known_failures=None):
@@ -362,6 +348,26 @@ def cluster_failures(source, known_failures=None):
   return long_key[:ORIGINAL_SOURCE_HASH_LENGTH]
 
 
+def run_sanity_checks(options, suppress, first_cmd, second_cmd):
+  """Run some fixed smoke tests in all configs to ensure nothing
+  is fundamentally wrong, in order to prevent bug flooding.
+  """
+  first_config_output = first_cmd.run(
+      SANITY_CHECKS, timeout=SANITY_CHECK_TIMEOUT_SEC)
+
+  second_config_output = second_cmd.run(
+      SANITY_CHECKS, timeout=SANITY_CHECK_TIMEOUT_SEC)
+
+  difference, _ = suppress.diff(first_config_output, second_config_output)
+  if difference:
+    # Special source key for sanity checks so that clusterfuzz dedupes all
+    # cases on this in case it's hit.
+    source_key = 'sanity check failed'
+    raise FailException(format_difference(
+        options, source_key, first_cmd, second_cmd,
+        first_config_output, second_config_output, difference))
+
+
 def main():
   options = parse_args()
 
@@ -378,80 +384,43 @@ def main():
     kwargs['encoding'] = 'utf-8'
   with open(options.testcase, 'r', **kwargs) as f:
     content = f.read()
-  if content_bailout(get_meta_data(content), suppress.ignore_by_metadata):
-    return RETURN_FAIL
-  if content_bailout(content, suppress.ignore_by_content):
-    return RETURN_FAIL
-
-  first_cmd = v8_commands.Command(
-      options,'first', options.first.d8, options.first.flags)
-  second_cmd = v8_commands.Command(
+  content_bailout(get_meta_data(content), suppress.ignore_by_metadata)
+  content_bailout(content, suppress.ignore_by_content)
+
+  first_cmd = Command(
+      options, 'first', options.first.d8, options.first.flags)
+  second_cmd = Command(
       options, 'second', options.second.d8, options.second.flags)
 
   # Sanity checks. Run both configurations with the sanity-checks file only and
   # bail out early if different.
   if not options.skip_sanity_checks:
-    first_config_output = first_cmd.run(
-        SANITY_CHECKS, timeout=SANITY_CHECK_TIMEOUT_SEC)
-
-    # Early bailout if first run was a timeout.
-    if timeout_bailout(first_config_output, 1):
-      return RETURN_PASS
-
-    second_config_output = second_cmd.run(
-        SANITY_CHECKS, timeout=SANITY_CHECK_TIMEOUT_SEC)
-
-    # Bailout if second run was a timeout.
-    if timeout_bailout(second_config_output, 2):
-      return RETURN_PASS
-
-    difference, _ = suppress.diff(first_config_output, second_config_output)
-    if difference:
-      # Special source key for sanity checks so that clusterfuzz dedupes all
-      # cases on this in case it's hit.
-      source_key = 'sanity check failed'
-      print_difference(
-          options, source_key, first_cmd, second_cmd,
-          first_config_output, second_config_output, difference)
-      return RETURN_FAIL
+    run_sanity_checks(options, suppress, first_cmd, second_cmd)
 
   first_config_output = first_cmd.run(
       options.testcase, timeout=TEST_TIMEOUT_SEC, verbose=True)
 
-  # Early bailout if first run was a timeout.
-  if timeout_bailout(first_config_output, 1):
-    return RETURN_PASS
-
   second_config_output = second_cmd.run(
       options.testcase, timeout=TEST_TIMEOUT_SEC, verbose=True)
 
-  # Bailout if second run was a timeout.
-  if timeout_bailout(second_config_output, 2):
-    return RETURN_PASS
-
   difference, source = suppress.diff(first_config_output, second_config_output)
 
   if difference:
     # Only bail out due to suppressed output if there was a difference. If a
     # suppression doesn't show up anymore in the statistics, we might want to
     # remove it.
-    if fail_bailout(first_config_output, suppress.ignore_by_output1):
-      return RETURN_FAIL
-    if fail_bailout(second_config_output, suppress.ignore_by_output2):
-      return RETURN_FAIL
+    fail_bailout(first_config_output, suppress.ignore_by_output1)
+    fail_bailout(second_config_output, suppress.ignore_by_output2)
 
     source_key = cluster_failures(source)
-    print_difference(
+    raise FailException(format_difference(
         options, source_key, first_cmd, second_cmd,
-        first_config_output, second_config_output, difference, source)
-    return RETURN_FAIL
+        first_config_output, second_config_output, difference, source))
 
   # Show if a crash has happened in one of the runs and no difference was
   # detected.
-  if first_config_output.HasCrashed():
-    print('# V8 correctness - C-R-A-S-H 1')
-  elif second_config_output.HasCrashed():
-    print('# V8 correctness - C-R-A-S-H 2')
+  if first_config_output.HasCrashed() or second_config_output.HasCrashed():
+    print('# V8 correctness - C-R-A-S-H')
   else:
     # TODO(machenbach): Figure out if we could also return a bug in case
     # there's no difference, but one of the line suppressions has matched -
@@ -464,6 +433,12 @@ def main():
 if __name__ == "__main__":
   try:
     result = main()
+  except FailException as e:
+    print(e.message)
+    result = RETURN_FAIL
+  except PassException as e:
+    print(e.message)
+    result = RETURN_PASS
   except SystemExit:
     # Make sure clusterfuzz reports internal errors and wrong usage.
     # Use one label for all internal and usage errors.

tools/clusterfuzz/v8_foozzie_test.py

@@ -201,8 +201,7 @@ v8-foozzie source: foo
   def testOutputCapping(self):
     def output(stdout, is_crash):
       exit_code = -1 if is_crash else 0
-      return v8_commands.Output(
-          exit_code=exit_code, timed_out=False, stdout=stdout, pid=0)
+      return v8_commands.Output(exit_code=exit_code, stdout=stdout, pid=0)
 
     def check(stdout1, stdout2, is_crash1, is_crash2, capped_lines1,
               capped_lines2):