Page MenuHomePhabricator

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

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



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.


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

Diff Detail


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
632 ↗(On Diff #209708)

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

113 ↗(On Diff #209708)

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

37 ↗(On Diff #209708)

This is unchecked in builds without assertions.

39 ↗(On Diff #209708)

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

45–47 ↗(On Diff #209708)

I'm not sure this second sentence is necessary?

546 ↗(On Diff #209708)

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

568 ↗(On Diff #209708)

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?

1814 ↗(On Diff #209708)

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.

1817 ↗(On Diff #209708)

DefNo -> DefIndex

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

1827 ↗(On Diff #209708)

Should this be parseNumericSubstitutionBlock?

1854 ↗(On Diff #209708)

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

1865–1866 ↗(On Diff #209708)

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

1878 ↗(On Diff #209708)

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
37 ↗(On Diff #209708)

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
37 ↗(On Diff #209708)

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

37 ↗(On Diff #209708)

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
632 ↗(On Diff #210062)

Where is <constraint> described?

40 ↗(On Diff #210062)

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");
471 ↗(On Diff #210062)

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

1878 ↗(On Diff #209708)

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

40 ↗(On Diff #210062)

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

42 ↗(On Diff #210062)

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.

1815 ↗(On Diff #210062)

Won't this create a StringRef to a temporary?

1823 ↗(On Diff #210062)

Append a copy of the command-line...

1834 ↗(On Diff #210062)

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();

5 ↗(On Diff #210062)

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.

4 ↗(On Diff #210062)

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

91 ↗(On Diff #210062)

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

172 ↗(On Diff #210062)

"when a variable"

174 ↗(On Diff #210062)

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?

85 ↗(On Diff #210062)

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

88 ↗(On Diff #210062)

"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?

94 ↗(On Diff #210062)

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.

332 ↗(On Diff #210062)

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

394 ↗(On Diff #210062)

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

561 ↗(On Diff #210062)

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
1878 ↗(On Diff #209708)

This comment hasn't been addressed.

38 ↗(On Diff #210410)


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

5 ↗(On Diff #210062)

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

85 ↗(On Diff #210062)

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

thopre added inline comments.Jul 18 2019, 3:08 AM
1878 ↗(On Diff #209708)

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

5 ↗(On Diff #210062)

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.

91 ↗(On Diff #210062)

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.

88 ↗(On Diff #210062)

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

561 ↗(On Diff #210062)

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.
1878 ↗(On Diff #209708)

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

5 ↗(On Diff #210062)

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
1878 ↗(On Diff #209708)

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.

5 ↗(On Diff #210062)

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:
(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
91 ↗(On Diff #210062)

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.

648 ↗(On Diff #210562)


114 ↗(On Diff #210562)

"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.Tue, Jul 23, 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.Tue, Jul 23, 12:51 PM
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.

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