Page MenuHomePhabricator

Detect incorrect FileCheck variable CLI definition
ClosedPublic

Authored by thopre on Dec 20 2018, 9:43 AM.

Details

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)

Diff Detail

Repository
rL LLVM

Event Timeline

thopre created this revision.Dec 20 2018, 9:43 AM

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

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.

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.

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'.

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 updated this revision to Diff 179716.Dec 29 2018, 3:37 PM
thopre edited the summary of this revision. (Show Details)

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

jdenny added a comment.Jan 2 2019, 2:38 PM

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 added inline comments.Jan 2 2019, 2:41 PM
llvm/test/FileCheck/defines.txt
8 ↗(On Diff #179716)

... and -D.

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 updated this revision to Diff 181016.Jan 10 2019, 2:52 AM
thopre marked 7 inline comments as done.
  • Forbid AlwaysPrefix option to look for their value in following argument
  • Add unit tests for changes to CommandLine
thopre added inline comments.Jan 10 2019, 2:54 AM
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.

thopre edited the summary of this revision. (Show Details)Jan 10 2019, 2:54 AM

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 updated this revision to Diff 181066.Jan 10 2019, 7:47 AM

Take CommandLine option code into a separate patch

jdenny added inline comments.Jan 10 2019, 7:48 AM
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?

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 accepted this revision.Jan 10 2019, 8:48 AM

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.

This revision is now accepted and ready to land.Jan 10 2019, 8:48 AM
thopre edited the summary of this revision. (Show Details)Jan 11 2019, 5:13 AM
thopre edited the summary of this revision. (Show Details)Jan 12 2019, 5:01 AM
This revision was automatically updated to reflect the committed changes.