[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
}
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62541/new/
https://reviews.llvm.org/D62541
More information about the llvm-commits
mailing list