[PATCH] [libcxx] Add numerous options to libc++ LIT test suite configuration.
Dan Albert
danalbert at google.com
Mon Nov 10 20:29:55 PST 2014
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.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:2
@@ +1,3 @@
+
+find_program(LIBCXX_GCOV gcov)
+if (NOT LIBCXX_GCOV)
----------------
This looks like a leftover.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:10
@@ +9,3 @@
+
+set(LIBCXX_CXX_COVERAGE_FLAGS "-g -O0 --coverage -fprofile-arcs -ftest-coverage")
+
----------------
iirc --coverage implies -fprofile-arcs and -ftest-coverage.
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:13
@@ +12,3 @@
+function(setup_lcov_test_target_coverage _coverage_dir)
+ if (NOT LIBCXX_LCOV)
+ message(FATAL_ERROR "Cannot find lcov...")
----------------
Can move these two checks up to where find_program is called
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:20
@@ +19,3 @@
+
+ file(MAKE_DIRECTORY ${_coverage_dir})
+
----------------
MAKE_DIRECTORY?
================
Comment at: cmake/Modules/LibcxxCodeCoverage.cmake:22
@@ +21,3 @@
+
+ add_custom_command(TARGET cxx
+ PRE_BUILD
----------------
Don't we already have a 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}
----------------
Why?
================
Comment at: test/lit.cfg:33
@@ +32,3 @@
+ test_root,
+ use_verify_for_fail = False,
+ generate_coverage = False,
----------------
Python style is kwarg=value (no space)
================
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
----------------
What was it before if not a string?
================
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)
----------------
I don't think this does what you think it does.
You're probably looking for coverage_flags.split(' ')
================
Comment at: test/lit.cfg:50
@@ +49,3 @@
+ self.coverage_flags = list(coverage_flags)
+ self.coverage_root = str(coverage_root)
+ self.use_scan_build = use_scan_build
----------------
Same as 47
================
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
----------------
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.
================
Comment at: test/lit.cfg:53
@@ +52,3 @@
+ self.scan_build_output = scan_build_output
+ self.scan_build_args = list(scan_build_args)
+ self.use_clang_tidy = use_clang_tidy
----------------
Same as 49
================
Comment at: test/lit.cfg:54
@@ +53,3 @@
+ self.scan_build_args = list(scan_build_args)
+ self.use_clang_tidy = use_clang_tidy
+ self.clang_tidy_args = list(clang_tidy_args)
----------------
Same as 51
================
Comment at: test/lit.cfg:55
@@ +54,3 @@
+ self.use_clang_tidy = use_clang_tidy
+ self.clang_tidy_args = list(clang_tidy_args)
+
----------------
Same as 49
================
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)
----------------
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).
================
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
----------------
Do we actually need these? It looks like it's just complicating things.
================
Comment at: test/lit.cfg:183
@@ -123,18 +182,3 @@
'-o', '/dev/null', source_path] + self.cpp_flags
- expected_rc = 1
- if use_verify:
- cmd += ['-Xclang', '-verify']
- expected_rc = 0
- out, err, rc = self.execute_command(cmd)
- if rc == expected_rc:
- return lit.Test.PASS, ""
- else:
- report = """Command: %s\n""" % ' '.join(["'%s'" % a
- for a in cmd])
- report += """Exit Code: %d\n""" % rc
- if out:
- report += """Standard Output:\n--\n%s--""" % out
- if err:
- report += """Standard Error:\n--\n%s--""" % err
- report += "\n\nExpected compilation to fail!"
- return lit.Test.FAIL, report
+ expectedExitCode = 1
+ if use_verify:
----------------
Variable style changed here.
We're kind of inconsistent here anyway, but we should probably drift toward actually matching PEP8.
================
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
----------------
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?
================
Comment at: test/lit.cfg:234
@@ +233,3 @@
+ cmd += compile_cmd
+ report,exitCode,_,_ = self.execute_command_and_report(cmd, cwd=build_cwd)
+ if exitCode != 0 or (self.use_scan_build and
----------------
spaces after ,
================
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
----------------
Why do we need on/off? Doesn't that just mean cmake is missing a pythonize_bool?
================
Comment at: test/lit.cfg:313
@@ +312,3 @@
+
+ def get_lit_switch(self, name):
+ s = self.get_lit_conf(name)
----------------
Why do we have two of these?
================
Comment at: test/lit.cfg:496
@@ +495,3 @@
+ self.lit_config.warning(
+ 'Conflicting options detected: libcxx_library and use_system_lib')
+ libcxx_library = None
----------------
Should say which one is being used if we aren't making this 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)]
----------------
Doesn't rpath need an argument?
================
Comment at: test/lit.cfg:572
@@ -440,3 +571,3 @@
self.compile_flags += ['-fsanitize=undefined',
- '-fno-sanitize=vptr,function',
- '-fno-sanitize-recover']
+ '-fno-sanitize=vptr,function',
+ '-fno-sanitize-recover']
----------------
Don't think you meant to unalign them.
================
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
----------------
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?
http://reviews.llvm.org/D5877
More information about the cfe-commits
mailing list