[coverage] Automatic merger for LLVM profile data #1126

harlanhaskins · 2016-01-28T18:03:50Z

When build-script is given --swift-analyze-code-coverage merged, this will fire off this worker which responds to tests and merges generated profile data into a final coverage file in the test results folder.

…mands, because we're using LLVM's environment variables

… test run and properly create profile directories. This removes the need for use_profdir.py and removes the valgrind hack from lit.site.cfg.in

… into build-script to propagate the different values

… are three states for SWIFT_ANALYZE_CODE_COVERAGE

…d up the implementation

…plicit choices to --swift-analyze-code-coverage

gribozavr · 2016-01-28T18:23:04Z

tools/SourceKit/tools/sourcekitd-test/CMakeLists.txt

@@ -23,9 +23,9 @@ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
    PROPERTIES
    LINK_FLAGS "-Wl,-rpath -Wl,@executable_path/../lib")


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.

gribozavr · 2016-01-28T18:25:16Z

I also don't see a definition of SWIFT_ANALYZE_CODE_COVERAGE in the top-level CMakeLists.txt, could you add it (with a list of possible values)?

…umented options for build-script flag.

jrose-apple · 2016-01-28T19:00:40Z

CMakeLists.txt

@@ -58,6 +58,10 @@ option(SWIFT_INCLUDE_DOCS
    "Create targets for building docs."
    TRUE)

+option(SWIFT_ANALYZE_CODE_COVERAGE


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

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

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

Yep, will do.

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

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

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.

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

Oh! Must've misread that, then. 👍

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

… comment about llvm-profdata

shahmishal · 2016-01-28T20:11:51Z

@swift-ci Please test

…t test lit.site.cfg.in

shahmishal · 2016-01-28T23:43:38Z

@swift-ci Please test

…ectory if it's "false"

modocache · 2016-02-01T22:53:51Z

test/lit.cfg

@@ -129,6 +132,43 @@ if config.test_exec_root is None:

 ###

+class SwiftTest(lit.formats.ShTest):
+    def __init__(self, coverage_mode=None, execute_external=True):
+        lit.formats.ShTest.__init__(self, execute_external=execute_external)


I think it's preferable to use super here, as in:

super(SwiftTest, self).__init__(execute_external=excute_external)

…ters

modocache · 2016-02-01T23:15:23Z

utils/profdata_merge_worker.py

+    stop.set_defaults(func=stop_server)
+
+    args = parser.parse_args()
+    args.func(args)


It would be nice to create a Python module for this, as opposed to a single file. It'd allow you to:

Add tests for the module. You could also add the tests to the Swift validation test suite, as in [validation-test] Add line-directive doctests #810.

Split the file up. Config, ProfdataMergerProcess, ProfdataTCPHandler, and even the argument parser could each get their own file. Where possible, it would be nice to unit test these classes.

modocache · 2016-02-01T23:18:49Z

utils/profdata_merge_worker.py

+        # FIXME: This doesn't necessarily always line up with the version
+        #        of clang++ used to build the binaries.
+        llvm_cmd = ("xcrun llvm-profdata merge -o %s %s"
+            % (self.profdata_path, cleaned_files))


In addition to not matching the clang++ used to build the binaries, this command also would not work on Linux. Is this not meant to be cross-platform?

Would it be possible to somehow access utils/swift_build_support? swift_build_support.clang includes utilities to find the host machine's clang++. These might be overridden by specifying build-script-impl args, though, so even that's not foolproof...

We'd love to make it work on Linux, but for right now we're okay with it being OS X-only. This depends on having a suitable version of compiler-rt installed alongside clang, which we can guarantee with Xcode.

Sounds good! Would it be possible to fail gracefully for Linux, then? I imagine users of this script may be confused to suddenly encounter a subprocess stack trace where xcrun is called.

It's gonna print an error and exit -- that's now in main.py. 👍

…a module

harlanhaskins · 2016-02-01T23:56:22Z

@modocache I agree that the script would be better off as a module. Let me know what you think, and thanks so much for the review!

modocache · 2016-02-01T23:59:28Z

utils/profdata_merge/main.py

+if __name__ == "__main__":
+    parser = argparse.ArgumentParser()
+
+    subparsers = parser.add_subparsers()


Wow, I had no idea about add_subparsers()! Very cool functionality. 👍

I actually found out about it in utils/build-script!

modocache · 2016-02-02T00:14:43Z

Thank you for the fascinating read, @harlanhaskins. And I've never seen anyone address comments so quickly! 😁

My parting thought is that it would be nice if we could verify the behavior of this code without actually building Swift itself--it seems like it would be difficult for someone new to this codebase to verify whether this was working correctly.

harlanhaskins · 2016-02-02T00:18:16Z

@modocache Thanks for the comprehensive review! I really appreciate your input.

The codebase relies really heavily on global out-of-process state, which makes it hard to test, but it's definitely not specific to Swift. This process could be used to merge profile data from instrumented builds of anything, (modulo some hard-coded filenames that explicitly say 'swift'). I could envision a small test runner that compiles a simple C file with profiling and shoots files at the server, then investigates the output, but I don't know if that's on the roadmap for me right away.

Thanks again!

harlanhaskins · 2016-02-02T21:39:36Z

@gribozavr I think this is good to merge. We should probably run the CI once more, though. (@shahmishal, too)

shahmishal · 2016-02-03T03:05:53Z

@swift-ci Please test

[coverage] Automatic merger for LLVM profile data

Harlan Haskins added 15 commits January 25, 2016 19:49

[coverage] Initial commit of profdata merging worker

eb27ca1

[coverage] Cleaned up profdata merge worker and added debug flag

2cf852f

[coverage] Removed explicit swift-%p.profraw arguments from CMake com…

c6c0b3d

…mands, because we're using LLVM's environment variables

[coverage] Made subclass of list test format so we can hook into each…

32fda29

… test run and properly create profile directories. This removes the need for use_profdir.py and removes the valgrind hack from lit.site.cfg.in

[coverage] Cleaned up argument parsing for swift coverage, and hooked…

a41ec1a

… into build-script to propagate the different values

[coverage] Added before_test and after_test hooks in SwiftTest

78612e5

[coverage] Simplified before_test and after_test

03964cc

[coverage] Fixed runtime error in lit.cfg

3ea61fb

[coverage] Reworked CMake invocation for coverage testing given there…

9eb729c

… are three states for SWIFT_ANALYZE_CODE_COVERAGE

[coverage] Added argument parsing to profdata merge worker and cleane…

fb6ee1a

…d up the implementation

[coverage] Cleaned up target generation and lit merging code

259af4d

[coverage] Fixed CMake invocation of profdata merge worker

c83fb1c

[coverage] Added license header to profdata_merge_worker and added ex…

1b30a49

…plicit choices to --swift-analyze-code-coverage

[coverage] Removed printing in CMake

5a2a809

[coverage] Fixed conflicts with master

9fccf50

gribozavr reviewed Jan 28, 2016
View reviewed changes

[coverage] Declared SWIFT_ANALYZE_CODE_COVERAGE in CMakeLists and doc…

bb8160a

…umented options for build-script flag.

jrose-apple reviewed Jan 28, 2016
View reviewed changes

Harlan Haskins added 2 commits January 28, 2016 11:12

[coverage] Documented modes for SWIFT_ANALYZE_CODE_COVERAGE and added…

b4c7678

… comment about llvm-profdata

[coverage] Changed 'none' to 'false' for coverage default

e997fc3

Harlan Haskins added 3 commits January 28, 2016 13:21

[coverage] Changed back to old coverage check in CMakeLists.txt files

44be500

[coverage] Removed print in built script

7608f88

[coverage] Added coverage_mode definitions in validation test and Uni…

b1d6e17

…t test lit.site.cfg.in

Harlan Haskins added 2 commits January 28, 2016 16:44

[coverage] Made sure code coverage doesn't rename the ninja build dir…

0e6f798

…ectory if it's "false"

[coverage] Merged and fixed conflicts

e9d009e

modocache reviewed Feb 1, 2016
View reviewed changes

Harlan Haskins added 2 commits February 1, 2016 15:09

[coverage] Converted global non-constants to explicitly passed parame…

f99fd75

…ters

[coverage] Converted to print function

d7eff48

modocache reviewed Feb 1, 2016
View reviewed changes

[coverage] Converted to explicit super in lit.cfg

693d7da

modocache reviewed Feb 1, 2016
View reviewed changes

Harlan Haskins added 2 commits February 1, 2016 15:52

[coverage] Split profdata_merge_worker.py into separate files within …

6572267

…a module

[coverage] Fixed missing import

dea48f4

modocache reviewed Feb 1, 2016
View reviewed changes

Harlan Haskins added 3 commits February 1, 2016 16:03

[coverage] Used gettempdir() instead of hard-coding /tmp

eb9fb9c

[coverage] Explicit super in process.py

45df849

[coverage] Fail gracefully on non-Darwin

873af8e

Harlan Haskins added 6 commits February 1, 2016 17:02

[coverage] Added license header to config.py

f713042

[coverage] Added README for the profdata module

c75fa76

[coverage] Added log file output to profdata merger

6ec0dd3

[coverage] Converted to python standard logging framework

cf30318

[coverage] pep8

13bae93

[coverage] Removed unused imports

2b22040

harlanhaskins mentioned this pull request Feb 2, 2016

[Python] Use sys.exit(…) instead of discouraged exit(…) #1172

Closed

gribozavr added a commit that referenced this pull request Feb 3, 2016

Merge pull request #1126 from harlanhaskins/profdata-merge

b728b89

[coverage] Automatic merger for LLVM profile data

gribozavr merged commit b728b89 into swiftlang:master Feb 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[coverage] Automatic merger for LLVM profile data #1126

[coverage] Automatic merger for LLVM profile data #1126

harlanhaskins commented Jan 28, 2016

gribozavr Jan 28, 2016

gribozavr commented Jan 28, 2016

jrose-apple Jan 28, 2016

harlanhaskins Jan 28, 2016

jrose-apple Jan 28, 2016

harlanhaskins Jan 28, 2016

harlanhaskins Jan 28, 2016

harlanhaskins Jan 28, 2016

harlanhaskins Jan 28, 2016

jrose-apple Jan 28, 2016

harlanhaskins Jan 28, 2016

gribozavr Feb 1, 2016

shahmishal commented Jan 28, 2016

shahmishal commented Jan 28, 2016

modocache Feb 1, 2016

harlanhaskins Feb 1, 2016

modocache Feb 1, 2016

modocache Feb 1, 2016

harlanhaskins Feb 1, 2016

modocache Feb 2, 2016

harlanhaskins Feb 2, 2016

harlanhaskins commented Feb 1, 2016

modocache Feb 1, 2016

harlanhaskins Feb 2, 2016

modocache commented Feb 2, 2016

harlanhaskins commented Feb 2, 2016

harlanhaskins commented Feb 2, 2016

shahmishal commented Feb 3, 2016

		@@ -23,9 +23,9 @@ if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
		PROPERTIES
		LINK_FLAGS "-Wl,-rpath -Wl,@executable_path/../lib")

[coverage] Automatic merger for LLVM profile data #1126

[coverage] Automatic merger for LLVM profile data #1126

Conversation

harlanhaskins commented Jan 28, 2016

Choose a reason for hiding this comment

gribozavr commented Jan 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahmishal commented Jan 28, 2016

shahmishal commented Jan 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harlanhaskins commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modocache commented Feb 2, 2016

harlanhaskins commented Feb 2, 2016

harlanhaskins commented Feb 2, 2016

shahmishal commented Feb 3, 2016