Skip to content

Conversation

lplarson
Copy link
Contributor

This patch adds support to the build system to allow instrumenting the Swift compiler for measuring code coverage.

@gribozavr Please review this patch.

@dabrahams
Copy link
Contributor

Is this for measuring coverage of the compiler’s source code, or of Swift code built with the compiler?

@@ -623,6 +623,13 @@ if (LLVM_ENABLE_DOXYGEN)
message(STATUS "Doxygen: enabled")
endif()

if(SWIFT_ANALYZE_CODE_COVERAGE)
set(LIBCLANG_RT_PROFILE "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION}/lib/darwin/libclang_rt.profile_osx.a")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use the just-built runtime libraries with the code built by the host compiler. Even if this succeeds, it is pure luck and an unsupported configuration as far as I know.

We need to track use the runtime library provided with the toolchain for code built by the host clang.

@lplarson
Copy link
Contributor Author

@dabrahams Measuring coverage of the compiler’s source code.

@lplarson
Copy link
Contributor Author

@gribozavr I've made the requested changes.

lplarson added a commit that referenced this pull request Jan 20, 2016
[CMake] Support code coverage analysis
@lplarson lplarson merged commit 534a191 into swiftlang:master Jan 20, 2016
@@ -234,6 +238,11 @@ macro(add_sourcekit_executable name)
set_target_properties(${name}
PROPERTIES
LINK_FLAGS "-Wl,-exported_symbol,_main")

if(SWIFT_ANALYZE_CODE_COVERAGE)
set_property(TARGET "${name}" APPEND_STRING PROPERTY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this snippet (and other similar ones below) still needed? I'd think that _add_variant_c_compile_link_flags would add these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gribozavr set_target_properties overwrites LINK_FLAGS. If we can remove the set_target_properties call without changing something important then we can remove the extra code coverage configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but what I'm saying is that _add_variant_c_compile_link_flags should return these two flags, and they will be added to LINK_FLAGS by other machinery in AddSwift.cmake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_add_variant_c_compile_link_flags does return the two code coverage flags, but they're overwritten later by set_target_properties. Building the project without these snippets results in linker errors due to missing code coverage libraries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, which set_target_properties call are you referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the snippet in this one because add_sourcekit_default_compiler_flags is called after:
./tools/SourceKit/CMakeLists.txt

I'm mostly talking about set_target_properties in these since the override is the last thing that happens to the target:
./tools/SourceKit/tools/complete-test/CMakeLists.txt
./tools/SourceKit/tools/sourcekitd-repl/CMakeLists.txt
./tools/SourceKit/tools/sourcekitd-test/CMakeLists.txt
./unittests/CMakeLists.txt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, those calls to set_target_properties look wrong to me. They should append instead of replacing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants