[PATCH] [libcxx] Add numerous options to libc++ LIT test suite configuration.
Eric Fiselier
eric at efcs.ca
Mon Nov 10 21:06:42 PST 2014
>>! 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.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:2
@@ +1,3 @@
+
+find_program(LIBCXX_GCOV gcov)
+if (NOT LIBCXX_GCOV)
----------------
danalbert wrote:
> This looks like a leftover.
lcov depends on gcov. I thought we should check if gcov is found and issue a hard error otherwise.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:10
@@ +9,3 @@
+
+set(LIBCXX_CXX_COVERAGE_FLAGS "-g -O0 --coverage -fprofile-arcs -ftest-coverage")
+
----------------
danalbert wrote:
> iirc --coverage implies -fprofile-arcs and -ftest-coverage.
Yep. My mistake. Thanks.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:20
@@ +19,3 @@
+
+ file(MAKE_DIRECTORY ${_coverage_dir})
+
----------------
danalbert wrote:
> MAKE_DIRECTORY?
It creates a directory where all of the coverage files will live.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:22
@@ +21,3 @@
+
+ add_custom_command(TARGET cxx
+ PRE_BUILD
----------------
danalbert wrote:
> Don't we already have a cxx target?
Yep. This adds a command that executes before the cxx target.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:37
@@ +36,3 @@
+ COMMAND ${CMAKE_COMMAND} -E remove_directory ${_coverage_dir}/test
+ COMMAND find libcxx_coverage/ -type d -exec chmod 750 ${_CHMOD_END}
+ COMMAND find libcxx_coverage/ -type f -exec chmod 640 ${_CHMOD_END}
----------------
danalbert wrote:
> Why?
In case the resulting HTML is hosted on a web server this minimizes the permissions.
================
Comment at: test/lit.cfg:47
@@ -37,19 +46,3 @@
self.exec_env = dict(exec_env)
-
- def execute_command(self, command, in_dir=None):
- kwargs = {
- 'stdin' :subprocess.PIPE,
- 'stdout':subprocess.PIPE,
- 'stderr':subprocess.PIPE,
- }
- if in_dir:
- kwargs['cwd'] = in_dir
- p = subprocess.Popen(command, **kwargs)
- out,err = p.communicate()
- exitCode = p.wait()
-
- # Detect Ctrl-C in subprocess.
- if exitCode == -signal.SIGINT:
- raise KeyboardInterrupt
-
- return out, err, exitCode
+ self.test_root = str(test_root)
+ self.generate_coverage = generate_coverage
----------------
danalbert wrote:
> What was it before if not a string?
I was being dumb.
================
Comment at: test/lit.cfg:49
@@ +48,3 @@
+ self.generate_coverage = generate_coverage
+ self.coverage_flags = list(coverage_flags)
+ self.coverage_root = str(coverage_root)
----------------
danalbert wrote:
> I don't think this does what you think it does.
>
> You're probably looking for coverage_flags.split(' ')
coverage_flags should already be a list. This performs a copy of the list so we have our own.
================
Comment at: test/lit.cfg:51
@@ +50,3 @@
+ self.coverage_root = str(coverage_root)
+ self.use_scan_build = use_scan_build
+ self.scan_build_output = scan_build_output
----------------
danalbert wrote:
> 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.
Yes. The tests instantiate the templates. Without the instantiations the static analyzer has nothing to do.
================
Comment at: test/lit.cfg:57
@@ +56,3 @@
+
+ def execute_command_and_report(self, command, cwd=None, env=None, compile_cmd=None):
+ exec_env = dict(os.environ)
----------------
danalbert wrote:
> 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).
Ok. Will do.
================
Comment at: test/lit.cfg:105
@@ -72,10 +104,3 @@
for ln in f:
- if 'XFAIL:' in ln:
- items = ln[ln.index('XFAIL:') + 6:].split(',')
- test.xfails.extend([s.strip() for s in items])
- elif 'REQUIRES:' in ln:
- items = ln[ln.index('REQUIRES:') + 9:].split(',')
- requires.extend([s.strip() for s in items])
- elif 'UNSUPPORTED:' in ln:
- items = ln[ln.index('UNSUPPORTED:') + 12:].split(',')
- unsupported.extend([s.strip() for s in items])
+ if self._handle_metadata_line(ln, 'REQUIRES-XFAIL:', requires_xfail):
+ pass
----------------
danalbert wrote:
> Do we actually need these? It looks like it's just complicating things.
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.
================
Comment at: test/lit.cfg:200
@@ +199,3 @@
+ # If we are generating code coverage data run the compile command from
+ # the directory the .gcno and .gcda files should be put in.
+ build_cwd = None
----------------
danalbert wrote:
> 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?
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.
================
Comment at: test/lit.cfg:306
@@ -219,3 +305,3 @@
return None
- if conf.lower() in ('1', 'true'):
+ if conf.lower() in ('1', 'true', 'on'):
return True
----------------
danalbert wrote:
> Why do we need on/off? Doesn't that just mean cmake is missing a pythonize_bool?
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.
================
Comment at: test/lit.cfg:313
@@ +312,3 @@
+
+ def get_lit_switch(self, name):
+ s = self.get_lit_conf(name)
----------------
danalbert wrote:
> Why do we have two of these?
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.
================
Comment at: test/lit.cfg:496
@@ +495,3 @@
+ self.lit_config.warning(
+ 'Conflicting options detected: libcxx_library and use_system_lib')
+ libcxx_library = None
----------------
danalbert wrote:
> Should say which one is being used if we aren't making this an error
I'll make it an error.
================
Comment at: test/lit.cfg:501
@@ +500,3 @@
+ if libcxx_library:
+ self.link_flags += [libcxx_library, '-Wl,-rpath',
+ '-Wl,' + os.path.dirname(libcxx_library)]
----------------
danalbert wrote:
> Doesn't rpath need an argument?
It gets its argument from the second `-Wl` flag but I'll join them for clarity.
================
Comment at: test/lit.cfg:597
@@ +596,3 @@
+ self.use_scan_build = self.get_lit_conf('use_scan_build')
+ if self.use_scan_build is None:
+ return
----------------
danalbert wrote:
> 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?
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.
http://reviews.llvm.org/D5877
More information about the cfe-commits
mailing list