[FileCheck] Fix --enable-var-scope for numvars after reassignment #157158

Clipi-12 · 2025-09-05T18:52:20Z

When --enable-var-scope is active, lib/FileCheck.cpp#clearLocalVars gets called.
That function loops through GlobalNumericVariableTable and then calls NumericVariable::clear on most items. It also removes them from GlobalNumericVariableTable.
When reassigning an already cleared variable, Pattern::match calls NumericVariable::setValue, but it doesn't reinsert it into GlobalNumericVariableTable. Therefore, the next time clearLocalVars is called, it won't be able to loop through the variables.

Fix it by reinserting them in GlobalNumericVariableTable inside Pattern::match.

github-actions · 2025-09-05T18:52:41Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-09-05T18:53:14Z

@llvm/pr-subscribers-testing-tools

Author: Clipi (Clipi-12)

Changes

This is my first contribution to LLVM, so I don't have merge perms. English is not my first language.

--enable-var-scope makes it so that lib/FileCheck.cpp#clearLocalVars gets called.
That function loops through GlobalNumericVariableTable and then calls NumericVariable#clear on every item. It also removes it from GlobalNumericVariableTable.
Pattern::match calls NumericVariable#setValue, but it assumes the variable is already in GlobalNumericVariableTable.

Due to this behaviour, if we:

Set a numeric variable.
Clear it with a CHECK-LABEL:.
Reassign it.
[Try to] clear it again with another CHECK-LABEL:.

it won't actually clear it the second time, since to clear it, it needs to be in GlobalNumericVariableTable so that clearLocalVars can loop through it.

This patch simply inserts the numeric variable in GlobalNumericVariableTable right before the NumericVariable#setValue (in Pattern::match) is called.

I edited the var-scope.txt test to account for this patch. I also added --implicit-check-not since we don't expect any other errors than the ones we have tested.

I also changed the --check-prefixes since it was really confusing that both the variables and the prefixes were called the same, plus some prefixes being called LOCAL[1-3] but they had nothing to do with the file-content's "local[1-3]".
I don't know if this last part conflicts with the "be an isolated change" rule, since this is a test-case-legibility-problem and not actual code, but i can remove it and force-push the fork if desired.

Full diff: https://github.com/llvm/llvm-project/pull/157158.diff

2 Files Affected:

(modified) llvm/lib/FileCheck/FileCheck.cpp (+1)
(modified) llvm/test/FileCheck/var-scope.txt (+48-16)

diff --git a/llvm/lib/FileCheck/FileCheck.cpp b/llvm/lib/FileCheck/FileCheck.cpp
index ce35a5bad7616..aed369b0d19a0 100644
--- a/llvm/lib/FileCheck/FileCheck.cpp
+++ b/llvm/lib/FileCheck/FileCheck.cpp
@@ -1218,6 +1218,7 @@ Pattern::MatchResult Pattern::match(StringRef Buffer,
     StringRef MatchedValue = MatchInfo[CaptureParenGroup];
     ExpressionFormat Format = DefinedNumericVariable->getImplicitFormat();
     APInt Value = Format.valueFromStringRepr(MatchedValue, SM);
+    Context->GlobalNumericVariableTable.try_emplace(NumericVariableDef.getKey(), DefinedNumericVariable);
     DefinedNumericVariable->setValue(Value, MatchedValue);
   }
 
diff --git a/llvm/test/FileCheck/var-scope.txt b/llvm/test/FileCheck/var-scope.txt
index 9b3ea0e95d143..b65eddb6d16a0 100644
--- a/llvm/test/FileCheck/var-scope.txt
+++ b/llvm/test/FileCheck/var-scope.txt
@@ -3,15 +3,15 @@
 
 ; Reference run: variables remain defined at all time when not using
 ; --enable-var-scope option.
-RUN: FileCheck --check-prefixes CHECK,LOCAL3,GLOBAL --input-file %s %s
+RUN: FileCheck --check-prefixes CHECK,CHECK-LOCAL-BOTH,CHECK-GLOBAL --input-file %s %s
 
-RUN: FileCheck --check-prefixes CHECK,GLOBAL --enable-var-scope --input-file %s %s
-RUN: %ProtectFileCheckOutput not FileCheck --check-prefixes CHECK,LOCAL1 --enable-var-scope --input-file %s %s 2>&1 \
-RUN:   | FileCheck --check-prefix ERRUNDEFLOCAL %s
-RUN: %ProtectFileCheckOutput not FileCheck --check-prefixes CHECK,LOCAL2 --enable-var-scope --input-file %s %s 2>&1 \
-RUN:   | FileCheck --check-prefix ERRUNDEFLOCNUM %s
-RUN: %ProtectFileCheckOutput not FileCheck --check-prefixes CHECK,LOCAL3 --enable-var-scope --input-file %s %s 2>&1 \
-RUN:   | FileCheck --check-prefixes ERRUNDEFLOCAL,ERRUNDEFLOCNUM %s
+RUN: FileCheck --check-prefixes CHECK,CHECK-GLOBAL --enable-var-scope --input-file %s %s
+RUN: %ProtectFileCheckOutput not FileCheck --check-prefixes CHECK,CHECK-LOCAL-TEXT --enable-var-scope --input-file %s %s 2>&1 \
+RUN:   | FileCheck --implicit-check-not "undefined variable:" --check-prefixes ERRUNDEF,ERRUNDEF-LOCAL %s
+RUN: %ProtectFileCheckOutput not FileCheck --check-prefixes CHECK,CHECK-LOCAL-NUM --enable-var-scope --input-file %s %s 2>&1 \
+RUN:   | FileCheck --implicit-check-not "undefined variable:" --check-prefixes ERRUNDEF,ERRUNDEF-LOCNUM %s
+RUN: %ProtectFileCheckOutput not FileCheck --check-prefixes CHECK,CHECK-LOCAL-BOTH --enable-var-scope --input-file %s %s 2>&1 \
+RUN:   | FileCheck --implicit-check-not "undefined variable:" --check-prefixes ERRUNDEF,ERRUNDEF-LOCAL,ERRUNDEF-LOCNUM %s
 
 local1
 global1
@@ -23,15 +23,47 @@ global2
 CHECK: [[LOCAL]][[#LOCNUM+1]]
 CHECK: [[$GLOBAL]][[#$GLOBNUM+1]]
 
-barrier:
-CHECK-LABEL: barrier
+// Barrier to clear local variables
+barrier1:
+CHECK-LABEL: barrier1
 
 local3
 global3
-LOCAL1: [[LOCAL]]3
-LOCAL2: local[[#LOCNUM+2]]
-LOCAL3: [[LOCAL]][[#LOCNUM+2]]
-GLOBAL: [[$GLOBAL]][[#$GLOBNUM+2]]
+CHECK-LOCAL-TEXT: [[LOCAL]]3
+CHECK-LOCAL-NUM: local[[#LOCNUM+2]]
+CHECK-LOCAL-BOTH: [[LOCAL]][[#LOCNUM+2]]
+CHECK-GLOBAL: [[$GLOBAL]][[#$GLOBNUM+2]]
 
-ERRUNDEFLOCAL: undefined variable: LOCAL
-ERRUNDEFLOCNUM: undefined variable: LOCNUM
+// Barrier to continue FileCheck execution even after the first fail
+barrier2:
+CHECK-LABEL: barrier2
+
+// Reassign the variables to check that clearing-after-reassigning works
+local4
+global4
+CHECK: [[LOCAL:loc[^[:digit:]]*]][[#LOCNUM:]]
+CHECK: [[$GLOBAL:glo[^[:digit:]]*]][[#$GLOBNUM:]]
+
+// Barrier to clear local variables
+barrier3:
+CHECK-LABEL: barrier3
+
+local5
+global5
+CHECK-LOCAL-TEXT: [[LOCAL]]5
+CHECK-LOCAL-NUM: local[[#LOCNUM+1]]
+CHECK-LOCAL-BOTH: [[LOCAL]][[#LOCNUM+1]]
+CHECK-GLOBAL: [[$GLOBAL]][[#$GLOBNUM+1]]
+
+
+// Check that the tests fail as expected
+ERRUNDEF-LOCAL: undefined variable: LOCAL
+ERRUNDEF-LOCNUM: undefined variable: LOCNUM
+ERRUNDEF-LOCAL: undefined variable: LOCAL
+ERRUNDEF-LOCNUM: undefined variable: LOCNUM
+
+// Look for "Input was:" to only match the error messages before the input-context.
+//
+// The regex /([[:space:]]|.)*/ matches all remaining characters,
+// to avoid fails due to --implicit-check-not
+ERRUNDEF: {{^Input was:([[:space:]]|.)*}}

Clipi-12 · 2025-09-06T10:35:18Z

@boomanaiden154 pinging you since i saw you have lots of PRs with the testing-tools label. I didn't know exactly who to ping since there is no test/Maintainers.md or something like that.

jh7370 · 2025-09-08T08:30:44Z

@Clipi-12, I've added a couple of people as reviewers who have done a lot of FileCheck stuff. In general, the best way to find possible reviewers is to look at past changes in the same area and ping the contributor and reviewer(s) for the previous patches.

I haven't looked at the change itself, but a couple of things to note:

By default in LLVM, the PR description and title is used as the commit description and title. You may want to remove the first bit of your description (about this being your first commit etc) and move it into a PR comment for posterity's sake.
You need to have a public GitHub email address to have commits merged into LLVM. See https://llvm.org/docs/DeveloperPolicy.html#email-addresses.

github-actions · 2025-09-08T08:31:57Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

github-actions · 2025-09-08T08:33:49Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Clipi-12 · 2025-09-08T14:52:41Z

Original PR description as per the last comment:

This is my first contribution to LLVM, so I don't have merge perms. English is not my first language.

--enable-var-scope makes it so that lib/FileCheck.cpp#clearLocalVars gets called.

That function loops through GlobalNumericVariableTable and then calls NumericVariable#clear on every item. It also removes it from GlobalNumericVariableTable.

Pattern::match calls NumericVariable#setValue, but it assumes the variable is already in GlobalNumericVariableTable.

Due to this behaviour, if we:

Set a numeric variable.

Clear it with a CHECK-LABEL:.

Reassign it.

[Try to] clear it again with another CHECK-LABEL:.

it won't actually clear it the second time, since to clear it, it needs to be in GlobalNumericVariableTable so that clearLocalVars can loop through it.

This patch simply inserts the numeric variable in GlobalNumericVariableTable right before the NumericVariable#setValue (in Pattern::match) is called.

I edited the var-scope.txt test to account for this patch. I also added --implicit-check-not since we don't expect any other errors than the ones we have tested.

I also changed the --check-prefixes since it was really confusing that both the variables and the prefixes were called the same, plus some prefixes being called LOCAL[1-3] but they had nothing to do with the file-content's "local[1-3]".
I don't know if this last part conflicts with the "be an isolated change" rule, since this is a test-case-legibility-problem and not actual code, but i can remove it and force-push the fork if desired.

Clipi-12 · 2025-09-08T15:20:54Z

I would have swear I had executed git-clang-format, weird...
I've also changed the email.
Let me know if there is any other problem and I'll try my best to fix it.

llvm/lib/FileCheck/FileCheck.cpp

RoboTux · 2025-09-09T14:07:18Z

I think the commit message should be rewritten because the real problem is that clearing of a local variable rely on the variable being in the global numeric table and thus a second clearning after a redefinition only works if the redefinition add it back to the global numeric variable table. Could you rephrase it accordingly? LGTM otherwise, thanks for your reply.

llvm/lib/FileCheck/FileCheck.cpp

Clipi-12 · 2025-09-09T16:11:03Z

Added the comment and rephrased the PR details to that it's (hopefully) more descriptive.
(Remainder for the next review that I don't have perms, and so I cannot merge)

RoboTux

LGTM

RoboTux · 2025-09-09T18:46:29Z

Can you merge the latest main? Github complains that the branch is out-of-date with the base branch. Thanks.

Clipi-12 · 2025-09-09T18:50:54Z

ugh, the update button does a merge, not a rebase
give me a second

Clipi-12 · 2025-09-09T18:58:27Z

ok done @RoboTux

RoboTux · 2025-09-09T19:12:00Z

Thanks, I'll wait for CI to pass then merge it, unless someone object in the meantime

github-actions · 2025-09-09T21:28:19Z

@Clipi-12 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Clipi-12 · 2025-09-09T21:34:04Z

Wait, I thought merge commits were not allowed. Does the GitHub bot automatically sync the changes to make a linear git-history? If so, why do we need to update the branch / avoid that it goes out-of-date with the base branch?

Is there any docs about this?

(btw thanks for merging!)

jh7370 · 2025-09-10T09:47:56Z

Wait, I thought merge commits were not allowed. Does the GitHub bot automatically sync the changes to make a linear git-history? If so, why do we need to update the branch / avoid that it goes out-of-date with the base branch?

Is there any docs about this?

(btw thanks for merging!)

The default setting is "Sqaush & Merge", which squashes the commits together and rebases them on the tip of main. This has the effect of removing all merge commits from main. In other words, doing a merge from main is the correct way of rebasing within a PR, since it avoids messing up any in progress review.

llvmbot added the testing-tools label Sep 5, 2025

jh7370 requested review from RoboTux and jdenny-ornl September 8, 2025 08:25

Clipi-12 force-pushed the main branch from 9e8206c to 8bb9886 Compare September 8, 2025 15:18

RoboTux reviewed Sep 9, 2025

View reviewed changes

llvm/lib/FileCheck/FileCheck.cpp Show resolved Hide resolved

RoboTux reviewed Sep 9, 2025

View reviewed changes

llvm/lib/FileCheck/FileCheck.cpp Show resolved Hide resolved

llvm/lib/FileCheck/FileCheck.cpp Show resolved Hide resolved

Clipi-12 force-pushed the main branch from 8bb9886 to 8b826ac Compare September 9, 2025 16:05

RoboTux approved these changes Sep 9, 2025

View reviewed changes

Clipi-12 force-pushed the main branch from 8de1ecc to 8b826ac Compare September 9, 2025 18:56

[FileCheck] Fix --enable-var-scope for numvars after reassignment

0cf5fe3

Clipi-12 force-pushed the main branch from 8b826ac to 0cf5fe3 Compare September 9, 2025 18:58

Merge branch 'main' into main

7e9f925

RoboTux merged commit d007099 into llvm:main Sep 9, 2025
9 checks passed

[FileCheck] Fix --enable-var-scope for numvars after reassignment #157158

[FileCheck] Fix --enable-var-scope for numvars after reassignment #157158

Uh oh!

Conversation

Clipi-12 commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 5, 2025

Uh oh!

llvmbot commented Sep 5, 2025

Uh oh!

Clipi-12 commented Sep 6, 2025

Uh oh!

jh7370 commented Sep 8, 2025

Uh oh!

github-actions bot commented Sep 8, 2025

Uh oh!

github-actions bot commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Clipi-12 commented Sep 8, 2025

Uh oh!

Clipi-12 commented Sep 8, 2025

Uh oh!

Uh oh!

RoboTux commented Sep 9, 2025

Uh oh!

Uh oh!

Uh oh!

Clipi-12 commented Sep 9, 2025

Uh oh!

RoboTux left a comment

Choose a reason for hiding this comment

Uh oh!

RoboTux commented Sep 9, 2025

Uh oh!

Clipi-12 commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Clipi-12 commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RoboTux commented Sep 9, 2025

Uh oh!

Uh oh!

github-actions bot commented Sep 9, 2025

Uh oh!

Clipi-12 commented Sep 9, 2025

Uh oh!

jh7370 commented Sep 10, 2025

Uh oh!

Uh oh!

Clipi-12 commented Sep 5, 2025 •

edited

Loading

github-actions bot commented Sep 8, 2025 •

edited

Loading

Clipi-12 commented Sep 9, 2025 •

edited

Loading

Clipi-12 commented Sep 9, 2025 •

edited

Loading