Adding a function for setting coverage output file.

[sajjadm]

Summary

User code can open a file on its own and pass it to the runtime, rather than
specifying a name and having the runtime open the file. This supports the use
case where a process cannot open a file on its own but can receive a file
descriptor from another process.

[efriedma]

It looks like this patch is exposing some new symbols which don't have an "__" prefix... do we have some formal policy about that for libclang_rt.profile? It's sort of orthogonal to this patch, though... lprofGetProfileFile and lprofSetProfileFile should probably be marked "static".

It looks like passing in a FILE* implicitly disables profile merging; is that intentional? If so, it should probably be documented somehow.

[davidxl]

There should be a test case.

compiler-rt/lib/profile/InstrProfiling.h

161

Document this interface. Also mention that when file descriptor is provided by the client, profiling runtime won't perform profile merging.

With this, the profile file name specified by either environment variable or command line will be ignored, so it should be documented.

[Dor1s]

I think it'll be bad if we lose merging with this approach. Should the change be done in a way that openFileForMerging can also use the runtime provided file pointer?

[sajjadm]

Added support for merging, docs, and test case.

Also set the two internal functions to static and removed the visibility macro from them.

[davidxl]

To support merging, the new interface needs to take more

compiler-rt/lib/profile/InstrProfilingFile.c

297

This is not safe to do always -- you need to make sure the file is exclusively accessed. Suggest changing the public interface with one additional parameter

__llvm_profile_set_file_object(FILE *F, bool is_exclusive /*with lock*/ );

If it is not, merging can not be performed.

298

The change makes the code less readable (especially the primary code path). Suggest:

if (!doMerging()) {

OutputFile = getFileObject(OutputName) ; // this returns user provided FileObject if exists

} else {

OutputFile = openFileForMerging(OutputName, &MergeDone); // this uses user provided FIleObject if exists

}

[sajjadm]

Cleaned up logic and added safe-for-merge bool to API.

[sajjadm]

Updated test.

[sajjadm]

@davidxl We could also have the openFileForMerging function invoke lprofLockFd on the user-provided file, rather than putting the onus for locking on the user. How would you feel about that?

[davidxl]

I think it is cleaner for user to make the intention clear by passing the flag if the file handle is passed to the runtime. Otherwise the runtime will have to guess/assume user's intention (e.g, also reopen the file with "r+b" mode for merging).

[davidxl]

compiler-rt/test/profile/instrprof-set-file-object.c

6

Check the result of the dump.

8

Also add a test for merging. Run the test program sequentially and there is no need for locking in the test, but merging results need to be checked to correct.

[sajjadm]

On further thought, I don't think having a boolean that tells the runtime "user code is controlling locking so it is safe to merge" is a good idea, because typically __llvm_profile_write_file is invoked at exit. User code would have to ensure that it locks the file at the very end of its execution, when it has collected the desired profiling data, but before __lvm_profile_write_file is invoked. Also, in the current implementation, whether or not merging happens is controlled by the pattern of the profiling file name (whether or not it has a %m in it).

Instead I think we should have a separate function, __llvm_profile_enable_merging. When this new function is invoked with true, it overrides the merging state set by the file name pattern. Then openFileForMerging can call lprofLockFd on the user-provided file. This allows the user to explicitly turn merging on if desired.

[davidxl]

Sounds reasonable -- the intention is clear with this -- the interface tells runtime to do whatever needed to ensure merging. Please send a patch with merging test case.

[sajjadm]

Added explicit merging API and test.

[sajjadm]

Fixed lock/unlock code.

Removed the locking for WIN32 builds because the lock has to be enabled
in CreateFileA, so it will be user code's responsibility in this case.

Added unlocking after write in case __llvm_profile_write_file is
manually invoked by user code before the end of the process.

[sajjadm]

Forgot to declare a variable.

[sajjadm]

Ran clang-format.

[davidxl]

compiler-rt/lib/profile/InstrProfiling.h

182

this interface should not be used seperately, so it is better to merge this with llvm_profile_set_file_object(FILE*, in enableMering)

compiler-rt/lib/profile/InstrProfilingFile.c

255

Add lprofLockFileHandle utility in InstrProfilingUtil.c

309–321

What is this for? Why locking the file again?

[sajjadm]

compiler-rt/lib/profile/InstrProfilingFile.c

309–321

Good catch, that was supposed to be an unlock.

[sajjadm]

Extracted lock/unlock code to util file, combined __llvm functions.

[davidxl]

lgtm. Watch out for problems on other platforms.

[sajjadm]

Could someone please commit for me?

[davidxl]

I suggest you apply for commit right first. There are likely fallouts that need to be dealt with.

[sajjadm]

Could you be more specific about your concerns? I'm happy to fix any problems in this change.

[davidxl]

It is just more convenient for you to contribute to LLVM in the future. I can help you commit this time.

[sajjadm]

Thank you! I did apply for committer status today but I don't know how long it takes to get it.

[Dor1s]

LGTM (was OOO), thanks a lot David for Sajjad for multiple iterations here. One question: was the test landed? I see it in the latest patchset (https://reviews.llvm.org/D62541?id=203034), but not in the code that was committed.

compiler-rt/trunk/lib/profile/InstrProfilingFile.c

270 ↗

(On Diff #203289)

nit: looks like this should start with a lowercase, GetFileObject -> getFileObject

[sajjadm]

In D62541#1532508, @Dor1s wrote:

LGTM (was OOO), thanks a lot David for Sajjad for multiple iterations here. One question: was the test landed? I see it in the latest patchset (https://reviews.llvm.org/D62541?id=203034), but not in the code that was committed.

Looks like the tests got added in a separate commit. 758c08921da7282a74a3779d7d89940263f5a6af

compiler-rt/lib/profile/InstrProfiling.h

 /*===- InstrProfiling.h- Support library for PGO instrumentation ----------===*\
 |*
 |* Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 |* See https://llvm.org/LICENSE.txt for license information.
 |* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 |*
 \*===----------------------------------------------------------------------===*/
 
 #ifndef PROFILE_INSTRPROFILING_H_
 #define PROFILE_INSTRPROFILING_H_
 
 #include "InstrProfilingPort.h"
+#include <stdio.h>
 
 #define INSTR_PROF_VISIBILITY COMPILER_RT_VISIBILITY
 #include "InstrProfData.inc"
 
 enum ValueKind {
 #define VALUE_PROF_KIND(Enumerator, Value, Descr) Enumerator = Value,
 #include "InstrProfData.inc"
 };

  * or if that's not set,  \c "default.profraw".
  */
 int __llvm_profile_write_file(void);
 
 int __llvm_orderfile_write_file(void);
 /*!
  * \brief this is a wrapper interface to \c __llvm_profile_write_file.
  * After this interface is invoked, a arleady dumped flag will be set
- * so that profile won't be dumped again during program exit. 
+ * so that profile won't be dumped again during program exit.
  * Invocation of interface __llvm_profile_reset_counters will clear
  * the flag. This interface is designed to be used to collect profile
  * data from user selected hot regions. The use model is
  *      __llvm_profile_reset_counters();
  *      ... hot region 1
  *      __llvm_profile_dump();
  *      .. some other code
  *      __llvm_profile_reset_counters();

  * Sets the filename to be used for subsequent calls to
  * \a __llvm_profile_write_file().
  *
  * \c Name is not copied, so it must remain valid.  Passing NULL resets the
  * filename logic to the default behaviour.
  */
 void __llvm_profile_set_filename(const char *Name);
 
+/*!
+ * \brief Set the FILE object for writing instrumentation data.
+ *
+ * Sets the FILE object to be used for subsequent calls to
+ * \a __llvm_profile_write_file(). The profile file name set by environment
+ * variable, command-line option, or calls to \a  __llvm_profile_set_filename
+ * will be ignored.
+ *
+ * \c File will not be closed after a call to \a __llvm_profile_write_file() but
+ * it may be flushed. Passing NULL restores default behavior.
+ */
+void __llvm_profile_set_file_object(FILE *File);
+
+/*!
+ * \brief Enable profile instrumentation merging.
+ *
+ * If \c EnableMerge is nonzero, the runtime will always merge profiling data
+ * with the contents of the profiling file. If EnableMerge is zero, the runtime
+ * may still merge the data if it would have merged for another reason (for
+ * example, because of a %m specifier in the file name).
+ */
+void __llvm_profile_enable_merge(int EnableMerge);
+
 /*! \brief Register to write instrumentation data to file at exit. */
 int __llvm_profile_register_write_file_atexit(void);
 
 /*! \brief Initialize file handling. */
 void __llvm_profile_initialize_file(void);
 
 /*!
  * \brief Return path prefix (excluding the base filename) of the profile data.

compiler-rt/lib/profile/InstrProfilingFile.c

 
 #include "InstrProfiling.h"
 #include "InstrProfilingInternal.h"
 #include "InstrProfilingUtil.h"
 
 /* From where is profile name specified.
  * The order the enumerators define their
  * precedence. Re-order them may lead to
- * runtime behavior change. */ 
+ * runtime behavior change. */
 typedef enum ProfileNameSpecifier {
   PNS_unknown = 0,
   PNS_default,
   PNS_command_line,
   PNS_environment,
   PNS_runtime_api
 } ProfileNameSpecifier;
 
typedef struct lprofFilename {
    * can only appear once at the end of the name pattern. */
   unsigned MergePoolSize;
   ProfileNameSpecifier PNS;
 } lprofFilename;
 
 COMPILER_RT_WEAK lprofFilename lprofCurFilename = {0,   0, 0, 0, {0},
                                                    {0}, 0, 0, 0, PNS_unknown};
 
+static int ProfileMergeRequested = 0;
+static int isProfileMergeRequested() { return ProfileMergeRequested; }
+static void setProfileMergeRequested(int EnableMerge) {
+  ProfileMergeRequested = EnableMerge;
+}
+
+COMPILER_RT_VISIBILITY void __llvm_profile_enable_merge(int EnableMerge) {
+  setProfileMergeRequested(EnableMerge);
+}
+
+static FILE *ProfileFile = NULL;
+static FILE *getProfileFile() { return ProfileFile; }
+static void setProfileFile(FILE *File) {
+  ProfileFile = File;
+}
+
+COMPILER_RT_VISIBILITY void __llvm_profile_set_file_object(FILE *File) {
+  setProfileFile(File);
+}
+
 static int getCurFilenameLength();
 static const char *getCurFilename(char *FilenameBuf, int ForceUseBuf);
-static unsigned doMerging() { return lprofCurFilename.MergePoolSize; }
+static unsigned doMerging() {
+  return lprofCurFilename.MergePoolSize || isProfileMergeRequested();
+}
 
 /* Return 1 if there is an error, otherwise return  0.  */
 static uint32_t fileWriter(ProfDataWriter *This, ProfDataIOVec *IOVecs,
                            uint32_t NumIOVecs) {
   uint32_t I;
   FILE *File = (FILE *)This->WriterCtx;
   for (I = 0; I < NumIOVecs; I++) {
     if (IOVecs[I].Data) {

  * file locking.  If the file has content which is compatible with the
  * current process, it also reads in the profile data in the file and merge
  * it with in-memory counters. After the profile data is merged in memory,
  * the original profile data is truncated and gets ready for the profile
  * dumper. With profile merging enabled, each executable as well as any of
  * its instrumented shared libraries dump profile data into their own data file.
 */
 static FILE *openFileForMerging(const char *ProfileFileName, int *MergeDone) {
-  FILE *ProfileFile;
-  int rc;
+  FILE *ProfileFile = NULL;
+  int rc, fd;
 
+  ProfileFile = getProfileFile();
+  if (ProfileFile) {
+#if !defined(_WIN32)
+    fd = fileno(ProfileFile);
+    lprofLockFd(fd);
+#endif
+  }
+  else {
     createProfileDir(ProfileFileName);
     ProfileFile = lprofOpenFileEx(ProfileFileName);
+  }
   if (!ProfileFile)
     return NULL;
 
   rc = doProfileMerging(ProfileFile, MergeDone);
   if (rc || (!*MergeDone && COMPILER_RT_FTRUNCATE(ProfileFile, 0L)) ||
       fseek(ProfileFile, 0L, SEEK_SET) == -1) {
     PROF_ERR("Profile Merging of file %s failed: %s\n", ProfileFileName,
              strerror(errno));
     fclose(ProfileFile);
     return NULL;
   }
   return ProfileFile;
 }
 
+static FILE* GetFileObject(const char *OutputName) {
+  FILE* File;
+  File = getProfileFile();
+  if (File != NULL) {
+    return File;
+  }
+
+  return fopen(OutputName, "ab");
+}
+
 /* Write profile data to file \c OutputName.  */
 static int writeFile(const char *OutputName) {
   int RetVal;
   FILE *OutputFile;
 
   int MergeDone = 0;
   VPMergeHook = &lprofMergeValueProfData;
-  if (!doMerging())
-    OutputFile = fopen(OutputName, "ab");
-  else
+  if (doMerging())
     OutputFile = openFileForMerging(OutputName, &MergeDone);
+  else
+    OutputFile = GetFileObject(OutputName);
 
   if (!OutputFile)
     return -1;
 
   FreeHook = &free;
   setupIOBuffer();
   ProfDataWriter fileWriter;
   initFileWriter(&fileWriter, OutputFile);
   RetVal = lprofWriteData(&fileWriter, lprofGetVPDataReader(), MergeDone);
 
+
+#if !defined(_WIN32)
+  if (doMerging()) {
+    fd = fileno(ProfileFile);
+    lprofLockFd(fd);
+  }
+#endif
+
+  if (OutputFile == getProfileFile())
+    fflush(OutputFile);
+  else
     fclose(OutputFile);
+
   return RetVal;
 }
 
 /* Write order data to file \c OutputName.  */
 static int writeOrderFile(const char *OutputName) {
   int RetVal;
   FILE *OutputFile;
 

 void __llvm_profile_initialize_file(void) {
   const char *EnvFilenamePat;
   const char *SelectedPat = NULL;
   ProfileNameSpecifier PNS = PNS_unknown;
   int hasCommandLineOverrider = (INSTR_PROF_PROFILE_NAME_VAR[0] != 0);
 
   EnvFilenamePat = getFilenamePatFromEnv();
   if (EnvFilenamePat) {
-    /* Pass CopyFilenamePat = 1, to ensure that the filename would be valid 
+    /* Pass CopyFilenamePat = 1, to ensure that the filename would be valid
        at the  moment when __llvm_profile_write_file() gets executed. */
     parseAndSetFilename(EnvFilenamePat, PNS_environment, 1);
     return;
   } else if (hasCommandLineOverrider) {
     SelectedPat = INSTR_PROF_PROFILE_NAME_VAR;
     PNS = PNS_command_line;
   } else {
     SelectedPat = NULL;

 COMPILER_RT_VISIBILITY
 int __llvm_profile_write_file(void) {
   int rc, Length;
   const char *Filename;
   char *FilenameBuf;
   int PDeathSig = 0;
 
   if (lprofProfileDumped()) {
-    PROF_NOTE("Profile data not written to file: %s.\n", 
-              "already written");
+    PROF_NOTE("Profile data not written to file: %s.\n", "already written");
     return 0;
   }
 
   Length = getCurFilenameLength();
   FilenameBuf = (char *)COMPILER_RT_ALLOCA(Length + 1);
   Filename = getCurFilename(FilenameBuf, 0);
 
   /* Check the filename. */

compiler-rt/test/profile/instrprof-set-file-object-merging.c

+// Test that the specified output merges the profiling data.
+// Run the program twice so that the counters accumulate.
+// RUN: %clang -fprofile-instr-generate -fcoverage-mapping -o %t %s
+// RUN: %run %t %t.merging.profraw
+// RUN: %run %t %t.merging.profraw
+// RUN: test -f %t.merging.profraw
+// RUN: llvm-profdata merge -o %t.merging.profdata %t.merging.profraw
+// RUN: llvm-cov show -instr-profile %t.merging.profdata %t | FileCheck %s --match-full-lines
+// RUN: rm %t.merging.profdata %t.merging.profraw
+#include <stdio.h>
+
+extern void __llvm_profile_set_file_object(FILE*);
+extern void __llvm_profile_enable_merge(int);
+
+int main(int argc, const char *argv[]) {
+  if (argc < 2)
+    return 1;
+
+  FILE* F = fopen(argv[1], "r+b");
+  if (!F) {
+    // File might not exist, try opening with truncation
+    F = fopen(argv[1], "w+b");
+  }
+  __llvm_profile_set_file_object(F);
+  __llvm_profile_enable_merge(1);
+
+  return 0;
+}
+// CHECK:   10|       |#include <stdio.h>
+// CHECK:   11|       |
+// CHECK:   12|       |extern void __llvm_profile_set_file_object(FILE*);
+// CHECK:   13|       |extern void __llvm_profile_enable_merge(int);
+// CHECK:   14|       |
+// CHECK:   15|      2|int main(int argc, const char *argv[]) {
+// CHECK:   16|      2|  if (argc < 2)
+// CHECK:   17|      0|    return 1;
+// CHECK:   18|      2|
+// CHECK:   19|      2|  FILE* F = fopen(argv[1], "r+b");
+// CHECK:   20|      2|  if (!F) {
+// CHECK:   21|      1|    // File might not exist, try opening with truncation
+// CHECK:   22|      1|    F = fopen(argv[1], "w+b");
+// CHECK:   23|      1|  }
+// CHECK:   24|      2|  __llvm_profile_set_file_object(F);
+// CHECK:   25|      2|  __llvm_profile_enable_merge(1);
+// CHECK:   26|      2|
+// CHECK:   27|      2|  return 0;
+// CHECK:   28|      2|}

compiler-rt/test/profile/instrprof-set-file-object.c

+// Test that the specified output has profiling data.
+// RUN: %clang -fprofile-instr-generate -fcoverage-mapping -o %t %s
+// RUN: %run %t %t.file.profraw
+// RUN: test -f %t.file.profraw
+// RUN: llvm-profdata merge -o %t.file.profdata %t.file.profraw
+// RUN: llvm-cov show -instr-profile %t.file.profdata %t | FileCheck %s --match-full-lines
+// RUN: rm %t.file.profraw %t.file.profdata
+#include <stdio.h>
+
+extern void __llvm_profile_set_file_object(FILE*);
+
+int main(int argc, const char *argv[]) {
+  if (argc < 2)
+    return 1;
+
+  FILE* F = fopen(argv[1], "w+b");
+  __llvm_profile_set_file_object(F);
+  return 0;
+}
+// CHECK:    8|       |#include <stdio.h>
+// CHECK:    9|       |
+// CHECK:   10|       |extern void __llvm_profile_set_file_object(FILE*);
+// CHECK:   11|       |
+// CHECK:   12|      1|int main(int argc, const char *argv[]) {
+// CHECK:   13|      1|  if (argc < 2)
+// CHECK:   14|      0|    return 1;
+// CHECK:   15|      1|
+// CHECK:   16|      1|  FILE* F = fopen(argv[1], "w+b");
+// CHECK:   17|      1|  __llvm_profile_set_file_object(F);
+// CHECK:   18|      1|  return 0;
+// CHECK:   19|      1|}