Page MenuHomePhabricator

[lit] Remove %T
AcceptedPublic

Authored by kubamracek on Jul 13 2017, 5:18 PM.

Details

Summary

I've recently found a weird non-deterministic failure in the case-insensitive-include-ms.c, case-insensitive-include.c and case-insensitive-system-include.c testcases. The reason was that they actually share the same %T directory and when they're run in parallel, they can overwrite other test's data. Pretty terrible, right?

This patch changes lit so it generates a per-test temporary directory for %T and not a per-test-suite temporary dir. Another option would of course be to fix the above-mentioned tests, but do we really need to require test writers to remember that %T is not unique to the test?

Diff Detail

Event Timeline

kubamracek created this revision.Jul 13 2017, 5:18 PM
rnk edited edge metadata.Jul 13 2017, 5:20 PM

Tests that have this bug simply did not read the documentation for %T: it is the parent directory of %t, which is a non-existent path unique for the test. Any test that fails this way should do RUN: mkdir %t and use %t instead of %T.

That would be inconsistent with TestingGuide.rst, if this change goes through, the documentation needs to be updated.
In any case, currently TestingGuide.rst directly contradicts lit.rst on the meaning of %T.

@rnk There are two documentation files. The lit.rst says it's unique, TestingGuide.rst says it's not.
So at least that should be fixed.
In any case, I disagree with your comment: what if the user still wants to have %t to signify a temporary file? Why an extra mkdir?

rnk added a comment.Jul 13 2017, 5:39 PM

@rnk There are two documentation files. The lit.rst says it's unique, TestingGuide.rst says it's not.
So at least that should be fixed.
In any case, I disagree with your comment: what if the user still wants to have %t to signify a temporary file? Why an extra mkdir?

Somebody has to do the mkdir, and it might as well be the test, since those execute in parallel. Right now lit makes Output directories for every suite, not for every test. Running mkdir for every llvm test seems like it would slow down startup.

@rnk Actually it would be quite easy to do the mkdir iff %T is present in the file, if performance is a concern.
I would argue that test runs should be isolated by the test runner, and test authors should not be trying to judge whether it's safe to share a directory.

In any case, inconsistent documentation should be updated one way or the other.

rnk added a comment.Jul 13 2017, 5:51 PM

@rnk Actually it would be quite easy to do the mkdir iff %T is present in the file, if performance is a concern.
I would argue that test runs should be isolated by the test runner, and test authors should not be trying to judge whether it's safe to share a directory.

In any case, inconsistent documentation should be updated one way or the other.

That sounds like a pretty good idea. :) FWIW, I am concerned about the performance. You can instrument lit to check out log it takes to make the Output directories.

I'm not suggesting to mkdir a new directory for each test. %T will point to a non-existent directory, just like %t points to a non-existent file. If the tests wants to use %T, it will have to mkdir it.

rnk added a comment.Jul 14 2017, 9:41 AM

I'm not suggesting to mkdir a new directory for each test. %T will point to a non-existent directory, just like %t points to a non-existent file. If the tests wants to use %T, it will have to mkdir it.

lit already ensures that tmpDir exists, so that's what this change will do. Look for callers of getTempPaths.

lit already ensures that tmpDir exists, so that's what this change will do. Look for callers of getTempPaths.

Oh. That's not what I meant then. I'll update the patch.

rnk added a comment.Jul 26 2017, 4:40 PM

lit already ensures that tmpDir exists, so that's what this change will do. Look for callers of getTempPaths.

Oh. That's not what I meant then. I'll update the patch.

This doesn't seem like it was addressed? As is, surely this will regress lit startup time.

In D35396#822242, @rnk wrote:

lit already ensures that tmpDir exists, so that's what this change will do. Look for callers of getTempPaths.

Oh. That's not what I meant then. I'll update the patch.

This doesn't seem like it was addressed? As is, surely this will regress lit startup time.

This was addressed, I changed the patch. dirname(tmpBase) is later being mkdir'd, but tmpDir isn't.

rnk accepted this revision.Jul 26 2017, 5:05 PM

This was addressed, I changed the patch. dirname(tmpBase) is later being mkdir'd, but tmpDir isn't.

Sounds good!

This revision is now accepted and ready to land.Jul 26 2017, 5:05 PM

Cool. Some tests actually need fixing before this can land.

MatzeB added a subscriber: MatzeB.Jul 26 2017, 5:21 PM

The fact that %T has the same values for all tests inside a directory is indeed terrible.

One thing to think about however: With this patch it seems %T and %t both point to a unique name that doesn't exist. So why have %T at all, you could just as well do mkdir %t and I think there already are a bunch of tests that do just that.

Works for me. @rnk, do you think that's better?

The fact that %T has the same values for all tests inside a directory is indeed terrible.

One thing to think about however: With this patch it seems %T and %t both point to a unique name that doesn't exist. So why have %T at all, you could just as well do mkdir %t and I think there already are a bunch of tests that do just that.

Should have read the discussion before answering and realize you already discussed all that :)
(Guess there is just the fact that I lean towards the more aggressive solution that drops the concept of %T entirely. But that is easy to say if you're not the one having to fixup all the tests. In any way I'm fine with this approach too.

rnk added a comment.Jul 27 2017, 10:30 AM

Works for me. @rnk, do you think that's better?

+1 for removing %T. It's just a trap.

I also think that removing %T entirely in this case is a good idea.

@kubamracek Wouldn't you need now to change ALL the tests using %T? :P

Yes, of course, this change is not ready to land.

Patch to remove %T from compiler-rt: https://reviews.llvm.org/D36434

delcypher added inline comments.Aug 11 2017, 12:34 PM
docs/TestingGuide.rst
459 ↗(On Diff #109391)

What are we supposed to do to have a test written that way to work on Windows? There is a mkdir command but I'm not familiar enough with it to know if doing mkdir "%t" will work. It certainly doesn't take a -p argument.

MatzeB added inline comments.Aug 11 2017, 12:42 PM
docs/TestingGuide.rst
459 ↗(On Diff #109391)

I have no experience with how we do things on windows, but I'm sure I've seen things like grep, sed etc. which aren't available on windows by default. I always assumed we expect people to install some posix compatible tools to run the tests on windows...

@delcypher I think LIT already does some magic to work with Windows, right? Many tests use mkdir, otherwise none of them would run.

@rnk will probably know. What is the story with mkdir -p %t on Windows?

MatzeB accepted this revision.Aug 11 2017, 12:47 PM

And this LGTM.

And some heads up: Removing the tmpDir parameter from getDefaultSubstitutions will need some changes to the llvm test-suite lit integration which calls this function. But that's no reason to stop this patch, I'll update the test-suite when this is committed.

@delcypher I think LIT already does some magic to work with Windows, right? Many tests use mkdir, otherwise none of them would run.

The lit shell tests have two modes. Internal and external. External just invokes bash and the internal shell implements a shell command parser in python.

The executeScriptInternal() function in TestRunner.py handles running commands using the internal shell. This eventually calls _executeShCmd(). If you look at the implementation is has special support of a few shell built-ins (namely cd, echo, export and env). I don't see any special support for mkdir though. If the command doesn't start with . then lit.util.which() will be used to find the executable. So AFAICT mkdir will just run the mkdir command when using the internal shell.

The approach I've used in the past to handle different system tools is to add a substitution that expands to the command appropriate for the platform.

E.g.

https://github.com/symbooglix/symbooglix/blob/master/test_programs/lit.site.cfg#L116

rnk added a comment.Aug 14 2017, 8:47 AM

@rnk will probably know. What is the story with mkdir -p %t on Windows?

It works because LLVM requires Unix core utilities to be available. We have some advice on where to get them tucked away here: https://llvm.org/docs/GettingStartedVS.html#software It should probably be more prominent.

kubamracek retitled this revision from [lit] Make %T return a per-test temporary directory to [lit] Remove %T.Aug 20 2017, 10:45 AM
In D35396#840758, @rnk wrote:

@rnk will probably know. What is the story with mkdir -p %t on Windows?

It works because LLVM requires Unix core utilities to be available. We have some advice on where to get them tucked away here: https://llvm.org/docs/GettingStartedVS.html#software It should probably be more prominent.

One thing to note here is that even if the GnuWin32 tools are available in PATH, if you type mkdir in a console window you seem to get the Windows version.

rnk accepted this revision.Aug 23 2017, 1:14 PM

Is this ready?

Almost, I'm still working on removing "%T" throughout all the repos. Clang is clean, LLVM has 1 instance left, which I wasn't able to remove, and I asked the responsible person for help. There's also some downstream projects that need to be cleaned first (Swift).

While at it, could the documentation also be clarified whether %t refers to a prefix (or note that it is created in a subdirectory such that one can know for sure that it is cleaned up)? A lot of tests use something like %clang -o %t.o.

As for whether mkdir -p can be used or not, based on a look in utils/lit/, there does not seem to be a special case for that. Very few builtins (export, echo, cd) are handled. Perhaps it would be worth adding more builtins and/or document whether it is safe to use those? Looking at a Windows builder (http://lab.llvm.org:8011/buildslaves/windows7-buildbot) though, the Profile/gcc-flag-compatibility.c test does get executed properly (it uses rm -rf and mkdir -p ../..).

@rnk Are the GnuWin32 tools just needed for the test suite or is it also needed for the build process? (Linux user here, no idea how LLVM is usually developed on Windows)

While at it, could the documentation also be clarified whether %t refers to a prefix (or note that it is created in a subdirectory such that one can know for sure that it is cleaned up)? A lot of tests use something like %clang -o %t.o.

%t is just a name/string that is based on the tests name and is therefore guaranteed to be unique for each test. There is no automatic cleanup, in fact I believe you don't want automatic cleanup as you want to inspect the intermediate results in case of errors.
However I believe the %t path is always in a subdirectory called Output so if you wanted to cleanup intermediate results between test-suite runs you could do something like find build/test -d -name "Output" -exec rm -rf {} ';'

As for whether mkdir -p can be used or not, based on a look in utils/lit/, there does not seem to be a special case for that. Very few builtins (export, echo, cd) are handled. Perhaps it would be worth adding more builtins and/or document whether it is safe to use those? Looking at a Windows builder (http://lab.llvm.org:8011/buildslaves/windows7-buildbot) though, the Profile/gcc-flag-compatibility.c test does get executed properly (it uses rm -rf and mkdir -p ../..).

The tests assume this is possible even on windows. So the expectation is that whoever runs the tests has those GnuWin32 tools installed.

@rnk Are the GnuWin32 tools just needed for the test suite or is it also needed for the build process? (Linux user here, no idea how LLVM is usually developed on Windows)

docs/GettingStartedVS.rst sounds like GnuWin32 is only needed for the tests.

espindola edited reviewers, added: espindola; removed: rafael.Mar 14 2018, 4:57 PM

Hi, what is the status with this patch? It was referred to by https://reviews.llvm.org/D36495 (which is committed), and there still seem to be a lot of users of %T.

Hi, what is the status with this patch? It was referred to by https://reviews.llvm.org/D36495 (which is committed), and there still seem to be a lot of users of %T.

Yes, that's why this one didn't land yet. We need to get rid of all the %T users first.

Hi, what is the status with this patch? It was referred to by https://reviews.llvm.org/D36495 (which is committed), and there still seem to be a lot of users of %T.

Yes, that's why this one didn't land yet. We need to get rid of all the %T users first.

Perhaps you could already adjust the documentation and mark %T as deprecated. I have honestly forgotten about this patch and used %T again in some new tests because it was documented (without any caveats).