Skip to content

[coverage] Automatic merger for LLVM profile data #1126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Choose a reason for hiding this comment

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

What are the possible option values? What does default mean? Could we just use an explicit value, like none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The possible values are:

utils/build-script --swift-analyze-code-coverage -> default
utils/build-script -> none
utils/build-script --swift-analyze-code-coverage merged -> merged

Copy link
Contributor

Choose a reason for hiding this comment

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

... so let's document them here :)

Copy link
Contributor

Choose a reason for hiding this comment

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

And would you mind renaming default to not-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.

No problem! 👍

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