This is an archive of the discontinued LLVM Phabricator instance.

Expose __gcov_flush for parity with libgcov in the gcc project
Needs RevisionPublic

Authored by ngie on Jun 21 2017, 10:54 PM.

Details

Reviewers
vsk
samsonov
void
Summary

Exposing __gcov_flush increases parity with libgcov project, and exposes
the symbol for use with applications that need to flush gcov information,
counters, etc, when exiting via calls which don't invoke __gcov_flush
indirectly, like _exit(2).

Update the tests to use the new header, gcov.h, and fix the tests so
they compile cleanly on [more] platforms by including the appropriate
headers.

Bug: 32555

Diff Detail

Repository
rL LLVM

Event Timeline

ngie created this revision.Jun 21 2017, 10:54 PM
ngie edited the summary of this revision. (Show Details)Jun 21 2017, 10:54 PM
ngie edited the summary of this revision. (Show Details)
ngie retitled this revision from Expose __gcov_flush for parity with libgcov in GNU project to Expose __gcov_flush for parity with libgcov in the gcc project.
emaste added a subscriber: emaste.Jul 18 2017, 5:17 PM
ngie abandoned this revision.Dec 16 2017, 5:14 PM

I guess no one cares. Oh well, at least the gcc folks respond to reviews/bugs in a <6 month timeframe.

davide added a reviewer: vsk.Dec 17 2017, 3:09 PM
davide added a subscriber: davide.

@ngie, Vedant can probably take a look.
Sometimes code reviews get lost, and it's generally good to ping people here or on IRC.

With that in mind:

  1. You may want to consider adding context to your patch, i.e. via git diff -U500000 &> blah.patch
  2. You may want not jump to conclusions so quickly. I'm pretty sure people care about your contributions, it's just that nobody got around to review your patch.
vsk added a comment.Dec 18 2017, 10:58 AM

Sorry this slipped through the cracks. It's OK to ping patches weekly to make sure they get attention.

This patch generally looks good. Could you add a test and source context?

vsk added a comment.Dec 18 2017, 11:07 AM

Here's a good example of how to write a test for this sort of thing: test/profile/instrprof-reset-counters.c

ngie reclaimed this revision.Feb 19 2019, 8:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 8:29 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
ngie added a comment.EditedFeb 19 2019, 8:30 PM

With that in mind:

  1. You may want to consider adding context to your patch, i.e. via git diff -U500000 &> blah.patch
  2. You may want not jump to conclusions so quickly. I'm pretty sure people care about your contributions, it's just that nobody got around to review your patch.

When I wrote my previous comment, I was really angry and disheartened at a number of things. This was just the straw that broke the camel's back :/.

I'll try to do what I was doing before, again, taking your advice.

ngie updated this revision to Diff 187516.Feb 19 2019, 9:19 PM

Update tests to reference gcov.h

Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 9:19 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vsk added inline comments.Feb 20 2019, 11:12 AM
compiler-rt/include/CMakeLists.txt
57 ↗(On Diff #187516)

I've noticed that the sanitizer headers fall under a dedicated directory. Is it worth doing that for gcov (i.e. have <gcov/gcov.h>)?

compiler-rt/include/gcov.h
7 ↗(On Diff #187516)

There's a new llvm license now, and I think it applies here:

|* Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
|* See https://llvm.org/LICENSE.txt for license information.                       
|* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
ngie marked 5 inline comments as done.Feb 20 2019, 9:50 PM
ngie added inline comments.
compiler-rt/include/CMakeLists.txt
57 ↗(On Diff #187516)

For compatibility reasons, this header should be installed under a toolchain visible path (which compiler-rt provides).

Here's the path for gcc 8.2.0:

$ find /usr/local/ -name gcov.h
/usr/local/lib/gcc8/gcc/x86_64-portbld-freebsd13.0/8.2.0/include/gcov.h

The compiler-rt headers will be installed in /usr/local/llvm80/lib/clang/8.0.0/include/ on FreeBSD for the third-party package.

compiler-rt/include/gcov.h
7 ↗(On Diff #187516)

*nod!*

19–21 ↗(On Diff #187516)

This seems like a mis-paste.

ngie updated this revision to Diff 187726.Feb 20 2019, 10:00 PM
ngie marked an inline comment as done.

Update licensing header for gcov.h and fix a mispaste with the comment block for __gcov_flush(..)

vsk accepted this revision.Feb 21 2019, 11:21 AM

Thanks, LGTM. If you don't have commit access, let me know and I can land this on your behalf.

This revision is now accepted and ready to land.Feb 21 2019, 11:21 AM
ngie added a comment.Mar 29 2019, 10:05 AM
In D34499#1406167, @vsk wrote:

Thanks, LGTM. If you don't have commit access, let me know and I can land this on your behalf.

Hi @vsk! I don't have commit access -- could you please land this for me?

vsk added a comment.Apr 2 2019, 10:19 AM

Hi @ngie,

Sorry for the delay here. I am hitting two test failures with this patch applied on Darwin. Could you take a look?

/Users/vsk/src/llvm-project-master/compiler-rt/test/profile/Posix/../Inputs/instrprof-gcov-fork.c.gcov:10:15: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT:function func2 called 2 returned 100% blocks executed 100%
              ^
instrprof-gcov-fork.c.gcov:10:1: note: scanning from here
function func2 called 1 returned 100% blocks executed 100%

...

/Users/vsk/src/llvm-project-master/compiler-rt/test/profile/Inputs/instrprof-gcov-__gcov_flush-multiple.c:6:7: warning: implicit declaration of function 'remove' is invalid in C99 [-Wimplicit-function-declaration]
  if (remove("instrprof-gcov-__gcov_flush-multiple.gcda") != 0) {
      ^
1 warning generated.
/Users/vsk/src/llvm-project-master/compiler-rt/test/profile/Inputs/instrprof-gcov-__gcov_flush-multiple.c.gcov:6:15: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT:    #####:    1:int main(void) {
              ^
instrprof-gcov-__gcov_flush-multiple.c.gcov:6:9: note: scanning from here
        -:    1:#include <gcov.h>
        ^
instrprof-gcov-__gcov_flush-multiple.c.gcov:8:5: note: possible intended match here
    #####:    3:int main(void) {
ngie added a comment.Apr 21 2019, 12:30 PM
In D34499#1451696, @vsk wrote:

Hi @ngie,

Sorry for the delay here. I am hitting two test failures with this patch applied on Darwin. Could you take a look?

/Users/vsk/src/llvm-project-master/compiler-rt/test/profile/Posix/../Inputs/instrprof-gcov-fork.c.gcov:10:15: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT:function func2 called 2 returned 100% blocks executed 100%
              ^
instrprof-gcov-fork.c.gcov:10:1: note: scanning from here
function func2 called 1 returned 100% blocks executed 100%

...

/Users/vsk/src/llvm-project-master/compiler-rt/test/profile/Inputs/instrprof-gcov-__gcov_flush-multiple.c:6:7: warning: implicit declaration of function 'remove' is invalid in C99 [-Wimplicit-function-declaration]
  if (remove("instrprof-gcov-__gcov_flush-multiple.gcda") != 0) {
      ^
1 warning generated.
/Users/vsk/src/llvm-project-master/compiler-rt/test/profile/Inputs/instrprof-gcov-__gcov_flush-multiple.c.gcov:6:15: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT:    #####:    1:int main(void) {
              ^
instrprof-gcov-__gcov_flush-multiple.c.gcov:6:9: note: scanning from here
        -:    1:#include <gcov.h>
        ^
instrprof-gcov-__gcov_flush-multiple.c.gcov:8:5: note: possible intended match here
    #####:    3:int main(void) {

@vsk: I will try and figure out how to do this in the meantime, but just in case I'm slow, I was wondering how to run the tests.

The compile-time warning on Darwin with remove() is because a header is likely missing (stdio.h?) from the test program and isn't being pulled in via header pollution.

ngie updated this revision to Diff 196014.Apr 21 2019, 1:36 PM

Add stdio.h to test to handle reference to posix API, remove(3)

ngie edited the summary of this revision. (Show Details)Apr 21 2019, 1:37 PM
ngie updated this revision to Diff 196015.Apr 21 2019, 1:37 PM

stdio.h should come before gcov.h for style reasons

Harbormaster completed remote builds in B30822: Diff 196015.
vsk added a comment.Apr 26 2019, 2:02 PM

Thanks for fixing the warning. To run the tests, try 'ninja check-profile' from your build directory. The failure from earlier just meant that the CHECK lines in a few *.gcov files needed to be updated. Here's a version of this patch which fixes up the tests which run on Darwin:

There are a number of tests which reference instrprof-shared-main-gcov-flush.c which still need to be fixed up -- these aren't supported on Darwin, and they weren't as easy for me to get to.

vsk requested changes to this revision.Apr 26 2019, 2:02 PM
This revision now requires changes to proceed.Apr 26 2019, 2:02 PM