This is an archive of the discontinued LLVM Phabricator instance.

[gcov] Fix wrong line hit counts when multiple blocks are on the same line
ClosedPublic

Authored by calixte on Jul 23 2018, 2:18 AM.

Details

Summary

The goal of this patch is to have the same behaviour than gcc-gcov.
Currently the hit counts for a line is the sum of the counts for each block on that line.
The idea is to detect the cycles in the graph of blocks in using the algorithm by Hawick & James.
The count for a cycle is the min of the counts for each edge in the cycle.
Once we've the count for each cycle, we can sum them and add the transition counts of those cycles.

Fix both https://bugs.llvm.org/show_bug.cgi?id=38065 and https://bugs.llvm.org/show_bug.cgi?id=38066

Diff Detail

Repository
rL LLVM

Event Timeline

calixte created this revision.Jul 23 2018, 2:18 AM
calixte edited the summary of this revision. (Show Details)Jul 23 2018, 4:29 AM

Don't review for now: need to fix some things

calixte updated this revision to Diff 156985.Jul 24 2018, 2:25 AM

Handle the case where a block has no predecessors and have a counter (such as entry blocks in functions).

calixte updated this revision to Diff 156986.Jul 24 2018, 2:27 AM

Fix a comment

This patch requires to accept https://reviews.llvm.org/D49721 too to fix tests in compiler-rt.

marco-c accepted this revision.Jul 27 2018, 9:23 AM

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

A suggestion to "smoke test" the patch: take a set of gcda files from a Firefox run and check that we can still parse everything without crashing :)

lib/ProfileData/GCOV.cpp
488 ↗(On Diff #156986)

These two functions are basically the same, modulo the name of the second argument.
Maybe you can have just one function GCOVBlock::inBlockVector and always use it?

528 ↗(On Diff #156986)

You might also use the inBlockVector function here

553 ↗(On Diff #156986)

Nit: you could use getNumSrcEdges() == 0 instead, to avoid adding a new function.

Also, it seems like we can always enter this branch when there are no predecessors, no matter if count is not 0.

556 ↗(On Diff #156986)

Maybe better to use the public function getCount like we were doing before

558 ↗(On Diff #156986)

I would add some comments, e.g. here you can say something along the lines of "Add counts coming from predecessor blocks that are not on the same line"

This revision is now accepted and ready to land.Jul 27 2018, 9:23 AM
lebedev.ri added inline comments.
include/llvm/ProfileData/GCOV.h
380 ↗(On Diff #156986)

I'm not sure this patch is formatted correctly.

vsk added a subscriber: vsk.Jul 30 2018, 10:25 AM

Thanks for working on this!

lib/ProfileData/GCOV.cpp
454 ↗(On Diff #156986)

Please use verb-like function names which describe the function's action, and end sentences in comments with periods (http://www.llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

466 ↗(On Diff #156986)
467 ↗(On Diff #156986)

If you'd prefer, you can use range-based helpers from STLExtras.h. E.g 'find(Blocked, U)'.

calixte updated this revision to Diff 160316.Aug 13 2018, 3:07 AM

Fix comments, function names and use llvm::find.

I collected 2808 gcda (over 3211 gcno) from firefox build and ran "llvm-cov gcov" on each of them and no crash at all.

This revision was automatically updated to reflect the committed changes.