Page MenuHomePhabricator

Flush counters before forking to avoid counting the execution before fork twice
AbandonedPublic

Authored by marco-c on Jul 17 2018, 5:10 PM.

Details

Reviewers
davidxl
Summary

If we don't flush the counters before fork, the counters of the execution up to the fork call will be added twice (by both processes).

This fixes https://bugs.llvm.org/show_bug.cgi?id=38180.

There's also another problem that probably should be fixed on the Clang/LLVM side but is not directly related to this patch (even though you can see its effects in the test): after fork there should be a separate block (right now everything in main in the test is in the same block).

Diff Detail

Event Timeline

marco-c created this revision.Jul 17 2018, 5:10 PM
Herald added subscribers: Restricted Project, llvm-commits, mgorny. ยท View Herald TranscriptJul 17 2018, 5:10 PM
davidxl added inline comments.Jul 18 2018, 11:52 AM
test/profile/Inputs/instrprof-gcov-fork.c.gcov
9

func1 is executed once not twice.

11

func2 is executed twice not 4 times.

20

this line should be executed twice.

22

same here.

One of the real issues is that func1, fork, and func2 are in the same BB and share the same profile counter. The BB ending with fork should be split so that func2 's block has its own counter.

Il 18/07/2018 20:52, David Li via Phabricator ha scritto:

davidxl added inline comments.

================
Comment at: test/profile/Inputs/instrprof-gcov-fork.c.gcov:9
+ CHECK-NEXT:function func1 called 1 returned 100% blocks executed 100%
+
CHECK-NEXT: 2: 3:void func1() {}
+// CHECK-NEXT:function func2 called 2 returned 100% blocks executed 100%


func1 is executed once not twice.

This is a pre-existing problem
(https://bugs.llvm.org/show_bug.cgi?id=38065), but you can see that
after the patch it is actually executed once ("called 1").

================
Comment at: test/profile/Inputs/instrprof-gcov-fork.c.gcov:11
+ CHECK-NEXT:function func2 called 2 returned 100% blocks executed 100%
+
CHECK-NEXT: 4: 4:void func2() {}
+// CHECK-NEXT: -: 5:


func2 is executed twice not 4 times.

Same here.

================
Comment at: test/profile/Inputs/instrprof-gcov-fork.c.gcov:20
+ CHECK-NEXT: -: 11:
+
CHECK-NEXT: 1: 12: func2();
+// CHECK-NEXT: -: 13:


this line should be executed twice.

================
Comment at: test/profile/Inputs/instrprof-gcov-fork.c.gcov:22
+ CHECK-NEXT: -: 13:
+
CHECK-NEXT: 1: 14: return 0;
+// CHECK-NEXT: -: 15:}


same here.
davidxl added a comment.

One of the real issues is that func1, fork, and func2 are in the same BB and share the same profile counter. The BB ending with fork should be split so that func2 's block has its own counter.

Exactly, these are due to the problem I mentioned in the summary, which
probably has to be fixed in the Clang/LLVM side.
I think they are unrelated problems though, this patch is going to fix
the double counting, the other patch would fix the BBs in the function
which calls fork.


llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

This revision is now accepted and ready to land.Jul 19 2018, 10:16 AM

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

davidxl requested changes to this revision.Jul 19 2018, 3:17 PM

Adding dependency on libdl is not desirable. I think you also need to change codegen (when coverage is on) to lower call to fork into call to the gcov version.

This revision now requires changes to proceed.Jul 19 2018, 3:17 PM
marco-c abandoned this revision.Oct 25 2018, 2:48 AM

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