Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
eb27ca1
[coverage] Initial commit of profdata merging worker
harlanhaskins Jan 25, 2016
2cf852f
[coverage] Cleaned up profdata merge worker and added debug flag
harlanhaskins Jan 26, 2016
c6c0b3d
[coverage] Removed explicit swift-%p.profraw arguments from CMake com…
harlanhaskins Jan 26, 2016
32fda29
[coverage] Made subclass of list test format so we can hook into each…
harlanhaskins Jan 26, 2016
a41ec1a
[coverage] Cleaned up argument parsing for swift coverage, and hooked…
harlanhaskins Jan 26, 2016
78612e5
[coverage] Added before_test and after_test hooks in SwiftTest
harlanhaskins Jan 26, 2016
03964cc
[coverage] Simplified before_test and after_test
harlanhaskins Jan 26, 2016
3ea61fb
[coverage] Fixed runtime error in lit.cfg
harlanhaskins Jan 27, 2016
9eb729c
[coverage] Reworked CMake invocation for coverage testing given there…
harlanhaskins Jan 27, 2016
fb6ee1a
[coverage] Added argument parsing to profdata merge worker and cleane…
harlanhaskins Jan 27, 2016
259af4d
[coverage] Cleaned up target generation and lit merging code
harlanhaskins Jan 27, 2016
c83fb1c
[coverage] Fixed CMake invocation of profdata merge worker
harlanhaskins Jan 27, 2016
1b30a49
[coverage] Added license header to profdata_merge_worker and added ex…
harlanhaskins Jan 27, 2016
5a2a809
[coverage] Removed printing in CMake
harlanhaskins Jan 28, 2016
9fccf50
[coverage] Fixed conflicts with master
harlanhaskins Jan 28, 2016
bb8160a
[coverage] Declared SWIFT_ANALYZE_CODE_COVERAGE in CMakeLists and doc…
harlanhaskins Jan 28, 2016
b4c7678
[coverage] Documented modes for SWIFT_ANALYZE_CODE_COVERAGE and added…
harlanhaskins Jan 28, 2016
e997fc3
[coverage] Changed 'none' to 'false' for coverage default
harlanhaskins Jan 28, 2016
44be500
[coverage] Changed back to old coverage check in CMakeLists.txt files
harlanhaskins Jan 28, 2016
7608f88
[coverage] Removed print in built script
harlanhaskins Jan 28, 2016
b1d6e17
[coverage] Added coverage_mode definitions in validation test and Uni…
harlanhaskins Jan 28, 2016
0e6f798
[coverage] Made sure code coverage doesn't rename the ninja build dir…
harlanhaskins Jan 29, 2016
e9d009e
[coverage] Merged and fixed conflicts
harlanhaskins Jan 29, 2016
44e44ad
Merged upstream master into profdata-merge
harlanhaskins Jan 29, 2016
b8a2249
Merged upstream master into profdata-merge
harlanhaskins Jan 29, 2016
96d4261
[coverage] Made some stylistic changes for CMake consistency
harlanhaskins Feb 1, 2016
1777c20
[coverage] Fixed indentation in CMake
harlanhaskins Feb 1, 2016
d41bd9a
[coverage] Made SWIFT_ANALYZE_CODE_COVERAGE a string option instead o…
harlanhaskins Feb 1, 2016
f99fd75
[coverage] Converted global non-constants to explicitly passed parame…
harlanhaskins Feb 1, 2016
d7eff48
[coverage] Converted to print function
harlanhaskins Feb 1, 2016
693d7da
[coverage] Converted to explicit super in lit.cfg
harlanhaskins Feb 1, 2016
6572267
[coverage] Split profdata_merge_worker.py into separate files within …
harlanhaskins Feb 1, 2016
dea48f4
[coverage] Fixed missing import
harlanhaskins Feb 1, 2016
eb9fb9c
[coverage] Used gettempdir() instead of hard-coding /tmp
harlanhaskins Feb 2, 2016
45df849
[coverage] Explicit super in process.py
harlanhaskins Feb 2, 2016
873af8e
[coverage] Fail gracefully on non-Darwin
harlanhaskins Feb 2, 2016
f713042
[coverage] Added license header to config.py
harlanhaskins Feb 2, 2016
c75fa76
[coverage] Added README for the profdata module
harlanhaskins Feb 2, 2016
6ec0dd3
[coverage] Added log file output to profdata merger
harlanhaskins Feb 2, 2016
cf30318
[coverage] Converted to python standard logging framework
harlanhaskins Feb 2, 2016
13bae93
[coverage] pep8
harlanhaskins Feb 2, 2016
2b22040
[coverage] Removed unused imports
harlanhaskins Feb 2, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ option(SWIFT_INCLUDE_DOCS
"Create targets for building docs."
TRUE)

option(SWIFT_ANALYZE_CODE_COVERAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use FALSE or NO as the default. See the CMake docs for if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's no longer a boolean flag -- it's one of three possibilities, none, not-merged, or merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, I just scrolled down far enough to see that this has modes now. Can you rephrase the description to make that clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though maybe we should use FALSE as the none case.
So the values are FALSE, NOT-MERGED, and MERGED?

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 OFF makes sense, but for consistency's sake I think we should stick with FALSE. I'll default it to that and make sure it propagates as FALSE through build-script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, we still won't be able to revert to if(SWIFT_ANALYZE_CODE_COVERAGE) because it'll default to FALSE for all the possible cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh? "if (<variable>): True if the variable is defined to a value that is not a false constant".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Must've misread that, then. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

CMake has a way to specify stringly-typed variables with multiple allowed values, see line 16.

"Build Swift with code coverage instrumenting enabled."
"NONE")

set(SWIFT_VERSION "3.0" CACHE STRING
"The user-visible version of the Swift compiler")
set(SWIFT_VENDOR "" CACHE STRING
Expand Down
4 changes: 2 additions & 2 deletions cmake/modules/AddSwift.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ function(_add_variant_c_compile_link_flags
"-m${SWIFT_SDK_${sdk}_VERSION_MIN_NAME}-version-min=${SWIFT_SDK_${sdk}_DEPLOYMENT_VERSION}")

if(analyze_code_coverage)
list(APPEND result "-fprofile-instr-generate=swift-%p.profraw"
list(APPEND result "-fprofile-instr-generate"
"-fcoverage-mapping")
endif()
endif()
Expand Down Expand Up @@ -111,7 +111,7 @@ function(_add_variant_c_compile_flags
endif()

if(analyze_code_coverage)
list(APPEND result "-fprofile-instr-generate=swift-%p.profraw"
list(APPEND result "-fprofile-instr-generate"
"-fcoverage-mapping")
endif()

Expand Down
22 changes: 22 additions & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ if(PYTHONINTERP_FOUND)

set(TEST_MODES optimize_none optimize optimize_unchecked)


foreach(SDK ${SWIFT_SDKS})
foreach(ARCH ${SWIFT_SDK_${SDK}_ARCHITECTURES})
foreach(TEST_MODE ${TEST_MODES})
Expand Down Expand Up @@ -196,30 +197,50 @@ if(PYTHONINTERP_FOUND)
"${CMAKE_CURRENT_SOURCE_DIR}/../validation-test/lit.site.cfg.in"
"${validation_test_bin_dir}/lit.site.cfg"
"validation-test${VARIANT_SUFFIX}.lit.site.cfg")
set(profdata_merge_worker
"${CMAKE_CURRENT_SOURCE_DIR}/../utils/profdata_merge_worker.py")

if(SWIFT_ANALYZE_CODE_COVERAGE STREQUAL "MERGED")
set(command_profdata_merge_start
Copy link
Contributor

Choose a reason for hiding this comment

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

2 spaces of indentation.

COMMAND ${PYTHON_EXECUTABLE} ${profdata_merge_worker} start -o ${swift_test_results_dir})
Copy link
Contributor

Choose a reason for hiding this comment

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

Please quote variables.

set(command_profdata_merge_stop
COMMAND ${PYTHON_EXECUTABLE} ${profdata_merge_worker} stop)
else()
set(command_profdata_merge_start)
set(command_profdata_merge_stop)
endif()

add_custom_target("check-swift${test_mode_target_suffix}${VARIANT_SUFFIX}"
${command_upload_stdlib}
${command_clean_test_results_dir}
${command_profdata_merge_start}
COMMAND ${lit_command} "${test_bin_dir}"
${command_profdata_merge_stop}
DEPENDS ${test_dependencies}
COMMENT "Running Swift tests for ${VARIANT_TRIPLE}"
${cmake_3_2_USES_TERMINAL})

add_custom_target("check-swift-validation${test_mode_target_suffix}${VARIANT_SUFFIX}"
${command_upload_stdlib}
${command_clean_test_results_dir}
${command_profdata_merge_start}
COMMAND ${lit_command} "${validation_test_bin_dir}"
${command_profdata_merge_stop}
DEPENDS ${test_dependencies} ${validation_test_dependencies}
COMMENT "Running Swift validation tests for ${VARIANT_TRIPLE}"
${cmake_3_2_USES_TERMINAL})

add_custom_target("check-swift-all${test_mode_target_suffix}${VARIANT_SUFFIX}"
${command_upload_stdlib}
${command_clean_test_results_dir}
${command_profdata_merge_start}
COMMAND ${lit_command} "${validation_test_bin_dir}" "${test_bin_dir}"
${command_profdata_merge_stop}
DEPENDS ${test_dependencies} ${validation_test_dependencies}
COMMENT "Running all Swift tests for ${VARIANT_TRIPLE}"
${cmake_3_2_USES_TERMINAL})


endforeach()
endforeach()
endforeach()
Expand All @@ -245,6 +266,7 @@ if(PYTHONINTERP_FOUND)

add_custom_target(check-swift-all${test_mode_target_suffix}
DEPENDS "check-swift-all${test_mode_target_suffix}${SWIFT_PRIMARY_VARIANT_SUFFIX}")

endforeach()

endif()
Expand Down
38 changes: 37 additions & 1 deletion test/lit.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ import re
import subprocess
import sys
import tempfile
import socket
import glob

import lit
import lit.formats
import lit.util

Expand Down Expand Up @@ -129,6 +132,39 @@ if config.test_exec_root is None:

###

class SwiftTest(lit.formats.ShTest):
def __init__(self, coverage_mode=None, execute_external=True):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from lit.site.cfg.in -- CMake fills it in with the appropriate value, and it's used to determine the pre- and post-test behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, whoops! I deleted my comment too late--I had misread 😅

lit.formats.ShTest.__init__(self, execute_external)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the keyword arg required here?

self.coverage_mode = None if coverage_mode == "NONE" else coverage_mode

def profdir_for_test(self, test):
_, tmp_base = lit.TestRunner.getTempPaths(test)
return tmp_base + ".profdir"

def before_test(self, test, litConfig):
if self.coverage_mode:
profdir = self.profdir_for_test(test)
if not os.path.exists(profdir):
os.makedirs(profdir)

test.config.environment["LLVM_PROFILE_FILE"] = os.path.join(profdir, "swift-%p.profraw")

def after_test(self, test, litConfig, result):
if self.coverage_mode == "MERGED":
profdir = self.profdir_for_test(test)
files = glob.glob(os.path.join(profdir, "*.profraw"))
sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
sock.connect(('localhost', 12400))
sock.send("\n".join(files))
sock.close()
return result


def execute(self, test, litConfig):
self.before_test(test, litConfig)
result = super(SwiftTest, self).execute(test, litConfig)
return self.after_test(test, litConfig, result)

# name: The name of this test suite.
config.name = 'Swift'

Expand All @@ -138,7 +174,7 @@ if platform.system() == 'Darwin':
config.environment['TOOLCHAINS'] = 'default'

# testFormat: The test format to use to interpret tests.
config.test_format = lit.formats.ShTest(execute_external=True)
config.test_format = SwiftTest(coverage_mode=config.coverage_mode)

# suffixes: A list of file extensions to treat as test files.
config.suffixes = ['.swift', '.ll', '.sil', '.gyb', '.m']
Expand Down
7 changes: 2 additions & 5 deletions test/lit.site.cfg.in
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ config.variant_sdk = "@VARIANT_SDK@"
config.swiftlib_dir = "@LIT_SWIFTLIB_DIR@"
config.darwin_xcrun_toolchain = "@SWIFT_DARWIN_XCRUN_TOOLCHAIN@"

config.coverage_mode = "@SWIFT_ANALYZE_CODE_COVERAGE@"

if "@SWIFT_ASAN_BUILD@" == "TRUE":
config.available_features.add("asan")
else:
Expand All @@ -42,11 +44,6 @@ if "@SWIFT_OPTIMIZED@" == "TRUE":
if "@SWIFT_HAVE_WORKING_STD_REGEX@" == "FALSE":
config.available_features.add('broken_std_regex')

if "@SWIFT_ANALYZE_CODE_COVERAGE@" == "TRUE":
lit_config.useValgrind = True
lit_config.valgrindArgs = [os.path.join(config.swift_src_root,
"utils/use_profdir.py")]

# Let the main config do the real work.
if config.test_exec_root is None:
config.test_exec_root = os.path.dirname(os.path.realpath(__file__))
Expand Down
6 changes: 3 additions & 3 deletions tools/SourceKit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,9 @@ macro(add_sourcekit_executable name)
PROPERTIES
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this invocation to be set_property(TARGET "..." APPEND_STRING PROPERTY LINK_FLAGS)? Then you won't need to duplicate the code coverage flags below?

LINK_FLAGS "-Wl,-exported_symbol,_main")

if(SWIFT_ANALYZE_CODE_COVERAGE)
set_property(TARGET "${name}" APPEND_STRING PROPERTY
LINK_FLAGS " -fprofile-instr-generate=swift-%p.profraw -fcoverage-mapping")
if(NOT SWIFT_ANALYZE_CODE_COVERAGE STREQUAL "NONE")
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.

2 spaces.

LINK_FLAGS " -fprofile-instr-generate -fcoverage-mapping")
endif()
endif()
endif()
Expand Down
4 changes: 2 additions & 2 deletions tools/SourceKit/tools/complete-test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
PROPERTIES
LINK_FLAGS "-Wl,-rpath -Wl,@executable_path/../lib")

if(SWIFT_ANALYZE_CODE_COVERAGE)
if(NOT SWIFT_ANALYZE_CODE_COVERAGE STREQUAL "NONE")
set_property(TARGET complete-test APPEND_STRING PROPERTY
LINK_FLAGS " -fprofile-instr-generate=swift-%p.profraw -fcoverage-mapping")
LINK_FLAGS " -fprofile-instr-generate -fcoverage-mapping")
endif()
endif()

Expand Down
4 changes: 2 additions & 2 deletions tools/SourceKit/tools/sourcekitd-repl/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
PROPERTIES
LINK_FLAGS "-Wl,-rpath -Wl,@executable_path/../lib")

if(SWIFT_ANALYZE_CODE_COVERAGE)
if(NOT SWIFT_ANALYZE_CODE_COVERAGE STREQUAL "NONE")
set_property(TARGET sourcekitd-repl APPEND_STRING PROPERTY
LINK_FLAGS " -fprofile-instr-generate=swift-%p.profraw -fcoverage-mapping")
LINK_FLAGS " -fprofile-instr-generate -fcoverage-mapping")
endif()
endif()

Expand Down
4 changes: 2 additions & 2 deletions tools/SourceKit/tools/sourcekitd-test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
PROPERTIES
LINK_FLAGS "-Wl,-rpath -Wl,@executable_path/../lib")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you change this to append to LINK_FLAGS, then you won't need to duplicate appending -fprofile-instr-generate below -- it will come from AddSwift.cmake. Because this call to set_target_properties does not append, it overwrites the flags set by AddSwift.cmake.


if(SWIFT_ANALYZE_CODE_COVERAGE)
if(NOT SWIFT_ANALYZE_CODE_COVERAGE STREQUAL "NONE")
set_property(TARGET sourcekitd-test APPEND_STRING PROPERTY
LINK_FLAGS " -fprofile-instr-generate=swift-%p.profraw -fcoverage-mapping")
LINK_FLAGS " -fprofile-instr-generate -fcoverage-mapping")
endif()
endif()

Expand Down
4 changes: 2 additions & 2 deletions unittests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ function(add_swift_unittest test_dirname)
set_property(TARGET "${test_dirname}" APPEND_STRING PROPERTY
LINK_FLAGS " -Xlinker -rpath -Xlinker ${SWIFT_LIBRARY_OUTPUT_INTDIR}/swift/macosx")

if(SWIFT_ANALYZE_CODE_COVERAGE)
if(NOT SWIFT_ANALYZE_CODE_COVERAGE STREQUAL "NONE")
set_property(TARGET "${test_dirname}" APPEND_STRING PROPERTY
LINK_FLAGS " -fprofile-instr-generate=swift-%p.profraw -fcoverage-mapping")
LINK_FLAGS " -fprofile-instr-generate -fcoverage-mapping")
endif()
endif()
endfunction()
Expand Down
11 changes: 6 additions & 5 deletions utils/build-script
Original file line number Diff line number Diff line change
Expand Up @@ -512,11 +512,12 @@ also build for Apple watchos, but disallow tests that require an watchOS device"
action="store_true")

parser.add_argument("--swift-analyze-code-coverage",
help="enable code coverage analysis in Swift",
action="store_const",
const=True,
dest="swift_analyze_code_coverage",
default=False)
help="""enable code coverage analysis in Swift (set to `merged` to merge
and remove intermediary profdata files)""",
const="not-merged",
choices=["none", "not-merged", "merged"],
nargs='?',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we always require the option value? This is an expert-only option and adding subtle implicit behavior only adds debugging time.

dest="swift_analyze_code_coverage")

parser.add_argument("--build-subdir",
help="""
Expand Down
4 changes: 2 additions & 2 deletions utils/build-script-impl
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ KNOWN_SETTINGS=(
llvm-enable-assertions "1" "enable assertions in LLVM and Clang"
swift-build-type "Debug" "the CMake build variant for Swift"
swift-enable-assertions "1" "enable assertions in Swift"
swift-analyze-code-coverage "0" "enable code coverage analysis in Swift"
swift-analyze-code-coverage "not-merged" "Code coverage analysis mode for Swift (none, not-merged, merged). Defaults to none if the argument is not present, and not-merged if the argument is present without a modifier."
swift-stdlib-build-type "Debug" "the CMake build variant for Swift"
swift-stdlib-enable-assertions "1" "enable assertions in Swift"
lldb-build-type "Debug" "the CMake build variant for LLDB"
Expand Down Expand Up @@ -1600,7 +1600,7 @@ for deployment_target in "${HOST_TARGET}" "${CROSS_TOOLS_DEPLOYMENT_TARGETS[@]}"
-DCMAKE_CXX_FLAGS="$(swift_c_flags ${deployment_target})"
-DCMAKE_BUILD_TYPE:STRING="${SWIFT_BUILD_TYPE}"
-DLLVM_ENABLE_ASSERTIONS:BOOL=$(true_false "${SWIFT_ENABLE_ASSERTIONS}")
-DSWIFT_ANALYZE_CODE_COVERAGE:BOOL=$(true_false "${SWIFT_ANALYZE_CODE_COVERAGE}")
-DSWIFT_ANALYZE_CODE_COVERAGE:STRING=$(toupper "${SWIFT_ANALYZE_CODE_COVERAGE}")
-DSWIFT_STDLIB_BUILD_TYPE:STRING="${SWIFT_STDLIB_BUILD_TYPE}"
-DSWIFT_STDLIB_ASSERTIONS:BOOL=$(true_false "${SWIFT_STDLIB_ENABLE_ASSERTIONS}")
-DSWIFT_NATIVE_LLVM_TOOLS_PATH:STRING="${native_llvm_tools_path}"
Expand Down
Loading