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

[David Li via Phabricator via llvm-commits]

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
}


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62541/new/

https://reviews.llvm.org/D62541