[profile] Create only prof header when no counters

[gulfem]

Summary

When we use selective instrumentation and instrument a file
that is not in the selected files list provided via -fprofile-list,
we generate an empty raw profile. This leads to empty_raw_profile
error when we try to read that profile. This patch fixes the issue by
generating a raw profile that contains only a profile header when
there are no counters and profile data.

A small reproducer for the above issue:
echo "src:other.cc" > code.list
clang++ -O2 -fprofile-instr-generate -fcoverage-mapping
-fprofile-list=code.list code.cc -o code
./code
llvm-profdata show default.profraw

[phosek]

The concern I have with this solution is that if there are some users that do

LLVM_PROFILE_FILE=foo.profraw ./foo`

and then immediately try to either process foo.profraw because they assume that this file is always going to be produced, then this change is going to break them. It may also be non-trivial for them to work around it if they don't have an ability to check for the foo.profraw existence (this can be the case in infrastructure that uses declarative configuration for example). I don't know how to check whether any such users exists though so this concern may be entirely unfounded.

A potentially safer solution would be to always produce an empty profile, that is a file that has a header but no data and/or counters, by removing this check https://github.com/llvm/llvm-project/blob/e33599371eb1ab1b782735f39b3f27c19d88da41/compiler-rt/lib/profile/InstrProfilingWriter.c#L279

A small reproducer for the above issue:
echo "src:other.cc" > code.list
clang++ -O2 -fprofile-instr-generate -fcoverage-mapping
-fprofile-list=code.list code.cc -o code
./code
llvm-profdata show default.profraw

Would it be possible to turn this reproducer into a test?

[gulfem]

Add a test case

[gulfem]

In D132094#3730990, @phosek wrote:

The concern I have with this solution is that if there are some users that do

LLVM_PROFILE_FILE=foo.profraw ./foo`

and then immediately try to either process foo.profraw because they assume that this file is always going to be produced, then this change is going to break them. It may also be non-trivial for them to work around it if they don't have an ability to check for the foo.profraw existence (this can be the case in infrastructure that uses declarative configuration for example). I don't know how to check whether any such users exists though so this concern may be entirely unfounded.

A potentially safer solution would be to always produce an empty profile, that is a file that has a header but no data and/or counters, by removing this check https://github.com/llvm/llvm-project/blob/e33599371eb1ab1b782735f39b3f27c19d88da41/compiler-rt/lib/profile/InstrProfilingWriter.c#L279

We have two options to solve this issue:

1. Do not generate any profile file at all, which I currently implemented.
2. Generate a profile with header & zero data and counts, which @phosek suggested.

I think option 1 is better if there are no such users/systems that assume that a profile is generated whenever a file is instrumented.
@davidxl @vsk and @ellis, please let us know if you are aware of such a system. If there is such a user, we can go with option 2.

[MaskRay]

I prefer always having a file. It:

• removes a behavior difference
• gives users a hint that PGO is effective (even if all functions are skipped)
• satisfies some build system requirement. For instance, for distributed ThinLTO we ensure that an index file always exists, even for non-extracted lazy object file

[ellis]

When using the -fprofile-function-groups option we could hit the case where one of the groups has no profiled functions. In that case it would be more surprising to not find a raw profile, rather than an empty raw profile. If we do go with option 1, maybe we could emit a warning explaining why no profile was emitted. Or if we go with option 2, maybe a warning would also be good to alert the user that it's an empty profile.

https://clang.llvm.org/docs/UsersManual.html#instrument-function-groups

[gulfem]

Implement option 2.

[gulfem]

Thanks for your feedback @MaskRay and @ellis, and I implemented option 2 (Generate a profile with header & zero data and counts).

[gulfem]

In D132094#3733666, @ellis wrote:

When using the -fprofile-function-groups option we could hit the case where one of the groups has no profiled functions. In that case it would be more surprising to not find a raw profile, rather than an empty raw profile. If we do go with option 1, maybe we could emit a warning explaining why no profile was emitted. Or if we go with option 2, maybe a warning would also be good to alert the user that it's an empty profile.

What kind of warning do you have in mind @ellis? Are you suggesting to put some text like Empty Profile when we read a profile via llvm-profdata show (which happens to contain only a header)?

[ellis]

In D132094#3750842, @gulfem wrote:

In D132094#3733666, @ellis wrote:

When using the -fprofile-function-groups option we could hit the case where one of the groups has no profiled functions. In that case it would be more surprising to not find a raw profile, rather than an empty raw profile. If we do go with option 1, maybe we could emit a warning explaining why no profile was emitted. Or if we go with option 2, maybe a warning would also be good to alert the user that it's an empty profile.

What kind of warning do you have in mind @ellis? Are you suggesting to put some text like Empty Profile when we read a profile via llvm-profdata show (which happens to contain only a header)?

On second thought, when the user runs llvm-profdata show they will see that zero functions are instrumented, so I'm not sure a warning there is necessary.

LGTM after resolving comments!

compiler-rt/test/profile/Posix/instrprof-empty-profile.c

6

I've have issues with using this sed command like this where it broke a test on Windows, but I think that was because my profile-list file had multiple lines. For this case, I'm not sure sed is necessary because there are no \ characters. Let's remove it to simplify the line.

9

Is this test necessary if we are going to use the raw profile in the llvm-profdata command below anyway?

11–12

I think it's simpler to use a pipe rather than creating a temporary file.

Also, it looks like this and the next FileCheck are checking the same output so we can reuse the same default prefix.

21

I think this might be the only important line to check. Do we need to check the other lines too?

compiler-rt/test/profile/Posix/instrprof-shared-empty-profile.test

9

Oh nice, thanks for handling and testing this case! (The same comments from the previous test apply here)

[ellis]

compiler-rt/test/profile/Posix/instrprof-shared-empty-profile.test

30

I think you're missing the run command to produce this raw profile.

[gulfem]

Addressed the feedback about the test

[gulfem]

compiler-rt/test/profile/Posix/instrprof-empty-profile.c

21

Appreciate for all the feedback @ellis!
I prefer to check all the other lines to make sure that we have only 0s.
For this test it might not be important, but for the shared library test (instrprof-shared-empty-profile.test), I would like to verify the accurate values.

[ellis]

compiler-rt/test/profile/Posix/instrprof-empty-profile.c

21

Sounds good!

compiler-rt/lib/profile/InstrProfilingWriter.c

const uint64_t CountersSectionSize =
       __llvm_profile_get_counters_size(CountersBegin, CountersEnd);
   const uint64_t NumCounters =
       __llvm_profile_get_num_counters(CountersBegin, CountersEnd);
   const uint64_t NamesSize = DebugInfoCorrelate ? 0 : NamesEnd - NamesBegin;
 
   /* Create the header. */
   __llvm_profile_header Header;
 
-  if (!NumData && (!DebugInfoCorrelate || !NumCounters))
-    return 0;
-
   /* Determine how much padding is needed before/after the counters and after
    * the names. */
   uint64_t PaddingBytesBeforeCounters, PaddingBytesAfterCounters,
       PaddingBytesAfterNames;
   __llvm_profile_get_padding_sizes_for_counters(
       DataSectionSize, CountersSectionSize, NamesSize,
       &PaddingBytesBeforeCounters, &PaddingBytesAfterCounters,
       &PaddingBytesAfterNames);

compiler-rt/test/profile/Posix/instrprof-empty-profile.c

+// Test a profile with only a header is generated when a src file is not in the
+//    selected files list provided via -fprofile-list.
+
+// RUN: mkdir -p %t.d
+// RUN: echo "src:other.c" > %t-file.list
+// RUN: %clang_profgen -fprofile-list=%t-file.list -o %t %s
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata show %t.profraw | FileCheck %s --check-prefix=RAW-PROFILE-HEADER-ONLY
+
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-profdata show %t.profdata | FileCheck %s --check-prefix=INDEXED-PROFILE-HEADER-ONLY
+
+int main() { return 0; }
+
+// RAW-PROFILE-HEADER-ONLY: Instrumentation level: Front-end
+// RAW-PROFILE-HEADER-ONLY-NEXT: Total functions: 0
+// RAW-PROFILE-HEADER-ONLY-NEXT: Maximum function count: 0
+// RAW-PROFILE-HEADER-ONLY-NEXT: Maximum internal block count: 0
+
+// INDEXED-PROFILE-HEADER-ONLY: Instrumentation level: Front-end
+// INDEXED-PROFILE-HEADER-ONLY-NEXT: Total functions: 0
+// INDEXED-PROFILE-HEADER-ONLY-NEXT: Maximum function count: 0
+// INDEXED-PROFILE-HEADER-ONLY-NEXT: Maximum internal block count: 0

compiler-rt/test/profile/Posix/instrprof-shared-empty-profile.test

+"""
+This test produces two shared libraries:
+
+1. libt-instr.so is instrumented
+2. libt-no-instr.so is built with profile rt linked in (via -u<hook>), but the object file is built
+   with instrumentation turned off.
+
+The test verifies concatenating profiles with only headers and no profile data and counters.
+"""
+
+RUN: mkdir -p %t.d
+RUN: %clang_profgen -o %t.d/libt-instr.so -fPIC -shared %S/../Inputs/instrprof-shared-lib.c
+RUN: %clang -c -o %t.d/instrprof-shared-lib-no-instr.o -fPIC  %S/../Inputs/instrprof-shared-lib.c
+RUN: %clang_profgen -o %t.d/libt-no-instr.so -fPIC -shared %t.d/instrprof-shared-lib-no-instr.o
+
+# Header + Header
+RUN: echo "src:other.c" > %t-file.list
+RUN: %clang_profgen -fprofile-list=%t-file.list -o %t-no-instr-no-instr -L%t.d -rpath %t.d -lt-no-instr  %S/../Inputs/instrprof-shared-main.c
+RUN: env LLVM_PROFILE_FILE=%t-no-instr-no-instr.profraw %run %t-no-instr-no-instr
+RUN: llvm-profdata show %t-no-instr-no-instr.profraw | FileCheck %s --check-prefix=HEADER-HEADER
+// HEADER-HEADER: Instrumentation level: Front-end
+// HEADER-HEADER-NEXT: Total functions: 0
+// HEADER-HEADER-NEXT: Maximum function count: 0
+// HEADER-HEADER-NEXT: Maximum internal block count: 0
+
+# Header + Profile
+RUN: %clang_profgen -fprofile-list=%t-file.list -o %t-no-instr-instr -L%t.d -rpath %t.d -lt-instr  %S/../Inputs/instrprof-shared-main.c
+RUN: env LLVM_PROFILE_FILE=%t-no-instr-instr.profraw %run %t-no-instr-instr
+RUN: llvm-profdata show %t-no-instr-instr.profraw | FileCheck %s --check-prefix=HEADER-PROFILE
+// HEADER-PROFILE: Instrumentation level: Front-end
+// HEADER-PROFILE-NEXT: Total functions: 1
+// HEADER-PROFILE-NEXT: Maximum function count: 1000000
+// HEADER-PROFILE-NEXT: Maximum internal block count: 360000
+
+# Profile + Header
+RUN: %clang_profgen -o %t-instr-no-instr -L%t.d -rpath %t.d -lt-no-instr %S/../Inputs/instrprof-shared-main.c
+RUN: env LLVM_PROFILE_FILE=%t-instr-no-instr.profraw %run %t-instr-no-instr
+RUN: llvm-profdata show %t-instr-no-instr.profraw | FileCheck %s --check-prefix=PROFILE-HEADER
+// PROFILE-HEADER: Instrumentation level: Front-end
+// PROFILE-HEADER-NEXT: Total functions: 1
+// PROFILE-HEADER-NEXT: Maximum function count: 1
+// PROFILE-HEADER-NEXT: Maximum internal block count: 1000000

llvm/lib/ProfileData/InstrProfReader.cpp

Error RawInstrProfReader<IntPtrT>::readValueProfilingData(
   // remapped into function name hashes.
   VDataPtrOrErr.get()->deserializeTo(Record, Symtab.get());
   CurValueDataSize = VDataPtrOrErr.get()->getSize();
   return success();
 }
 
 template <class IntPtrT>
 Error RawInstrProfReader<IntPtrT>::readNextRecord(NamedInstrProfRecord &Record) {
-  if (atEnd())
+  // Keep reading profiles that consist of only headers and no profile data and
+  // counters.
+  while (atEnd())
     // At this point, ValueDataStart field points to the next header.
     if (Error E = readNextHeader(getNextHeaderPos()))
       return error(std::move(E));
 
   // Read name ad set it in Record.
   if (Error E = readName(Record))
     return error(std::move(E));