[numfuzz] Gracefully ignore contradictory flags

NumFuzz passes various flags to V8 testing randomly, which can lead to
various flag contradictions with existing flags. Up to now the system
ignored the check for contradictions and kept running the test cases,
leading to false positives.

This change adds a new v8 flag --exit-on-contradictory-flags that
exists gracefully when a contradiction is detected. On the numfuzz
side we now filter simple contradictions beforehand.

Measurements showed that ~2% of all numfuzz tests ran into
contradictions. Around half of them are simple contradictions
(repetitions and inversions), which are now filtered beforehand.
The remaining ones (redundant or contradictory implications) are
now ignored.

Bug: v8:11826
Change-Id: I9942e203ba9668a097fabe1343dd1365c9da94c1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3650746
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Almothana Athamneh <almuthanna@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80589}

src/flags/flag-definitions.h

@@ -277,6 +277,12 @@ DEFINE_BOOL(abort_on_contradictory_flags, false,
 // This implication is also hard-coded into the flags processing to make sure it
 // becomes active before we even process subsequent flags.
 DEFINE_NEG_IMPLICATION(fuzzing, abort_on_contradictory_flags)
+// As abort_on_contradictory_flags, but it will simply exit with return code 0.
+DEFINE_BOOL(exit_on_contradictory_flags, false,
+            "Exit with return code 0 on contradictory flags.")
+// We rely on abort_on_contradictory_flags to turn on the analysis.
+DEFINE_WEAK_IMPLICATION(exit_on_contradictory_flags,
+                        abort_on_contradictory_flags)
 // This is not really a flag, it affects the interpretation of the next flag but
 // doesn't become permanently true when specified. This only works for flags
 // defined in this file, but not for d8 flags defined in src/d8/d8.cc.

src/flags/flags.cc

@@ -277,6 +277,8 @@ struct Flag {
           break;
         case SetBy::kCommandLine:
           if (new_set_by == SetBy::kImplication && check_command_line_flags) {
+            // Exit instead of abort for certain testing situations.
+            if (FLAG_exit_on_contradictory_flags) base::OS::ExitProcess(0);
             if (is_bool_flag) {
               FATAL(
                   "Flag --%s: value implied by --%s conflicts with explicit "
@@ -290,6 +292,8 @@ struct Flag {
             }
           } else if (new_set_by == SetBy::kCommandLine &&
                      check_command_line_flags) {
+            // Exit instead of abort for certain testing situations.
+            if (FLAG_exit_on_contradictory_flags) base::OS::ExitProcess(0);
             if (is_bool_flag) {
               FATAL(
                   "Command-line provided flag --%s specified as both true and "

tools/testrunner/num_fuzzer.py

@@ -124,9 +124,7 @@ class NumFuzzer(base_runner.BaseTestRunner):
   def _runner_flags(self):
     """Extra default flags specific to the test runner implementation."""
     return [
-      '--no-abort-on-contradictory-flags',
-      '--testing-d8-test-runner',
-      '--no-fail'
+        '--exit-on-contradictory-flags', '--testing-d8-test-runner', '--no-fail'
     ]
 
   def _get_statusfile_variables(self, options):

tools/testrunner/objects/testcase.py

@@ -301,8 +301,14 @@ class TestCase(object):
     return self._create_cmd(shell, shell_flags + params, env, timeout)
 
   def _get_cmd_params(self):
-    """Gets command parameters and combines them in the following order:
+    """Gets all command parameters and combines them in the following order:
       - files [empty by default]
+      - all flags
+    """
+    return (self._get_files_params() + self.get_flags())
+
+  def get_flags(self):
+    """Gets all flags and combines them in the following order:
       - random seed
       - mode flags (based on chosen mode)
       - extra flags (from command line)
@@ -315,7 +321,6 @@ class TestCase(object):
     methods for getting partial parameters.
     """
     return (
-        self._get_files_params() +
         self._get_random_seed_flags() +
         self._get_mode_flags() +
         self._get_extra_flags() +

tools/testrunner/outproc/webkit.py

@@ -9,6 +9,10 @@ class OutProc(base.ExpectedOutProc):
   def _is_failure_output(self, output):
     if output.exit_code != 0:
       return True
+    if output.exit_code == 0 and not output.stdout.strip():
+      # Special case for --exit-on-contradictory-flags. It lets tests exit
+      # fast with exit code 0. Normally passing webkit tests all have output.
+      return False
     return super(OutProc, self)._is_failure_output(output)
 
   def _ignore_expected_line(self, line):

tools/testrunner/testproc/fuzzer.py

@@ -52,6 +52,7 @@ EXTRA_FLAGS = [
     (0.1, '--turbo-force-mid-tier-regalloc'),
 ]
 
+
 def random_extra_flags(rng):
   """Returns a random list of flags chosen from the configurations in
   EXTRA_FLAGS.
@@ -59,6 +60,49 @@ def random_extra_flags(rng):
   return [flag for prob, flag in EXTRA_FLAGS if rng.random() < prob]
 
 
+def _flag_prefix(flag):
+  """Returns the flag part before an equal sign."""
+  if '=' not in flag:
+    return flag
+  else:
+    return flag[0:flag.index('=')]
+
+
+def _invert_flag(flag):
+  """Flips a --flag and its --no-flag counterpart."""
+  assert flag.startswith('--')
+  if flag.startswith('--no-'):
+    return '--' + flag[len('--no-'):]
+  else:
+    return '--no-' + flag[2:]
+
+
+def _drop_contradictory_flags(new_flags, existing_flags):
+  """Drops flags that have a simple contradiction with an existing flag.
+
+  Contradictions checked for:
+  - Repetition: --flag --flag
+  - Repetition with param: --flag=foo --flag=bar
+  - Negation: --flag --no-flag
+  - Inverse negation: --no-flag --flag
+  - For simplicity also drops combinations of negation and param, which don't
+    occur in practice.
+
+  Args:
+    new_flags: new flags to filter from
+    existing_flags: existing flags checked against
+  Returns: A list of flags without contradictions.
+  """
+  existing_flag_prefixes = set(_flag_prefix(flag) for flag in existing_flags)
+
+  def contradictory_flag(flag):
+    flag_prefix = _flag_prefix(flag)
+    return (flag_prefix in existing_flag_prefixes or
+            _invert_flag(flag_prefix) in existing_flag_prefixes)
+
+  return [flag for flag in new_flags if not contradictory_flag(flag)]
+
+
 class FuzzerConfig(object):
   def __init__(self, probability, analyzer, fuzzer):
     """
@@ -190,6 +234,8 @@ class FuzzerProc(base.TestProcProducer):
           flags += next(gen)
 
       flags.append('--fuzzer-random-seed=%s' % self._next_seed())
+
+      flags = _drop_contradictory_flags(flags, test.get_flags())
       yield self._create_subtest(test, str(i), flags=flags)
 
       i += 1

tools/testrunner/testproc/fuzzer_test.py

+#!/usr/bin/env python3
+# Copyright 2022 the V8 project authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+"""
+Test functionality used by the fuzzer test processor.
+"""
+
+import os
+import sys
+import unittest
+
+# Needed because the test runner contains relative imports.
+TOOLS_PATH = os.path.dirname(
+    os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+sys.path.append(TOOLS_PATH)
+
+from testrunner.testproc import fuzzer
+
+
+class TestFuzzerProcHelpers(unittest.TestCase):
+
+  def test_invert_flag(self):
+    self.assertEqual(fuzzer._invert_flag('--no-flag'), '--flag')
+    self.assertEqual(fuzzer._invert_flag('--flag'), '--no-flag')
+
+  def test_flag_prefix(self):
+    self.assertEqual(fuzzer._flag_prefix('--flag'), '--flag')
+    self.assertEqual(fuzzer._flag_prefix('--flag=42'), '--flag')
+
+  def test_contradictory_flags(self):
+
+    def check(new_flags, old_flags, expected_flags):
+      actual = fuzzer._drop_contradictory_flags(new_flags, old_flags)
+      self.assertEqual(actual, expected_flags)
+
+    old_flags = ['--foo-bar', '--no-bar', '--baz', '--doom-melon=42', '--flag']
+    check([], old_flags, [])
+    check(['--flag', '--no-baz', '--bar'], old_flags, [])
+    check(['--foo-bar=42', '--doom-melon=0'], old_flags, [])
+    check(['--flag', '--something', '--bar'], old_flags, ['--something'])
+    check(['--something', '--doom-melon=0'], old_flags, ['--something'])
+    check(old_flags, old_flags, [])
+    check(['--a', '--b'], old_flags, ['--a', '--b'])
+
+    old_flags = []
+    check(['--flag', '--no-baz'], old_flags, ['--flag', '--no-baz'])
+
+
+if __name__ == '__main__':
+  unittest.main()