[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

5 ↗

(On Diff #455777)

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.

8 ↗

(On Diff #455777)

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

10–11 ↗

(On Diff #455777)

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.

20 ↗

(On Diff #455777)

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

8 ↗

(On Diff #455777)

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

29 ↗

(On Diff #455777)

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

20 ↗

(On Diff #455777)

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

20 ↗

(On Diff #455777)

Sounds good!

compiler-rt/lib/profile/InstrProfilingFile.c

void __llvm_profile_initialize_file(void) {
   parseAndSetFilename(SelectedPat, PNS, 0);
 }
 
 /* This method is invoked by the runtime initialization hook
  * InstrProfilingRuntime.o if it is linked in.
  */
 COMPILER_RT_VISIBILITY
 void __llvm_profile_initialize(void) {
+  uint64_t CountersSize = __llvm_profile_get_counters_size(
+      __llvm_profile_begin_counters(), __llvm_profile_end_counters());
+  if (!CountersSize)
+    return;
+
   __llvm_profile_initialize_file();
   if (!__llvm_profile_is_continuous_mode_enabled())
     __llvm_profile_register_write_file_atexit();
 }
 
 /* This API is directly called by the user application code. It has the
  * highest precedence compared with LLVM_PROFILE_FILE environment variable
  * and command line option -fprofile-instr-generate=<profile_name>.