marco-c (Marco Castelluccio)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 24 2017, 3:02 AM (59 w, 5 d)

Recent Activity

Today

marco-c added inline comments to D54599: [Profile] Avoid race condition when dumping GCDA files..
Fri, Nov 16, 4:36 AM
marco-c accepted D54195: Fix linker option for -fprofile-arcs -ftest-coverage.
Fri, Nov 16, 2:59 AM
marco-c accepted D54600: [Clang] Add options -fprofile-filter-files and -fprofile-exclude-files to filter the files to instrument with gcov (after revert https://reviews.llvm.org/rL346659).
Fri, Nov 16, 2:58 AM

Wed, Nov 14

marco-c accepted D54195: Fix linker option for -fprofile-arcs -ftest-coverage.

Thanks!

Wed, Nov 14, 10:29 AM

Mon, Nov 12

marco-c accepted D54416: [GCOV] fix test after patch rL346642.
Mon, Nov 12, 1:53 AM

Thu, Nov 8

marco-c requested changes to D54213: [Profile] Mark gcov-fork test as failinf for i386 target.

Not needed anymore, I think you can close this.

Thu, Nov 8, 5:07 PM
marco-c accepted D54263: [Profile] The test for gcov-fork seems to be ok on arm.
Thu, Nov 8, 7:26 AM

Wed, Nov 7

marco-c accepted D54209: [Profile] Mark gcov-fork test as failing for arm.
Wed, Nov 7, 7:47 AM
marco-c accepted D54208: Fix unit tests after patch https://reviews.llvm.org/rL346313.
Wed, Nov 7, 6:45 AM
marco-c accepted D53593: [GCOV] Flush counters before to avoid counting the execution before fork twice and for exec** functions we must flush before the call.
Wed, Nov 7, 5:33 AM
marco-c accepted D54167: [Profile] Fix fork test and add tests for execlp and execvp after patch https://reviews.llvm.org/D53593.

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.

Wed, Nov 7, 5:32 AM
marco-c closed D53988: Close file mapping handle on Windows, so flushed gcda files can be removed while the process is in execution.

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

Wed, Nov 7, 1:43 AM
marco-c added inline comments to D53593: [GCOV] Flush counters before to avoid counting the execution before fork twice and for exec** functions we must flush before the call.
Wed, Nov 7, 1:37 AM
marco-c accepted D52034: [Clang] Add options -fprofile-filter-files and -fprofile-exclude-files to filter the files to instrument with gcov.
Wed, Nov 7, 1:21 AM
marco-c updated the diff for D53988: Close file mapping handle on Windows, so flushed gcda files can be removed while the process is in execution.

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

Wed, Nov 7, 1:18 AM

Tue, Nov 6

marco-c added inline comments to D53988: Close file mapping handle on Windows, so flushed gcda files can be removed while the process is in execution.
Tue, Nov 6, 4:20 AM
marco-c updated the diff for D53988: Close file mapping handle on Windows, so flushed gcda files can be removed while the process is in execution.

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

Tue, Nov 6, 2:22 AM

Mon, Nov 5

marco-c updated the diff for D53988: Close file mapping handle on Windows, so flushed gcda files can be removed while the process is in execution.

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

Mon, Nov 5, 4:37 PM

Thu, Nov 1

marco-c created D53988: Close file mapping handle on Windows, so flushed gcda files can be removed while the process is in execution.
Thu, Nov 1, 11:13 AM
marco-c retitled D53988: Close file mapping handle on Windows, so flushed gcda files can be removed while the process is in execution from Close file mapping handle on Windows, so flushed gcda files can be removed while th to Close file mapping handle on Windows, so flushed gcda files can be removed while the process is in execution.
Thu, Nov 1, 11:13 AM

Tue, Oct 30

marco-c added inline comments to D53601: [GCOV] Add a test for function defined on one line (follow-up of https://reviews.llvm.org/D53600).
Tue, Oct 30, 2:47 PM

Thu, Oct 25

marco-c accepted D53600: [GCOV] Function counters are wrong when on one line.
Thu, Oct 25, 2:52 AM
marco-c abandoned D49460: Flush counters before forking to avoid counting the execution before fork twice.

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

Thu, Oct 25, 2:49 AM
marco-c accepted D53601: [GCOV] Add a test for function defined on one line (follow-up of https://reviews.llvm.org/D53600).
Thu, Oct 25, 2:30 AM

Sep 25 2018

marco-c accepted D52456: [Profile] Fix gcov tests.
Sep 25 2018, 4:06 AM

Sep 21 2018

marco-c accepted D49917: [profile] Fix the tests for patch in https://reviews.llvm.org/D49916..
Sep 21 2018, 2:11 AM

Sep 20 2018

marco-c accepted D52033: [GCOV] Add options to filter files which must be instrumented..
Sep 20 2018, 4:20 AM
marco-c added inline comments to D52088: [GCOV] Don't add a useless block in the entry.
Sep 20 2018, 2:42 AM
marco-c accepted D52088: [GCOV] Don't add a useless block in the entry.

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

Sep 20 2018, 2:42 AM
marco-c added inline comments to D49917: [profile] Fix the tests for patch in https://reviews.llvm.org/D49916..
Sep 20 2018, 2:40 AM
marco-c accepted D49916: [CodeGen] Add to emitted DebugLoc information about coverage when it's required.
Sep 20 2018, 2:40 AM
marco-c added a comment to D51943: [GCOV] Remove function names from gcno when using option -coverage-no-function-names-in-data.

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.

Sep 20 2018, 2:27 AM
marco-c accepted D52090: [profile] Fix a test.
Sep 20 2018, 2:13 AM
marco-c added inline comments to D52034: [Clang] Add options -fprofile-filter-files and -fprofile-exclude-files to filter the files to instrument with gcov.
Sep 20 2018, 2:06 AM

Jul 31 2018

marco-c added a comment to D49915: [IR] Add a boolean field in DILocation to know if a line must covered or not.

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

Jul 31 2018, 4:46 PM · debug-info

Jul 30 2018

marco-c added a comment to D49915: [IR] Add a boolean field in DILocation to know if a line must covered or not.
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?

Jul 30 2018, 10:38 AM · debug-info

Jul 27 2018

marco-c accepted D49854: [profile] Fix the gcov tests after the patch in D49853 landed..
Jul 27 2018, 10:08 AM
marco-c accepted D49853: [gcov] Display the hit counter for the line of a function definition.

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.

Jul 27 2018, 10:07 AM
marco-c accepted D49721: [profile] Fix tests in compiler-rt for patch in gcov (https://reviews.llvm.org/D49659).
Jul 27 2018, 9:33 AM
marco-c accepted D49659: [gcov] Fix wrong line hit counts when multiple blocks are on the same line.

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).

Jul 27 2018, 9:23 AM

Jul 19 2018

marco-c added a comment to D49460: Flush counters before forking to avoid counting the execution before fork twice.

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".

Jul 19 2018, 2:58 PM

Jul 17 2018

marco-c created D49460: Flush counters before forking to avoid counting the execution before fork twice.
Jul 17 2018, 5:10 PM

Jul 10 2018

marco-c created D49161: Fix reading 32 bit gcov tag values on little-endian machines.
Jul 10 2018, 4:34 PM
marco-c accepted D49134: Fix ABI when calling llvm_gcov_... routines from instrumentation code.

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

Jul 10 2018, 8:02 AM
marco-c accepted D49132: Fix gcov profiling on big-endian machines.

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.

Jul 10 2018, 7:37 AM

Jul 5 2018

marco-c updated the diff for D48538: Make __gcov_flush flush counters for all shared libraries.

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

Jul 5 2018, 8:53 AM
marco-c added a comment to D48538: Make __gcov_flush flush counters for all shared libraries.
  • 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).
Jul 5 2018, 6:05 AM

Jul 4 2018

marco-c added a comment to D48538: Make __gcov_flush flush counters for all shared libraries.
 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.

Jul 4 2018, 3:05 PM

Jul 3 2018

marco-c updated subscribers of D48538: Make __gcov_flush flush counters for all shared libraries.

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.

Jul 3 2018, 5:25 PM
marco-c updated the diff for D48538: Make __gcov_flush flush counters for all shared libraries.

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.

Jul 3 2018, 4:28 PM
marco-c updated the diff for D48538: Make __gcov_flush flush counters for all shared libraries.

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.

Jul 3 2018, 9:33 AM

Jul 2 2018

marco-c updated the diff for D48538: Make __gcov_flush flush counters for all shared libraries.

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.

Jul 2 2018, 3:21 AM

Jun 29 2018

marco-c added a comment to D45454: Add llvm_gcov_flush to be called outside a shared library.

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).

Jun 29 2018, 12:31 PM
marco-c added a comment to D45454: Add llvm_gcov_flush to be called outside a shared library.

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.

Jun 29 2018, 11:06 AM

Jun 26 2018

marco-c added a comment to D45454: Add llvm_gcov_flush to be called outside a shared library.

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.

Jun 26 2018, 2:45 PM
marco-c added inline comments to D48538: Make __gcov_flush flush counters for all shared libraries.
Jun 26 2018, 2:15 PM
marco-c updated subscribers of D45454: Add llvm_gcov_flush to be called outside a shared library.

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

Jun 26 2018, 2:08 PM

Jun 25 2018

marco-c added a comment to D45454: Add llvm_gcov_flush to be called outside a shared library.

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

Jun 25 2018, 7:07 AM
marco-c created D48538: Make __gcov_flush flush counters for all shared libraries.
Jun 25 2018, 3:26 AM

Jan 8 2018

marco-c added a comment to D41802: Allow users of the GCOV API to extend the FileInfo class to implement custom output formats.

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

Jan 8 2018, 4:02 AM
marco-c updated the diff for D41802: Allow users of the GCOV API to extend the FileInfo class to implement custom output formats.
Jan 8 2018, 4:02 AM

Jan 6 2018

marco-c removed rL LLVM as the repository for D41802: Allow users of the GCOV API to extend the FileInfo class to implement custom output formats.
Jan 6 2018, 7:24 AM
marco-c created D41802: Allow users of the GCOV API to extend the FileInfo class to implement custom output formats.
Jan 6 2018, 7:24 AM

Jan 3 2018

marco-c added a comment to D38124: Hide some symbols to avoid a crash on shutdown when using code coverage.

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

Jan 3 2018, 3:25 AM · Restricted Project
marco-c added a comment to D40610: Flush gcda files before unlocking them.

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

Jan 3 2018, 3:24 AM
marco-c updated the diff for D40610: Flush gcda files before unlocking them.
Jan 3 2018, 3:13 AM

Dec 8 2017

marco-c added a reviewer for D40610: Flush gcda files before unlocking them: zturner.
Dec 8 2017, 5:46 AM

Nov 29 2017

marco-c created D40610: Flush gcda files before unlocking them.
Nov 29 2017, 9:15 AM

Nov 8 2017

marco-c closed D38221: Add CoreOption flag to "-coverage" option to make it available for clang-cl.
Nov 8 2017, 11:22 AM
marco-c closed D38891: Implement flock for Windows in compiler-rt.
Nov 8 2017, 11:12 AM

Oct 17 2017

marco-c closed D38984: Use O_BINARY when opening GCDA file on Windows.
Oct 17 2017, 5:22 PM
marco-c updated the diff for D38984: Use O_BINARY when opening GCDA file on Windows.

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

Oct 17 2017, 6:01 AM

Oct 16 2017

marco-c created D38984: Use O_BINARY when opening GCDA file on Windows.
Oct 16 2017, 5:14 PM
marco-c updated the diff for D38891: Implement flock for Windows in compiler-rt.

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

Oct 16 2017, 4:39 PM

Oct 13 2017

marco-c created D38891: Implement flock for Windows in compiler-rt.
Oct 13 2017, 9:27 AM
marco-c closed D38223: Disable gcov instrumentation of functions using funclet-based exception handling.
Oct 13 2017, 6:49 AM

Oct 12 2017

marco-c updated the diff for D38223: Disable gcov instrumentation of functions using funclet-based exception handling.

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

Oct 12 2017, 3:41 AM
marco-c retitled D38223: Disable gcov instrumentation of functions using funclet-based exception handling from Make sure the basic block has an insertion point before dereferencing it to Disable gcov instrumentation of functions using funclet-based exception handling.
Oct 12 2017, 3:32 AM

Oct 5 2017

marco-c updated the diff for D38223: Disable gcov instrumentation of functions using funclet-based exception handling.

Disabled instrumentation for functions using funclet-based EH.

Oct 5 2017, 12:22 PM

Oct 4 2017

marco-c updated the diff for D38223: Disable gcov instrumentation of functions using funclet-based exception handling.

Updated patch to add catchswitch successors to ComplexEdgeSuccs

Oct 4 2017, 12:57 PM
marco-c added a comment to D38223: Disable gcov instrumentation of functions using funclet-based exception handling.

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.

Oct 4 2017, 6:36 AM

Sep 26 2017

marco-c added a reviewer for D38221: Add CoreOption flag to "-coverage" option to make it available for clang-cl: rnk.
Sep 26 2017, 3:30 PM

Sep 25 2017

marco-c created D38224: Don't move llvm.localescape outside the entry block in the GCOV profiling pass.
Sep 25 2017, 5:03 AM
marco-c created D38223: Disable gcov instrumentation of functions using funclet-based exception handling.
Sep 25 2017, 5:03 AM
marco-c created D38221: Add CoreOption flag to "-coverage" option to make it available for clang-cl.
Sep 25 2017, 5:03 AM