This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Allow use of ShTest in libc++ tests along with other changes.
ClosedPublic

Authored by EricWF on Jan 20 2015, 1:07 PM.

Details

Summary

This patch allows the use of LIT's ShTest format in the libc++ test suite. ShTests have the suffix '.sh.cpp'. It also introduces a series of other changes. These changes are:

  • More functionality including parsing test metadata has been moved into LIT.
  • LibcxxTestFormat now supports multi-part suffixes.
  • the CXXCompiler functionality has been used to shrink the size of LibcxxTestFormat.
  • The recursive loading of the site config has been turned into libcxx.test.config.loadSiteConfig so it can be used with libc++abi.
  • Temporary files are now created in the build directory of libc++. This follows how it is down in ShTest.
  • not.py was added as a utility executable that mirrors the functionality of LLVM's not executable.
  • The first ShTest test was added under test/libcxx/double_include.sh.cpp

Diff Detail

Event Timeline

EricWF updated this revision to Diff 18453.Jan 20 2015, 1:07 PM
EricWF retitled this revision from to [libcxx] Allow use of ShTest in libc++ tests along with other changes..
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added reviewers: danalbert, jroelofs.
EricWF added a subscriber: Unknown Object (MLST).
EricWF updated this revision to Diff 18454.Jan 20 2015, 1:13 PM
EricWF updated this object.

Misc cleanups and fixes.

jroelofs added inline comments.Jan 20 2015, 1:57 PM
test/libcxx/double_include.sh.cpp
96

I think this needs to be guarded out using that libcpp-has-no-threads thing. Same thing for <future>, <mutex>, <shared_mutex>, and <atomic>.

test/libcxx/test/config.py
25

Is this TODO still relevant?

EricWF updated this revision to Diff 18460.Jan 20 2015, 2:01 PM

Allow switching between the LIT internal shell and the external shell for ShTests. See the added documentation for a description on how to do this.

EricWF updated this revision to Diff 18461.Jan 20 2015, 2:13 PM

Address @jroelofs comments.

jroelofs edited edge metadata.Jan 20 2015, 2:24 PM

Regarding not.py: what did you try in terms of trying to import it from lit proper? (I need to figure out a solution to the same problem for loading the libcxx bits from libcxxabi's config).

Also, looks good on my end, though I'd like to hear from @danalbert on it. he's better at catching python-isms than I am.

Regarding not.py: what did you try in terms of trying to import it from lit proper? (I need to figure out a solution to the same problem for loading the libcxx bits from libcxxabi's config).

Nothing. In particular because I want not.py to work entirely standalone of LIT. The best solution I can think of to import LIT is to add the path to the LIT module to the PYTHONPATH environment variable when running the ShTests. However I'm not sure there is an easy way to detect where LIT is, even inside our lit.cfg file.

danalbert edited edge metadata.Jan 20 2015, 6:04 PM

Definitely run pylint and pep8. Mostly good, though I do have some questions and nits.

test/libcxx/compiler.py
1

Blank line?

95

Two args have different style: infile vs object_file. Prefer in_file (or even source_file). Same goes for the public API to this.

102

Alignment. Should run pylint and pep8 to catch these things.

test/libcxx/test/config.py
34

No kidding. I thought we were already safe from this before...

179

Parentheses aren't necessary.

181

Why isn't this just a string comparison?

492

No extra spaces inside parens.

494

Style nit: I'd kill the vertical alignment. PEP8 agrees (https://www.python.org/dev/peps/pep-0008/#pet-peeves)

498

I'm not a fan of the vertical alignment here. If you want to keep things a little cleaner than many calls to append,

sub.extend([
    ('foo', foo),
    ('bar', bar),
])

looks cleaner imo.

514

k, v

test/libcxx/test/format.py
60

Glad to see all the code backing this go away. Thanks!

126

This breaks the expectation of what _clean is used for. In the case of building on one machine but running on another, there's no way to determine which files need to be cleaned from which machine now. (I think in my case we're just ignoring any failures from clean failures, so there's no behavior change, but it's still odd). Why isn't object_path cleaning up after itself anymore?

If there is a good reason for object_path not cleaning up after itself, this is fine for now and I'll clean it up later if I think of a good way to do so.

134

Don't you want to check for '// USE_VERIFY' in contents?

This is slower than it used to be. We no longer stop processing after the first non-empty non-comment line, so this could take a long time on large tests.

135

extra_flags = ['-Xclang', '-verify'] if use_verify else [] is fine for something this simple.

138

Is there some sort of os.null that would work on Windows for this?

(not required for this patch, just thinking)

utils/not/not.py
8

What's wrong with // RUN: ! %rest_of_cmd?

15

Why not? We're already importing other things from lit. (Not going to actually review any of the rest of this file since I expect it's just copied directly.)

Address @danalbert's comments. Changes to come.

test/libcxx/compiler.py
95

Will change.

102

I always find that pylint is too noisy. I'll try pep8.

test/libcxx/test/config.py
179

Will fix.

181

Yep. I just copied it from the clang lit.cfg but I'll change it in ours.

test/libcxx/test/format.py
126

I just cleaned it up using _clean because _clean does exactly what I need. I'll create another function that does the same thing as _clean and call that instead.

134

Don't you want to check for '// USE_VERIFY' in contents?

The old behavior only checked to see if it was in the line. This is definitely slower but .fail.cpp tests should be very small anyway so it shouldn't make too much of a difference.

I would like to move this into parseIntegratedTestScript but this is the best way to currently do it.
I'll add a TODO.

135

Will do.

138

os.devnull should work. Thanks.

utils/not/not.py
8

! is not available in the LIT internal shell. LLVM and clang use llvm/utils/not for this purpose. This is a simplified version that doesn't introduce a dependency on a bunch of LLVM libraries.

15

Because I have no idea how to find the location of the lit module during configuration and there aren't any easy ways to transfer the lit module location to not.py.

If you have any ideas on how to find the location of the lit module I'll try and pass that information to not.py.

EricWF updated this revision to Diff 18477.Jan 20 2015, 7:37 PM
EricWF edited edge metadata.

Implement @danalbert's comments.

EricWF updated this revision to Diff 18478.Jan 20 2015, 7:59 PM

Modify not.py so that it takes --lit-site=path/to/llvm/utils/lit as it's first parameter. This allows it to reuse the functionality in lit.util.
The not substitution now expands to path/to/python path/to/libcxx/utils/not//not.py --lit-site=path/to/llvm/utils/lit which is a bit hairy but still do-able.

EricWF updated this revision to Diff 18479.Jan 20 2015, 8:03 PM

Remove unneeded module imports in not.py.

EricWF updated this revision to Diff 18483.Jan 20 2015, 11:07 PM

Fix some bugs and cleanup how CXXCompiler.compileLinkTwoStep handles the object file.

It occurs to me that we ought to have an shtest that exercises not.py. How about something like this?

// -*- C++ -*-
//===----------------------------------------------------------------------===//
//
//                     The LLVM Compiler Infrastructure
//
// This file is dual licensed under the MIT and the University of Illinois Open
// Source Licenses. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//

// RUN: %cxx -c %s %flags %compile_flags %link_flags
// RUN: not %run

int main() {
  return 1;
}

It occurs to me that we ought to have an shtest that exercises not.py. How about something like this?

Probably a good idea. We should probably have a directory that contains tests for the test suite if that makes sense. Any idea where that should live?

PS. %run expands to %exec %t.exe so you need to put -o %t.exe in the compile command. I'll write up some documentation on the substitutions once this patch gets through.

// -*- C++ -*-
//===----------------------------------------------------------------------===//
//
//                     The LLVM Compiler Infrastructure
//
// This file is dual licensed under the MIT and the University of Illinois Open
// Source Licenses. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//

// RUN: %cxx -c %s %flags %compile_flags %link_flags
// RUN: not %run

int main() {
  return 1;
}

Yo dawg, I do think we should have testsuite tests. test/libcxx/selftest perhaps?

EricWF updated this revision to Diff 18522.Jan 21 2015, 8:26 AM

Added a selftest directory with tests that test the test suite (@jroelofs Yo, Dawg). One of these tests exercises not.py.

LGTM as long as @danalbert is also happy with it.

danalbert added inline comments.Jan 21 2015, 11:58 AM
test/libcxx/compiler.py
6

pylint really hates [] as a default argument (and apparently for good reason).

Making the defaults be None and using self.flags = flags or [] looks nice.

(I understand that using self.flags = list(flags) actually works around this problem, but I'd like it if we could change the style so it cuts down on noise for those of us that do run linters.)

113

source_files (not a part of this change, but might as well cause some collateral cleanup).

test/libcxx/test/config.py
336

Use os.path.join.

511

This alignment isn't correct. Align with the first element.

535

Unused.

test/libcxx/test/format.py
30

self and litConfig are unused (this could be a function rather than a method). Is this "overriding" something from lit.formats.FileBasedTest?

35

No parens surrounding conditionals.

76

This is "private" to lit.TestRunner. Should this function be given a public name in lit.TestRunner? Is there some other API that should be used?

128

The last three arguments aren't used.

test/libcxx/util.py
6

Unused.

When we talked about this you had mentioned that this would make it easier to access the tests that were built for debugging. I don't see anything here that does that. Is that going to be a follow up patch?

test/libcxx/util.py
35

This is really a NullContextManager (nothing is specific to a file name). I'm not sure what a good name for the function would be. Possibly just nullContext?

utils/not/not.py
4

The block should be the module docstring.

13

sys.argv should be treated as constant, imo. Make a copy if you're going to use it this way.

21

Why does LIT use its own home grown version of this? https://docs.python.org/2/distutils/apiref.html#module-distutils.spawn

There's even a comment in the code that says "Would be nice if Python had a lib function for this". Was distutils.spawn.find_executable not available when that was written?

If you use the distutils API (and just use subprocess instead of lit.util.executeCommand for this you can get rid of the need for --lit-site, which makes up most of the gross in this file.

26

This doesn't directly mirror the one in llvm/utils. The other one only considers negative values success if passed --crash.

Aside from that, it's kind of a weird flag imo. I would have made it a separate expect_crash command. not --crash reads to me as "doesn't crash".

EricWF updated this revision to Diff 18552.Jan 21 2015, 2:13 PM

Address first half of @danalbert's comments.

EricWF updated this revision to Diff 18555.Jan 21 2015, 2:31 PM

Address the rest of @danalbert's comments. Mostly surrounding not.py.

danalbert accepted this revision.Jan 21 2015, 2:55 PM
danalbert edited edge metadata.

With the docstring nit fixed, LGTM.

test/libcxx/compiler.py
41

Ah, lots of these. Thanks.

utils/not/not.py
6

Module docstrings go at the very top, before imports.

Whole file looks much better now, btw. Thanks. I'd still like it if this wasn't necessary, so we should maybe look at adding ! to the builtin shell at some point, but this is plenty fine for now.

This revision is now accepted and ready to land.Jan 21 2015, 2:55 PM

Answer @danalbert's questions. Thanks for the review.

test/libcxx/compiler.py
6

Huh, I had no idea default arguments worked like that in python.

I still think you should copy the list in so you would need to write self.flags = list(flags or []). Which is more complicated that just self.flags = list(flags). I'll make the change to reduce noise but I'm not convinced it is a good one.

test/libcxx/test/config.py
34

Two reasons for this change.

  1. In order to turn this into a reusable function that function should probably return. Recursive reloading would prevent this.
  2. I actually have plans to introduce a second benchmarking test suite that relies on the same site config file but it can't load the current lit.cfg
test/libcxx/test/format.py
30

It's replacing the method by the same name in lit.formats.FileBasedTest with a small tweak that allows multipart suffixes. I didn't want to make the change in LIT proper because (as previously discussed) we might try and move away from metadata in test names.

76

Maybe? I split _runShTest() from lit.TestRunner.executeShTest() specifically for use with libc++. I'm hesitant to make the function public if we are the only people that need it. There is no other API in lit.TestRunner that would serve our needs.

utils/not/not.py
6

I've looked at the internal shell a bit and it seems that it could use a lot of love. I'll look into adding ! but it is super low on my list.

EricWF updated this revision to Diff 18622.Jan 22 2015, 10:05 AM
EricWF edited edge metadata.

Move docstring in not.py and merge with upstream libc++.

EricWF closed this revision.Jan 22 2015, 10:07 AM