[FileCheck] Report missing prefixes when more than one is provided.

[mtrofin]

Summary

If more than a prefix is provided - e.g. --check-prefixes=CHECK,FOO - we don't report if (say) FOO is never used. This may lead to a gap in our test coverage.

This patch introduces a new option, --allow-unused-prefixes. It currently is set to true, keeping today's behavior. After we explicitly set it in tests where this behavior was actually intentional, we will switch it to false by default.

http://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html

[mtrofin]

Please note: this patch isn't ready to land as-is. There are about 1500 tests failing, for the reason covered here. The goal of the patch is to start the discussion as to how we want to proceed - assuming we believe that we shouldn't have tests with prefixes that aren't exercised - so I suppose that's the first question.

Thanks!

[jdoerfert]

So, this will break with the way the Attributor tests are designed. I will describe briefly how that is and why below. That said, I can see this to be useful, however, 1500 failures leads me to believe the Attributor tests are not the only ones that use such a pattern and it might be worth to have a way to keep the old behavior. Maybe we only error on tests that do not have auto generated check lines or we add a flag to FileCheck or something else ;)

---

Most Attributor tests have 4 run lines: {oldPM, newPM} x {module pass, cgscc pass}
Since we have 99 tests, 4 run lines each and a lot of things we want to check, we decided early on to use the update scripts.
Given all the enhancements we had to put in, some still downstream with me (incl. checking for global metadata), I think it is fair to say the Attributor was stress testing the update scripts quite a bit.
Anyway, to get the number of run lines down, and to make it explicit when the different combinations are equivalent or different, we use the 7 check prefixes per line, for example
CHECK,NOT_TUNIT_NPM,NOT_TUNIT_OPM,NOT_CGSCC_NPM,IS__CGSCC____,IS________OPM,IS__CGSCC_OPM
As we modify things we can simply run the update script and always find a valid set of check lines that is concise and expressive.
If we were to strip the unused prefixes (without having an automatic way to put them back) we would have substantially more work during the updates.
For us, the extra prefixes are not a problem or a bug. Whatever we do here, I would like a way to opt-in/out to keeping them around.

[mtrofin]

In D90281#2358154, @jdoerfert wrote:

So, this will break with the way the Attributor tests are designed. I will describe briefly how that is and why below. That said, I can see this to be useful, however, 1500 failures leads me to believe the Attributor tests are not the only ones that use such a pattern and it might be worth to have a way to keep the old behavior. Maybe we only error on tests that do not have auto generated check lines or we add a flag to FileCheck or something else ;)

---

Most Attributor tests have 4 run lines: {oldPM, newPM} x {module pass, cgscc pass}
Since we have 99 tests, 4 run lines each and a lot of things we want to check, we decided early on to use the update scripts.
Given all the enhancements we had to put in, some still downstream with me (incl. checking for global metadata), I think it is fair to say the Attributor was stress testing the update scripts quite a bit.
Anyway, to get the number of run lines down, and to make it explicit when the different combinations are equivalent or different, we use the 7 check prefixes per line, for example
CHECK,NOT_TUNIT_NPM,NOT_TUNIT_OPM,NOT_CGSCC_NPM,IS__CGSCC____,IS________OPM,IS__CGSCC_OPM
As we modify things we can simply run the update script and always find a valid set of check lines that is concise and expressive.
If we were to strip the unused prefixes (without having an automatic way to put them back) we would have substantially more work during the updates.
For us, the extra prefixes are not a problem or a bug. Whatever we do here, I would like a way to opt-in/out to keeping them around.

To make sure I understand, is this capturing it: these test files are generated by a script. A test file will always list the exact same --check-prefixes list, but then internally will only use a subset of the prefixes list. Also for the sake of me understanding, an alternative would be to only generate the strict --check-prefixes subset that's used in the test file; but the added complexity here is that some functionality is still downstream.

If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?

[jdoerfert]

In D90281#2358184, @mtrofin wrote:

[...]
To make sure I understand, is this capturing it: these test files are generated by a script.

The IR code is provided by us, the check lines are generated by the llvm/utils/update_test_checks.py script.

A test file will always list the exact same --check-prefixes list, but then internally will only use a subset of the prefixes list.

Yes. Though the subset can change over time.

Also for the sake of me understanding, an alternative would be to only generate the strict --check-prefixes subset that's used in the test file; but the added complexity here is that some functionality is still downstream.

No. Downstream stuff is just to make the llvm/utils/update_test_checks.py better and allow more check lines to be generated. This is unrelated and I should not have mentioned it.

If we reduce it to a subset, it will work just fine for now. Then someone will change something and the test will break. At that point that person needs to either put the prefixes back, run the script, remove the unused prefixes again, *or* manually update the tests. The latter is not a great option for us.

If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?

The run lines are written/copied by us. We could easily switch to a new option like that.

[MaskRay]

This strengthens D78024 (--implicit-check-not is specified) to the cases when --implicit-check-not is not used.
@jdenny had a use case similar to @jdoerfert https://reviews.llvm.org/D78024#inline-716591 (also CC @grimar and @probinson)

[grimar]

In D90281#2358188, @jdoerfert wrote:

In D90281#2358184, @mtrofin wrote:

[...]
If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?

The run lines are written/copied by us. We could easily switch to a new option like that.

I think we could add an option like --allow-unused-prefixes. It is probably sounds a bit simpler and can easily be added to generator I think.

[grimar]

llvm/lib/FileCheck/FileCheck.cpp

27

Seems unused?

1943

StringMap iteration order is not guaranteed to be deterministic. I think
some different container like MapVector probably should be used here instead.

[jhenderson]

+1 to the principle of this change - I wouldn't be surprised to see typos in at least some of the failing tests meaning that the incorrect check prefix isn't noticed. +1 also to an option to disable the check for the suggested reasons (I don't think I've personally got a use-case for it, but it seems like there are reasonable ones) - I'd be inclined to put the option in independently of the change that actually uses it, so that the tests can be updated independently, and gradually.

llvm/lib/FileCheck/FileCheck.cpp

1829

I'd prefer no auto here - what's the type of CP?

[mtrofin]

using std::set for deterministic output and simpler code.

[mtrofin]

In D90281#2358359, @grimar wrote:

In D90281#2358188, @jdoerfert wrote:

In D90281#2358184, @mtrofin wrote:

[...]
If there were a (more terse) "--check-at-least-one-of-the-prefixes" with today's semantics, I assume the test generator produce tests using that option instead?

The run lines are written/copied by us. We could easily switch to a new option like that.

I think we could add an option like --allow-unused-prefixes. It is probably sounds a bit simpler and can easily be added to generator I think.

That's a better name, indeed.

The remaining question is how to stage this. I think there are 2 broad ways to do it:

1. we enable it by default, and this patch goes and patches all failing tests with an opt-out.
2. we start with it disabled. Then, people opt in explicitly.

The problem I see with "2" is that there's no pressure to move to the "strict" place, which I think is what we want for a test suite.

The problem with "1" is 1500 failures. I think the way to mitigate that would be:

• we start with the flag, and it does nothing
• we annotate the tests (I can provide the list) - and this can be parallelized (i.e. perhaps folks can help with this)
• we then land this patch

My preference is to go with nr "1".
What do folks think?

llvm/lib/FileCheck/FileCheck.cpp

1829

removed this.

1943

How about a std::set - it addresses determinism, the code is more readable (no "PrefixesNotFound.keys()" abstraction leaks), and any overhead it may have is likely not an issue, as we don't expect the prefixes list to be too large.

[jdoerfert]

The remaining question is how to stage this. I think there are 2 broad ways to do it:

1. we enable it by default, and this patch goes and patches all failing tests with an opt-out.
2. we start with it disabled. Then, people opt in explicitly.

I think 1) is better but we need to address the 1500 failures. ~99 will go away when I patch the attributor tests, so I would suggest we first add the opt-out flag and start using it. Then we can keep track of problems in this commit and we don't have to touch 1500 files here. We should also look for patterns and write a script ;)

[dblaikie]

+1 to what other folks have said here, would be good to have a sample of the sort of tests that fail with this functionality - do we have lots of these either programmatically or otherwise symmetrically structured tests (some might be manually written but still benefit from a consistent pattern that may include unused check prefixes)? Or is it only a handful of places that would need to be fixed. But in any case, adding the flag upstream then committing changes incrementally to the tests or test-generators that need them, then eventually flipping the switch would probably be the way forward (unless we're talking like a huge % of all tests that need this flag added, then maybe it's not worthwhile/might need to look at more samples to discuss what the right tradeoffs/techniques are)

[MaskRay]

@mtrofin I favor a detect-potential-mistake-by-default approach as well. If you want to take the lead here, we can take a busy beavers approach. Add the option (initially true by default), create a spreadsheet with ~1500 failures grouped by test directories, post to llvm-dev and encourage maintainers to fix the issues (I would expect most are user errors, not deliberate --allow-unused-prefixes). After the issues are fixed, make the option false by default.

[mtrofin]

In D90281#2359673, @MaskRay wrote:

@mtrofin I favor a detect-potential-mistake-by-default approach as well. If you want to take the lead here, we can take a busy beavers approach. Add the option (initially true by default), create a spreadsheet with ~1500 failures grouped by test directories, post to llvm-dev and encourage maintainers to fix the issues (I would expect most are user errors, not deliberate --allow-unused-prefixes). After the issues are fixed, make the option false by default.

Awesome, let's do that! The list is actually captured here - the failed unittests - but we can do a shared spreadsheet and go that way.

Let me add the flag first (need a few mins, should have it in by lunch)

[mtrofin]

In D90281#2359685, @mtrofin wrote:

In D90281#2359673, @MaskRay wrote:

@mtrofin I favor a detect-potential-mistake-by-default approach as well. If you want to take the lead here, we can take a busy beavers approach. Add the option (initially true by default), create a spreadsheet with ~1500 failures grouped by test directories, post to llvm-dev and encourage maintainers to fix the issues (I would expect most are user errors, not deliberate --allow-unused-prefixes). After the issues are fixed, make the option false by default.

Awesome, let's do that! The list is actually captured here - the failed unittests - but we can do a shared spreadsheet and go that way.

Let me add the flag first (need a few mins, should have it in by lunch)

Oh - I was going to say, let's add the flag, then wait like a day or so, if bulk sets of tests can be handled that way by their authors (e.g. @jdoerfert 's scenario).

[MaskRay]

You can add the option in this patch and create a RFC on llvm-dev (I think people have favored the idea in the past but I cannot recall the thread now). Maintainers do one of the two:

• Add --allow-unused-prefixes if their use cases are similar to @jdoerfert and @jdenny's
• Fix the test prefixes (usually due to stale RUN lines after some check prefixes related changes). I think the majority of ~1500 tests belongs to this category.

This can be a collaborative spreadsheet so that people can claim the work.

[mtrofin]

included the opt/in/out flag, and a test.

Also fixed one test that (from what I read) doesn't actually need FileCheck

[mtrofin]

In D90281#2360025, @MaskRay wrote:

You can add the option in this patch and create a RFC on llvm-dev (I think people have favored the idea in the past but I cannot recall the thread now). Maintainers do one of the two:

• Add --allow-unused-prefixes if their use cases are similar to @jdoerfert and @jdenny's
• Fix the test prefixes (usually due to stale RUN lines after some check prefixes related changes). I think the majority of ~1500 tests belongs to this category.

This can be a collaborative spreadsheet so that people can claim the work.

SGTM - I'll post a RFC tomorrow, with list. This will also give a chance to folks in other time zones to chime in, if there are some additional considerations (less churn to the RFC, if that is the case)

[grimar]

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

2

You probably need to test the error reported?

3

I think it also worth to add a test for the default value, i.e. when no --allow-unused-prefixes is present.

11

No EOL.

[jhenderson]

Please also update the FileCheck documentation with the new option.

llvm/lib/FileCheck/FileCheck.cpp

1941–1942

If I follow this correctly, let's say I have specified --check-prefixes=FOO,BAR, and then only use FOO, I'll get the message error: no check strings found with prefixes 'BAR', but really it should be with prefix 'BAR' (i.e. no "es"). The logic here probably wants to be using PrefixesNotFound.size(), right?

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

3

5

I don't think you need this line, because the P1 line will match itself (by default FileCheck doesn't require full line matches).

6

Be consistent with your "comment" markers - you've used both ';' and '//' in this file. They're actually not needed here at all, but are probably useful to help the RUN/CHECK lines stand out from the rest of the data.

llvm/test/Transforms/NewGVN/todo-pr37121-seens-this-value-a-lot.ll

3 ↗

(On Diff #301500)

Seems unrelated?

[mtrofin]

incorporated feedback

[mtrofin]

removed unrelated test change

[mtrofin]

llvm/lib/FileCheck/FileCheck.cpp

1941–1942

Yup - fixed.

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

2

Added for both single and multiple missing prefixes. Note that, because the way prefixes are reported, it required a separation of the test input and this verification. (otherwise <comment> .* Prefix: would match to a prefix)

5

--allow-empty is false by default

[mtrofin]

Gentle reminder - is this good to go?

[jdoerfert]

LGTM

[mtrofin]

Sorry, got a bit eager submitting, as I wanted to unblock uses of the new flag. Happy to address post-submit feedback, if any!

[jhenderson]

The documentation wasn't updated. Please do that.

llvm/test/FileCheck/Inputs/one-check.txt

3

Needs new line at EOF.

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

1

Should this test be using %ProtectFileCheckOutput?

5–6

I try to encourage using double comment markers for real comments as opposed to lit directive markers, e.g. ;; here.

11

This comment wasn't addressed.

[mtrofin]

In D90281#2367724, @jhenderson wrote:

The documentation wasn't updated. Please do that.

Sorry I missed that - please see https://reviews.llvm.org/D90621

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

1

Not sure why, we're not setting env variables, or am I missing something?

[dblaikie]

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

1

The test isn't setting environment variables, but users can (see: https://reviews.llvm.org/D65121 ) to configure FileCheck's behavior for their convenience/to produce more actionable errors (I think this is to allow customization both by users, but also on buildbots so buildbots fail with more context that makes it easier to debug from the logs when you can't necessarily reproduce the command locally). So any test that relies on the output of FileCheck probably wants to ensure FILECHECK_OPTS can't effect the output of the FileCheck command under test and can/should do so using %ProtectFileCheckOutput

[mtrofin]

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

1

Thanks for the explanation, that is the bit that I couldn't grok from %ProtectFileCheckOutput, somehow I read "developers" as "developers writing tests and setting variables in their tests' RUN line".

[mtrofin]

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

1

https://reviews.llvm.org/D90631

[RKSimon]

@mtrofin llvm/test/Analysis now passes with -allow-unused-prefixes=false - is there a way that we can use lit.local.cfg to keep it clean?

[mtrofin]

lit.local.cfg would "blanket-allow" future tests, too. Would that be
desirable?

[RKSimon]

In D90281#2385772, @mtrofin wrote:

lit.local.cfg would "blanket-allow" future tests, too. Would that be desirable?

Yes I'm finding that 99% of llvm tests that were failing were just poor check-prefix setup (usually copy+paste or too much emphasis on an unnecessarily complex prefix hierarchy) - so until you can flip the default value in FileCheck itself we should be able to use lit.local.cfg files in the subfolder root to keep them clean once they've been fixed up.

[mtrofin]

Sorry, I misread your original email: you're saying: use lit.local.cfg to
blanket-set the "don't allow unused prefixes" behavior on conforming
directories (I read it backwards).

I *think* the main challenge is that the -allow-unused-prefixes flag must
occur only once?

llvm/include/llvm/FileCheck/FileCheck.h

 /// Contains info about various FileCheck options.
 struct FileCheckRequest {
   std::vector<StringRef> CheckPrefixes;
   std::vector<StringRef> CommentPrefixes;
   bool NoCanonicalizeWhiteSpace = false;
   std::vector<StringRef> ImplicitCheckNot;
   std::vector<StringRef> GlobalDefines;
   bool AllowEmptyInput = false;
+  bool AllowUnusedPrefixes = false;
   bool MatchFullLines = false;
   bool IgnoreCase = false;
   bool IsDefaultCheckPrefix = false;
   bool EnableVarScope = false;
   bool AllowDeprecatedDagOverlap = false;
   bool Verbose = false;
   bool VerboseVerbose = false;
 };

llvm/lib/FileCheck/FileCheck.cpp

 #include "FileCheckImpl.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/CheckedArithmetic.h"
 #include "llvm/Support/FormatVariadic.h"
 #include <cstdint>
 #include <list>
+#include <set>
 #include <tuple>
 #include <utility>
 
 using namespace llvm;
 
 StringRef ExpressionFormat::toString() const {
   switch (Value) {
   case Kind::NoFormat:
     return StringRef("<none>");
   case Kind::Unsigned:
bool FileCheck::readCheckFile(
   }
 
   std::vector<Pattern> DagNotMatches = ImplicitNegativeChecks;
 
   // LineNumber keeps track of the line on which CheckPrefix instances are
   // found.
   unsigned LineNumber = 1;
 
-  bool FoundUsedCheckPrefix = false;
-  while (1) {
+  std::set<StringRef> PrefixesNotFound(Req.CheckPrefixes.begin(),
+                                       Req.CheckPrefixes.end());
+  const size_t DistinctPrefixes = PrefixesNotFound.size();
+  while (true) {
     Check::FileCheckType CheckTy;
 
     // See if a prefix occurs in the memory buffer.
     StringRef UsedPrefix;
     StringRef AfterSuffix;
     std::tie(UsedPrefix, AfterSuffix) =
         FindFirstMatchingPrefix(Req, PrefixRE, Buffer, LineNumber, CheckTy);
     if (UsedPrefix.empty())
       break;
     if (CheckTy != Check::CheckComment)
-      FoundUsedCheckPrefix = true;
+      PrefixesNotFound.erase(UsedPrefix);
 
     assert(UsedPrefix.data() == Buffer.data() &&
            "Failed to move Buffer's start forward, or pointed prefix outside "
            "of the buffer!");
     assert(AfterSuffix.data() >= Buffer.data() &&
            AfterSuffix.data() < Buffer.data() + Buffer.size() &&
            "Parsing after suffix doesn't start inside of buffer!");
 
while (true) {
     // Okay, add the string we captured to the output vector and move on.
     CheckStrings->emplace_back(P, UsedPrefix, PatternLoc);
     std::swap(DagNotMatches, CheckStrings->back().DagNotStrings);
     DagNotMatches = ImplicitNegativeChecks;
   }
 
   // When there are no used prefixes we report an error except in the case that
   // no prefix is specified explicitly but -implicit-check-not is specified.
-  if (!FoundUsedCheckPrefix &&
+  const bool NoPrefixesFound = PrefixesNotFound.size() == DistinctPrefixes;
+  const bool SomePrefixesUnexpectedlyNotUsed =
+      !Req.AllowUnusedPrefixes && !PrefixesNotFound.empty();
+  if ((NoPrefixesFound || SomePrefixesUnexpectedlyNotUsed) &&
       (ImplicitNegativeChecks.empty() || !Req.IsDefaultCheckPrefix)) {
     errs() << "error: no check strings found with prefix"
-           << (Req.CheckPrefixes.size() > 1 ? "es " : " ");
-    for (size_t I = 0, E = Req.CheckPrefixes.size(); I != E; ++I) {
-      if (I != 0)
+           << (PrefixesNotFound.size() > 1 ? "es " : " ");
+    bool First = true;
+    for (StringRef MissingPrefix : PrefixesNotFound) {
+      if (!First)
         errs() << ", ";
-      errs() << "\'" << Req.CheckPrefixes[I] << ":'";
+      errs() << "\'" << MissingPrefix << ":'";
+      First = false;
     }
     errs() << '\n';
     return true;
   }
 
   // Add an EOF pattern for any trailing --implicit-check-not/CHECK-DAG/-NOTs,
   // and use the first prefix as a filler for the error message.
   if (!DagNotMatches.empty()) {

llvm/test/FileCheck/Inputs/one-check.txt

+hello
+; P1: hello
 No newline at end of file

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

+; RUN: 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: 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 
+
+; 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 
+
+; MISSING-ONE: error: no check strings found with prefix 'P2:' 
+; MISSING-MORE: error: no check strings found with prefixes 'P2:', 'P3:' 
 No newline at end of file

llvm/utils/FileCheck/FileCheck.cpp

GlobalDefines("D", cl::AlwaysPrefix,
                   cl::desc("Define a variable to be used in capture patterns."),
                   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::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."));
 
 static cl::opt<bool> EnableVarScope(
     "enable-var-scope", cl::init(false),
if (EqIdx == 0) {
       continue;
     }
     Req.GlobalDefines.push_back(G);
   }
   if (GlobalDefineError)
     return 2;
 
   Req.AllowEmptyInput = AllowEmptyInput;
+  Req.AllowUnusedPrefixes = AllowUnusedPrefixes;
   Req.EnableVarScope = EnableVarScope;
   Req.AllowDeprecatedDagOverlap = AllowDeprecatedDagOverlap;
   Req.Verbose = Verbose;
   Req.VerboseVerbose = VerboseVerbose;
   Req.NoCanonicalizeWhiteSpace = NoCanonicalizeWhiteSpace;
   Req.MatchFullLines = MatchFullLines;
   Req.IgnoreCase = IgnoreCase;