This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Implement --ignore-case option.
ClosedPublic

Authored by Kai on Sep 27 2019, 10:30 AM.

Diff Detail

Event Timeline

Kai created this revision.Sep 27 2019, 10:30 AM

Oh, this is painful.
I'm curious - what do you think about some other options, e.g. about extending FileCheck to add support for case insensitiveness,
but maybe smb else might have a better idea

In D68146#1686536, @alexshap wrote:

Oh, this is painful.
I'm curious - what do you think about some other options, e.g. about extending FileCheck to add support for case insensitiveness,
but maybe smb else might have a better idea

I agree it's painful, and I think a FileCheck option is ultimately the best solution, but as an interim non-terrible solution, how about using tr to convert the od output from upper-case to lower-case:

# RUN: od -t x1 %t | tr '[:upper:]' '[:lower:]' | FileCheck ...

You can also use tr a-z A-Z in theory, but that relies on posix conformity of the od tool, which from experience I don't think we can rely on (e.g. I know that at least one version of od used on the Greendragon buildbots that doesn't allow command-lines to be ordered in somewhat arbitrary ways). I think therefore the character classes might be more reliable.

The command od -t x is used to dump data in hex format. The LIT tests assumes that the hex characters are in lowercase. However, there are also platforms which use uppercase letter. The test cases are changed to accept both formats.

Can you just name the platform? Is it AIX? Please give some examples of its output. IIRC it does not support -t x4.

Please also add -A n to the od command line. POSIX says "The format of the input offset is unspecified."

I think updating FileCheck to allow case-insensitive matchers is a good way to fix this. It may be tricky but it's the right thing to do.

Kai added a comment.Oct 1 2019, 12:49 AM

Yeah, I agree that this is painful. I'll have a look at FileCheck because this really feels like the right solution.
I am not on AIX but z/OS USS.

Kai updated this revision to Diff 222817.Oct 2 2019, 6:27 AM

FileCheck has now a new option --ignore-case.
I updated the test cases to use the new option. I touched only test cases which have the lower case/upper case letter problem. I also did not add -A n because some tests also check for the address.

thopre added a comment.Oct 2 2019, 6:31 AM
In D68146#1691237, @Kai wrote:

FileCheck has now a new option --ignore-case.
I updated the test cases to use the new option. I touched only test cases which have the lower case/upper case letter problem. I also did not add -A n because some tests also check for the address.

Can you split the FileCheck change into a separate revision? Please add some testing (both unittest and regular test if possible).

Thanks for adding the new option --ignore-case. This will be very useful, though it is not clear from the option name that it uses a lowercase pattern:

size_t Pos = IgnoreCase ? Buffer.find_lower(FixedStr)
                        : Buffer.find(FixedStr);

To make it really --ignore-case, the pattern should also be changed to lowercase. It may be worth splitting the patch into two

  1. [FileCheck] Add --ignore-case ... and add tests to test/FileCheck/
  2. Fix od tests with the new FileCheck option.
llvm/lib/Support/FileCheckImpl.h
432

bool IgnoreCase = false; and delete the member initializer below.

I'm pleasantly surprised by how simple that FileCheck change is! You also need to update the FileCheck documentation (see llvm/docs/CommandGuide/FileCheck.rst) for the new option.

Seems like a good idea to me!

Kai updated this revision to Diff 224017.Oct 9 2019, 4:21 AM
Kai retitled this revision from [tests] Output of od can be lower or upper case (llvm-objcopy/yaml2obj). to [FileCheck] Implement --ignore-case option..
Kai edited the summary of this revision. (Show Details)

Added a test case and documentation for the --ignore-case option.
Removed the changed test cases.

Kai added a comment.Oct 9 2019, 4:26 AM

To make it really --ignore-case, the pattern should also be changed to lowercase.

I am not sure which pattern I have missed. The fixed string match uses the lowercase find and the regex match uses the Regex::IgnoreCase flag.

thopre added inline comments.Oct 9 2019, 5:25 AM
llvm/test/FileCheck/check-ignore-case.txt
18

s/5/6/ ?

Kai updated this revision to Diff 224037.Oct 9 2019, 6:18 AM

Change single digit in test case to form a sequence.

Kai marked 2 inline comments as done.Oct 9 2019, 6:19 AM
thopre added a comment.Oct 9 2019, 6:25 AM

LGTM but I'll let others comment.

LGTM

llvm/test/FileCheck/check-ignore-case.txt
23

newline?

Kai updated this revision to Diff 224042.Oct 9 2019, 6:49 AM

Added newline at end of test case.

Kai marked an inline comment as done.Oct 9 2019, 6:49 AM
jhenderson added inline comments.Oct 9 2019, 9:21 AM
llvm/test/FileCheck/check-ignore-case.txt
46

Nit: no new line at EOF.

Kai marked an inline comment as done.Oct 9 2019, 9:33 AM
Kai added inline comments.
llvm/test/FileCheck/check-ignore-case.txt
46

When I download the raw file and look at it in hex mode, then the last byte is 0x0A. That's a new line at EOF, isn't it?

Mostly LGTM, just a couple test nits. Thanks for fixing FileCheck!

In D68146#1701152, @Kai wrote:

To make it really --ignore-case, the pattern should also be changed to lowercase.

I am not sure which pattern I have missed. The fixed string match uses the lowercase find and the regex match uses the Regex::IgnoreCase flag.

StringRef::find_lower() is a bad name for something that should really be called StringRef::find_case_insensitive(), which I think is the confusion here.

llvm/test/FileCheck/check-ignore-case.txt
2

Can you split up these lines with descriptions of what each one does? e.g.

## Check that ...
# RUN: FileCheck ...

## Check that other thing ...
# RUN: FileCheck ...
6

Can you add a check that case is ignored for -implicit-check-not? e.g. not FileCheck -ignore-case -implicit-check-not=sTrinG

Kai updated this revision to Diff 224281.Oct 10 2019, 2:03 AM

Added a test for CHECK-SAME and for -implicit-check-not.

Kai marked 3 inline comments as done.Oct 10 2019, 2:04 AM
MaskRay added inline comments.Oct 10 2019, 2:35 AM
llvm/utils/FileCheck/FileCheck.cpp
53

Use case-insensitive matching? (To be consistent with the doc).

Kai updated this revision to Diff 224293.Oct 10 2019, 2:52 AM

Changed description of option.

Kai marked an inline comment as done.Oct 10 2019, 2:52 AM
MaskRay added inline comments.Oct 10 2019, 3:04 AM
llvm/test/FileCheck/check-ignore-case.txt
2

independent of case. Or, how about case insensitively?

Please also add full stops in these comments.

6

-input-file=%s makes it clear -input-file accepts an argument.

I think @jhenderson may prefer --input-file=%s. Binary utilities (almost) exclusively use double-dashed forms now.

One bad thing with -input-file is that it is incompatible with POSIX Utility Syntax Guideline 5 (grouped options). Though FileCheck is not a POSIX utility and it does not necessarily abide by the rules...

18

sed /^#/d

44

Consider using a regex for 22.

Kai updated this revision to Diff 224305.Oct 10 2019, 4:13 AM

Updated test case:

  • changed description
  • options now use double dash
  • replaced sed expression
  • added a regex to match column
Kai marked 4 inline comments as done.Oct 10 2019, 4:14 AM
MaskRay accepted this revision.Oct 10 2019, 4:35 AM

LGTM, but please wait for another reviewer to take a final look.

This revision is now accepted and ready to land.Oct 10 2019, 4:35 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 10 2019, 6:27 AM

The test fails on Linux: http://lab.llvm.org:8011/builders/clang-cmake-x86_64-sde-avx512-linux/builds/28537/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Acheck-ignore-case.txt If it takes a while to fix please revert while you investigate.

Also, https://github.com/llvm/llvm-project/commit/dfd2b6f07fc40a190335f580d8a965bbebfe94df looks like you touched ~all lines in docs/CommandGuide/FileCheck.rst and llvm/include/llvm/Support/FileCheck.h Maybe you converted them to windows line endings? If so, please undo that. (Maybe revert and reland with fixed line endings so that the diff for the actual change is readable.)

This change broke tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/19066

I reverted it in r374359. Please run ninja check-all before committing.

The test fails on Linux: http://lab.llvm.org:8011/builders/clang-cmake-x86_64-sde-avx512-linux/builds/28537/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Acheck-ignore-case.txt If it takes a while to fix please revert while you investigate.

Also, https://github.com/llvm/llvm-project/commit/dfd2b6f07fc40a190335f580d8a965bbebfe94df looks like you touched ~all lines in docs/CommandGuide/FileCheck.rst and llvm/include/llvm/Support/FileCheck.h Maybe you converted them to windows line endings? If so, please undo that. (Maybe revert and reland with fixed line endings so that the diff for the actual change is readable.)

As described in http://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git, the right way to checkout the repository on windows is:

% git clone --config core.autocrlf=false https://github.com/llvm/llvm-project.git

This is the second time I've reviewed a change like this without realizing, would appreciate tips if this could be more visible in Phab somehow...

Kai added a comment.Oct 11 2019, 4:42 AM

The test fails on Linux: http://lab.llvm.org:8011/builders/clang-cmake-x86_64-sde-avx512-linux/builds/28537/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Acheck-ignore-case.txt If it takes a while to fix please revert while you investigate.

Also, https://github.com/llvm/llvm-project/commit/dfd2b6f07fc40a190335f580d8a965bbebfe94df looks like you touched ~all lines in docs/CommandGuide/FileCheck.rst and llvm/include/llvm/Support/FileCheck.h Maybe you converted them to windows line endings? If so, please undo that. (Maybe revert and reland with fixed line endings so that the diff for the actual change is readable.)

As described in http://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git, the right way to checkout the repository on windows is:

% git clone --config core.autocrlf=false https://github.com/llvm/llvm-project.git

This is the second time I've reviewed a change like this without realizing, would appreciate tips if this could be more visible in Phab somehow...

Sorry, I really screwed this up. Not only that I managed to insert an additional character in the regex after my last test but the day before I had trouble with the repository (the COM1.o and COM2.o names on Windows) and cloned the repository again - obviously missing the CR/LF setting I had previously in.

jhenderson added inline comments.Oct 29 2019, 4:33 AM
llvm/test/FileCheck/check-ignore-case.txt
6

Thanks. I may have overly-strong feelings about the -/-- arguments, but I always find it off-putting seeing single-dash long options :)

FWIW, I'm less bothered when it's consistent within a test that only uses executables that don't need to care about single versus double dashes, such as this one.

46

I think this may have been a stale comment for the version I was viewing. At some point, Phabricator said "No new line at end of file" for me. Anyway, the final version you committed doesn't have the issue.