Details
Diff Detail
Event Timeline
This looks like a move in the right direction to me, but I'm biased. Hopefully others will chime in, especially individual test suite maintainers.
Here are some minor style suggestions, but feel free to outright ignore them if I'm the only one who cares:
- Sometimes you add COM: only to lines your diagnostic might catch and not to other lines in the same comment block. I'd prefer to see it on every line in a comment block.
- Where ;; was used to differentiate true comments from FileCheck check directives, you now have ;; COM:. I think ; COM: is sufficient.
Thanks for having a look!
Hopefully others will chime in, especially individual test suite maintainers.
Here are some minor style suggestions, but feel free to outright ignore them if I'm the only one who cares:
- Sometimes you add COM: only to lines your diagnostic might catch and not to other lines in the same comment block. I'd prefer to see it on every line in a comment block.
I was aiming for the latter, though it looks like the style I ended up with for all of the "here's what each of the CHECKs mean" descriptions having their first comment without the COM: directive. Happy to make those consistent with the rest.
- Where ;; was used to differentiate true comments from FileCheck check directives, you now have ;; COM:. I think ; COM: is sufficient.
I waffled about that, and even considered adding ;; as a comment directive in those files. Likewise for ##. Seems not worth it to add more flavors of COM: just for that, and I could go either way on ;; COM: vs ; COM:. Happy to update those too.
Thanks for working on all this.
Hopefully others will chime in, especially individual test suite maintainers.
Here are some minor style suggestions, but feel free to outright ignore them if I'm the only one who cares:
- Sometimes you add COM: only to lines your diagnostic might catch and not to other lines in the same comment block. I'd prefer to see it on every line in a comment block.
I was aiming for the latter, though it looks like the style I ended up with for all of the "here's what each of the CHECKs mean" descriptions having their first comment without the COM: directive. Happy to make those consistent with the rest.
Thanks.
- Where ;; was used to differentiate true comments from FileCheck check directives, you now have ;; COM:. I think ; COM: is sufficient.
I waffled about that, and even considered adding ;; as a comment directive in those files. Likewise for ##. Seems not worth it to add more flavors of COM: just for that,
I strongly agree that we should not go down that path. I feel like it's just asking for new dimensions of silent FileCheck prefix mistakes. Let's not deviate from COM: except as a last resort.
and I could go either way on ;; COM: vs ; COM:. Happy to update those too.
That's my vote, but I'll leave it up to you and anyone else who might comment. It's a minor point.
I saw the benefit initially when the proposal was made, but now seeing the consequence in practice in the tools tests at least makes me realise I'm not a fan of the change to make FileCheck error if it sees the string somewhere else in the line. In particular, GNU and LLVM are common prefixes used to distinguish checks for different output modes, but also will often appear in comments to explain the test cases. Similar comments apply to the machine names etc. Needing to add COM is unobvious, and makes the comments messier to read, I found, as soon as I started reading. I think people will repeatedly trip over this when they are writing tests, both new-comers and existing users, because it's not a requirement to use COM in most situations. I think this will mostly lead to annoyance and frustration, and consequently, less of a desire to do any of 1) write comments, 2) write tests, or 3) contribute to LLVM (due to too many hurdles to understand), which in turn will impact the quality of our codebase.
Let’s not lose sight of the fact that this whole change is a trade off: we are attempting to trade comment succinctness/obviousness for the ability to diagnose a particularly nasty failure mode of missing colons. This comment thing may not be the right tradeoff, but given how constrained the problem space is, I suspect we will end up sacrificing a bit of intuitiveness somewhere regardless. So long as these new diagnosed-but-unintuitive cases aren’t too frequent/annoying, IMO that’s a better place to be than having room for the other not-diagnosable-and-unintuitive failure mode.
I don’t have an exact number but since I started this investigation a couple of months ago, a small handful of cases of it have been added to the test suite... this is an ongoing problem.
I think people will repeatedly trip over this when they are writing tests,
It may help make things more obvious if the missing colon diagnostic mentions how to work around the issue.
both new-comers and existing users, because it's not a requirement to use COM in most situations. I think this will mostly lead to annoyance and frustration, and consequently, less of a desire to do any of 1) write comments, 2) write tests, or 3) contribute to LLVM (due to too many hurdles to understand), which in turn will impact the quality of our codebase.
Something we’re a bit too early to see, which I expect we might, is people changing their habits w.r.t. choosing check prefixes & how they refer to them in comments.
; COM: seems non-obvious and harms readability. The inconvenience may discourage contributors from writing comments.
For file types using ; or # as line comment markers, won't ;; or ## be clearer?
Personally I use ;; and ## for new tests all the time since I learned the practice, however, I would be hesitant to enforce this because others' tendency may be different.
IMHO, any comment style will become obvious once you're used to it, and I don't see the readability issue being raised here. Of course, those issues are highly subjective.
I saw the ;; and ## suggestion before I prototyped COM:, and here are some (perhaps more objective) reasons why I prototyped COM: instead:
- For those new to FileCheck or maybe just to these issues, I doubt ;; and ## look like they should have any relevance to FileCheck (or lit in the future). COM: looks like a FileCheck directive. As for any FileCheck directive, if you've never seen it, check the docs to find out what it does.
- ;; and ## varies with file type. Must we configure something different for every file type, or will we enable all of them for all files? Will there be a /// and @@? Any other styles to learn? COM: shouldn't need to vary.
- If you don't know that ;;, ##, ///, @@, etc. are special, you might happen to like such a comment style for some purpose and then accidentally suppress a FileCheck directive. Quietly ignored FileCheck directives are exactly what this proposed diagnostic and several others in the past have been trying to combat. I have trouble imagining anyone accidentally inserting COM: in front of a directive and thinking the latter directive will continue to function. It then looks like COM: becomes the directive.
- Someone in a previous thread pointed out that ## is already a comment style in some assemblers. I don't know if we'll need that for LLVM testing, but will there every be another comment style we cannot support in this manner because such an expansion of an existing comment style collides with it?
If people are really opposed to COM:, perhaps we should take this discussion to the mailing list again.
I've been pondering this readability objection. Putting the other issues (like obviousness) aside for a moment, would anyone find COM: easier to read in lowercase? That would make it a little more distinct from the uppercase letter that begins a sentence in a comment, and it would make it more distinct from typical FileCheck/LIT directives. For example, from llvm/test/MC/RISCV/rv64b-aliases-valid.s:
# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+experimental-b < %s \ # RUN: | llvm-objdump -d -r --mattr=+experimental-b - \ # RUN: | FileCheck -check-prefixes=CHECK-S-OBJ %s # com: The following check prefixes are used in this test: # com: CHECK-S-OBJ Match both the .s and objdumped object output with # com: aliases enabled # com: CHECK-S-OBJ-NOALIAS Match both the .s and objdumped object output with # com: aliases disabled # CHECK-S-OBJ-NOALIAS: andi t0, t1, 255 # CHECK-S-OBJ: zext.b t0, t1 zext.b x5, x6
com: might not be the best choice because it occurs so often in, for example, git URLs. However, if lowercase helps, we could consider rem: or something similar.
The first patch seemed fine and pretty useful. I'm not certain that this is a particularly fun idea as far as having to redo all comments etc. Can you elaborate more on how much you've seen this happen etc?
This patch addresses all of the cases where a CHECK prefix is used in a comment (but without a trailing colon), so aside from new tests, we won't need to rewrite any more comments to use this COM: prefix (or however it ends up being spelled).
Can you elaborate more on how much you've seen this happen etc?
See D77352 and D77354 (and several other follow-up patches) should make it clear the problem is quite pervasive, and ongoing.
- Almost all of the ones in D77352 were benign.
- All of the ones in D77354 were legit test case bugs. These are cases where the test hadn't been updated, was broken when the colon gets added, but the underlying codegen was more or less good.
- That said, I haven't found any that pointed at real codegen bugs... yet.
Why was this landed? There was varying degrees of dislike from more than one reviewer (myself, and @MaskRay at least). Please revert pending a conclusion to the discussion. This has hardly reached consensus.
5a8db275f8fc8ee19b184f831bda1cdfc6771776
Sorry, misread the tone of the discussion as having been addressed by @jdenny & my follow-up comments.
No worries. Text is hard to communicate with! I think the discussion needs re-raising on llvm-dev, using this as an example of the consequence of the new FileCheck diagnostic.
Here's what I'm imagining the other patch will do to alleviate this:
llvm/test/CodeGen/Mips/cconv/arguments.ll:67:3: error: colon required after check directive 'O32' ; O32 starts loading from the stack. The addresses start at 16 because space is ^ llvm/test/CodeGen/Mips/cconv/arguments.ll:67:3: note: to silence this diagnostic, prefix the line with a COM: directive ; O32 starts loading from the stack. The addresses start at 16 because space is ^
Does that address your un-obviousness concerns, @jhenderson?
Also, how does everyone feel about the relative weight of these two cases of un-obviousness:
- Diagnosed syntactic errors for things that were previously ok (i.e. the note: above)
- Undiagnosed syntactic mis-use (i.e. the missing colon bug)
IMO this tradeoff is a net improvement, especially given the volume of (2) I've continuously been fixing.
Thank James for weighing in:) And thanks for the revert. Here are my thoughts:
If a check directive is preceded by another word (e.g. please CHECK) there are plenty of tests in this diff), my preference is not to diagnose the potential problem (the false positive rate is very high).
If a check directive is preceded by a predefined punctuation set, it may be a Gotcha A described in "[RFC] Improving FileCheck". Diagnose. Some of touched tests are related.
COM: can suppress the diagnostic, but personally I prefer repeating the punctuation, i.e. ## CHECK is a comment (assembly,yaml,etc) or ;; CHECK is a comment (.ll) or /// CHECK (BCPL/C99/C++)
There are multi-byte comment markers e.g. /*. It may not be clear how we should suppress such diagnostics. My thought is that these are probably too few to worry about for the initial implementation.
(I recall that @probinson has a comprehensive list of the predefined punctuation set. Starting with # ; // initially can probably cover most of misuses. We can extend the set on demand in the future.)
This preferred commenting approach indeed requires a separate llvm-dev discussion.
| llvm/test/CodeGen/Mips/cconv/arguments-varargs.ll | ||
|---|---|---|
| 145 | Is COM; intentional? | |
The error message improvement will help somewhat, but it doesn't solve the initial problem of "I wrote my comment in the same style as all the surrounding tests, it is easy to read in its current form, but not FileCheck wants me to change it to something that doesn't read as well (because it starts looking like a FileCheck/lit directive which has some meaning)."
I'm similar in thinking to @MaskRay, I think. Whilst I absolutely agree that missing colons are an issue and that something should be done, I think we just need to be a bit more refined with the approach. I think the absolutely vast majority of FileCheck CHECKs are either at the start of the line (with possibly some whitespace before), or preceded by some form of comment marker. How about we make the check only pick up cases where one of these two criteria are met? To make things even simpler, I'd define "comment marker" as any non-ASCII alphanumeric, non-whitespace character. The following would be the result:
CHECK (would be diagnosed)
# CHECK (would be diagnosed)
# a CHECK (would not be diagnosed)
## CHECK (would be diagnosed - allowed to make life simpler for other comment sequences like '//')
# COM: CHECK (would not be diagnosed)
some text # CHECK (would be diagnosed)
@#/*&% CHECK (would be diagnosed)This will still cause a small number of false positives (e.g. the three llvm-readobj tests would all generate them, I believe), but it would be easier to reorganise the comment to not trip over it without making it look "ugly" and inconsistent with other comments elsewhere. I guess there might be some false negatives, but I doubt there'd be all that many at all?
I'm similar in thinking to @MaskRay, I think. Whilst I absolutely agree that missing colons are an issue and that something should be done, I think we just need to be a bit more refined with the approach. I think the absolutely vast majority of FileCheck CHECKs are either at the start of the line (with possibly some whitespace before), or preceded by some form of comment marker. How about we make the check only pick up cases where one of these two criteria are met? To make things even simpler, I'd define "comment marker" as any non-ASCII alphanumeric, non-whitespace character. The following would be the result:
CHECK (would be diagnosed) # CHECK (would be diagnosed) # a CHECK (would not be diagnosed) ## CHECK (would be diagnosed - allowed to make life simpler for other comment sequences like '//') # COM: CHECK (would not be diagnosed) some text # CHECK (would be diagnosed) @#/*&% CHECK (would be diagnosed)This will still cause a small number of false positives (e.g. the three llvm-readobj tests would all generate them, I believe), but it would be easier to reorganise the comment to not trip over it without making it look "ugly" and inconsistent with other comments elsewhere. I guess there might be some false negatives, but I doubt there'd be all that many at all?
I think this will work for me. It'll make it harder to temporarily disable a particular subset of a test (at least I use ## for that), but this seems more reasonable. Probably needs to be documented in the developer manual as well around writing testcases.
Yeah, also pretty bad is getting caught myself with a COM; in this very patch. Gosh this is hard ;)
I'm similar in thinking to @MaskRay, I think. Whilst I absolutely agree that missing colons are an issue and that something should be done, I think we just need to be a bit more refined with the approach. I think the absolutely vast majority of FileCheck CHECKs are either at the start of the line (with possibly some whitespace before), or preceded by some form of comment marker. How about we make the check only pick up cases where one of these two criteria are met? To make things even simpler, I'd define "comment marker" as any non-ASCII alphanumeric, non-whitespace character. The following would be the result:
CHECK (would be diagnosed) # CHECK (would be diagnosed) # a CHECK (would not be diagnosed) ## CHECK (would be diagnosed - allowed to make life simpler for other comment sequences like '//') # COM: CHECK (would not be diagnosed) some text # CHECK (would be diagnosed) @#/*&% CHECK (would be diagnosed)This will still cause a small number of false positives (e.g. the three llvm-readobj tests would all generate them, I believe), but it would be easier to reorganise the comment to not trip over it without making it look "ugly" and inconsistent with other comments elsewhere. I guess there might be some false negatives, but I doubt there'd be all that many at all?
I like that too. I'll prototype this, and see if I can get some concrete numbers on the change in diagnosed cases. Also promise to summarize on llvm-dev (haven't sparked that discussion back up yet since I've been quite busy, but I definitely see the importance / value of it).
After some experimenting, this seems like a pretty decent filter:
static bool shouldDiagnoseMissingColon(StringRef LineBegin) {
StringRef Preamble = LineBegin.rtrim(" \t");
if (Preamble.size() == 0)
return true;
// C++ style line comments.
if (Preamble.endswith("//"))
return true;
// C style block comments.
if (Preamble.endswith("/*"))
return true;
char Ending = Preamble.back();
// Other forms of line comments.
if (StringRef("#;*!@&%").count(Ending))
return true;
return false;
}In the llvm/test suite, this leaves ~46 tests that trigger on things that are clearly comments about the CHECK directives themselves:
e.g:
llvm/test/CodeGen/Mips/cconv/arguments.ll:67
A few more cases of missing colons I hadn't seen before (I have a patch to fix these, haven't pushed it yet):
llvm/test/MC/ARM/ldr-pseudo-cond-darwin.s llvm/test/MC/ARM/ldr-pseudo-cond.s llvm/test/tools/dsymutil/ARM/scattered.c llvm/test/tools/dsymutil/fat-binary-output.test llvm/test/tools/llvm-objdump/ELF/ARM/v5te-subarch.s llvm/test/tools/llvm-readobj/COFF/codeview-linetables.test llvm/test/tools/llvm-symbolizer/flag-grouping.test
And some new false positives on mips tests that use things like --check-prefix=16 where the diagnostic fires on the llvm value with the same name %16 = ... [3].
llvm/test/CodeGen/Mips/selgt.ll llvm/test/CodeGen/Mips/hf16_1.ll
I'm warming up to the idea of using ## as the way to silence the diagnostic, since it's less invasive than COM:. That makes a lot of sense in source languages where # is already the comment character, but what about elsewhere? Should we do @ ## in assembler, and // ## in C, for example?
I haven't used these other languages in tests much in the past, but I think in one or two cases, I've just duplicated the comment pattern (e.g. @@ or ////). I'm not so keen on the C++-style comments duplication (too many characters), but I followed that for consistency with what I've done with ##. The problem with that approach of course is that you'd have to somehow figure out the pattern to treat as a comment for any given file, which doesn't seem particularly fun (especially when some files can have more than one variety - .s assembly I believe can be any of ;, // or # at least, for example. An option would be to just allow all the patterns, if doubled, regardless of file type. There's probably not much risk of false negatives with that, but I don't know for sure.
I haven't seen any CHECKs with missing colons where the comment marker was doubled up. I'd have to verify, but I also suspect that there are very few if any existing CHECK:s with doubled up comment markers.
This is basically my feeling (https://reviews.llvm.org/D79963#2051242). ;; CHECK @@ CHECK ## CHECK /// CHECK => the false positive rate will be pretty low.
FileCheck does not need to understand the file format (assembly, textual format LLVM IR, BCPL, ...). As long as any of @@ ## /// (maybe others) is seen, suppress the diagnostic.
Repeating the comment marker has been very common in LLVM binary utilities and LLD, and used somewhere in MC. It is much less common in CodeGen/Transforms/etc, but hope it is easier for people to accept than ; COM:
Is COM; intentional?