[PATCH] D49659: [gcov] Fix wrong line hit counts when multiple blocks are on the same line

[Marco Castelluccio via Phabricator via llvm-commits]

marco-c accepted this revision.
marco-c added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: lib/ProfileData/GCOV.cpp:488
+/// Check if a vertex is on the same line as the others
+bool GCOVBlock::isBlockOnSameLine(const GCOVBlock *V, const BlockVector &Blocks) {
+  return std::find(Blocks.begin(), Blocks.end(), V) != Blocks.end();
----------------
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?


================
Comment at: lib/ProfileData/GCOV.cpp:528
+      BlockVector &List = BlockLists[index];
+      if (std::find(List.begin(), List.end(), V) == List.end()) {
+        List.push_back(V);
----------------
You might also use the inBlockVector function here


================
Comment at: lib/ProfileData/GCOV.cpp:553
+  for (auto Block : Blocks) {
+    if (!Block->hasSrcEdges() && Block->Counter) {
+      // the block has no predecessor and a non-null counter
----------------
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.


================
Comment at: lib/ProfileData/GCOV.cpp:556
+      // (can be the case with entry block in functions)
+      Count += Block->Counter;
+    } else {
----------------
Maybe better to use the public function getCount like we were doing before


================
Comment at: lib/ProfileData/GCOV.cpp:558
+    } else {
+      for (auto E : Block->srcs()) {
+        const GCOVBlock *W = &E->Src;
----------------
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"


Repository:
  rL LLVM

https://reviews.llvm.org/D49659