[foozzie] Prioritize reporting differences with x64 if present

tldr: This adds an on-demand comparison with x64 when a difference to
a non-x64 build is detected.

Normally foozzie compares the baseline build (just ignition), with
two secondary builds. One, the default, always uses the shipping
configuration, the second passes additional flags. Both can use a
different architecture than the baseline build as well.

Differences between ignition and turbofan are then often detected
independent of the architectures used, but reported several times
(for each compared architecture).

This makes the reporting more specific, by running another build on
demand that uses the baseline architecture, but otherwise the same
configuration that showed a difference. If it shows the difference as
well, the baseline architecture is used for the report.

As a result only pure architecture differences will be reported with
an architecture other than x64.

This also adds some minor refactorings to reduce the code complexity
when looping over comparisons.

For testing this, the fake-d8s are extended with different behavior
for different flags passed. We add two test cases for testing:
x64 vs. ia32 with difference in x64 and ia32
x64 vs. ia32 with difference only in ia32

Bug: chromium:1196633
No-Try: true
Test: tools/clusterfuzz/v8_foozzie_test.py
Change-Id: Ic470ae8f0b37fb1628b32e4fafc0c39377e16f8c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2897099Reviewed-by: Liviu Rau <liviurau@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74612}

tools/clusterfuzz/testdata/baseline/d8.py

@@ -5,6 +5,8 @@
 # for py2/py3 compatibility
 from __future__ import print_function
 
+import sys
+
 print("""
 1
 v8-foozzie source: name/to/a/file.js
@@ -15,3 +17,6 @@ v8-foozzie source: name/to/file.js
 3
 unknown
 """)
+
+if '--bad-flag' in sys.argv:
+  print('bad behavior')

tools/clusterfuzz/testdata/build3/d8.py

@@ -5,6 +5,8 @@
 # for py2/py3 compatibility
 from __future__ import print_function
 
+import sys
+
 print("""
 1
 v8-foozzie source: name/to/a/file.js
@@ -15,3 +17,8 @@ v8-foozzie source: name/to/file.js
 3
 unknown
 """)
+
+if '--bad-flag' in sys.argv:
+  print('bad behavior')
+if '--very-bad-flag' in sys.argv:
+  print('very bad behavior')

tools/clusterfuzz/testdata/failure_output_arch.txt

+#
+# V8 correctness failure
+# V8 correctness configs: x64,ignition:x64,ignition_turbo
+# V8 correctness sources: f60
+# V8 correctness suppression: 
+#
+# CHECK
+#
+# Compared x64,ignition with x64,ignition_turbo
+#
+# Flags of x64,ignition:
+--correctness-fuzzer-suppressions --expose-gc --fuzzing --allow-natives-for-differential-fuzzing --invoke-weak-callbacks --omit-quit --es-staging --wasm-staging --no-wasm-async-compilation --suppress-asm-messages --random-seed 12345 --turbo-filter=~ --noopt --liftoff --no-wasm-tier-up
+# Flags of x64,ignition_turbo:
+--correctness-fuzzer-suppressions --expose-gc --fuzzing --allow-natives-for-differential-fuzzing --invoke-weak-callbacks --omit-quit --es-staging --wasm-staging --no-wasm-async-compilation --suppress-asm-messages --random-seed 12345 --bad-flag
+#
+# Difference:
++ bad behavior
+#
+# Source file:
+name/to/file.js
+#
+### Start of configuration x64,ignition:
+
+1
+v8-foozzie source: name/to/a/file.js
+2
+v8-foozzie source: name/to/file.js
+  weird error
+        ^
+3
+unknown
+
+
+### End of configuration x64,ignition
+#
+### Start of configuration x64,ignition_turbo:
+
+1
+v8-foozzie source: name/to/a/file.js
+2
+v8-foozzie source: name/to/file.js
+  weird error
+        ^
+3
+unknown
+
+bad behavior
+
+### End of configuration x64,ignition_turbo
+

tools/clusterfuzz/testdata/failure_output_second.txt

+#
+# V8 correctness failure
+# V8 correctness configs: x64,ignition:ia32,ignition_turbo
+# V8 correctness sources: f60
+# V8 correctness suppression: 
+#
+# CHECK
+#
+# Compared x64,ignition with ia32,ignition_turbo
+#
+# Flags of x64,ignition:
+--correctness-fuzzer-suppressions --expose-gc --fuzzing --allow-natives-for-differential-fuzzing --invoke-weak-callbacks --omit-quit --es-staging --wasm-staging --no-wasm-async-compilation --suppress-asm-messages --random-seed 12345 --turbo-filter=~ --noopt --liftoff --no-wasm-tier-up
+# Flags of ia32,ignition_turbo:
+--correctness-fuzzer-suppressions --expose-gc --fuzzing --allow-natives-for-differential-fuzzing --invoke-weak-callbacks --omit-quit --es-staging --wasm-staging --no-wasm-async-compilation --suppress-asm-messages --random-seed 12345 --very-bad-flag
+#
+# Difference:
++ very bad behavior
+#
+# Source file:
+name/to/file.js
+#
+### Start of configuration x64,ignition:
+
+1
+v8-foozzie source: name/to/a/file.js
+2
+v8-foozzie source: name/to/file.js
+  weird error
+        ^
+3
+unknown
+
+
+### End of configuration x64,ignition
+#
+### Start of configuration ia32,ignition_turbo:
+
+1
+v8-foozzie source: name/to/a/file.js
+2
+v8-foozzie source: name/to/file.js
+  weird other error
+^
+3
+unknown
+
+very bad behavior
+
+### End of configuration ia32,ignition_turbo
+

tools/clusterfuzz/v8_foozzie.py

@@ -211,14 +211,14 @@ class ExecutionArgumentsConfig(object):
         'default: bundled in the directory of this script',
         default=DEFAULT_D8)
 
-  def make_options(self, options, default_config=None):
+  def make_options(self, options, default_config=None, default_d8=None):
     def get(name):
       return getattr(options, '%s_%s' % (self.label, name))
 
     config = default_config or get('config')
     assert config in CONFIGS
 
-    d8 = get('d8')
+    d8 = default_d8 or get('d8')
     if not os.path.isabs(d8):
       d8 = os.path.join(BASE_PATH, d8)
     assert os.path.exists(d8)
@@ -239,6 +239,13 @@ class ExecutionConfig(object):
     flags = getattr(options, label).flags
     self.command = Command(options, label, d8, flags)
 
+    # Options for a fallback configuration only exist when comparing
+    # different architectures.
+    fallback_label = label + '_fallback'
+    self.fallback = None
+    if getattr(options, fallback_label, None):
+      self.fallback = ExecutionConfig(options, fallback_label)
+
   @property
   def flags(self):
     return self.command.flags
@@ -278,7 +285,15 @@ def parse_args():
   options.first = first_config_arguments.make_options(options)
   options.second = second_config_arguments.make_options(options)
   options.default = second_config_arguments.make_options(
-      options, DEFAULT_CONFIG)
+      options, default_config=DEFAULT_CONFIG)
+
+  # Use fallback configurations only on diffrent architectures. In this
+  # case we are going to re-test against the first architecture.
+  if options.first.arch != options.second.arch:
+    options.second_fallback = second_config_arguments.make_options(
+        options, default_d8=options.first.d8)
+    options.default_fallback = second_config_arguments.make_options(
+        options, default_config=DEFAULT_CONFIG, default_d8=options.first.d8)
 
   # Ensure we make a valid comparison.
   if (options.first.d8 == options.second.d8 and
@@ -315,10 +330,12 @@ def fail_bailout(output, ignore_by_output_fun):
 
 
 def format_difference(
-    source_key, first_config, second_config,
-    first_config_output, second_config_output, difference, source=None):
+    first_config, second_config,
+    first_config_output, second_config_output,
+    difference, source_key=None, source=None):
   # The first three entries will be parsed by clusterfuzz. Format changes
   # will require changes on the clusterfuzz side.
+  source_key = source_key or cluster_failures(source)
   first_config_label = '%s,%s' % (first_config.arch, first_config.config)
   second_config_label = '%s,%s' % (second_config.arch, second_config.config)
   source_file_text = SOURCE_FILE_TEMPLATE % source if source else ''
@@ -376,6 +393,29 @@ def cluster_failures(source, known_failures=None):
   return long_key[:ORIGINAL_SOURCE_HASH_LENGTH]
 
 
+class RepeatedRuns(object):
+  """Helper class for storing statistical data from repeated runs."""
+  def __init__(self, test_case, timeout, verbose):
+    self.test_case = test_case
+    self.timeout = timeout
+    self.verbose = verbose
+
+    # Stores if any run has crashed or was simulated.
+    self.has_crashed = False
+    self.simulated = False
+
+  def run(self, config):
+    comparison_output = config.command.run(
+        self.test_case, timeout=self.timeout, verbose=self.verbose)
+    self.has_crashed = self.has_crashed or comparison_output.HasCrashed()
+    self.simulated = self.simulated or config.is_error_simulation
+    return comparison_output
+
+  @property
+  def crash_state(self):
+    return '_simulated_crash_' if self.simulated else '_unexpected_crash_'
+
+
 def run_comparisons(suppress, execution_configs, test_case, timeout,
                     verbose=True, ignore_crashes=True, source_key=None):
   """Runs different configurations and bails out on output difference.
@@ -393,20 +433,15 @@ def run_comparisons(suppress, execution_configs, test_case, timeout,
     source_key: A fixed source key. If not given, it will be inferred from the
         output.
   """
-  run_test_case = lambda config: config.command.run(
-      test_case, timeout=timeout, verbose=verbose)
+  runner = RepeatedRuns(test_case, timeout, verbose)
 
   # Run the baseline configuration.
   baseline_config = execution_configs[0]
-  baseline_output = run_test_case(baseline_config)
-  has_crashed = baseline_output.HasCrashed()
-  simulated = baseline_config.is_error_simulation
+  baseline_output = runner.run(baseline_config)
 
   # Iterate over the remaining configurations, run and compare.
   for comparison_config in execution_configs[1:]:
-    comparison_output = run_test_case(comparison_config)
-    has_crashed = has_crashed or comparison_output.HasCrashed()
-    simulated = simulated or comparison_config.is_error_simulation
+    comparison_output = runner.run(comparison_config)
     difference, source = suppress.diff(baseline_output, comparison_output)
 
     if difference:
@@ -416,12 +451,25 @@ def run_comparisons(suppress, execution_configs, test_case, timeout,
       fail_bailout(baseline_output, suppress.ignore_by_output)
       fail_bailout(comparison_output, suppress.ignore_by_output)
 
-      source_key = source_key or cluster_failures(source)
+      # Check if a difference also occurs with the fallback configuration and
+      # give it precedence. E.g. we always prefer x64 differences.
+      if comparison_config.fallback:
+        fallback_output = runner.run(comparison_config.fallback)
+        fallback_difference, fallback_source = suppress.diff(
+            baseline_output, fallback_output)
+        if fallback_difference:
+          fail_bailout(fallback_output, suppress.ignore_by_output)
+          source = fallback_source
+          comparison_config = comparison_config.fallback
+          comparison_output = fallback_output
+          difference = fallback_difference
+
       raise FailException(format_difference(
-          source_key, baseline_config, comparison_config,
-          baseline_output, comparison_output, difference, source))
+          baseline_config, comparison_config,
+          baseline_output, comparison_output,
+          difference, source_key, source))
 
-  if has_crashed:
+  if runner.has_crashed:
     if ignore_crashes:
       # Show if a crash has happened in one of the runs and no difference was
       # detected. This is only for the statistics during experiments.
@@ -429,9 +477,8 @@ def run_comparisons(suppress, execution_configs, test_case, timeout,
     else:
       # Subsume simulated and unexpected crashes (e.g. during smoke tests)
       # with one failure state.
-      crash_state = '_simulated_crash_' if simulated else '_unexpected_crash_'
       raise FailException(FAILURE_HEADER_TEMPLATE % dict(
-          configs='', source_key='', suppression=crash_state))
+          configs='', source_key='', suppression=runner.crash_state))
 
 
 def main():
@@ -448,7 +495,7 @@ def main():
   content_bailout(content, suppress.ignore_by_content)
 
   # Prepare the baseline, default and a secondary configuration to compare to.
-  # The baseline (turbofan) takes precedence as many of the secondary configs
+  # The default (turbofan) takes precedence as many of the secondary configs
   # are based on the turbofan config with additional parameters.
   execution_configs = [
     ExecutionConfig(options, 'first'),

tools/clusterfuzz/v8_foozzie_test.py

@@ -263,7 +263,7 @@ class SystemTest(unittest.TestCase):
 
   Overview of fakes:
     baseline: Example foozzie output including a syntax error.
-    build1: Difference to baseline is a stack trace differece expected to
+    build1: Difference to baseline is a stack trace difference expected to
             be suppressed.
     build2: Difference to baseline is a non-suppressed output difference
             causing the script to fail.
@@ -312,6 +312,36 @@ class SystemTest(unittest.TestCase):
     self.assertIn('v8_mock_archs.js', lines[1])
     self.assertIn('v8_mock_archs.js', lines[3])
 
+  def testDifferentArchFailFirst(self):
+    """Test that we re-test against x64. This tests the path that also fails
+    on x64 and then reports the error as x64.
+    """
+    with open(os.path.join(TEST_DATA, 'failure_output_arch.txt')) as f:
+      expected_output = f.read()
+    # Build 3 simulates x86 and produces a difference on --bad-flag, but
+    # the baseline build shows the same difference when --bad-flag is passed.
+    with self.assertRaises(subprocess.CalledProcessError) as ctx:
+      run_foozzie('build3', '--skip-smoke-tests',
+                  '--second-config-extra-flags=--bad-flag')
+    e = ctx.exception
+    self.assertEqual(v8_foozzie.RETURN_FAIL, e.returncode)
+    self.assertEqual(expected_output, cut_verbose_output(e.output, 3))
+
+  def testDifferentArchFailSecond(self):
+    """As above, but we test the path that only fails in the second (ia32)
+    run and not with x64 and then reports the error as ia32.
+    """
+    with open(os.path.join(TEST_DATA, 'failure_output_second.txt')) as f:
+      expected_output = f.read()
+    # Build 3 simulates x86 and produces a difference on --very-bad-flag,
+    # which the baseline build doesn't.
+    with self.assertRaises(subprocess.CalledProcessError) as ctx:
+      run_foozzie('build3', '--skip-smoke-tests',
+                  '--second-config-extra-flags=--very-bad-flag')
+    e = ctx.exception
+    self.assertEqual(v8_foozzie.RETURN_FAIL, e.returncode)
+    self.assertEqual(expected_output, cut_verbose_output(e.output, 3))
+
   def testJitless(self):
     """Test that webassembly is mocked out when comparing with jitless."""
     stdout = run_foozzie(