Page MenuHomePhabricator

FileCheck [8/12]: Define numeric var from expr
ClosedPublic

Authored by thopre on Apr 7 2019, 4:21 PM.

Details

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch lift the restriction for a
numeric expression to either be a variable definition or a numeric
expression to try to match.

This commit allows a numeric variable to be set to the result of the
evaluation of a numeric expression after it has been matched
successfully. When it happens, the variable is allowed to be used on
the same line since its value is known at match time.

It also makes use of this possibility to reuse the parsing code to
parse a command-line definition by crafting a mirror string of the
-D option with the equal sign replaced by a colon sign, e.g. for option
'-D#NUMVAL=10' it creates the string
'-D#NUMVAL=10 (parsed as #NUMVAL:10)' where the numeric expression
is parsed to define NUMVAL. This result in a few tests needing updating
for the location diagnostics on top of the tests for the new feature.

It also enables empty numeric expression which match any number without
defining a variable. This is done here rather than in commit #5 of the
patch series because it requires to dissociate automatic regex insertion
in RegExStr from variable definition which would make commit #5 even
bigger than it already is.

Copyright:

  • Linaro (changes up to diff 183612 of revision D55940)
  • GraphCore (changes in later versions of revision D55940 and in new revision created off D55940)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
thopre updated this revision to Diff 209488.Jul 12 2019, 7:59 AM

Rebase on top of latest changes in patch series

thopre updated this revision to Diff 209708.Jul 13 2019, 4:11 PM

Add more unit tests

jhenderson added inline comments.Jul 16 2019, 2:50 AM
llvm/docs/CommandGuide/FileCheck.rst
626

If I'm not mistaken, the %<fmtspec> bit isn't implemented in this patch?

llvm/include/llvm/Support/FileCheck.h
97

Add to this comment what the value of the variable is if ExpressionAST is null.

llvm/lib/Support/FileCheck.cpp
28

This is unchecked in builds without assertions.

29–31

I'm not sure this second sentence is necessary?

30

I can't parse this string here. I can't even really figure out what this means to suggest an alternative.

548

You don't need the explicit StringRef(...) here. Just do MatchRegexp = "[0-9]+";

570

DefineCmdlineVariables doesn't conform to style guide. I haven't bothered looking it up, but I imagine this comment is a bit stale, or the function needs renaming?

1767

Suffix3? What happened to Suffix2?

Honestly, these names don't make a huge amount of sense to me.

Also, do we need all of them? As far as I can see, Suffix1 and Suffix3 are only used once each, so should just be inlined.

1770

DefNo -> DefIndex

("No" here is not sufficiently clear as to its context, IMO).

1780

Should this be parseNumericSubstitutionBlock?

1811

This comment sounds like a lie? You aren't skipping the error here...

1817–1826

"Parse the definition both to check that the syntax is correct and to create ..."

1834

Why is the expression being evaluated here? This probably needs a comment.

thopre updated this revision to Diff 210058.Jul 16 2019, 3:49 AM
thopre marked 14 inline comments as done.

Address review comments

thopre added inline comments.Jul 16 2019, 3:49 AM
llvm/lib/Support/FileCheck.cpp
28

That's fine. setValue is called after substitution has happened and match was successful. Therefore ExpressionAST ought to evaluate fine. This is just in case code is reworked in the future and this invariant is broken.

jhenderson added inline comments.Jul 16 2019, 3:59 AM
llvm/lib/Support/FileCheck.cpp
28

It isn't. LLVM Errors and Expecteds HAVE to be checked. If they aren't it will lead to an abort. From the Programmer's Manual:

All Error instances, whether success or failure, must be either checked or moved from (via std::move or a return) before they are destructed. Accidentally discarding an unchecked error will cause a program abort at the point where the unchecked value’s destructor is run, making it easy to identify and fix violations of this rule.

thopre updated this revision to Diff 210062.Jul 16 2019, 4:27 AM
thopre marked 2 inline comments as done.

Address review comments

llvm/lib/Support/FileCheck.cpp
28

Oh my bad, didn't make the connection with the Expected type. I think an abort would be fine in this case since that error should never occur in any of the executions. By that I mean that code path should be dead (that function will not be called if the expression does not evaluate). If it does, it's a bug somewhere in the code.

Does that make sense? I've added some comments on the expectation for the caller both in the declaration and just on top of the assert since that was missing.

probinson added inline comments.Jul 16 2019, 9:58 AM
llvm/docs/CommandGuide/FileCheck.rst
626

Where is <constraint> described?

llvm/lib/Support/FileCheck.cpp
31

You should check the Expected return value with something other than an assert. It is not sufficient to claim that the caller should call this only when the Expected result must be success. The contract for using Expected is that you will check the result. You could do something like

if (!EvaluatedValue || *EvaluatedValue != NewValue)
  llvm_unreachable("New value does not match expression for this variable");
483

? this should be the default, you should not have to say = StringRef() explicitly.

FTR, I haven't done a rigorous check of all the different new code paths to ensure you've got testing for them all, I'm afraid - the change is too large to sensibly do that, although I accept that it probably needs to be this large.

llvm/lib/Support/FileCheck.cpp
31

I think cantFail is intended for the kind of situation we're in here.

33

Isn't the assertion that the eval() function succeeded. In that case, let's talk about that. "Failed to evaluate expression when sanity checking variable value" or something to that effect.

1768

Won't this create a StringRef to a temporary?

1776

Append a copy of the command-line...

1787

Prefix1 + DefIndex + Prefix2 is used twice, once in each part of the if. If you factor it out into a separate variable, it'll be much easier to read, I think, and you can then get rid of Prefix1 and Prefix2 as separate variables. You can even fold them into the place where DefIndex is created, to avoid additional non-Twine string concatenations:

std::string SomeName = ("Global define #" + Twine(++I) + ": ").str();

1834

"Now evaluate the expression, whose value this variable should be set to, since the expression of a command-line variable definition should only use variables defined earlier on the command-line."

llvm/test/FileCheck/numeric-defines-diagnostics.txt
5

By the way, if you were to use --match-full-lines, you wouldn't need to use the regular expression in the last line of each of these checks, and your arrow would point closer to where it should be.

llvm/test/FileCheck/numeric-defines.txt
4–9

There should probably be a test case like the original, i.e. with no expression, just a literal numeric value.

llvm/test/FileCheck/numeric-expression.txt
77

This is allowed? That's surprising to me, although I suppose it's a natural consequence of the rules.

138

"when a variable"

140

Double dashes here, please.

Is it worth checking the error message in this context, to show that the failure is due to the expression match rather than due to something to do with the definition?

llvm/unittests/Support/FileCheckTest.cpp
51

Perhaps "Variable defined by numeric expression:" would make more sense.

54

"UseUnique" doesn't seem to line up with what this test is supposed to be doing, from what I can tell. What is "Unique" about it?

60

FoobarVar is the "undefined variable being set", right? In that case, I think a more purposeful name might be useful, e.g. FromVarExprVar or something like that. Happy with alternative names though.

277

I might be being dumb here, but where does the "same line" come into the following 4 EXPECTs?

339

How about EXPECT_FALSE/TRUE(Test.matchExpect(""));?

534

There are probably many other places where this is a problem already, but this should be ASSERT_TRUE, since the test cannot continue if it fails, and will crash on the next line (probably). Perhaps a separate commit to clean those up would be appropriate.

thopre updated this revision to Diff 210410.Jul 17 2019, 2:08 PM
thopre marked 23 inline comments as done.
  • Address review comments
  • rebase on top of @LINE substitution fix
jhenderson added inline comments.Jul 18 2019, 2:02 AM
llvm/lib/Support/FileCheck.cpp
29

clang-format?

Rephrase the second string to "Value being set to different from variable evaluation"

1834

This comment hasn't been addressed.

llvm/test/FileCheck/numeric-defines-diagnostics.txt
5

This has been marked as Done, but I don't see any changes?

llvm/unittests/Support/FileCheckTest.cpp
51

I wanted the "Undefined" bit deleting. I don't think it's needed.

thopre added inline comments.Jul 18 2019, 3:08 AM
llvm/lib/Support/FileCheck.cpp
1834

I'm confused by what aspect of this you want to be explained. The reworded text seems to explain why is the expression evaluated but I thought you were asking why is it evaluated *here*. If the former, the text should be reworded further to remove the mention about errors and only say that the value of a variable can be given by an expression using variable earlier on the command line. If the latter, then the "only" becomes important since we are saying both that it is safe to evaluate now (we have the value of all variables) and that we should be doing it now (there is no input to match, all variables used are defined either from earlier command-line or from literal values).

llvm/test/FileCheck/numeric-defines-diagnostics.txt
5

Rather further away since the previous line would still need {{ and escaping for [ while the caret check would not need anything. It does reduce the amount of syntax for that line but it also make other lines slightly more difficult to read since I have to remote the space after the colon due to the --strict-whitespace:

NUMERRCLIFMT:Global defines:1:46: error: invalid variable name

Do you think it's worth doing? I can schedule a separate patch if yes.

llvm/test/FileCheck/numeric-expression.txt
77

As you said it's a natural consequence and I thought it could actually be useful (to check you have a number in an error message but don't care about the value) so I didn't forbid it.

llvm/unittests/Support/FileCheckTest.cpp
54

There is already a FooVarUse of type FileCheckNumericVariableUse. This one is a unique_ptr of that type. I've renamed into FooVarUsePtr.

534

Wasn't aware of ASSERT_TRUE. Another patch to schedule.

thopre marked 2 inline comments as done.Jul 18 2019, 3:13 AM
thopre added inline comments.
llvm/lib/Support/FileCheck.cpp
1834

Sorry, forgot to submit my replies. Done marks automatically get pushed when submitting a new revision.

llvm/test/FileCheck/numeric-defines-diagnostics.txt
5

Marked it done because I wrote a reply and was expecting an answer about whether you think it is actually worth it. In the event you think it is indeed worth it, I would only mark the answer as done once the work has been completed.

Unfortunately I forgot to submit my replies.

thopre updated this revision to Diff 210523.Jul 18 2019, 3:25 AM
thopre marked 3 inline comments as done.

Fix more review comments

jhenderson added inline comments.Jul 18 2019, 3:51 AM
llvm/lib/Support/FileCheck.cpp
1834

The message content is fine as it stands in the current version. I've just suggested some grammar improvements. I think the meaning remains the same with them.

llvm/test/FileCheck/numeric-defines-diagnostics.txt
5

I guess it's not worth it. I just find the whole caret checking basically unreadable due to the regex as it stands. It feels quite fragile too.

Since you've got --strict-whitespace on, you could simplify the caret's line a little:
NUMERRCLIFMT-NEXT: {{^}} ^
(with appropriate numbers of spaces).

I don't think the trailing $ is important, really, in this case, but could be persuaded either way. In that case though, I'd put that in it's own {{$}} pattern. If you are happy with that, then please schedule a patch for that.

probinson added inline comments.Jul 18 2019, 6:42 AM
llvm/test/FileCheck/numeric-expression.txt
77

So [[#]] is equivalent to {{[0-9]+}} ? Might be worth a note in the documentation.

thopre updated this revision to Diff 210562.Jul 18 2019, 7:33 AM
thopre marked 4 inline comments as done.

Address remaining review comments

jhenderson accepted this revision.Jul 18 2019, 7:43 AM

LGTM, I think. You'd best get someone else (e.g. @probinson) to confirm too.

This revision is now accepted and ready to land.Jul 18 2019, 7:43 AM

A couple of minor things, otherwise LGTM.

llvm/docs/CommandGuide/FileCheck.rst
632–634

constraint?

llvm/include/llvm/Support/FileCheck.h
98

"it is set from a value in the input" ? This is not a statement about what the ctor does, it sounds like an expectation.
"Otherwise, it should be set later from a value in the input." ?

thopre updated this revision to Diff 211339.Jul 23 2019, 11:25 AM
thopre marked 2 inline comments as done.
  • Remove reference to expression constraint, this is patch 11/12 material
  • Clarify what the FileCheckNumericVariable constructor does and what is rather an expectation
probinson accepted this revision.Jul 23 2019, 12:51 PM
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.
llvm/trunk/lib/Support/FileCheck.cpp
345 ↗(On Diff #211365)

gcc 7.3.1 seems to be unhappy with this line. I think it wants a std::move around ExpressionAST.