[FileCheck]: Fix diagnostics for NOT prefixes #78412
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be 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 If you have received no comments on your PR for a week, you can request a review 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. |
|
@llvm/pr-subscribers-testing-tools Author: Vinayak Dev (vinayakdsci) ChangesFixes #70221 Fix a bug in FileCheck that corrects the error message when multiple prefixes are provided Earlier, only the first of the provided prefixes was displayed as the erroneous prefix, while the Now, a variable keeps track of the last NOT prefix, and that prefix is used in the error message. Full diff: https://github.com/llvm/llvm-project/pull/78412.diff 2 Files Affected:
diff --git a/llvm/lib/FileCheck/FileCheck.cpp b/llvm/lib/FileCheck/FileCheck.cpp
index b728c14d288aa5b..526adf3349f099f 100644
--- a/llvm/lib/FileCheck/FileCheck.cpp
+++ b/llvm/lib/FileCheck/FileCheck.cpp
@@ -1814,6 +1814,7 @@ bool FileCheck::readCheckFile(
}
std::vector<Pattern> DagNotMatches = ImplicitNegativeChecks;
+ StringRef TrailingNotPrefix;
// LineNumber keeps track of the line on which CheckPrefix instances are
// found.
@@ -1927,6 +1928,7 @@ bool FileCheck::readCheckFile(
// Handle CHECK-DAG/-NOT.
if (CheckTy == Check::CheckDAG || CheckTy == Check::CheckNot) {
DagNotMatches.push_back(P);
+ TrailingNotPrefix = UsedPrefix;
continue;
}
@@ -1957,11 +1959,11 @@ bool FileCheck::readCheckFile(
}
// Add an EOF pattern for any trailing --implicit-check-not/CHECK-DAG/-NOTs,
- // and use the first prefix as a filler for the error message.
+ // and use the prefix from the last/trailing CHECK-NOT for the error message
if (!DagNotMatches.empty()) {
CheckStrings->emplace_back(
Pattern(Check::CheckEOF, PatternContext.get(), LineNumber + 1),
- *Req.CheckPrefixes.begin(), SMLoc::getFromPointer(Buffer.data()));
+ TrailingNotPrefix, SMLoc::getFromPointer(Buffer.data()));
std::swap(DagNotMatches, CheckStrings->back().DagNotStrings);
}
diff --git a/llvm/test/FileCheck/check-not-custom-prefix-trailing.txt b/llvm/test/FileCheck/check-not-custom-prefix-trailing.txt
new file mode 100644
index 000000000000000..6f9d13538304d6c
--- /dev/null
+++ b/llvm/test/FileCheck/check-not-custom-prefix-trailing.txt
@@ -0,0 +1,12 @@
+; RUN: rm %t && \
+; RUN: echo "LEADING: placeholder1" >>%t && echo "TRAILING-NOT: placeholder2" >>%t && \
+; RUN: %ProtectFileCheckOutput not FileCheck --strict-whitespace --check-prefixes LEADING,TRAILING --input-file %t %t 2>&1 | \
+; RUN: FileCheck %s
+; END
+
+CHECK: error: TRAILING-NOT: excluded string found in input
+CHECK-NEXT: TRAILING-NOT: placeholder2
+CHECK-NEXT: {{^}} ^{{$}}
+CHECK-NEXT: note: found here
+CHECK-NEXT: TRAILING-NOT: placeholder2
+CHECK-NEXT: {{^}} ^~~~~~~~~~~~{{$}}
|
| @@ -0,0 +1,12 @@ | |||
| ; RUN: rm %t && \ | |||
There was a problem hiding this comment.
That fails for me when the file does not already exists. If works if I add a -f
| ; RUN: echo "LEADING: placeholder1" >>%t && echo "TRAILING-NOT: placeholder2" >>%t && \ | ||
| ; RUN: %ProtectFileCheckOutput not FileCheck --strict-whitespace --check-prefixes LEADING,TRAILING --input-file %t %t 2>&1 | \ | ||
| ; RUN: FileCheck %s | ||
| ; END | ||
|
|
||
| CHECK: error: TRAILING-NOT: excluded string found in input | ||
| CHECK-NEXT: TRAILING-NOT: placeholder2 | ||
| CHECK-NEXT: {{^}} ^{{$}} | ||
| CHECK-NEXT: note: found here | ||
| CHECK-NEXT: TRAILING-NOT: placeholder2 | ||
| CHECK-NEXT: {{^}} ^~~~~~~~~~~~{{$}} |
There was a problem hiding this comment.
That does not seem to show the problem as the test pass even without your patch applied.
There was a problem hiding this comment.
Ok, thanks for replying!
I will amend the mistakes immediately.
Thanks
There was a problem hiding this comment.
@RoboTux I have fixed all the errors as you pointed out, and now the test fails without the patch, and passes with it as expected. It turns out that the missing -f flag was causing the issues.
Accordingly I have rebased and pushed the commit.
Thanks!
vinayakdsci
force-pushed
the
fix-prefix-error-file-check-upd
branch
2 times, most recently
from
c9a5dc4
to
239ab4f
Compare
|
That works when the prefix of the last CHECK-NOT correspond to the prefix of the failing trailing CHECK-NOT. It fails in the following case though: and FileCheck is called with --prefixes=LEADING,MIDDLE,TRAILING: it shows TRAILING-NOT even though it's the MIDDLE-NOT that fail. Likewise if we have this instead with --implicit-check-not "placeholder2": placeholder1 placeholder2 LEADING: placeholder1 TRAILING-NOT: placeholder3It would show TRAILING-NOT in the error message even though it's the inplicit check not that failed. |
|
I feel what we need is to change DagNotStrings to be a vector of FileCheckString instead of a vector of Pattern since it's FileCheckString that associate a Prefix, Loc and Pattern. All we need then is to create FileCheckString for CHECK-NOTs and CHECK-DAGs with an empty DagNotStrings for them. Does that make sense? |
|
Unfortunately, that is a part of a bigger problem. In llvm/lib/FileCheck/FileCheck.cpp checkInput() is called from the file of the same name in the llvm/utils/FileCheck/ folder, after the readCheckFile() function is used to populate the CheckStrings vector in the FileCheck struct. The problem is that readCheckFile seems to be designed around using only CHECK as a prefix, and it does not populate CheckStrings for any of the custom NOT prefixes, and doing so makes all the errors disappear, which is wrong behaviour. Thus when CheckStr.Check() is called to check for NOT strings, the prefix passed into the Check function is that of CheckStr, which will never be the one followed by a NOT by design. That is the actual issue, and I am unable to find a workaround. It would take considerable time and code fixing to make the test you mention pass, and I did discover it while fixing this issue. Should I go ahead and dig deeper(I might need help on this, as I am very new to LLVM) ? Thanks! |
That is one if the approaches I tried, but I was hesitant to push it. Should I go ahead? |
It would still require the code in checkInput () to be changed to include custom NOT prefixes, which seems a little tedious, and would require a lot of change! EDIT: I think I will go ahead and try to work in you suggestion, it seems sound. I think I might be able to get it working. |
From a quick look I think it only needs to have an if around the call to CheckStr.Check() to exclude CHECK-NOT and CHECK-DAG. No doubt other places will need to be adapted. Alternatively Check() could bail out on CHECK-NOT and CHECK-DAG. I realize it's a much bigger work than the original patch. You can reach me on discord if you have questions along the way. In any case it'll be a good way to get familiar with FileCheck's codebase. |
|
@RoboTux I am unable to get the code to detect the NOT prefix where an implicit-check-not occurs. Other than that the solution I have arrived seems to be working fine. Would it be advisable if I simply set the prefix to IMPLICIT CHECK-NOT: ...? Otherwise I could leave the existing default behaviour for implicit-check-not, but it would point to the wrong prefix. |
Yes I think in the |
|
Thanks for replying! I will refactor and push the code immediately. Thanks! |
vinayakdsci
force-pushed
the
fix-prefix-error-file-check-upd
branch
2 times, most recently
from
eb5fd0d
to
1e14adc
Compare
vinayakdsci
changed the title
|
@RoboTux I have rebased and pushed the code. Had to push twice because clang-format also auto-formatted CMakeLists.txt. Please do take a look at the code once you find time. It fixes the issues you presented yesterday. I made all attempts to change the original code as little as possible. Also, it was difficult to declare DagNotMatches as a vector of FileCheckString because then DagNotStrings would require a linear time update to extract the patterns(it is declared under FileCheckString too, so it wouldn't be possible to declare DagNotStrings as a vector of FileCheckString!). Thus I created a struct that holds both the Pattern and the Prefix for the DAG/NOT string(another option was to use std::pair<>, but I decided against it). Then, both DagNotStrings, ImplicitCheckNots, and DagNotMatches are redeclared as std::vectorFileCheckString::DagNotPrefixInfo(the name of the struct). I really, really hope this does not break the style guidelines! Thanks! |
There was a problem hiding this comment.
Almost there, it's missing the printMatch() and printNoMatch() changes to use the prefix from DagNotPrefixInfo instead of the CheckString that called Check().
Also can you remove all the codestyle changes on lines not changed by you? You can use git diff -U0 | clang-format-diff.py -i p1 to only format your lines on a patch. clang-format-diff is in clang/tools/clang-format/clang-format-diff.py
llvm/lib/FileCheck/FileCheck.cpp
Outdated
| assert((Pat.getCheckTy() == Check::CheckDAG || | ||
| Pat.getCheckTy() == Check::CheckNot) && | ||
| "Invalid CHECK-DAG or CHECK-NOT!"); | ||
|
|
||
| if (Pat.getCheckTy() == Check::CheckNot) { | ||
| NotStrings.push_back(&Pat); | ||
| NotStrings.emplace_back(Pat, PatItr->DagNotPrefix); |
There was a problem hiding this comment.
Why not make NotString a std::vector<DagNotPrefixInfo *> and just push_back(&*PatItr)? It avoids creating a new instance here as well.
There was a problem hiding this comment.
That really completely escaped my guard. I will keep that in mind, and fix this too.
llvm/lib/FileCheck/FileCheck.cpp
Outdated
| *Req.CheckPrefixes.begin(), SMLoc::getFromPointer(Buffer.data())); | ||
| DagNotMatches.back().DagNotPrefix, | ||
| SMLoc::getFromPointer(Buffer.data())); |
There was a problem hiding this comment.
I think this can remain unchanged now, we won't be using that prefix for the error message once you change printMatch and printNotMatch to handle CheckNot
|
@RoboTux I apologise, but I am a tad bit confused what you mean when you say that I would want to modify printMatch and printNotMatch to handle CheckNot. Do you mean I need to modify their definitions? I could simply pass in the correct prefix through reportMatchResult being called in CheckDag() and CheckNot(); Wouldn't that make it immaterial what Prefix is exactly held at CheckStr.Check(), if the current prefix is a DAG or NOT string? All other suggestions you gave I have already incorporated, and been able to get all tests to pass satisfactorily. Thanks a lot! |
vinayakdsci
force-pushed
the
fix-prefix-error-file-check-upd
branch
from
1e14adc
to
373b570
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vinayakdsci
force-pushed
the
fix-prefix-error-file-check-upd
branch
from
373b570
to
101ac5a
Compare
No my bad, it was me who missed something. It looks good now, thanks! |
|
@vinayakdsci Do you need me to land this for you? |
Thanks a lot!
It would be really great if you would! This would be my first patch to LLVM. Thanks a lot for your patience and guidance! |
I think the commit message need to be rephrased slightly now. How about the following:
|
Done! |
Fixes #70221
Fix a bug in FileCheck that corrects the error message when multiple prefixes are provided
through --check-prefixes and one of them is a PREFIX-NOT.
Earlier, only the first of the provided prefixes was displayed as the erroneous prefix, while the
actual error might be on the prefix that occurred at the end of the prefix list in the input file.
Now, the right NOT prefix is shown in the error message.