This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add numerous options to libc++ LIT test suite configuration.
ClosedPublic

Authored by EricWF on Oct 20 2014, 7:55 PM.

Details

Summary

In order to fully replace the testit script we need to update LIT so it provides the same functionality.
This patch adds a number of different configuration options to LIT to do that. It also adds documentation for all of the command line parameters that LIT supports.

Generic options added:

  • libcxx_headers
  • libcxx_library
  • compile_flags

Generic options modified:

  • link_flags: Changed from overriding the default args to adding extra args instead (to match compile flags)
  • use_sanitizer: Renamed from llvm_use_sanitizer

Please see the added documentation for more information about the switches. As for the actual documentation I'm not sure if it should be kept in libc++ forever since it adds an undue maintenance burden, but I think it should be added for the time being while the changes are new. I'm verify unskilled with HTML so if the documentation needs any changes please let me know.

Hopefully this will kill testit.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 15163.Oct 20 2014, 7:55 PM
EricWF retitled this revision from to [libcxx] Add numerous options to libc++ LIT test suite configuration. .
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added reviewers: mclow.lists, danalbert, jroelofs.
EricWF added a subscriber: Unknown Object (MLST).
alexfh added a subscriber: alexfh.Oct 22 2014, 6:14 AM
alexfh added inline comments.
test/lit.cfg
578

I wouldn't like this to depend on the default set of checks. And I don't think you want all static analyzer checks. I'd better change this to:

-checks=-*,llvm*,clang-analyzer*,-clang-analyzer-alpha*

Also, could you just use an appropriate .clang-tidy file instead?

Thanks for the information on the default clang tidy settings, I wasn't quite sure what to use :D

The reason I didn't use a clang-tidy is because much of the clang-tidy invocation is sensative to information that could change from run to run due to different configurations.
I'll take a look into it further to see if there is any information that is consistent.

EricWF updated this revision to Diff 15293.Oct 22 2014, 8:26 PM

Update patch so it applies cleanly to TOT. Also change the default clang-tidy checkers. I didn't add a .clang-tidy file since a user may want to disable the default checkers by overriding them on the command line.

alexfh added inline comments.Oct 22 2014, 11:16 PM
test/lit.cfg
572

It should be -checks=-*,llvm*,... i.e. the first item in the list should be "-*" which means "disable all checks".

EricWF updated this revision to Diff 15302.Oct 22 2014, 11:56 PM

Woops. Fix clang-tidy default args again. Thanks.

EricWF updated this revision to Diff 15303.Oct 22 2014, 11:59 PM

Update docs for last change. Sorry for the spam.

I know nothing about libcxx testing, but clang-tidy-related bits look good now ;) (assuming you have run this already and it works).

Taking this offline.

Thanks for the input. Using clang-tidy is a nice feature. (even though
every single test fails w/ clang-tidy and the default options).
I guess we have plenty of work to do.

/Eric

EricWF updated this revision to Diff 15968.Nov 10 2014, 12:48 AM

Add support for building and generating code coverage using lcov and genhtml. To produce code coverage you must:

  • configure with '-DLIBCXX_GENERATE_COVERAGE=ON'
  • make
  • make check-libcxx
  • make check-libcxx-coverage

The resulting HTML can be found in <build-libcxx>/test/coverage/libcxx_coverage

This also adds support for UNSUPPORTED-XFAIL and REQUIRES-XFAIL tags that ensure compilation fail but report the test as UNSUPPORTED instead of XFAIL.

danalbert requested changes to this revision.Nov 10 2014, 8:29 PM
danalbert edited edge metadata.

Mega patches make me sad. I see 5 distinct changes here:

  1. Add missing testit features.
  2. scan-build support.
  3. clang-tidy support.
  4. Coverage support.
  5. REQUIRES-XFAIL/UNSUPPORTED-XFAIL.

Please split up this change into separate patches. Voltron-esque patches are hard to review, since we have to context switch so many times, and they're less useful when looking at git history because it isn't always immediately obvious which of the many things a given change was for.

cmake/Modules/LibcxxCodeCoverage.cmake
2 ↗(On Diff #15968)

This looks like a leftover.

10 ↗(On Diff #15968)

iirc --coverage implies -fprofile-arcs and -ftest-coverage.

13 ↗(On Diff #15968)

Can move these two checks up to where find_program is called

20 ↗(On Diff #15968)

MAKE_DIRECTORY?

22 ↗(On Diff #15968)

Don't we already have a cxx target?

37 ↗(On Diff #15968)

Why?

test/lit.cfg
33

Python style is kwarg=value (no space)

47

What was it before if not a string?

49

I don't think this does what you think it does.

You're probably looking for coverage_flags.split(' ')

50

Same as 47

51

Is this actually interesting to run on the tests? I would think this is the kind of thing we should be running on libc++ itself.

53

Same as 49

54

Same as 51

55

Same as 49

57

Diff fail :(

I think the new name is actually a better match for the old behavior (since this doesn't actually do the reporting now). I'd change the name back.

Hard to see what else changed... (should be more clear if the name changes back).

105

Do we actually need these? It looks like it's just complicating things.

183

Variable style changed here.

We're kind of inconsistent here anyway, but we should probably drift toward actually matching PEP8.

200

Hmm. I thought the driver would dump the gcno in the same directory as the output file (ditto for the gcd). What is this for?

234

spaces after ,

306

Why do we need on/off? Doesn't that just mean cmake is missing a pythonize_bool?

313

Why do we have two of these?

496

Should say which one is being used if we aren't making this an error

501

Doesn't rpath need an argument?

572

Don't think you meant to unalign them.

597

I don't quite understand why '' means yes, but 'true' means no. Shouldn't this just be

if not self.use_scan_build:
    ...
else:
    ...

? It changes the meaning slightly, but I think this behavior makes more sense. Am I missing something?

This revision now requires changes to proceed.Nov 10 2014, 8:29 PM
In D5877#18, @danalbert wrote:

Mega patches make me sad. I see 5 distinct changes here:

  1. Add missing testit features.
  2. scan-build support.
  3. clang-tidy support.
  4. Coverage support.
  5. REQUIRES-XFAIL/UNSUPPORTED-XFAIL.

Please split up this change into separate patches. Voltron-esque patches are hard to review, since we have to context switch so many times, and they're less useful when looking at git history because it isn't always immediately obvious which of the many things a given change was for.

Sorry about that. I'll split them up. I was hoping to get away with it this once. It's hard to maintain and merge 5 separate patches that exist in such a small space.

cmake/Modules/LibcxxCodeCoverage.cmake
2 ↗(On Diff #15968)

lcov depends on gcov. I thought we should check if gcov is found and issue a hard error otherwise.

10 ↗(On Diff #15968)

Yep. My mistake. Thanks.

20 ↗(On Diff #15968)

It creates a directory where all of the coverage files will live.

22 ↗(On Diff #15968)

Yep. This adds a command that executes before the cxx target.

37 ↗(On Diff #15968)

In case the resulting HTML is hosted on a web server this minimizes the permissions.

test/lit.cfg
47

I was being dumb.

49

coverage_flags should already be a list. This performs a copy of the list so we have our own.

51

Yes. The tests instantiate the templates. Without the instantiations the static analyzer has nothing to do.

57

Ok. Will do.

105

I'll pull these out for now, but the change is something I would like. It gives the ability to file the test as unsupported while ensuring it fails. It would be helpful for ignoring the std::tuple tests in c++03 mode.

200

The driver dumps the gcno file in the cwd used to invoke the compiler. Since the gcno will be named <test-name>.gcno and not the unique executable output name we need to mirror the test directory structure to ensure that gcno files for each test don't conflict.

306

This input could also come from the command line. I'm not opposed to only using one name for true and false but this gives flexibility.

313

Different semantics. I'm not a fan of the get_lit_bool semantics.

get_lit_bool:

  • None -> None
  • "" -> False

get_lit_switch:

  • None -> False
  • "" -> True

get_lit_switch allows the no-argument case to return True, and always returns a bool.

496

I'll make it an error.

501

It gets its argument from the second -Wl flag but I'll join them for clarity.

597

The semantics of use_scan_build are intended to be:

--param=use_scan_build[=<path/to/scan-build>]

Where the argument defaults to scan-build if omitted.

EricWF updated this revision to Diff 16026.Nov 10 2014, 9:52 PM
EricWF updated this object.
EricWF edited edge metadata.

I've stripped the patch down to a minimal set of changes so that it has the same functionality as the testit script.

I've removed:

  • UNSUPPORTED-XFAIL and REQUIRES-XFAIL
  • support for scan-build
  • support for clang-tidy
  • support for code coverage.
EricWF updated this revision to Diff 16028.Nov 10 2014, 10:45 PM
EricWF edited edge metadata.

Changed enable_warnings so it sets -Werror. Otherwise the switch is pretty useless.

danalbert requested changes to this revision.Nov 11 2014, 9:53 AM
danalbert edited edge metadata.

A bunch of style nits and a few doc changes, but otherwise looks good.

test/lit.cfg
41

Spaces after commas.

42

Why the docstring? I see you didn't actually put them here, but what's the point?

Same goes for the rest of them.

55

Spaces after commas.

128

Spaces after commas.

142–144

Spaces after commas.

154–155

Again.

219

Don't need the trailing \ when it's inside parentheses.

344

Alignment.

359

Agreed. Fatal makes more sense.

370

Alignment.

www/lit_usage.html
16

Space after colon.

54

s/it's/its/

62

s/"/'/

136

Should say how the list is delimited. Same for the others.

143

1z should probably come after 14.

This revision now requires changes to proceed.Nov 11 2014, 9:53 AM
mclow.lists edited edge metadata.Nov 12 2014, 10:18 PM

General comments:

  1. I'm liking how this is going. A lot.
  2. The symlink thing in using lit seems to be misguided to me. You should be able to specify a lit.site.cfg file on the command line. This will make it much easier to support multiple test configurations. I can think of 32 right off the bat: c++03/11/14/1z vs. 32/64 bit vs. No Sanitizer/ASAN/TSAN/UBSAN
  1. Did I mention that I like the fast that it's much faster than testit? :-)
EricWF updated this revision to Diff 16244.Nov 14 2014, 1:09 PM
EricWF edited edge metadata.

Fix more of @danalbert's comments. I would like to go forward with this patch even though there is a conflicting patch out against LIT.

danalbert accepted this revision.Dec 16 2014, 6:21 PM
danalbert edited edge metadata.

LGTM with the one doc change.

www/lit_usage.html
128

I had meant explicitly state that it is space delimited.

This revision is now accepted and ready to land.Dec 16 2014, 6:21 PM
EricWF updated this revision to Diff 17569.Dec 22 2014, 12:48 PM
EricWF updated this object.
EricWF edited edge metadata.

I've rewritten the patch against TOT. There are a couple of changes.

  • Removed ability to enable warnings for now.
  • Added documentation of the libcxx_site_config option.
  • Added documentation about how compile_flags and link_flags are delimited.
EricWF closed this revision.Dec 22 2014, 12:50 PM

LGTM. Thanks!