Sorry, I was on holiday.
Yes, this LGTM!

How does the patch cause a new problem? This is a pre-existing problem with the previous atexit approach.

There is only a problem with the (possibly very rare) case where calling exec* fails:

• the process calls exec*;
• llvm_writeout_files is called, which removes the writeout list;
• exec* fails;
• the process continues, the coverage is lost because when it ends there is no writeout list anymore.

I wonder if this fixed the scenario from https://reviews.llvm.org/D54599 too

I wonder if https://reviews.llvm.org/D76206 didn't fix this too

Could you test the last iteration of the patch on Mozilla's CI (with the workaround for the mismatch in LLVM version used by Rust)?

Also, as we discussed, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93623#c5 regarding exec.

LGTM. GCC is doing the same.

Very nice simplification, I hope this won't introduce regressions.
I think we should test the patch on Mozilla CI before landing, so we can be reasonably confident that it doesn't break things.

The code is a bit confusing now, too many if-else etc.. Let's see what happens if you apply my comments. Maybe you could also move the code to open a file with O_EXCL first and without O_EXCL in a separate function, since you're using it twice.

Thanks!

Not needed anymore, I think you can close this.

We could add tests for the other functions too, but they are going to be basically the same. Feel free to land it as is or after adding tests for the other functions.

Landed in rCRT346300 (sorry, forgot to land it via arc).

Moved DWORD_HI and DWORD_LO to WindowsMMap.h, and removed FlushFileBuffers as it fails with ERROR_INVALID_HANDLE.

Add calls to FlushViewOfFile and FlushFileBuffers when unmapping to be sure that all the data is written to disk.

Made mmap_handle NULL by default (as CreateFileMapping returns NULL on failure and not INVALID_HANDLE_VALUE).

Calixte is working on this in https://reviews.llvm.org/D53593.

This looks good to me, modulo the changes requested by efriedma (and the additional tests to write).

We might have to introduce a separate option here.
-coverage-no-function-names-in-data could be useful when you want smaller GCDA files, but still want to be able to parse the function names.

In D49915#1181637, @dblaikie wrote:

I couldn't quite spot it - where's the code that sets or constructs a DILocation with covered = true?

In D49915#1180453, @vsk wrote:

Some lines have a hit counter where they should not have one.
For example, in C++, some cleanup is adding at the end of a scope represented by a '}'.
So such a line has a hit counter where a user expects to not have one.

Could you share an example of this resulting in an incorrect coverage report?

I hope this won't have any side effects we are not expecting. The updated tests in compiler-rt all look good, so I'm reasonably confident.

Looks good to me overall. One difference I noticed with GCC is that they repeat an additional cycle count when there's a negative cycle. But in our case we don't have negative counts, so this could not happen (here's a bug I found about negative counts in GCC: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67937).

There's a problem that I hadn't noticed while running tests (as they all pass), but is noticeable when building a standalone source file.
The interception library is using functions from libdl (dlsym and dlvsym), which can't be resolved unless you build with "-ldl".

LGTM! I assume you caught all the affected function calls.

I've just relanded https://reviews.llvm.org/D48538 in r336678, once the issues which made the tests fail are fixed we should remove the XFAILs. I've filed https://bugs.llvm.org/show_bug.cgi?id=38121 to track it.

Patch rebased. I've removed llvm_gcov_flush and made __gcov_flush visible.

• instrprof-shared-main-gcov-flush_no-writeout.c.gcov: lines 23, 26 and 30 should not be covered; lines 33 and 35 should not be covered (they are considered as uncoverable instead).

 Sorry, what I meant is that we could remove __llvm_gcov_writeout and only keep __llvm_gcov_flush (they are defined in lib/Transforms/Instrumentation/GCOVProfiling.cpp and passed to llvm_gcov_init). We currently have writeout functions and flush functions, where the difference is just that the flush function is also resetting the counters to 0.
 At exit, instead of calling __llvm_gcov_writeout, we could call __llvm_gcov_flush. Since we are at exit, the fact that we reset the counters to 0 for the current module doesn't matter (as the module is not usable anymore after that).
At exit we would still call __llvm_gcov_flush only for the module that is exiting, at __gcov_flush we would call __llvm_gcov_flush for all modules.

This sounds safe, but this does not belong to this patch.

On Tue, Jul 3, 2018 at 4:29 PM Marco Castelluccio via Phabricator <reviews@reviews.llvm.org> wrote:

marco-c updated this revision to Diff 154015.
marco-c added a comment.

I've taken another approach which looks simpler to me. Instead of having two lists, I only have a shared list and I use a static variable's address as an identifier for a dynamic object.

I've also added another test (which would have failed with the previous iteration of the patch) which dlopens three libraries.

I will file follow-up bugs for things that don't look right in the llvm-cov output which are not regressions from this patch:

- instrprof-shared-main-gcov-flush_no-writeout.c.gcov: lines 23, 26 and 30 should not be covered; lines 33 and 35 should not be covered (they are considered as uncoverable instead).
- instrprof-dlopen-func{2,3}.c.gcov: the only line in the function should be covered once and not thrice

Regarding removing writeout and only always using __gcov_flush, do you see any disadvantage if we do that?

writeout function only writes the profile for the current dynamic module which is different from the behavior of __gcov_flush. The form is expected when shared lib is unloaded -- i.e., do not dump other load module's profile unless being explicitly told so (to use gcov_flush). The write out is similar to gcc's gcov_exit. In short, we should keep it as it is.

Sorry, what I meant is that we could remove llvm_gcov_writeout and only keep llvm_gcov_flush (they are defined in lib/Transforms/Instrumentation/GCOVProfiling.cpp and passed to llvm_gcov_init). We currently have writeout functions and flush functions, where the difference is just that the flush function is also resetting the counters to 0.
At exit, instead of calling llvm_gcov_writeout, we could call llvm_gcov_flush. Since we are at exit, the fact that we reset the counters to 0 for the current module doesn't matter (as the module is not usable anymore after that).
At exit we would still call llvm_gcov_flush only for the module that is exiting, at gcov_flush we would call __llvm_gcov_flush for all modules.

I've taken another approach which looks simpler to me. Instead of having two lists, I only have a shared list and I use a static variable's address as an identifier for a dynamic object.

I've merged all the shared / __gcov_flush tests into a single file and made the test a little more involved by adding a "bar" function in the main file.

I'm using two lists, one for the current dynamic object, one shared between all. We could make the shared one a list of lists, but I feel it would make the code more complex, with no large benefit (if you disagree, I can update the patch).
At writeout, we only write out the current dynamic object counters; at __gcov_flush, we write out and reset counters for all dynamic objects.

OK! Sounds good to me to keep it hidden until
https://reviews.llvm.org/D48538 is done (I'm going to try to finish it
soon).

Why keeping a gcov_flush which is incompatible with GCC and previous versions of LLVM and adding a llvm_gcov_flush which is compatible? I feel the other way around (or even just having an unhidden "gcov_flush" and not introducing "llvm_gcov_flush") would be more sensible.

Yes, the behavior changed very recently, I would be surprised if
somebody came to depend on it. It's more likely that some clients are
depending on the old behavior.

Wouldn't it be better to keep compatibility with GCC and make
__gcov_flush have default visibility?

In D45454#1070884, @belleyb wrote:

@chh I had a chance to try out your proposed changes. It's not causing us any trouble. In fact, __gcov_flush() is not even used at all (at least in LLVM 5.0.1).. I can recompile llvm, compiler_rt and clang and re-run all the tests with __gcov_flush commented out! No problem.

I would suggest adding a bit more documentation to __gcov_flush(), thus describing what those "special cases" are...

Here's an example usage of this: https://github.com/marco-c/grcov/blob/5607a99f32748de23cf96ebbfe4edf3e2197a9cf/src/c/llvmgcov.cpp#L8

The change was already positively reviewed with the request for a test, which has been written since then.

The change is very contained and safe and we need it in 6.0.

Replaced fopen with fdopen again, added O_BINARY when originally opening file with open.

Updated patch to handle asynchronous I/O case and to implement all flock functionality as suggested.

Simply moved the isUsingFuncletBasedEH check before setting Result, otherwise it could be set to true even when it wasn't supposed to.

Disabled instrumentation for functions using funclet-based EH.

Updated patch to add catchswitch successors to ComplexEdgeSuccs

I think any invoke or cleanupret that unwinds to a catchswitch needs to look through the successors of the catchswitch. The unwind edge may be another catchswitch, which we must also look through. We should add all the non-catchswitch successors to ComplexEdgeSuccs.