Add support for collating profiles for use with code coverage

[vsk]

Summary

• Add a cmake option to build with clang code coverage.
• Specify a profile data directory along with -fprofile-instr-generate: this lets us gather profiles from instrumented binaries in a dedicated location.
• Add a utility script that generates code coverage artifacts.

This patch uses the new in-process raw profile merging functionality in compiler-rt. We no longer index raw profiles after every test invocation, so no lit changes are required.

Running ninja check-llvm-unit generates 682M of raw profile data using a merge pool size of 4. In practice, this turns out to be a lot faster (and also much more compact) than doing llvm-profdata merge -sparse after each test run.

[MatzeB]

This adds a bigger bunch of code to lit, I wonder if it is necessary:

• I assume the necessity for the logic inside lit is just the fact that the profile data overwrites each other after running a command. Is setting LLVM_PROFILE_FILE with a '%p' placeholder not enough to avoid this? The final profile data merging can always be done outside of llvm-lit afterwards. Or are there other reasons why you need to intercept the opt/llc/etc. calls?

• The lit tests don't strike me as the most typical llvm usage (they are often designed to test corner cases of the compiler), wouldn't it be better to gather profiles by compiling the llvm test-suite or something similar?

[davidxl]

How many profile data files are generated in a full run? It may take huge amount of space.

I have not gotten time to submit my patch for the runtime to support online/in-process merge yet, but will try to get it done soon. Once that is ready, we can reduce the overhead greatly.

[vsk]

In D20993#449092, @MatzeB wrote:

This adds a bigger bunch of code to lit, I wonder if it is necessary:

• I assume the necessity for the logic inside lit is just the fact that the profile data overwrites each other after running a command. Is setting LLVM_PROFILE_FILE with a '%p' placeholder not enough to avoid this?

Using PID substitution gets close to solving the problem of having overwritten profiles, but not all the way. On 32-bit systems PID wraparound would pose a real problem. In this patch I include the hash of the test command to minimize loss of profiles.

The final profile data merging can always be done outside of llvm-lit afterwards.

On a practical level, I don't think this is possible. Raw profiles are too large. Turning off the cleanup step and running check-llvm produces over half a terrabyte of data. There are a few ways to address this without touching lit:

1. Reduce the size of raw profiles. This would make the compiler runtime larger and more complex.
2. Run a monitor process that does the merging/cleanup. I don't think that approach has any advantages compared to modifying lit.

Alternatively, we could do in-place raw profile merging in the compiler runtime. I don't think that's preferable because we'd have to introduce a lot of complexity to compiler-rt (e.g portable mandatory file locking).

Or are there other reasons why you need to intercept the opt/llc/etc. calls?

Intercepting the calls makes it possible to generate 'unique' hashes for the profiles, see my comment above.

• The lit tests don't strike me as the most typical llvm usage (they are often designed to test corner cases of the compiler), wouldn't it be better to gather profiles by compiling the llvm test-suite or something similar?

It's true that the profiles gathered from check-llvm aren't ideal for PGO. But, they are perfect for measuring code coverage of the llvm codebase. Moreover because lit is portable, other projects can use these changes to gather coverage reports with their tests.

(IIRC the test-suite also uses lit?)

[MatzeB]

In D20993#449116, @vsk wrote:

In D20993#449092, @MatzeB wrote:

This adds a bigger bunch of code to lit, I wonder if it is necessary:

• I assume the necessity for the logic inside lit is just the fact that the profile data overwrites each other after running a command. Is setting LLVM_PROFILE_FILE with a '%p' placeholder not enough to avoid this?

Using PID substitution gets close to solving the problem of having overwritten profiles, but not all the way. On 32-bit systems PID wraparound would pose a real problem. In this patch I include the hash of the test command to minimize loss of profiles.

This is a really annoying problem to have... Maybe we should find a solution within the profile infrastructure itself (can we add another flag that adds a unique suffix to the filename if it already exists?) so not every user of the profiling infrastructure has to jump through the same hoops (I've done a similar dance in the test-suite profile support).

The final profile data merging can always be done outside of llvm-lit afterwards.

On a practical level, I don't think this is possible. Raw profiles are too large. Turning off the cleanup step and running check-llvm produces over half a terrabyte of data. There are a few ways to address this without touching lit:

1. Reduce the size of raw profiles. This would make the compiler runtime larger and more complex.
2. Run a monitor process that does the merging/cleanup. I don't think that approach has any advantages compared to modifying lit.

Alternatively, we could do in-place raw profile merging in the compiler runtime. I don't think that's preferable because we'd have to introduce a lot of complexity to compiler-rt (e.g portable mandatory file locking).

Ah, so that seems harder to solve without an external driver merging the profile data. I'm still not a big fan of injecting this stuff into the core of lit, but we may have no other choice today.

Or are there other reasons why you need to intercept the opt/llc/etc. calls?

Intercepting the calls makes it possible to generate 'unique' hashes for the profiles, see my comment above.

• The lit tests don't strike me as the most typical llvm usage (they are often designed to test corner cases of the compiler), wouldn't it be better to gather profiles by compiling the llvm test-suite or something similar?

It's true that the profiles gathered from check-llvm aren't ideal for PGO. But, they are perfect for measuring code coverage of the llvm codebase. Moreover because lit is portable, other projects can use these changes to gather coverage reports with their tests.

Oh indeed I did not think about code coverage.

(IIRC the test-suite also uses lit?)

The test-suite uses lit to execute the benchmarks. I already added profiling support for that (see the TEST_SUITE_PROFILE_GENERATE/TEST_SUITE_PROFILE_USE variants). This is however about running the benchmarks. It does not help you when you want to generate profile data from the clang executable that compiled the benchmarks as that is only done by make (or ninja) and not lit.

[vsk]

Some of the discussion moved onto the mailing list and Phab couldn't pick up the comments.

The TL;DR: The plan is to add in-process raw profile merging support to compiler-rt. In fact, David has already added the basic mandatory file-locking constructs we'll need (see r271864). Apart from bringing general usability improvements, this will let us trim this patch down significantly.

[vsk]

• Reworked the patch to drop all lit changes. See the new title/summary.

[davidxl]

Why choosing default size of 9? Do you have build time comparison numbers i.e., pool size from 1 to 9?

[silvas]

Nit about the shell script.

utils/prepare-code-coverage-artifact.sh

1

I mentioned this in one of the reviews of some stuff that Chris Bieneman was adding to clang for PGO (http://reviews.llvm.org/D15462), but these kinds of utility scripts are generally much more readable, robust, and portable when written in Python.

For example, the standard library operations all will throw exceptions nicely in cases of errors (which will stop the program dead, give you a complete stacktrace that is e.g. easy to read in a bot log, etc.). Also you can easily say assert <cond>, "reason" for quick sanity checks / "error handling" (again, you get a full stack trace if it fails, so for this kind of script more elaborate error handling isn't really needed).

It's really hard to get that kind of robustness from a shell script. (also it avoids general shell script problems like what happens when paths contain spaces, which lead to tremendous head scratching)

Also, writing it in Python makes it pretty portable for free and is generally more readable.

[vsk]

In D20993#454452, @davidxl wrote:

Why choosing default size of 9? Do you have build time comparison numbers i.e., pool size from 1 to 9?

I chose 9 because it's the highest option: a larger pool of raw profiles should theoretically lead to less lock contention. Ideally this would be some factor of the number of threads on the system. However, I don't think it's worth optimizing this default.

Also, writing it in Python makes it pretty portable for free and is generally more readable.

Sure, I'll upload a new diff with this change.

[vsk]

• Pythonify the artifact prep script.
• Use a default merge pool size of 4 (see the mailing list discussion for some justification for this).

[davidxl]

utils/prepare-code-coverage-artifact.py

20 ↗

(On Diff #60430)

is profiles.manifest file needed here? does it simplify the process?

[vsk]

utils/prepare-code-coverage-artifact.py

20 ↗

(On Diff #60430)

It's a bit of future-proofing. It was needed with the previous patch because of the sheer number of raw profiles. Now, it helps avoid failures during the check_call step due to too many arguments.

[davidxl]

lgtm

[silvas]

Thanks for converting the script to python!

One advisory comment on the script (not a big deal), but this LGTM.

utils/prepare-code-coverage-artifact.py

20 ↗

(On Diff #60430)

%m only supports up to 9, which should be fine even on windows. It would make things slightly simpler to pass directly on the command line.

[vsk]

Thanks for the feedback and reviews. I'm planning on committing after double-checking that this doesn't break any of our PGO bots.

utils/prepare-code-coverage-artifact.py

20 ↗

(On Diff #60430)

Fair point, though that does get multiplied by the number of distinct binaries invoked.

CMakeLists.txt

 endif()
 
 if (MSVC_IDE AND NOT (MSVC_VERSION LESS 1900))
   option(LLVM_ADD_NATIVE_VISUALIZERS_TO_SOLUTION "Configure project to use Visual Studio native visualizers" TRUE)
 else()
   set(LLVM_ADD_NATIVE_VISUALIZERS_TO_SOLUTION FALSE CACHE INTERNAL "For Visual Studio 2013, manually copy natvis files to Documents\\Visual Studio 2013\\Visualizers" FORCE)
 endif()
 
+if (LLVM_BUILD_INSTRUMENTED OR LLVM_BUILD_INSTRUMENTED_COVERAGE)
+  if(NOT LLVM_PROFILE_DATA_DIR)
+    message(FATAL_ERROR "Must set LLVM_PROFILE_DATA_DIR to point to a directory")
+  endif()
+  if(NOT LLVM_PROFILE_MERGE_POOL_SIZE)
+    set(LLVM_PROFILE_MERGE_POOL_SIZE "9")
+  endif()
+  if(NOT LLVM_PROFILE_FILE_PATTERN)
+    file(TO_NATIVE_PATH "${LLVM_PROFILE_DATA_DIR}/%${LLVM_PROFILE_MERGE_POOL_SIZE}m.profraw" LLVM_PROFILE_FILE_PATTERN)
+  endif()
+endif()
+
 # All options referred to from HandleLLVMOptions have to be specified
 # BEFORE this include, otherwise options will not be correctly set on
 # first cmake run
 include(config-ix)
 
 string(REPLACE "Native" ${LLVM_NATIVE_ARCH}
   LLVM_TARGETS_TO_BUILD "${LLVM_TARGETS_TO_BUILD}")
 list(REMOVE_DUPLICATES LLVM_TARGETS_TO_BUILD)

cmake/modules/HandleLLVMOptions.cmake

 option(LLVM_ENABLE_EH "Enable Exception handling" OFF)
 option(LLVM_ENABLE_RTTI "Enable run time type information" OFF)
 if(LLVM_ENABLE_EH AND NOT LLVM_ENABLE_RTTI)
   message(FATAL_ERROR "Exception handling requires RTTI. You must set LLVM_ENABLE_RTTI to ON")
 endif()
 
 option(LLVM_BUILD_INSTRUMENTED "Build LLVM and tools with PGO instrumentation (experimental)" Off)
 mark_as_advanced(LLVM_BUILD_INSTRUMENTED)
-append_if(LLVM_BUILD_INSTRUMENTED "-fprofile-instr-generate"
+append_if(LLVM_BUILD_INSTRUMENTED "-fprofile-instr-generate=${LLVM_PROFILE_FILE_PATTERN}"
+  CMAKE_CXX_FLAGS
+  CMAKE_C_FLAGS
+  CMAKE_EXE_LINKER_FLAGS
+  CMAKE_SHARED_LINKER_FLAGS)
+
+option(LLVM_BUILD_INSTRUMENTED_COVERAGE "Build LLVM and tools with Code Coverage instrumentation (experimental)" Off)
+mark_as_advanced(LLVM_BUILD_INSTRUMENTED_COVERAGE)
+append_if(LLVM_BUILD_INSTRUMENTED_COVERAGE "-fprofile-instr-generate=${LLVM_PROFILE_FILE_PATTERN} -fcoverage-mapping"
   CMAKE_CXX_FLAGS
   CMAKE_C_FLAGS
   CMAKE_EXE_LINKER_FLAGS
   CMAKE_SHARED_LINKER_FLAGS)
 
 set(LLVM_ENABLE_LTO OFF CACHE STRING "Build LLVM with LTO. May be specified as Thin or Full to use a particular kind of LTO")
 string(TOUPPER "${LLVM_ENABLE_LTO}" uppercase_LLVM_ENABLE_LTO)
 if(uppercase_LLVM_ENABLE_LTO STREQUAL "THIN")

utils/prepare-code-coverage-artifact.sh

+#!/bin/sh
+
+if [ "$#" -ne 4 ]; then
+  echo "Usage:"
+  echo "$0 <HOST_LLVM_PROFDATA> <HOST_LLVM_COV> <LLVM_PROFILE_DATA_DIR> <LLVM_BIN_DIR>"
+  exit 1
+fi
+
+HOST_LLVM_PROFDATA=$1
+HOST_LLVM_COV=$2
+LLVM_PROFILE_DATA_DIR=$3
+LLVM_BIN_DIR=$4
+
+echo ":: llvm-profdata = " $HOST_LLVM_PROFDATA
+echo ":: llvm-cov = " $HOST_LLVM_COV
+
+echo ":: Merging raw profiles in $LLVM_PROFILE_DATA_DIR"
+find $LLVM_PROFILE_DATA_DIR -name \*.profraw > $LLVM_PROFILE_DATA_DIR/profiles.list
+$HOST_LLVM_PROFDATA merge -sparse -f $LLVM_PROFILE_DATA_DIR/profiles.list -o $LLVM_PROFILE_DATA_DIR/Coverage.profdata
+
+echo ":: Deleting raw profiles in $LLVM_PROFILE_DATA_DIR"
+find $LLVM_PROFILE_DATA_DIR -name \*.profraw -exec rm {} \;
+
+echo ":: Copying covmappings from $LLVM_BIN_DIR into $LLVM_PROFILE_DATA_DIR"
+for prog in $(find $LLVM_BIN_DIR -type f | grep -v llvm-lit); do
+  $HOST_LLVM_COV convert-for-testing $prog -o $LLVM_PROFILE_DATA_DIR/$(basename $prog).covmapping
+done