[FileCheck] Default --allow-unused-prefixes to false

[MaskRay]

Summary

Link: https://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html "[RFC] FileCheck: (dis)allowing unused prefixes"

If a downstream project using lit needs time for transition,
add the following to lit.local.cfg:

from lit.llvm.subst import ToolSubst

fc = ToolSubst('FileCheck', unresolved='fatal')
config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))

[jhenderson]

I assume this now runs cleanly. If so, LGTM. Probably worth mentioning on the mailing list thread too, so wrap up that conversation. By the way, did we ever figure out how many of the failures were genuine bugs versus deliberate isntances?

[mtrofin]

In D95849#2535939, @jhenderson wrote:

I assume this now runs cleanly. If so, LGTM. Probably worth mentioning on the mailing list thread too, so wrap up that conversation. By the way, did we ever figure out how many of the failures were genuine bugs versus deliberate isntances?

Except for the tests under llvm/test/Transforms/Attributor/, which D94744 explicitly opts into allowing unused prefixes (and which @jdoerfert described as intentionally so), the rest (~1300) were bugs: i.e. unintentionally unused prefixes.

Out of them, a small handful were spelling mistakes (identified as such during code review by their authors) and fixed rather than removed.

[nikic]

Just to be clear here: Only the small handful where a spelling mistake was fixed were actually bugs. All other unused check prefixes were there for convenience, and are now casualties of this unnecessary crusade. I regret not speaking out against this at the time.

[MaskRay]

In D95849#2536559, @nikic wrote:

Just to be clear here: Only the small handful where a spelling mistake was fixed were actually bugs. All other unused check prefixes were there for convenience, and are now casualties of this unnecessary crusade. I regret not speaking out against this at the time.

Hi, I think the situation is the converse: most are code smell, only a small handful of tests leverage this property. So far the most convincing usage is something like (a) --check-prefix=%something where %something can expand to multiple prefixes and (b) a customized tool integrating FileCheck feature which expands to multiple prefixes, some prefixes may be omitted.

Issues include (a) misspelled prefixes (b) bitrot prefixes due to refactoring (c) omitted -NOT patterns (e.g. some tests use --check-prefixes=CHECK,CHECK-C for C specific UB and --check-prefixes=CHECK,CHECK-CPP. CHECK-CPP was absent but was intended to be a CHECK-CPP-NOT:)

I think the good uses of --allow-unused-prefixes=true are much fewer than the opposite.

[MaskRay]

Rebase to re-cycle bots

[MaskRay]

@nikic D95849 has already changed llvm/test/ to default to --allow-unused-prefixes. I updated a few directories to be --allow-unused-prefixes clean (this uncovered improvement to many tests).

Some update{,_llc,_analyzer}_test_checks.py use cases may find --allow-unused-prefixes=false mode better, but such use cases still appear to be a minority (e.g. @RKSimon/@pengfei/my/others' fixes to test/CodeGen directories).
https://lists.llvm.org/pipermail/llvm-dev/2021-February/148331.html

clang/test/Driver/crash-report-null.test

 // RUN: env FORCE_CLANG_DIAGNOSTICS_CRASH=1 not %clang -fsyntax-only -x c /dev/null -lstdc++ 2>&1 | FileCheck %s
 
 // FIXME: Investigating. "fatal error: file 'nul' modified since it was first processed"
-// XFAIL: windows-gnu
+// UNSUPPORTED: system-windows
 
 // CHECK: PLEASE submit a bug report to {{.*}} and include the crash backtrace, preprocessed source, and associated run script.
 // CHECK: Preprocessed source(s) and associated run script(s) are located at:
 // CHECK-NEXT: note: diagnostic msg: {{.*}}null-{{.*}}.c

clang/test/OpenMP/lit.local.cfg

 # -*- Python -*- vim: set ft=python ts=4 sw=4 expandtab tw=79:
 from lit.llvm.subst import ToolSubst
 
 fc = ToolSubst('FileCheck', unresolved='fatal')
-# the parent introduced the opposite rule, so we replace it if we see it.
-if len(config.substitutions) > 0 and config.substitutions[0] == (fc.regex, 'FileCheck --allow-unused-prefixes=false'):
-    config.substitutions[0] = (
-        fc.regex, 'FileCheck --allow-unused-prefixes=true')
-else:
-    config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes=true'))
+config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))

clang/test/lit.cfg.py

 
 # test_exec_root: The root path where tests should be run.
 config.test_exec_root = os.path.join(config.clang_obj_root, 'test')
 
 llvm_config.use_default_substitutions()
 
 llvm_config.use_clang()
 
-# FIXME: remove this when we flip the default value for --allow-unused-prefixes
-# to false.
-fc = ToolSubst('FileCheck', unresolved='fatal')
-# Insert this first. Then, we'll first update the blank FileCheck command; then,
-# the default substitution of FileCheck will replace it to its full path.
-config.substitutions.insert(0, (fc.regex,
-    'FileCheck --allow-unused-prefixes=false'))
-
 config.substitutions.append(
     ('%src_include_dir', config.clang_src_dir + '/include'))
 
 config.substitutions.append(
     ('%target_triple', config.target_triple))
 
 # Propagate path to symbolizer for ASan/MSan.
 llvm_config.with_system_environment(

llvm/test/FileCheck/allow-unused-prefixes.txt

 ; RUN: %ProtectFileCheckOutput not FileCheck --allow-unused-prefixes=false --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 2>&1 | FileCheck --check-prefix=MISSING-ONE %s
 ; RUN: %ProtectFileCheckOutput not FileCheck --allow-unused-prefixes=false --check-prefixes=P1,P2,P3 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 2>&1 | FileCheck --check-prefix=MISSING-MORE %s
 ; RUN: FileCheck --allow-unused-prefixes=true  --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 
 ; RUN: FileCheck --allow-unused-prefixes=false --allow-unused-prefixes=true --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt
-
-;; Note: the default will be changed to 'false', at which time this run line
-;; should be changed accordingly.
-; RUN: FileCheck --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt 
+; RUN: not FileCheck --check-prefixes=P1,P2 --input-file %S/Inputs/one-check.txt %S/Inputs/one-check.txt
 
 ; MISSING-ONE: error: no check strings found with prefix 'P2:' 
 ; MISSING-MORE: error: no check strings found with prefixes 'P2:', 'P3:'

llvm/test/FileCheck/lit.local.cfg

 # %ProtectFileCheckOutput's purpose is to ensure correct test results when
 # developers set FileCheck environment variables (e.g.,
 # FILECHECK_OPTS=-dump-input=fail) to tweak the output of FileCheck while
 # debugging tests.  If a developer sets values that affect FileCheck's exit
 # status (e.g., FILECHECK_OPTS=-strict-whitespace), he shouldn't be surprised
 # that test results throughout all test suites are affected.
 config.substitutions.append(('%ProtectFileCheckOutput',
     'env -u FILECHECK_OPTS'))
-
-# FIXME: remove this once the default is flipped.
-from lit.llvm.subst import ToolSubst
-
-fc = ToolSubst('FileCheck', unresolved='fatal')
-# the parent introduced the opposite rule, so we replace it if we see it.
-if len(config.substitutions) > 0 and config.substitutions[0] == (fc.regex, 'FileCheck --allow-unused-prefixes=false'):
-    del config.substitutions[0]
-

llvm/test/Other/opt-bisect-legacy-pass-manager.ll

 ; FIXME: There are still some AMDGPU passes being skipped that run at -O0.
 ; XFAIL: r600, amdgcn
 
 ; Verify that we can use the opt-bisect-helper.py script (derived from
 ; utils/bisect) to locate the optimization that inlines the call to
 ; f2() in f3().
 
 ; RUN: %python %S/opt-bisect-helper.py --start=0 --end=256 --optcmd=opt \
-; RUN:         --filecheckcmd=%FileCheckRaw% --test=%s \
+; RUN:         --filecheckcmd=FileCheck --test=%s \
 ; RUN:         --prefix=CHECK-BISECT-INLINE-HELPER \
 ; RUN:         | FileCheck %s --check-prefix=CHECK-BISECT-INLINE-RESULT
 ; The helper script uses this to find the optimization that inlines the call.
 ; CHECK-BISECT-INLINE-HELPER: call i32 @f2()
 ; These checks verifies that the optimization was found.
 ; CHECK-BISECT-INLINE-RESULT-NOT: Last good count: 0
 ; CHECK-BISECT-INLINE-RESULT: Last good count: {{[0-9]+}}
 

llvm/test/Reduce/lit.local.cfg

-# -*- Python -*- vim: set ft=python ts=4 sw=4 expandtab tw=79:
-# FIXME: remove this file when we flip the default for --allow-unused-prefixes.
-from lit.llvm.subst import ToolSubst
-
-fc = ToolSubst('FileCheck', unresolved='fatal')
-# the parent introduced the opposite rule, so we replace it if we see it.
-if len(config.substitutions) > 0 and config.substitutions[0] == (fc.regex, 'FileCheck --allow-unused-prefixes=false'):
-    del config.substitutions[0]

llvm/test/Transforms/Attributor/lit.local.cfg

 # -*- Python -*- vim: set ft=python ts=4 sw=4 expandtab tw=79:
 from lit.llvm.subst import ToolSubst
 
 fc = ToolSubst('FileCheck', unresolved='fatal')
-# the parent introduced the opposite rule, so we replace it if we see it.
-if len(config.substitutions) > 0 and config.substitutions[0] == (fc.regex, 'FileCheck --allow-unused-prefixes=false'):
-    config.substitutions[0] = (
-        fc.regex, 'FileCheck --allow-unused-prefixes=true')
-else:
-    config.substitutions.insert(0, (fc.regex, 
-                                    'FileCheck --allow-unused-prefixes=true'))
+config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))

llvm/test/lit.cfg.py

def get_asan_rtlib():
     asan_dylib_dir_pattern = host_lib_dir + \
         '/clang/*/lib/darwin/libclang_rt.asan_osx_dynamic.dylib'
     found_dylibs = glob.glob(asan_dylib_dir_pattern)
     if len(found_dylibs) != 1:
         return ''
     return found_dylibs[0]
 
 
-####################################################
-# FIXME: remove this when we flip the default value for --allow-unused-prefixes
-# to false.
-fc = ToolSubst('FileCheck', unresolved='fatal')
-# Insert this first. Then, we'll first update the blank FileCheck command; then,
-# the default substitution of FileCheck will replace it to its full path.
-config.substitutions.insert(0, (fc.regex,
-    'FileCheck --allow-unused-prefixes=false'))
-# When addressing this fixme, replace %FileCheckRaw% with just FileCheck.
-config.substitutions.append(('%FileCheckRaw%', 'FileCheck'))
-# Also remove the lit.local.cfg under llvm/test/Reduce
-# and the pertinent FIXME in llvm/test/FileCheck
-####################################################
-
 llvm_config.use_default_substitutions()
 
 # Add site-specific substitutions.
 config.substitutions.append(('%llvmshlibdir', config.llvm_shlib_dir))
 config.substitutions.append(('%shlibext', config.llvm_shlib_ext))
 config.substitutions.append(('%exeext', config.llvm_exe_ext))
 
 
tools = [
     ToolSubst('%llvm-install-name-tool', FindTool('llvm-install-name-tool')),
     ToolSubst('%llvm-bitcode-strip', FindTool('llvm-bitcode-strip')),
     ToolSubst('%split-file', FindTool('split-file')),
 ]
 
 # FIXME: Why do we have both `lli` and `%lli` that do slightly different things?
 tools.extend([
     'dsymutil', 'lli', 'lli-child-target', 'llvm-ar', 'llvm-as',
-    'llvm-addr2line', 'llvm-bcanalyzer', 'llvm-bitcode-strip', 'llvm-config', 
-    'llvm-cov', 'llvm-cxxdump', 'llvm-cvtres', 'llvm-diff', 'llvm-dis', 
+    'llvm-addr2line', 'llvm-bcanalyzer', 'llvm-bitcode-strip', 'llvm-config',
+    'llvm-cov', 'llvm-cxxdump', 'llvm-cvtres', 'llvm-diff', 'llvm-dis',
     'llvm-dwarfdump', 'llvm-dlltool', 'llvm-exegesis', 'llvm-extract',
     'llvm-isel-fuzzer', 'llvm-ifs',
     'llvm-install-name-tool', 'llvm-jitlink', 'llvm-opt-fuzzer', 'llvm-lib',
     'llvm-link', 'llvm-lto', 'llvm-lto2', 'llvm-mc', 'llvm-mca',
     'llvm-modextract', 'llvm-nm', 'llvm-objcopy', 'llvm-objdump',
     'llvm-pdbutil', 'llvm-profdata', 'llvm-ranlib', 'llvm-rc', 'llvm-readelf',
     'llvm-readobj', 'llvm-rtdyld', 'llvm-size', 'llvm-split', 'llvm-strings',
     'llvm-strip', 'llvm-tblgen', 'llvm-undname', 'llvm-c-test', 'llvm-cxxfilt',

llvm/utils/FileCheck/FileCheck.cpp

GlobalDefines("D", cl::AlwaysPrefix,
                   cl::value_desc("VAR=VALUE"));
 
 static cl::opt<bool> AllowEmptyInput(
     "allow-empty", cl::init(false),
     cl::desc("Allow the input file to be empty. This is useful when making\n"
              "checks that some error message does not occur, for example."));
 
 static cl::opt<bool> AllowUnusedPrefixes(
-    "allow-unused-prefixes", cl::init(true), cl::ZeroOrMore,
+    "allow-unused-prefixes", cl::init(false), cl::ZeroOrMore,
     cl::desc("Allow prefixes to be specified but not appear in the test."));
 
 static cl::opt<bool> MatchFullLines(
     "match-full-lines", cl::init(false),
     cl::desc("Require all positive matches to cover an entire input line.\n"
              "Allows leading and trailing whitespace if --strict-whitespace\n"
              "is not also passed."));