FileCheck: Add support for numeric variables and expressions

[thopre]

Summary

Sometimes there's a need to verify output which contain several numeric
values that are related by a numeric expression. For example, one might
want to check that two numbers are consecutive but accept any value for
the smallest of the two.

This patch introduces support for numeric variables and expressions to
express this sort of constraint. A user can then define variables to
have a numeric value and write numeric expression using those variable.
A matching format specifier allows to match numbers in a variety of
format: signed and unsigned decimal numbers as well as hex number in
lower or capital case. For example, matching 2 consecutive signed
decimal integers separated by a space is done via:

[[%d,#N:]] #N+1

The syntax is modeled after that of use and definition of the already
supported variables flavour (renamed pattern variable for clarity) and
pseudo variable by adding a # just after the double opening square
bracket [[. Full details of the syntax can be found in the updated
docs/CommandGuide/FileCheck.rst.

One notable pitfall is that CHECK pattern with numeric expression using
variable defined on the same line might fail to match despite there be a
line which satisfy the conditions expressed by those numeric expression.
This is due to lack of needed support from the regular expression engine
that FileCheck relies on and is documented in the updated
docs/CommandGuide/FileCheck.rst as well as possible workaround.

Copyright:

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

[probinson]

Maybe this should be a new class of "numeric variables" with distinct syntax, e.g., [[#VAR]] or something, with an implicit regex of [[:digit:]]+ instead of making the test-writer use a valid regex.

[jhenderson]

First up, I want to say thank you for doing this! This is one of the major missing features from FileCheck over the test harness I am used to using, and leads to fragile testing where people have hard-coded in values in tests that are subject to change.

Do you intend to add hexadecimal support for this? I think it's fine not in this change, but I think it would be a valuable further addition - a lot of binary dumping tools print in hex. I think we should consider whether this impacts the way it's written as well (because presumably we'll need some special syntax to indicate we are capturing a variable as a hex number, not an integer - note that not all hex numbers are prefixed with "0x" when printed by llvm tools).

Also, what about situations like [[Var1-Var2]]? Also, what about [[Var1 - 1]] (with the spaces - this is more readable to me)?

I don't fully follow the implementation, as I'm not too familiar with FileCheck's code. I do have a number general nits, which I've tried to comment on where I saw them. One repeated one, I wanted to point out was variable naming, where you've often used lower case - see https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly for details.

docs/CommandGuide/FileCheck.rst

514–518

I'm wondering why this comment has changed?

test/FileCheck/var-expression.txt

3

Not sure if the LLVM coding style applies for test comments, but assuming they do, these need capital letters and trailing full stops.

29

VAREL? I have no idea how to expand that name...!

utils/FileCheck/FileCheck.cpp

112

Missing trailing full stop.

113

Shouldn't this be a capitalized name?

422

Spurious space before CheckOnly.

I might be being a bit dim, but why can't "[[Foo]]" be an expression, if it is missing any arithmetic?

428

ie -> i.e.

435

val -> Val

I'd actually avoid using both val and Value in the same scope, as it gets very confusing. Please come up with a different name for one of them. E.g. Value could become ValueString.

470–472

op -> Op
offset -> Offset

490

variable Name -> variable \p Name (I think?)

497

index -> Index

500

isExpression -> IsExpression

502–505

same line due -> same line, due
Remove extra space before "Therefore"
explicitely -> explicitly

509

exist -> exists

531–532

same-line use are -> either "same-line uses are" or "same-line use is"

533

git? Please use a more descriptive name.

Also, should be capitalised.

569–570

I don't think you need the initialiser here.

575–581

Are we referring to a specific variable here? If so, please use "the variable" not just "variable". If not, please use "a variable". If there are multiple variables the first sentences refer to, please use the plural to make it clear.
"First pass" -> "The first pass". Similarly "Second pass" -> "The second pass".

I'm a little confused by the first use of variable in this bit: "Note that non-expression variable use of variables...". Maybe it needs re-wording.

590–591

is... -> Is...

604

I'm not sure how to parse this sentence. I think you might mean: "Wait for the second pass to replace uses in the expression." But I'm not certain.

622

During first pass -> During the first pass

625

in turns -> in turn

626

expression -> the expression.

655

I'd get rid of "for each of the definition" in this comment, as I think it's implicit. If you think it's important get rid of the "of the".
Also "expression" -> "the expression" or "expressions"

679

Use llvm_unreachable here.

[grimar]

Few comments from me.

utils/FileCheck/FileCheck.cpp

348

I would inline:

if (IsExpression)
  Name = Expr.substr(0, Expr.find_first_of(validArithOps));

354

You do a lookup in the VariableDefs two times here, please avoid.
Also can you use vector's back() instead of constructions like --(VariableDefs[Name].end())?

463

You do not need else after return:

if (Expr.empty() && CheckOnly)
  return true;

 if (!Expr.empty()) {
   ...

507

You are doing a search of validArithOps in Name second time here. I would try to avoid.

667

In this block you have a StringMap LocalVariableTable and you perform 3 lookups on this map,
though can do that only once I think. For example like that:

auto P = LocalVariableTable.insert({VariableDef.first, {}});
if (P.second)
  P.first->second.push_back(
      {MatchInfo[VariableDefIt.first], VariableDefIt.second});

671

Since there are only 2 cases here, that might be better looking as IFs:

if (State == Unprocessed)
  State = DefProcessed;
else if (State == DefProcessed)
  State = ExprProcessed;
else
  llvm_unreachable("...");

690

I would suggest something without decrementing of the iterators:

for (const auto &VariableDef : VariableDefs) {
  const std::pair<unsigned, unsigned > &P = VariableDef.second.back();
  assert(P.first < MatchInfo.size() && "Internal paren error");
  GlobalVariableTable[VariableDef.first] = MatchInfo[P.first];
}

Also, btw this assert does not make sense here, because MatchInfo is the SmallVector
and it's operator[] already has an assert, so you can remove it I think.

[grimar]

utils/FileCheck/FileCheck.cpp

373

You are doing a lookup in VariableDefs 3 times here. Please avoid.

[thopre]

Thanks all for the valuable comments. It seems clear that more discussion is needed on the syntax before doing any code change. I'll think about it and post a proposal to the ML.

[thopre]

In D49084#1156379, @probinson wrote:

Maybe this should be a new class of "numeric variables" with distinct syntax, e.g., [[#VAR]] or something, with an implicit regex of [[:digit:]]+ instead of making the test-writer use a valid regex.

I'm not sure I'm following you. Do you mean using a different syntax when (i) defining numeric variable or (ii) when using numeric expression?
I also do not understand how this would prevent the test-writer from having to use a valid regex. Could you give an example of the problem you see?

[thopre]

In D49084#1158838, @thopre wrote:

In D49084#1156379, @probinson wrote:

Maybe this should be a new class of "numeric variables" with distinct syntax, e.g., [[#VAR]] or something, with an implicit regex of [[:digit:]]+ instead of making the test-writer use a valid regex.

I'm not sure I'm following you. Do you mean using a different syntax when (i) defining numeric variable or (ii) when using numeric expression?
I also do not understand how this would prevent the test-writer from having to use a valid regex. Could you give an example of the problem you see?

Oh you mean using #VAR to define a numeric variable, therefore avoiding the need to have a regular expression. Yes, I like the idea, especially since it can work with the idea of a syntax to specify the base in which to interpret things as suggested by @jhenderson. What it needs though is a way to distinguish between definition and use though. I'll think about it.

[arichardson]

I case people are interested I have also implemented something similar in our fork of llvm. I chose [[@EXPR var + 0x42]] as the syntax. I also added some functions such as [[@EXPR hex(VAR)]].
Variables can either by referenced by name or if they conflict with one of the builtin functions using ${x} syntax: [[@EXPR toupper(hex(${hex}))]]
Some examples of what it can do can be seen here: https://github.com/CTSRD-CHERI/llvm/blob/master/test/FileCheck/expressions.txt

The implementation is IMO quite ugly (mostly a stripped down copy of lld's linker script parser) so I never got around to cleaning it up for upstream submission.

[jhenderson]

In D49084#1160851, @arichardson wrote:

I case people are interested I have also implemented something similar in our fork of llvm. I chose [[@EXPR var + 0x42]] as the syntax. I also added some functions such as [[@EXPR hex(VAR)]].
Variables can either by referenced by name or if they conflict with one of the builtin functions using ${x} syntax: [[@EXPR toupper(hex(${hex}))]]

Simple functions are also on my wish-list! The main ones we care about are converting to and from hex numbers, and being able to do additions and subtractions. But I think just the basic integer and hex reading and additions would be a good start.

[arichardson]

In D49084#1161262, @jhenderson wrote:

In D49084#1160851, @arichardson wrote:

I case people are interested I have also implemented something similar in our fork of llvm. I chose [[@EXPR var + 0x42]] as the syntax. I also added some functions such as [[@EXPR hex(VAR)]].
Variables can either by referenced by name or if they conflict with one of the builtin functions using ${x} syntax: [[@EXPR toupper(hex(${hex}))]]

Simple functions are also on my wish-list! The main ones we care about are converting to and from hex numbers, and being able to do additions and subtractions. But I think just the basic integer and hex reading and additions would be a good start.

I added hex, oct, bin, dec + toupper/tolower (https://github.com/CTSRD-CHERI/llvm/blob/master/utils/FileCheck/FileCheck.cpp#L697) but I think I only ever ended up using dec/hex/toupper in the tests that I've written since.

[rnk]

FWIW I had a use case for this recently, and found it hadn't landed. Is there anything blocking that you need help with?

[thopre]

In D49084#1203189, @rnk wrote:

FWIW I had a use case for this recently, and found it hadn't landed. Is there anything blocking that you need help with?

My apologies for not replying earlier, didn't see the comment until now. Nothing blocking, splitting my time between GNU and LLVM work so progress has been slower than expected. I am nearly done now, I expect to publish a completely new version sometimes this week. Stay tuned!

Best regards.

[thopre]

FileCheck: Add support for numeric variables and expressions

Summary:
Sometimes there's a need to verify output which contain several numeric
values that are related by a numeric expression. For example, one might
want to check that two numbers are consecutive but accept any value for
the smallest of the two.

This patch introduces support for numeric variables and expressions to
express this sort of constraint. A user can then define variables to
have a numeric value and write numeric expression using those variable.
A matching format specifier allows to match numbers in a variety of
format: signed and unsigned decimal numbers as well as hex number in
lower or capital case. For example, matching 2 consecutive signed
decimal integers separated by a space is done via:

[[%d,#N:]] #N+1

The syntax is modeled after that of use and definition of the already
supported variables flavour (renamed pattern variable for clarity) and
pseudo variable by adding a # just after the double opening square
bracket [[. Full details of the syntax can be found in the updated
docs/CommandGuide/FileCheck.rst.

One notable pitfall is that CHECK pattern with numeric expression using
variable defined on the same line might fail to match despite there be a
line which satisfy the conditions expressed by those numeric expression.
This is due to lack of needed support from the regular expression engine
that FileCheck relies on and is documented in the updated
docs/CommandGuide/FileCheck.rst as well as possible workaround.

Reviewers: probinson, arichardson, jhenderson, rnk, jdenny

Subscribers: llvm-commits

[probinson]

I am willing to review this; however it is a very large patch, which will take time to go through. Also, after tomorrow, I am on holiday until 3 January.

Some parts of it look like they could be split off into separate patches; for example, the command-line option handling looks like it could be done separately. I wonder if the refactoring of the @LINE handling into the new expression handling could also be separated out.
If not, that's okay, it will just make the review take a bit longer.

As a first step, I have a structural suggestion for the documentation. Once that is done, I will probably have some more detailed comments on wording, but they can wait for now.

docs/CommandGuide/FileCheck.rst

516

The topic and syntax are more complicated than anything else FileCheck does, so I think it would be better separate the syntax description into two parts: using numeric expressions, and defining numeric variables. The syntax for a numeric expression would be
[[# <constraint> <expr> ]]
and then you can describe how to define the variables,
[[# <fmtspec>,<NUMVAR>: <constraint> <expr> ]]
which builds on the syntax for using variables. I think this ordering will be easier to explain and to understand.

542

Please give examples of what these lines will match, and what they will not match.

[thopre]

Document how to set numeric variable with -D option

[arichardson]

Thanks for updating this patch. It will be very useful!

include/llvm/Support/FileCheck.h

225 ↗

(On Diff #179194)

Does this need to be a shared_ptr? Could it be a unique_ptr instead?

lib/Support/FileCheck.cpp

81 ↗

(On Diff #179194)

Could this return a llvm::Optional<std::string> to avoid the output-parameter?

1076 ↗

(On Diff #179194)

This could be deleted if you initialize CaptureParenNeeded to false at the beginning of the scope. It would also ensure that the variable is never uninitialized.

[hfinkel]

Thanks for updating this patch. It will be very useful!

+1

[thopre]

docs/CommandGuide/FileCheck.rst

514–518

It changes for 2 reasons:

• it redefines @LINE as a (pseudo) numeric variable as opposed to a pattern variable since it behaves like a numeric variable
• it removes the explanation about offset since it comes naturally from being a numeric variable and then more (eg. #@LINE + OFFSETVAR).

In short, because we now have numeric variable it is easier to say @LINE is one and say what it evaluates to and leave what can be done to it to the general description of numeric variable.

include/llvm/Support/FileCheck.h

225 ↗

(On Diff #179194)

Several numeric expressions can refer to the same numeric variable in which case you want the reference counting so that when calling the destructor for those numeric expression delete is called only once on that variable.

lib/Support/FileCheck.cpp

81 ↗

(On Diff #179194)

Yes indeed and perhaps in a few other places as well. Thanks

[jhenderson]

This change is so large that it's going to take quite a while to review. However, I've made a few initial comments on the header. I'll come back to this next week and continue from there.

include/llvm/Support/FileCheck.h

41 ↗

(On Diff #179194)

Nit: Code -> code

44 ↗

(On Diff #179194)

Why the declaration here?

52 ↗

(On Diff #179194)

Nit: print -> printed

57 ↗

(On Diff #179194)

I don't really understand this comment. I think you are saying that this "Set" variable indicates whether the format is explicit or not, right? If so, rename the variable to something like IsExplicit or similar.

60 ↗

(On Diff #179194)

Several -> If set, there are ...

81 ↗

(On Diff #179194)

Don't namespace this. Just make the enum inside a scoped enum (i.e. enum class).

82–84 ↗

(On Diff #179194)

I think the general style in LLVM is single space after full stop (applies here and elsewhere).

"is a numeric expression (and thus value ...) available" -> "a numeric expression is available (and thus the value ...)"

95–96 ↗

(On Diff #179194)

I'm not sure the second sentence here is necessary.

97 ↗

(On Diff #179194)

I'm not sure you need to name all your classes "FileCheckXXX". You can shorten their names by dropping the FileCheck part, I reckon.

99–103 ↗

(On Diff #179194)

Why not just always store in a single uint64_t field called Value and convert to signed when requested?

Also, don't unnecessarily abbreviate variables like this.

131–132 ↗

(On Diff #179194)

I don't think you need this comment. operator== is fairly self-explanatory as to its purpose...!

139 ↗

(On Diff #179194)

LLVM style is for camel-case, starting with lower-case letters for method and function names. Also, don't bother commenting if the function purpose is obvious from its name.

145 ↗

(On Diff #179194)

What is a tentative value?

169 ↗

(On Diff #179194)

You don't need the "struct" here or in any of the other function arguments or variable declarations etc.

169–170 ↗

(On Diff #179194)

This should probably return an Expected<std::string> or Optional<std::string> and not use an output parameter.

172–180 ↗

(On Diff #179194)

Could these two just be overloads of operator+ and operator-?

183–184 ↗

(On Diff #179194)

Only the first sentence here is needed. The other two are implicit by the fact that this is a virtual class with abstract methods.

191–192 ↗

(On Diff #179194)

What does "null" mean in this context? We're not dealing with pointers... Assuming it's some kind of no-format, perhaps calling it by the corresponding variable name might be better.

Again, the last sentence is unnecessary, since it's implicit in the "= 0" part of the function declaration.

195–198 ↗

(On Diff #179194)

This isn't the behaviour I'd expect a function of this name to do. I'd expect it to return a vector of names, not append to an input parameter.

Other nit in the comment: overrided -> overridden.

201 ↗

(On Diff #179194)

Nit: litteral -> literal

209–213 ↗

(On Diff #179194)

Similar to the FileCheckNumExprVal class, I don't think you need both constructors. One should suffice.

215–217 ↗

(On Diff #179194)

I'd drop the first sentence and rephrase the second:
"Return the node itself and set \p Fmt to none, since literals do not have conversion formats" (where none is a placeholder for a better word than null).

227–228 ↗

(On Diff #179194)

from x 2 -> by

232 ↗

(On Diff #179194)

It's a little unclear what is meant by "matching format" here. Is it the format this expression can match against (i.e. hex/decimal etc), or what?

236 ↗

(On Diff #179194)

Like struct, you don't nee the enum here.

246 ↗

(On Diff #179194)

for numeric -> for a numeric

247–248 ↗

(On Diff #179194)

eg. -> e.g.

I'd drop the bit in parentheses at the end of the sentence. If somebody starts using this for another person, this comment will almost certainly go stale.

252–253 ↗

(On Diff #179194)

Drop the reference to the shared_ptr in this sentence, and change the second sentence to something like "The pointer is guaranteed to be valid as long as this object is."

256 ↗

(On Diff #179194)

What is meant by "matched" in this context?

268 ↗

(On Diff #179194)

Remove the first "can"

274 ↗

(On Diff #179194)

Rather than returning a bool, and passing in a parameter that will be set, this and similar functions should probably return an Expected<T> or Optional<T>.

294–299 ↗

(On Diff #179194)

Why are these two different functions?

[thopre]

Besides typo and wording improvements:

• Merge Eval and SetValueFromEval
• Use union to store an int64_t or uint64_t in FileCheckNumExprVal
• Turn Add and Sub into operator overloading.

[thopre]

include/llvm/Support/FileCheck.h

81 ↗

(On Diff #179194)

I was following what is done for the Check enum. Should that enum be changed (in a separate patch)?

82–84 ↗

(On Diff #179194)

I saw many occurrences of 2 spaces after dot in FileCheck and thought it had a different code style. Upon inspection I suppose it must be mistakes as single space is more common:

$ git grep -E -c "\. [^ ]" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/lib/Support/FileCheck.cpp:20
$ git grep -E -c "\. [^ ]" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/lib/Support/FileCheck.cpp:12

$ git grep -E -c "\. [^ ]" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:4
$ git grep -E -c "\. [^ ]" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:3

97 ↗

(On Diff #179194)

The FileCheck prefix was added when FileCheck logic was moved into a library. See comment from Bogner in https://reviews.llvm.org/D50283

99–103 ↗

(On Diff #179194)

Since unsigned to signed conversion is implementation defined and reinterpret_cast does not work between integer types, I would have to do the two-complement manually. I've used a union instead, hope you like it better.

139 ↗

(On Diff #179194)

There are actually more examples of camel-case starting with capital letter: all the CheckFoo (eg. CheckSame), the PrintFoo (eg. PrintMatch).

$ git grep -E -c "[a-zA-Z]+ [a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:7
$ git grep -E -c "[a-zA-Z]+ [a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp
<no result>

$ git grep -E -c "[a-zA-Z]+ [A-Z][a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:17
$ git grep -E -c "[a-zA-Z]+ [A-Z][a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/lib/Support/FileCheck.cpp:13

Do you still want me to fix that?

209–213 ↗

(On Diff #179194)

My same concern applies here: I can easily "reinterpret_cast" to a uint64_t parameter when calling the constructor and pass an extra Signed parameter but then to do signed arithmetic I cannot think of an *easy* way to reinterpret it back into an int64_t without tripping on undefined or implementation defined behaviour.

294–299 ↗

(On Diff #179194)

I can't remember now except that I needed it at some point. It is clearly not needed now, maybe as a result of having the known phase info. Anyway, merged into one.

[jhenderson]

For some reason the new diff has lost all of the inline comments...

I was following what is done for the Check enum. Should that enum be changed (in a separate patch)?

Yes, probably.

The FileCheck prefix was added when FileCheck logic was moved into a library. See comment from Bogner in https://reviews.llvm.org/D50283

Ah, I hadn't noticed that it had moved to Support. Okay, this is fine as is.

There are actually more examples of camel-case starting with capital letter: all the CheckFoo (eg. CheckSame), the PrintFoo (eg. PrintMatch).

$ git grep -E -c "[a-zA-Z]+ [a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:7
$ git grep -E -c "[a-zA-Z]+ [a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp
<no result>

$ git grep -E -c "[a-zA-Z]+ [A-Z][a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:17
$ git grep -E -c "[a-zA-Z]+ [A-Z][a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/lib/Support/FileCheck.cpp:13

Do you still want me to fix that?

As there are already a mixture, we should follow the LLVM style for the new code, especially given the amount of new code being added. If it was all a different style there might be a different case to be made.

---

I definitely think that you need to fragment this up into small incremental patches, as I've found I'm not getting enough time to digest the whole thing at once (and I'm guessing the same goes for other reviewers). If you could try to find incremental ways of doing things that would be great. One example would be to add the command-line switches as non-functional switches first (make them hidden until you're ready for them to be published), then incrementally add features (and expand testing) to them. Similarly accepting some forms of the pattern etc could be done in one change (but do nothing with them) and then add more functionality over time.

llvm/include/llvm/Support/FileCheck.h

109 ↗

(On Diff #183612)

Nit: ie -> i.e.

129 ↗

(On Diff #183612)

Nit: other -> Other

174–177 ↗

(On Diff #183612)

Why not make these member functions? I.e:

FileCheckNumExprVal operator+(const FileChcekNumExprVal &Other);
FileCheckNumExprVal operator-(const FileChcekNumExprVal &Other);

Also Nit: lhs/rhs should be LHS/RHS (but this is moot if switching to member operators).

191 ↗

(On Diff #183612)

Nit: overriden -> overridden.

205–208 ↗

(On Diff #183612)

My same concern applies here: I can easily "reinterpret_cast" to a uint64_t parameter when calling the constructor and pass an extra Signed parameter but then to do signed arithmetic I cannot think of an *easy* way to reinterpret it back into an int64_t without tripping on undefined or implementation defined behaviour.

Okay, fair enough. I wonder if they could be templated then instead? I.e:

template <typename T> FileCheckNumExprLiteral(T Val) : Value(Val) {}

Also, should these be explicit, or are you deliberately allowing implicit conversions?

221–223 ↗

(On Diff #183612)

The second sentence is hard to parse. Could you maybe break it up into the different bits, as separate sentences?

226 ↗

(On Diff #183612)

Nit: eg. -> e.g.

245 ↗

(On Diff #183612)

is -> is alive.

264 ↗

(On Diff #183612)

correspond -> corresponds

[thopre]

In D49084#1384876, @jhenderson wrote:

I definitely think that you need to fragment this up into small incremental patches, as I've found I'm not getting enough time to digest the whole thing at once (and I'm guessing the same goes for other reviewers). If you could try to find incremental ways of doing things that would be great. One example would be to add the command-line switches as non-functional switches first (make them hidden until you're ready for them to be published), then incrementally add features (and expand testing) to them. Similarly accepting some forms of the pattern etc could be done in one change (but do nothing with them) and then add more functionality over time.

Done in patch series starting from https://reviews.llvm.org/D60381. Thus closing this revision. I've just noticed there's a few of your comments (casing of function notably) I haven't incorporated in the new patch series. I'll address them now.

docs/CommandGuide/FileCheck.rst

 local.  All local variables get undefined at the beginning of each
 CHECK-LABEL block. Global variables are not affected by CHECK-LABEL.
 This makes it easier to ensure that individual tests are not affected
 by variables set in preceding tests.
 
 FileCheck Expressions
 ~~~~~~~~~~~~~~~~~~~~~
 
+:program:`FileCheck` also allows to use the result of a simple arithmetic
+expression involving variables. This allows to capture relations between
+variables such as the need for consecutive registers to be used. The syntax
+is ``[[<VAR><op><immediate>]]`` where ``<VAR>`` is a :program:`FileCheck`
+variable, ``<OP>`` is a supported arithmetic operation and ``<immediate>`` is
+an integer. Currently supported arithmetic operations are ``+`` and ``-``. For
+example:
+
+.. code-block:: llvm
+
+    ; CHECK: op [[REG:r[0-9]+]], [[REG]], [[REG+1]]
+
+Important note: in its current implementation variable defined in a pattern
+with expressions must match the right text when replacing each variable
+expressions by the regular expression `[[:digit:]]+`. This is because variable
+expression are not handled by the regular expression engine and are implemented
+by a 2 passes process where the first pass define the variables and replace the
+expression with the above regular expression. The second pass can then do a
+second match with the variable expressions substituted by their value.
+
+FileCheck Pseudo Variables
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
 Sometimes there's a need to verify output which refers line numbers of the
 match file, e.g. when testing compiler diagnostics.  This introduces a certain
 fragility of the match file structure, as "``CHECK:``" lines contain absolute
 line numbers in the same file, which have to be updated whenever line numbers
 change due to text addition or deletion.
 
-To support this case, FileCheck allows using ``[[@LINE]]``,
-``[[@LINE+<offset>]]``, ``[[@LINE-<offset>]]`` expressions in patterns. These
-expressions expand to a number of the line where a pattern is located (with an
-optional integer offset).
+To support this case, FileCheck understands the ``@LINE`` pseudo variable in
+patterns in both regular variable uses ``[[@LINE]]`` and in expressions
+``[[@LINE+<offset>]]``, ``[[@LINE-<offset>]]``. These expand to a number of
+the line where a pattern is located (with an optional integer offset).
 
 This way match patterns can be put near the relevant test lines and include
 relative line number references, for example:
 
 .. code-block:: c++
 
    // CHECK: test.cpp:[[@LINE+4]]:6: error: expected ';' after top level declarator
    // CHECK-NEXT: {{^int a}}
    // CHECK-NEXT: {{^     \^}}
    // CHECK-NEXT: {{^     ;}}
    int a
 
 Matching Newline Characters
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 To match newline characters in regular expressions the character class
 ``[[:space:]]`` can be used. For example, the following pattern:
 
 .. code-block:: c++
 
    // CHECK: DW_AT_location [DW_FORM_sec_offset] ([[DLOC:0x[0-9a-f]+]]){{[[:space:]].*}}"intd"
 
 matches output of the form (from llvm-dwarfdump):
 
 .. code-block:: text
 
        DW_AT_location [DW_FORM_sec_offset]   (0x00000233)
        DW_AT_name [DW_FORM_strp]  ( .debug_str[0x000000c9] = "intd")
 
 letting us set the :program:`FileCheck` variable ``DLOC`` to the desired value 
 ``0x00000233``, extracted from the line immediately preceding "``intd``".

test/FileCheck/var-expression.txt

+; RUN: FileCheck  -input-file %s %s
+
+; expression using variables defined on other lines
+START
+1
+2
+0
+3
+; expression using variables defined on same line
+MARK1
+11 12
+11 10
+11 13
+; expression using variables redefined on same line
+MARK2
+21
+22 31 32
+30 41 40
+END
+
+; Ensure we try to match each line with digits by wrapping them between START
+; and END and using CHECK-NEXT to ensure each line is covered.
+CHECK-LABEL: START
+CHECK-NEXT: [[VAR1:[0-9]+]]
+CHECK-NEXT: [[VAR1+1]]
+CHECK-NEXT: [[VAR1-1]]
+CHECK-NOT: [[VAR1+1]]
+CHECK-LABEL: MARK1
+CHECK-NEXT: [[VAREL:[0-9]+]] [[VAREL+1]]
+CHECK-NEXT: [[VAREL]] [[VAREL-1]]
+CHECK-NOT: [[VAREL]] [[VAREL+1]]
+CHECK-LABEL: MARK2
+CHECK-NEXT: [[VAR:[0-9]+]]
+CHECK-NEXT: [[VAR+1]] [[VAR:[0-9]+]] [[VAR+1]]
+CHECK-NEXT: [[VAR-1]] [[VAR:[0-9]+]] [[VAR-1]]
+CHECK-NEXT: END

utils/FileCheck/FileCheck.cpp

enum CheckType {
   CheckEOF,
 
   /// Marks when parsing found a -NOT check combined with another CHECK suffix.
   CheckBadNot
 };
 }
 
 class Pattern {
+  /// Valid operations in expressions
+  static constexpr StringRef validArithOps = StringRef("+-", 2);
+
   SMLoc PatternLoc;
 
   /// A fixed string to match as the pattern or empty if this pattern requires
   /// a regex match.
   StringRef FixedStr;
 
   /// A regex string to match as the pattern or empty if this pattern requires
   /// a fixed string to match.
   std::string RegExStr;
 
   /// Entries in this vector map to uses of a variable in the pattern, e.g.
   /// "foo[[bar]]baz".  In this case, the RegExStr will contain "foobaz" and
   /// we'll get an entry in this vector that tells us to insert the value of
   /// bar at offset 3.
   std::vector<std::pair<StringRef, unsigned>> VariableUses;
 
-  /// Maps definitions of variables to their parenthesized capture numbers.
-  /// 
-  /// E.g. for the pattern "foo[[bar:.*]]baz", VariableDefs will map "bar" to
-  /// 1.
-  std::map<StringRef, unsigned> VariableDefs;
+  /// Maps defined variables to each of their definitions' parenthesized
+  /// capture numbers and position in the pattern string.
+  ///
+  /// E.g. for the pattern "foo[[bar:.*]]baz[[bar:.*]]", VariableDefs will map
+  /// "bar" to the vector {{1, 3}, {2, 16}}.
+  std::map<StringRef, std::vector<std::pair<unsigned, unsigned>>> VariableDefs;
 
   Check::CheckType CheckTy;
 
   /// Contains the number of line this pattern is in.
   unsigned LineNumber;
 
 public:
   explicit Pattern(Check::CheckType Ty) : CheckTy(Ty) {}
public:
   Check::CheckType getCheckTy() const { return CheckTy; }
 
 private:
   bool AddRegExToRegEx(StringRef RS, unsigned &CurParen, SourceMgr &SM);
   void AddBackrefToRegEx(unsigned BackrefNum);
   unsigned
   ComputeMatchDistance(StringRef Buffer,
                        const StringMap<StringRef> &VariableTable) const;
-  bool EvaluateExpression(StringRef Expr, std::string &Value) const;
+  bool GetVariableValue(StringRef Name, unsigned index,
+                        StringMap<StringRef> &GlobalVariableTable,
+                        StringMap<std::vector<std::pair<StringRef, unsigned>>>
+                            &LocalVariableTable,
+                        StringRef &Value) const;
+  bool EvaluateExpression(StringRef Expr, unsigned index,
+                          StringMap<StringRef> &GlobalVariableTable,
+                          StringMap<std::vector<std::pair<StringRef, unsigned>>>
+                              &LocalVariableTable,
+                          std::string &Value, bool CheckOnly) const;
   size_t FindRegexVarEnd(StringRef Str, SourceMgr &SM);
 };
 
+constexpr StringRef Pattern::validArithOps;
+
 /// Parses the given string into the Pattern.
 ///
 /// \p Prefix provides which prefix is being matched, \p SM provides the
 /// SourceMgr used for error reports, and \p LineNumber is the line number in
 /// the input file from which the pattern string was read. Returns true in
 /// case of an error, false otherwise.
 bool Pattern::ParsePattern(StringRef PatternStr, StringRef Prefix,
                            SourceMgr &SM, unsigned LineNumber) {
if (PatternStr.startswith("[[")) {
       StringRef Name = MatchStr.substr(0, NameEnd);
 
       if (Name.empty()) {
         SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
                         "invalid name in named regex: empty name");
         return true;
       }
 
-      // Verify that the name/expression is well formed. FileCheck currently
-      // supports @LINE, @LINE+number, @LINE-number expressions. The check here
-      // is relaxed, more strict check is performed in \c EvaluateExpression.
+      // Verify that the name/expression is well formed.  Variable names are
+      // composed of alphanumeric characters and underscores.  Expressions
+      // are defined by what EvaluateExpression accepts.
       bool IsExpression = false;
       for (unsigned i = 0, e = Name.size(); i != e; ++i) {
         if (i == 0) {
+          bool TryExpression =
+              (Name.find_first_of(validArithOps) != StringRef::npos);
           if (Name[i] == '$')  // Global vars start with '$'
             continue;
-          if (Name[i] == '@') {
-            if (NameEnd != StringRef::npos) {
+          if (Name[i] == '@' || TryExpression) {
+            std::string Value;
+            StringMap<StringRef> VariableTable;
+            StringMap<std::vector<std::pair<StringRef, unsigned>>>
+                LocalVariableTable;
+            if (NameEnd != StringRef::npos ||
+                !EvaluateExpression(Name, RegExStr.size(), VariableTable,
+                                    LocalVariableTable, Value,
+                                    true /*CheckOnly*/)) {
               SM.PrintMessage(SMLoc::getFromPointer(Name.data()),
                               SourceMgr::DK_Error,
                               "invalid name in named regex definition");
               return true;
             }
             IsExpression = true;
             continue;
           }
         }
-        if (Name[i] != '_' && !isalnum(Name[i]) &&
-            (!IsExpression || (Name[i] != '+' && Name[i] != '-'))) {
+        if (Name[i] != '_' && !isalnum(Name[i]) && !IsExpression) {
           SM.PrintMessage(SMLoc::getFromPointer(Name.data() + i),
                           SourceMgr::DK_Error, "invalid name in named regex");
           return true;
         }
       }
 
       // Name can't start with a digit.
       if (isdigit(static_cast<unsigned char>(Name[0]))) {
         SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
                         "invalid name in named regex");
         return true;
       }
 
       // Handle [[foo]].
       if (NameEnd == StringRef::npos) {
-        // Handle variables that were defined earlier on the same line by
-        // emitting a backreference.
-        if (VariableDefs.find(Name) != VariableDefs.end()) {
-          unsigned VarParenNum = VariableDefs[Name];
+        StringRef Expr = Name;
+
+        // Get the name part of expressions.
+        if (IsExpression) {
+          StringRef::size_type OpStart = Expr.find_first_of(validArithOps);
+          Name = Expr.substr(0, OpStart);
+        }
+
+        // Handle non-expression use of variables that were defined earlier on
+        // the same line by emitting a backreference.
+        if (VariableDefs.find(Name) != VariableDefs.end() && !IsExpression) {
+          unsigned VarParenNum = (--(VariableDefs[Name].end()))->first;
           if (VarParenNum < 1 || VarParenNum > 9) {
             SM.PrintMessage(SMLoc::getFromPointer(Name.data()),
                             SourceMgr::DK_Error,
                             "Can't back-reference more than 9 variables");
             return true;
           }
           AddBackrefToRegEx(VarParenNum);
         } else {
-          VariableUses.push_back(std::make_pair(Name, RegExStr.size()));
+          VariableUses.push_back(std::make_pair(Expr, RegExStr.size()));
         }
         continue;
       }
 
       // Handle [[foo:.*]].
-      VariableDefs[Name] = CurParen;
+      if (VariableDefs.find(Name) == VariableDefs.end())
+        VariableDefs[Name] = std::vector<std::pair<unsigned, unsigned>>();
+      VariableDefs[Name].emplace_back(
+          std::make_pair(CurParen, RegExStr.size()));
       RegExStr += '(';
       ++CurParen;
 
       if (AddRegExToRegEx(MatchStr.substr(NameEnd + 1), CurParen, SM))
         return true;
 
       RegExStr += ')';
     }
 

 }
 
 void Pattern::AddBackrefToRegEx(unsigned BackrefNum) {
   assert(BackrefNum >= 1 && BackrefNum <= 9 && "Invalid backref number");
   std::string Backref = std::string("\\") + std::string(1, '0' + BackrefNum);
   RegExStr += Backref;
 }
 
-/// Evaluates expression and stores the result to \p Value.
+/// Evaluates expression Expr at \p index in the pattern and stores the result
+/// to \p Value. GlobalVariableTable holds the value of variables defined in
+/// patterns in earlier lines while LocalVariableTable holds the values of
+/// all definitions of variables defined in the same line.  CheckOnly allows
+/// to only check that the expression is well formed, in which case
+/// GlobalVariableTable and LocalVariableTable are not used.
+///
+/// Valid expressions are addition and substraction of a constant from a
+/// user-defined variable or LINE builtin variable. For the latter case the
+/// operation is optional, ie @LINE is a valid expression.
 ///
-/// Returns true on success and false when the expression has invalid syntax.
-bool Pattern::EvaluateExpression(StringRef Expr, std::string &Value) const {
-  // The only supported expression is @LINE([\+-]\d+)?
-  if (!Expr.startswith("@LINE"))
-    return false;
-  Expr = Expr.substr(StringRef("@LINE").size());
-  int Offset = 0;
-  if (!Expr.empty()) {
-    if (Expr[0] == '+')
-      Expr = Expr.substr(1);
-    else if (Expr[0] != '-')
+/// Returns whether the arithmetic expression is valid.
+bool Pattern::EvaluateExpression(
+    StringRef Expr, unsigned index, StringMap<StringRef> &GlobalVariableTable,
+    StringMap<std::vector<std::pair<StringRef, unsigned>>> &LocalVariableTable,
+    std::string &Value, bool CheckOnly) const {
+  int val;
+  StringRef VarName;
+  if (Expr.startswith("@LINE")) {
+    VarName = "LINE";
+    if (!CheckOnly)
+      val = LineNumber;
+  } else {
+    StringRef::size_type OpStart = Expr.find_first_of(validArithOps);
+    // Reject reference to user-defined variable if no arithmetic
+    // expression is involved.
+    if (OpStart == StringRef::npos)
+      return false;
+    VarName = Expr.substr(0, OpStart);
+    if (!CheckOnly) {
+      StringRef ValueRef;
+      // Undefined variable.
+      if (!GetVariableValue(Expr, index, GlobalVariableTable,
+                            LocalVariableTable, ValueRef))
         return false;
-    if (Expr.getAsInteger(10, Offset))
+      if (ValueRef.getAsInteger(10, val))
+        return false;
+    }
+  }
+
+  Expr = Expr.substr(VarName.size());
+  if (Expr.empty() && CheckOnly)
+    return true;
+  else if (!Expr.empty()) {
+    // Invalid arithmetic operation
+    if (StringRef(validArithOps).find_first_of(Expr[0]) == StringRef::npos)
       return false;
+    else if (CheckOnly)
+      return true;
+
+    char op = Expr[0];
+    Expr = Expr.substr(1);
+    int offset = 0;
+    if (Expr.getAsInteger(10, offset))
+      return false;
+    switch (op) {
+    case '+':
+      val += offset;
+      break;
+    case '-':
+      val -= offset;
+      break;
+    default:
+      return false;
+    }
+  }
+  Value = llvm::itostr(val);
+  return true;
+}
+
+/// Evaluates use of variable Name at \p index in the pattern and stores the
+/// result to \p Value. GlobalVariableTable holds the value of variables
+/// defined in patterns in earlier lines while LocalVariableTable holds the
+/// values of all definitions of variables defined in the same line.
+///
+/// Returns whether the variable used was defined.
+bool Pattern::GetVariableValue(
+    StringRef Name, unsigned index, StringMap<StringRef> &GlobalVariableTable,
+    StringMap<std::vector<std::pair<StringRef, unsigned>>> &LocalVariableTable,
+    StringRef &Value) const {
+  bool isExpression = (Name.find_first_of(validArithOps) != StringRef::npos);
+
+  // Expressions cannot use backreference when referencing variable defined
+  // earlier on the same line due to the resulting value being different than
+  // the one defined.  Therefore we need to explicitely check here whether the
+  // variable was defined on the same line at an earlier index for expressions.
+  if (isExpression) {
+    Name = Name.substr(0, Name.find_first_of(validArithOps));
+    auto VariableDefIterator = VariableDefs.find(Name);
+    // At least one definition of this variable exist on the same line at an
+    // earlier index. Search for its value.
+    if (VariableDefIterator != VariableDefs.end() &&
+        VariableDefIterator->second.begin()->second < index) {
+      auto LocalVariableIterator = LocalVariableTable.find(Name);
+      assert(LocalVariableIterator != LocalVariableTable.end());
+
+      auto VariableVectorIterator = LocalVariableIterator->second.begin();
+      assert(VariableVectorIterator != LocalVariableIterator->second.end());
+      for (; VariableVectorIterator != LocalVariableIterator->second.end() &&
+             VariableVectorIterator->second < index;
+           ++VariableVectorIterator)
+        ;
+      --VariableVectorIterator;
+      assert(VariableVectorIterator->second < index);
+
+      Value = VariableVectorIterator->first;
+      return true;
+    }
   }
-  Value = llvm::itostr(LineNumber + Offset);
+
+  // No local definition found or this is a non-expression variable use, in
+  // which case it refers to a definition on another line since same-line use
+  // are handled by back-references. Search for its earlier definition.
+  StringMap<StringRef>::iterator git = GlobalVariableTable.find(Name);
+  if (git == GlobalVariableTable.end())
+    return false;
+
+  Value = git->second;
   return true;
 }
 
 /// Matches the pattern string against the input buffer \p Buffer
 ///
 /// This returns the position that is matched or npos if there is no match. If
 /// there is a match, the size of the matched string is returned in \p
 /// MatchLen.
 ///
-/// The \p VariableTable StringMap provides the current values of filecheck
-/// variables and is updated if this match defines new values.
+/// The \p GlobalVariableTable StringMap provides the current values of
+/// filecheck variables and is updated if this match defines new values.
 size_t Pattern::Match(StringRef Buffer, size_t &MatchLen,
-                      StringMap<StringRef> &VariableTable) const {
+                      StringMap<StringRef> &GlobalVariableTable) const {
   // If this is the EOF pattern, match it immediately.
   if (CheckTy == Check::CheckEOF) {
     MatchLen = 0;
     return Buffer.size();
   }
 
   // If this is a fixed string pattern, just match it now.
   if (!FixedStr.empty()) {
     MatchLen = FixedStr.size();
     return Buffer.find(FixedStr);
   }
 
   // Regex match.
 
   // If there are variable uses, we need to create a temporary string with the
   // actual value.
   StringRef RegExToMatch = RegExStr;
   std::string TmpStr;
+  StringMap<std::vector<std::pair<StringRef, unsigned>>> LocalVariableTable =
+      {};
+  SmallVector<StringRef, 4> MatchInfo;
+  enum MatchState { Unprocessed, DefProcessed, ExprProcessed };
+  enum MatchState State = Unprocessed;
+
+  // Do two passes over the pattern to handle expressions using variable
+  // defined earlier on the same line. First pass will replace all other uses
+  // of variable by their value and then do temporary records of the value of
+  // variables defined in the pattern. Second pass replaces all uses, using
+  // that temporary record. Note that non-expression variable use of variables
+  // defined on the same line are handled via back-reference and thus are not
+  // replaced by any of those passes.
+  while (State != ExprProcessed) {
     if (!VariableUses.empty()) {
       TmpStr = RegExStr;
 
       unsigned InsertOffset = 0;
       for (const auto &VariableUse : VariableUses) {
         std::string Value;
+        StringRef Expr = VariableUse.first;
+        bool isPseudo = (Expr[0] == '@');
+        bool isExpression =
+            (Expr.find_first_of(validArithOps) != StringRef::npos);
+
+        // Pseudo variable (eg. @LINE)
+        if (isPseudo) {
+          if (!EvaluateExpression(Expr, VariableUse.second, GlobalVariableTable,
+                                  LocalVariableTable, Value,
+                                  false /*CheckOnly*/))
+            return StringRef::npos;
+        } else {
+          StringRef ValueRef;
+          bool VarFound = false;
 
-      if (VariableUse.first[0] == '@') {
-        if (!EvaluateExpression(VariableUse.first, Value))
+          // Wait second pass to replace uses in expression
+          if (State >= DefProcessed || !isExpression)
+            VarFound =
+                GetVariableValue(Expr, VariableUse.second, GlobalVariableTable,
+                                 LocalVariableTable, ValueRef);
+
+          if (VarFound) {
+            if (isExpression) {
+              if (!EvaluateExpression(Expr, VariableUse.second,
+                                      GlobalVariableTable, LocalVariableTable,
+                                      Value, false /*CheckOnly*/))
                 return StringRef::npos;
             } else {
-        StringMap<StringRef>::iterator it =
-            VariableTable.find(VariableUse.first);
+              // Look up the value and escape it so that we can put it into the
+              // regex.
+              Value += Regex::escape(ValueRef);
+            }
+          } else {
+            // During first pass, allow any number of digits instead of the
+            // expressions. This allows more matches in this pass than the
+            // actual pattern which might lead to the wrong temporary record
+            // of variable definition at the end of the pass. This in turns
+            // might resolve expression incorrectly in the second pass and
+            // give incorrect match, hence the documented limitations on use
+            // of expressions.
+            if (isExpression && State < DefProcessed) {
+              Value += std::string("[[:digit:]]+");
+            } else {
               // If the variable is undefined, return an error.
-        if (it == VariableTable.end())
               return StringRef::npos;
-
-        // Look up the value and escape it so that we can put it into the regex.
-        Value += Regex::escape(it->second);
+            }
+          }
         }
 
         // Plop it into the regex at the adjusted offset.
         TmpStr.insert(TmpStr.begin() + VariableUse.second + InsertOffset,
                       Value.begin(), Value.end());
         InsertOffset += Value.size();
       }
 
       // Match the newly constructed regex.
       RegExToMatch = TmpStr;
     }
 
-  SmallVector<StringRef, 4> MatchInfo;
     if (!Regex(RegExToMatch, Regex::Newline).match(Buffer, &MatchInfo))
       return StringRef::npos;
 
     // Successful regex match.
     assert(!MatchInfo.empty() && "Didn't get any match");
-  StringRef FullMatch = MatchInfo[0];
+
+    // If this defines any variables, temporarily record their values for each
+    // of the definition to resolve expression in the next pass.
+    if (State == Unprocessed) {
+      for (const auto &VariableDef : VariableDefs) {
+        for (auto VariableDefIt : VariableDef.second) {
+          assert(VariableDefIt.first < MatchInfo.size() &&
+                 "Internal paren error");
+          if (LocalVariableTable.find(VariableDef.first) ==
+              LocalVariableTable.end())
+            LocalVariableTable[VariableDef.first] =
+                std::vector<std::pair<StringRef, unsigned>>();
+          LocalVariableTable[VariableDef.first].emplace_back(std::make_pair(
+              MatchInfo[VariableDefIt.first], VariableDefIt.second));
+        }
+      }
+    }
+
+    switch (State) {
+    case Unprocessed:
+      State = DefProcessed;
+      break;
+    case DefProcessed:
+      State = ExprProcessed;
+      break;
+    default:
+      assert(0);
+      break;
+    }
+  }
 
   // If this defines any variables, remember their values.
   for (const auto &VariableDef : VariableDefs) {
-    assert(VariableDef.second < MatchInfo.size() && "Internal paren error");
-    VariableTable[VariableDef.first] = MatchInfo[VariableDef.second];
+    assert((--(VariableDef.second.end()))->first < MatchInfo.size() &&
+           "Internal paren error");
+    GlobalVariableTable[VariableDef.first] =
+        MatchInfo[(--(VariableDef.second.end()))->first];
   }
 
+  StringRef FullMatch = MatchInfo[0];
   MatchLen = FullMatch.size();
   return FullMatch.data() - Buffer.data();
 }
 
-
 /// Computes an arbitrary estimate for the quality of matching this pattern at
 /// the start of \p Buffer; a distance of zero should correspond to a perfect
 /// match.
 unsigned
 Pattern::ComputeMatchDistance(StringRef Buffer,
                               const StringMap<StringRef> &VariableTable) const {
   // Just compute the number of matching characters. For regular expressions, we
   // just compare against the regex itself and hope for the best.
void Pattern::PrintFailureInfo(
     const StringMap<StringRef> &VariableTable) const {
   // If this was a regular expression using variables, print the current
   // variable values.
   if (!VariableUses.empty()) {
     for (const auto &VariableUse : VariableUses) {
       SmallString<256> Msg;
       raw_svector_ostream OS(Msg);
       StringRef Var = VariableUse.first;
-      if (Var[0] == '@') {
+      bool TryExpression =
+          (Var.find_first_of(validArithOps) != StringRef::npos);
+      if (Var[0] == '@' || TryExpression) {
         std::string Value;
-        if (EvaluateExpression(Var, Value)) {
+        StringMap<StringRef> VariableTable;
+        StringMap<std::vector<std::pair<StringRef, unsigned>>>
+            LocalVariableTable;
+        if (EvaluateExpression(Var, VariableUse.second, VariableTable,
+                               LocalVariableTable, Value,
+                               false /*CheckOnly*/)) {
           OS << "with expression \"";
           OS.write_escaped(Var) << "\" equal to \"";
           OS.write_escaped(Value) << "\"";
         } else {
           OS << "uses incorrect expression \"";
           OS.write_escaped(Var) << "\"";
         }
       } else {