Detect incorrect FileCheck variable CLI definition

[thopre]

Summary

While the backend code of FileCheck relies on definition of variable
from the command-line to have an equal sign '=' and a variable name
before that, the frontend does not actually enforce it. This leads to
FileCheck crashing when invoked with invalid syntax for the -D option.

This patch adds the missing validation in the frontend. It also makes
the -D option an AlwaysPrefix option to be able to detect -D=FOO as
being a define without variable and -D as missing its value.

Copyright:

• Linaro (changes in version 2 of revision D55940)
• GraphCore (changes in later versions)

[probinson]

This will permit -DVALUE= (with no value) is that a supported usage?
It also permits -D=10 which I daresay doesn't mean anything.

[jdenny]

Better diagnostics would definitely be helpful here.

I've also been meaning to make redefinitions an error so that -D isn't the only FileCheck option where order matters. Currently, earlier definitions have precedence, but that's surely wrong, and I don't see a use case for letting later definitions have precedence. Care to add that to your patch?

By the way, your summary says FileCheck crashes. Did you mean it just produces a confusing diagnostic?

Thanks.

[jdenny]

In D55940#1337906, @probinson wrote:

This will permit -DVALUE= (with no value) is that a supported usage?

I think it should be. You could have a string that sometimes appears and sometimes is excluded entirely.

It also permits -D=10 which I daresay doesn't mean anything.

Agreed.

[thopre]

In D55940#1337957, @jdenny wrote:

In D55940#1337906, @probinson wrote:

This will permit -DVALUE= (with no value) is that a supported usage?

I think it should be. You could have a string that sometimes appears and sometimes is excluded entirely.

It also permits -D=10 which I daresay doesn't mean anything.

Agreed.

I've just given it a try but there seems to be some sorcery involved in that case: G contains just 10 instead of =10. Any way I could prevent the parsing from eating a leading equal sign? If not, I could reword the error message to mention leading equal sign but we would fail to diagnose an issue when passing -D'=Expression: VAR-5=10' typed by error instead of -D'TXT=Expression: VAR-5=10'.

[jdenny]

In D55940#1338920, @thopre wrote:

I've just given it a try but there seems to be some sorcery involved in that case: G contains just 10 instead of =10. Any way I could prevent the parsing from eating a leading equal sign? If not, I could reword the error message to mention leading equal sign but we would fail to diagnose an issue when passing -D'=Expression: VAR-5=10' typed by error instead of -D'TXT=Expression: VAR-5=10'.

I agree that command-line parsing is current set up to recognize -D=VAR=VAL to be the same as -DVAR=VAL. That doesn't seem useful.

One possible fix is to add a new member (maybe AlwaysPrefix to mean the usual -OPT=VAL form is never accepted) for options like this to FormattingFlags in CommandLine.h, and then adjust CommandLineParser::LookupOption to return nullptr for it.

[thopre]

Add AlwaysPrefix option formatting flag and define FileCheck's -D option with it.
Add detection of missing variable name.

[jdenny]

Other than nits I've added as inline comments, LGTM. However, let's give Paul some time to respond now that the holidays are over.

llvm/lib/Support/CommandLine.cpp

436 ↗

(On Diff #179716)

Is it ever possible for O to be nullptr here?

llvm/test/FileCheck/defines.txt

8 ↗

(On Diff #179716)

I suggest testing -D= too.

llvm/utils/FileCheck/FileCheck.cpp

531 ↗

(On Diff #179716)

I think these two diagnostics would be clearer mentioning -D. For example:

errs() << "Missing equal sign in command-line definition: -D" << G << "\n";

534 ↗

(On Diff #179716)

LLVM coding standards say not to use else if after continue.

[jdenny]

llvm/test/FileCheck/defines.txt

8 ↗

(On Diff #179716)

... and -D.

[probinson]

Changes to CommandLine.* should be tested in llvm/unittests/Support/CommandLineTest.cpp, as a rule. We don't want to rely on FileCheck tests to verify code in Support.

llvm/lib/Support/CommandLine.cpp

430 ↗

(On Diff #179716)

You've made this comment incorrect now, please update.

[thopre]

• Forbid AlwaysPrefix option to look for their value in following argument
• Add unit tests for changes to CommandLine

[thopre]

llvm/lib/Support/CommandLine.cpp

436 ↗

(On Diff #179716)

I guess not because in LookupNearestOption whatever option is found in OptionsMap is dereferenced without check.

[JonChesterfield]

It looks like the changes to support/commandline are useful independent of the file check use case. Would it be worth submitting the command line enhancement by itself before updating filecheck to take advantage?

[thopre]

Take CommandLine option code into a separate patch

[jdenny]

llvm/lib/Support/CommandLine.cpp

430 ↗

(On Diff #179716)

I think you mean "does not support". When the option only supports the prefix form, we return nullptr (and Arg is "unmolested"), so we don't match.

While you're at it, could you clarify that the "unmolested" case returns nullptr?

[probinson]

A couple of comment phrasing nits.

I agree that the CommandLine feature should be added first in its own patch, and the FileCheck change done separately.

llvm/lib/Support/CommandLine.cpp

430 ↗

(On Diff #181016)

Say "supports" rather than "does support" here, it would be a more natural phrasing.

546 ↗

(On Diff #181016)

"or the option only supports the prefix form" will read better.

[jdenny]

The summary needs to be updated for the extraction of D56549. Otherwise, LGTM. I've accepted, but others have time to comment while D56549 is reviewed.

[probinson]

I'm happy.

llvm/trunk/test/FileCheck/defines.txt

 ; RUN: FileCheck -DVALUE=10 -input-file %s %s
 ; RUN: not FileCheck -DVALUE=20 -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRMSG
 ;
 ; RUN: not FileCheck -DVALUE=10 -check-prefix NOT -input-file %s %s 2>&1 | FileCheck %s -check-prefix NOT-ERRMSG
 ; RUN: FileCheck -DVALUE=20 -check-prefix NOT -input-file %s %s
+; RUN: not FileCheck -DVALUE10 -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIEQ1
+; RUN: not FileCheck -D -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIEQ2
+; RUN: not FileCheck -D=10 -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIVAR1
+; RUN: not FileCheck -D= -input-file %s %s 2>&1 | FileCheck %s -check-prefix ERRCLIVAR2
+; RUN: FileCheck -DVALUE= -check-prefix EMPTY -input-file %s %s 2>&1
 
 Value = 10
 ; CHECK: Value = [[VALUE]]
 ; NOT-NOT: Value = [[VALUE]]
 
-; ERRMSG: defines.txt:8:10: error: CHECK: expected string not found in input
+; ERRMSG: defines.txt:[[@LINE-3]]:10: error: CHECK: expected string not found in input
 ; ERRMSG: defines.txt:1:1: note: scanning from here
 ; ERRMSG: defines.txt:1:1: note: with variable "VALUE" equal to "20"
-; ERRMSG: defines.txt:7:1: note: possible intended match here
+; ERRMSG: defines.txt:[[@LINE-7]]:1: note: possible intended match here
 
-; NOT-ERRMSG: defines.txt:9:12: error: {{NOT}}-NOT: excluded string found in input
-; NOT-ERRMSG: defines.txt:7:1: note: found here
-; NOT-ERRMSG: defines.txt:7:1: note: with variable "VALUE" equal to "10"
+; NOT-ERRMSG: defines.txt:[[@LINE-7]]:12: error: {{NOT}}-NOT: excluded string found in input
+; NOT-ERRMSG: defines.txt:[[@LINE-10]]:1: note: found here
+; NOT-ERRMSG: defines.txt:[[@LINE-11]]:1: note: with variable "VALUE" equal to "10"
+
+; ERRCLIEQ1: Missing equal sign in command-line definition '-DVALUE10'
+
+; ERRCLIEQ2: FileCheck: for the -D option: requires a value!
+
+; ERRCLIVAR1: Missing pattern variable name in command-line definition '-D=10'
+
+; ERRCLIVAR2: Missing pattern variable name in command-line definition '-D='
+
+Empty value = @@
+; EMPTY: Empty value = @[[VALUE]]@

llvm/trunk/utils/FileCheck/FileCheck.cpp

 
 static cl::list<std::string> ImplicitCheckNot(
     "implicit-check-not",
     cl::desc("Add an implicit negative check with this pattern to every\n"
              "positive check. This can be used to ensure that no instances of\n"
              "this pattern occur which are not matched by a positive pattern"),
     cl::value_desc("pattern"));
 
-static cl::list<std::string> GlobalDefines("D", cl::Prefix,
+static cl::list<std::string>
+    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> MatchFullLines(
     "match-full-lines", cl::init(false),
int main(int argc, char **argv) {
 
   FileCheckRequest Req;
   for (auto Prefix : CheckPrefixes)
     Req.CheckPrefixes.push_back(Prefix);
 
   for (auto CheckNot : ImplicitCheckNot)
     Req.ImplicitCheckNot.push_back(CheckNot);
 
-  for (auto G : GlobalDefines)
+  bool GlobalDefineError = false;
+  for (auto G : GlobalDefines) {
+    size_t EqIdx = G.find('=');
+    if (EqIdx == std::string::npos) {
+      errs() << "Missing equal sign in command-line definition '-D" << G
+             << "'\n";
+      GlobalDefineError = true;
+      continue;
+    }
+    if (EqIdx == 0) {
+      errs() << "Missing pattern variable name in command-line definition '-D"
+             << G << "'\n";
+      GlobalDefineError = true;
+      continue;
+    }
     Req.GlobalDefines.push_back(G);
+  }
+  if (GlobalDefineError)
+    return 2;
 
   Req.AllowEmptyInput = AllowEmptyInput;
   Req.EnableVarScope = EnableVarScope;
   Req.AllowDeprecatedDagOverlap = AllowDeprecatedDagOverlap;
   Req.Verbose = Verbose;
   Req.VerboseVerbose = VerboseVerbose;
   Req.NoCanonicalizeWhiteSpace = NoCanonicalizeWhiteSpace;
   Req.MatchFullLines = MatchFullLines;