[FileCheck] Make FILECHECK_OPTS useful for its test suite

[jdenny]

Summary

Without this patch, FILECHECK_OPTS isn't propagated to FileCheck's
test suite so that FILECHECK_OPTS doesn't inadvertently affect test
results by affecting the output of FileCheck calls under test. As a
result, FILECHECK_OPTS is useless for debugging FileCheck's test
suite.

In llvm/test/FileCheck/lit.local.cfg, this patch provides a new
subsitution, %ProtectFileCheckOutput, to address this problem for
both FILECHECK_OPTS and the deprecated
FILECHECK_DUMP_INPUT_ON_FAILURE. The rest of the patch uses
%ProtectFileCheckOutput throughout the test suite

Fixes PR40284.

[probinson]

The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks. That bit should be its own patch, regardless of how we decide to handle FileCheck itself.

[jdenny]

In D65121#1597486, @probinson wrote:

The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks.

If this issue were present in more test suites, I'd be tempted to extend FileCheck with a command-line option to encapsulate this use case in some manner. As things are, that doesn't seem worth the trouble to me, but maybe others feel differently.

That bit should be its own patch, regardless of how we decide to handle FileCheck itself.

Sure, I'll separate out the lit change.

[probinson]

Regarding the FileCheck part, it should be split off to its own patch and the mass modification of FileCheck's test suite incorporated, because it's hard to grasp the effects without seeing those.

I have to say my initial reaction was "omg not another layer of indirection in the FileCheck test suite" but rereading PR40284, my own comments there have persuaded me to rethink that position. :-)

Reading the comments in FileCheck/lit.local.cfg, yes at some low level the point is whether varying the output affects the test, but I don't think that's a properly principled way to look at it. Also thinking that way is likely to cause brain damage in anyone coming along later and trying to decipher when to use which spelling of "run this tool."

Better to look at it this way (this is a multi-paragraph argument, sorry): This is a tool, these are its tests, and we want to run the tool and then FileCheck its output. This is how we test llvm-objdump and llvm-dwarfdump and all the rest of those tools. By a bizarre quirk of fate, the command to run the tool itself is spelled the same as FileCheck. So, let's change the spelling of "run this tool" because we'd like to keep the RUN lines following a pattern of toolname | FileCheck. We can call it something like %FileCheckUnderTest or %TestFileCheck or something else that implies "run the FileCheck I'm trying to test." (This would happen to be implemented as "run FileCheck with no environment.") Then your basic test of FileCheck would have a RUN line something like RUN: %TestFileCheck blah | FileCheck %s and it should be clear to the easily-trained eye which invocation of FileCheck is fulfilling which role. Importantly the *choice* of which spelling to use is based solely on the role within the test RUN line, not some more complicated "does it affect the test result" decision.

Sadly there are a few cases where a test actually does want to set an environment variable, and in those cases the above substitution will not do the right thing. Apparently we want (or also want) a substitution that will clean out the environment but not actually run the tool, so that we can set new environment variables before running it. Maybe call it %CleanEnv. Then your basic FileCheck-with-environment test's RUN line would look something like %CleanEnv env FILECHECK_OPTS=-v FileCheck blah | FileCheck %s

At that point, %TestFileCheck can be considered shorthand for %CleanEnv FileCheck (although likely the substitution wouldn't actually be implemented that way).

WDYT?

[probinson]

In D65121#1597516, @jdenny wrote:

In D65121#1597486, @probinson wrote:

The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks.

If this issue were present in more test suites, I'd be tempted to extend FileCheck with a command-line option to encapsulate this use case in some manner. As things are, that doesn't seem worth the trouble to me, but maybe others feel differently.

Then every test that had this issue would have to be modified to add the option. I think it would have to be an environment variable that suppresses the other environment variables: FILECHECK_IGNORE_ENV=1 or something. Then test suites could add that in their config files, and tests would be written as normal.

Luckily, so far we don't seem to need that level of confusion.

[jdenny]

In D65121#1597540, @probinson wrote:

Regarding the FileCheck part, it should be split off to its own patch and the mass modification of FileCheck's test suite incorporated, because it's hard to grasp the effects without seeing those.

I have to say my initial reaction was "omg not another layer of indirection in the FileCheck test suite"

I've been sitting on this patch for a few weeks in part because I had the same concern. :-) But I ultimately convinced myself that this is worth it for debugging FileCheck's test suite, which is challenging.

We can call it something like %FileCheckUnderTest or %TestFileCheck or something else that implies "run the FileCheck I'm trying to test."

This is actually what I originally talked about too in some phab reviews somewhere. One of them led to PR40284. I even started to implement that approach and then became convinced it's actually more complicated than the approach in this patch. See below.

Importantly the *choice* of which spelling to use is based solely on the role within the test RUN line, not some more complicated "does it affect the test result" decision.

This is an interesting point. At first, I thought the same. Then I realized that I cannot easily look at a FileCheck call in a RUN line and determine which role it serves. I can easily determine whether FileCheck's stdout or stderr is redirected/piped for further examination, and then I should surely use %TestFileCheckOut.

Now you might argue that the person originally writing a test surely knows which FileCheck call serves which role. Moreover, if he uses your %FileCheckUnderTest correctly, then everyone in the future will know, and that might be helpful for understanding tests besides just protecting from environment variables.

I'm not optimistic that approach will always be followed correctly and avoid rot over time. Actually, I think my simple check is much easier to follow when looking back at old tests: if the stdout/stderr is examined, then use %TestFileCheckOut. (Hmm, maybe the documentation I wrote could be simpler.) And then I can even use FILECHECK_OPTS to debug some FileCheck calls that are under test.

Sadly there are a few cases where a test actually does want to set an environment variable, and in those cases the above substitution will not do the right thing. Apparently we want (or also want) a substitution that will clean out the environment but not actually run the tool, so that we can set new environment variables before running it. Maybe call it %CleanEnv. Then your basic FileCheck-with-environment test's RUN line would look something like %CleanEnv env FILECHECK_OPTS=-v FileCheck blah | FileCheck %s

I went down this path at one point too... even with my does-the-output-matter approach. That is, %TestFileCheckOut originally included the FileCheck call, so I had a second substitution too.

Then I tried to document it, and I thought about people trying to maintain these in tests. Ugh. Yet another quirky substitution to confuse people. That's when I decided one simple substitution with a simple rule that handles all cases is easier to understand. Anyone who knows how env works can figure out how to set FILECHECK_OPTS just by glancing at the %TestFileCheckOut implementation, and there are examples in the documentation for everyone else.

WDYT?

In summary, I understand every point you made as I thought all the same things as I was working through this. It was my attempt to implement and document that led me to believe my current approach is actually simpler and more useful. Hopefully I didn't become too close to the implementation along the way to see it clearly. :-/

In a moment, I'll update the patch so you can see the mass changes throughout FileCheck's test suite, and I'll simplify the documentation at little. Maybe that'll help.

[jdenny]

• Remove changes for lit's test suite (will post soon in another review).
• Simplify %TestFileCheckOut documentation some.
• Propagate %TestFileCheckOut throughout FileCheck's test suite.

[jdenny]

In D65121#1597553, @probinson wrote:

In D65121#1597516, @jdenny wrote:

In D65121#1597486, @probinson wrote:

The lit part seems like a good thing. It's unfortunate that lit/test/lit.cfg needs to know about FileCheck environment variables, but them's the breaks.

If this issue were present in more test suites, I'd be tempted to extend FileCheck with a command-line option to encapsulate this use case in some manner. As things are, that doesn't seem worth the trouble to me, but maybe others feel differently.

Then every test that had this issue would have to be modified to add the option.

I was thinking of a command-line option that prints something like -u FILECHECK_OPTS -u FILECHECK_DUMP_INPUT_ON_FAILURE. The idea is that lit/test/lit.cfg wouldn't need to maintain a list of variables.

I think it would have to be an environment variable that suppresses the other environment variables: FILECHECK_IGNORE_ENV=1 or something. Then test suites could add that in their config files, and tests would be written as normal.

Makes sense too.

Luckily, so far we don't seem to need that level of confusion.

Agreed. It's just one external test suite, and there's no evidence the list of variables is going to evolve much further. FILECHECK_OPTS is intended to be a catch-all. FILECHECK_DUMP_INPUT_ON_FAILURE is deprecated.

[thopre]

Hi,

Could you add a section FileCheck-specific substitutions section in llvm/docs/TestingGuide.rst to describe %TestFileCheckOut?

[jdenny]

In D65121#1597772, @thopre wrote:

Hi,

Hi Thomas.

Could you add a section FileCheck-specific substitutions section in llvm/docs/TestingGuide.rst to describe %TestFileCheckOut?

That seems like a good idea. We'll need to decide what goes there and what stays in llvm/test/FileCheck/lit.local.cfg.

I'll make a note to work on this after we reach consensus about the actual behavior.

[probinson]

Getting back to this after a long month on first-responder-to-bugs duty. I know you're trying to get the lit/env issue sorted out, but I had a half hour available to look at this.

It makes me very sad to see
RUN: not FileCheck blahblah | FileCheck yadayada
become

RUN: not %TestFileCheckOut \
RUN: FileCheck blahblah | FileCheck yadayada

Wouldn't it work just as well to say

RUN: %TestFileCheckOut \
RUN: not FileCheck blahblah | FileCheck yadayada

which keeps the not adjacent to the FileCheck (and incidentally sidesteps the "not runs env as an external command" problem).

llvm/test/FileCheck/lit.local.cfg

3

Okay, that seems pretty straightforward and easy to understand.

51

This will clearly have to be changed to solve the problems found after D65156. But it can wait until that is sorted out.

[jdenny]

In D65121#1610682, @probinson wrote:

Wouldn't it work just as well to say

RUN: %TestFileCheckOut \
RUN: not FileCheck blahblah | FileCheck yadayada

which keeps the not adjacent to the FileCheck (and incidentally sidesteps the "not runs env as an external command" problem).

Yes, I like that better too. Thanks!

llvm/test/FileCheck/check-count.txt

7 ↗

(On Diff #211322)

Instead of that, just add a preceding line with

; RUN: %TestFileCheckOut \

Likewise everywhere else.

See https://reviews.llvm.org/D65121#1610682.

[jdenny]

Rebase. Specify %TestFileCheckOut before not.

Still need to write the suggested documentation and deal with the "env -u" issue.

[jdenny]

Forgot to git add a change for that last update.

[probinson]

More bikeshedding: The new substitution could be called %TestFileCheckOutput (thus not abbreviating Output to Out) which helps it to be more self-documenting.
Or, it could be called %TestFileCheckEnv because its function is to set the environment, and it can optionally take an environment-variable definition as an argument. (Sorry if I already suggested this and we decided against.)
Everything else about this looks fine, LGTM once the substitution's name is settled.

[jdenny]

In D65121#1621579, @probinson wrote:

More bikeshedding: The new substitution could be called %TestFileCheckOutput (thus not abbreviating Output to Out) which helps it to be more self-documenting.

That works for me, and I'll go with that if there's no further discussion.

Or, it could be called %TestFileCheckEnv because its function is to set the environment, and it can optionally take an environment-variable definition as an argument. (Sorry if I already suggested this and we decided against.)

I like seeing Env there. I don't like dropping Output because I want the reminder that FileCheck output is the reason to use this substitution.

I want to say %SetEnvForTestingFileCheckOutput, but that's too much. Can we cram all that into something shorter?

Another possibility: %ProtectFileCheckOutput

Everything else about this looks fine, LGTM once the substitution's name is settled.

We have time. I want to see how things go with my related lit test suite work first, and there was a suggestion about documentation here that I haven't worked on.

Thanks again.

[probinson]

In D65121#1621616, @jdenny wrote:

Another possibility: %ProtectFileCheckOutput

Oh, I like that better.

[jdenny]

• Rebased.
• Renamed %TestFileCheckOut to %ProtectFileCheckOutput.
• Added documentation to llvm/docs/TestingGuide.rst.

This patch isn't ready to be pushed yet. FileCheck's test suite doesn't use lit's internal shell, and this patch introduces calls to env -u, which D65335 explains is not portable across external env implementations. One solution is to remove the use of env -u here. Another solution is to change FileCheck's test suite to use lit's internal shell. The latter solution is probably generally better for portability, but it won't help this patch without the patches I'm working on for lit's internal shell in order to push D65156 again. I'm inclined to wait on that anyway to be sure we've actually fully understood the trouble there.

[jdenny]

Now that D65156 and its many dependencies seem to have survived the bots, I'm finally returning to this patch.

This update rebases onto a recent master and adds %ProtectFileCheckOutput to a few new RUN lines that have since appeared in FileCheck's test suite.

As discussed at D65121#1637145, we need to decide whether to eliminate env -u uses in this patch or to switch FileCheck's test suite to lit's internal shell. Either looks easy to do, but I vote for the latter as it seems better for portability in the long term. It appears to just require this in llvm/test/FileCheck/lit.local.cfg (llvm/utils/lit/tests/lit.cfg already has this):

import lit
config.test_format = lit.formats.ShTest(execute_external=False)

What do people think? I'll probably make that a separate review if there are no immediate objections to the idea.

[jdenny]

It appears the last update before today did not add documentation to llvm/docs/TestingGuide.rst as I intended, but today's update did, so that's another new change to look at.

llvm/test/FileCheck/lit.local.cfg

-# Unset environment variables that the FileCheck tests
-# expect not to be set.
-file_check_expected_unset_vars = [
-  'FILECHECK_DUMP_INPUT_ON_FAILURE',
-  'FILECHECK_OPTS',
-]
-
-for env_var in file_check_expected_unset_vars:
-  if env_var in config.environment:
-    lit_config.note('Removing {} from environment for FileCheck tests'.format(
-      env_var)
-    )
-    config.environment.pop(env_var)
+# There are a few main points to remember about %TestFileCheckOut:
+#
+# 1. %TestFileCheckOut should precede a FileCheck call in FileCheck's test
+#    suite if and only if the call's textual output affects test results.
+#    If only the call's exit status matters, don't use %TestFileCheckOut.
+# 2. %TestFileCheckOut expands to an "env" command that unsets all FileCheck
+#    environment variables.  Thus, any FileCheck environment variables that
+#    need to be tested must be assigned between %TestFileCheckOut and the
+#    FileCheck command.
+# 3. %TestFileCheckOut's purpose is to ensure correct test results when
+#    developers set FileCheck environment variables (e.g.,
+#    FILECHECK_OPTS=-dump-input=fail) to tweak the output of FileCheck while
+#    debugging tests.  If a developer sets values that affect FileCheck's exit
+#    status (e.g., FILECHECK_OPTS=-strict-whitespace), he shouldn't be
+#    surprised that test results throughout all test suites are affected.
+#
+# Examples:
+#
+#   ; Test another program, using FileCheck to verify its textual output.
+#   ; Only FileCheck's exit status affects test results, so a bare FileCheck
+#   ; call is sufficient and more convenient for debugging.
+#   ;
+#   ; RUN: %t | FileCheck %s
+#   ; CHECK: {{[0-9]+\.0}}
+#
+#   ; Test FileCheck itself, but only examine its exit status, so a bare
+#   ; FileCheck call is still sufficient and more convenient for debugging.
+#   ;
+#   ; RUN: FileCheck -input-file %s %s
+#   ; CHECK: {{[0-9]+\.0}}
+#   ; 5.0
+#
+#   ; Check that the FileCheck trace is off by default.  The first FileCheck
+#   ; call's textual output affects test results, so it requires
+#   ; %TestFileCheckOut to be safe.
+#   ;
+#   ; RUN: %TestFileCheckOut FileCheck -input-file %s %s 2>&1 \
+#   ; RUN: | FileCheck -allow-empty -check-prefix QUIET %s
+#   ;
+#   ; CHECK: {{[0-9]+\.0}}
+#   ; 5.0
+#   ; QUIET-NOT: expected string found in input
+#
+#   ; Check that the FileCheck trace is on when FILECHECK_OPTS=-v.
+#   ; FILECHECK_OPTS must be set after %TestFileCheckOut, which clears
+#   ; FILECHECK_OPTS beforehand.
+#   ;
+#   ; RUN: %TestFileCheckOut FILECHECK_OPTS=-v \
+#   ; RUN: FileCheck -input-file %s %s 2>&1 \
+#   ; RUN: | FileCheck -check-prefix TRACE %s
+#   ;
+#   ; CHECK: {{[0-9]+\.0}}
+#   ; 5.0
+#   ; TRACE: expected string found in input
+#
+# The name %TestFileCheckOut is verbose.  However, %TestFileCheckOut is
+# unlikely to prove useful in other test suites, and most developers don't
+# spend much time in FileCheck's test suite.  A strong reminder of its intended
+# usage at every occurrence seems worth the verbosity.
+config.substitutions.append(('%TestFileCheckOut',
+    'env -u FILECHECK_OPTS -u FILECHECK_DUMP_INPUT_ON_FAILURE'))

llvm/utils/lit/tests/lit.cfg

 llvm_config.with_environment('PYTHONPATH', lit_path, append_path=True)
 
 # Add llvm and lit tools directories if this config is being loaded indirectly.
 for attribute in ('llvm_tools_dir', 'lit_tools_dir'):
     directory = getattr(config, attribute, None)
     if directory:
         llvm_config.with_environment('PATH', directory, append_path=True)
 
+# This test suite calls %{lit} to test lit's behavior for the sample test
+# suites in %{inputs}.  This test suite's results are then determined in part
+# by %{lit}'s textual output, which includes the output of FileCheck calls
+# within %{inputs}'s test suites.  Thus, %{lit} clears environment variables
+# that can affect FileCheck's output.
 config.substitutions.append(('%{inputs}', os.path.join(
     config.test_source_root, 'Inputs')))
-config.substitutions.append(('%{lit}', "%%{python} %s" % (
-    os.path.join(lit_path, 'lit.py'),)))
+config.substitutions.append(('%{lit}',
+    "{env} %{{python}} {lit}".format(
+        env="env -u FILECHECK_OPTS -u FILECHECK_DUMP_INPUT_ON_FAILURE",
+        lit=os.path.join(lit_path, 'lit.py'))))
 config.substitutions.append(('%{python}', '"%s"' % (sys.executable)))
 
 # Enable coverage.py reporting, assuming the coverage module has been installed
 # and sitecustomize.py in the virtualenv has been modified appropriately.
 if lit_config.params.get('check-coverage', None):
     config.environment['COVERAGE_PROCESS_START'] = os.path.join(
         os.path.dirname(__file__), ".coveragerc")
 
 # Add a feature to detect if psutil is available
 try:
     import psutil
     lit_config.note('Found python psutil module')
     config.available_features.add("python-psutil")
 except ImportError:
     lit_config.warning('Could not import psutil. Some tests will be skipped and'
                        ' the --timeout command line argument will not work.')

llvm/utils/lit/tests/shtest-run-at-line.py

 # Check that -vv makes the line number of the failing RUN command clear.
 # (-v is actually sufficient in the case of the internal shell.)
 #
-# RUN: env -u FILECHECK_OPTS not %{lit} -j 1 -vv %{inputs}/shtest-run-at-line > %t.out
+# RUN: not %{lit} -j 1 -vv %{inputs}/shtest-run-at-line > %t.out
 # RUN: FileCheck --input-file %t.out %s
 #
 # END.
 
 
 # CHECK: Testing: 4 tests