[PATCH] D62541: Adding a function for setting coverage output file.

David Li via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 29 15:31:14 PDT 2019

davidxl added a comment.

To support merging, the new interface needs to take more

Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:268
+  if (ProfileFilePtr != NULL) {
+    OutputFile = ProfileFilePtr;
+    if (doMerging()) {
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.

Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:269
+    OutputFile = ProfileFilePtr;
+    if (doMerging()) {
+      RetVal = doProfileMerging(OutputFile, &MergeDone);
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

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list