This is an archive of the discontinued LLVM Phabricator instance.

Adding a function for setting coverage output file.
ClosedPublic

Authored by sajjadm on May 28 2019, 11:44 AM.

Event Timeline

sajjadm created this revision.May 28 2019, 11:44 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 28 2019, 11:44 AM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript

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.

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 added a comment.May 29 2019, 9:13 AM

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 updated this revision to Diff 202061.EditedMay 29 2019, 2:41 PM

Added support for merging, docs, and test case.

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

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 updated this revision to Diff 202074.May 29 2019, 4:03 PM
sajjadm marked an inline comment as done.

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

sajjadm updated this revision to Diff 202075.May 29 2019, 4:05 PM

Updated test.

Harbormaster completed remote builds in B32653: Diff 202075.
sajjadm marked 2 inline comments as done.May 29 2019, 4:07 PM

@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?

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 added inline comments.May 29 2019, 5:07 PM
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 added a comment.EditedMay 30 2019, 2:53 PM

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.

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 updated this revision to Diff 202834.Jun 3 2019, 6:05 PM

Added explicit merging API and test.

sajjadm updated this revision to Diff 202836.Jun 3 2019, 6:22 PM

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 marked 2 inline comments as done.Jun 3 2019, 6:23 PM
sajjadm updated this revision to Diff 202991.Jun 4 2019, 11:23 AM

Forgot to declare a variable.

sajjadm updated this revision to Diff 202992.Jun 4 2019, 11:25 AM

Ran clang-format.

Harbormaster completed remote builds in B32892: Diff 202992.
davidxl added inline comments.Jun 4 2019, 2:09 PM
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 marked an inline comment as done.Jun 4 2019, 2:21 PM
sajjadm added inline comments.
compiler-rt/lib/profile/InstrProfilingFile.c
309–321

Good catch, that was supposed to be an unlock.

sajjadm updated this revision to Diff 203034.Jun 4 2019, 2:54 PM

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

sajjadm marked 3 inline comments as done.Jun 4 2019, 2:57 PM
davidxl accepted this revision.Jun 5 2019, 9:47 AM

lgtm. Watch out for problems on other platforms.

This revision is now accepted and ready to land.Jun 5 2019, 9:47 AM

Could someone please commit for me?

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

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

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

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

This revision was automatically updated to reflect the committed changes.
Dor1s added a comment.Jun 6 2019, 7:47 AM

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

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