[PATCH] D51619: [gcov] Fix branch counters with switch statements

[Vedant Kumar via Phabricator via llvm-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM.



================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:630
   bool Result = false;
-  bool InsertIndCounterIncrCode = false;
   for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {
     SmallVector<std::pair<GlobalVariable *, MDNode *>, 8> CountersBySP;
----------------
calixte wrote:
> vsk wrote:
> > Reading through the diff I noticed that this `i` is shadowed. But, what is the purpose of this outer loop? Can we just delete it (maybe in a follow-up)?
> I think the explanation is here:
> https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Instrumentation/GCOVProfiling.cpp#L545
> 
> But you're right: the previous implementation was buggy since i and e were shadowed.
Here's the inline comment:
```
    // Each compile unit gets its own .gcno file. This means that whether we run
    // this pass over the original .o's as they're produced, or run it after
    // LTO, we'll generate the same .gcno files.
```

I somewhat follow that comment, but it's left me confused about the purpose of the outer loop in emitProfileArcs. The loop body doesn't appear to depend on `i` at all and doesn't inspect CU_Nodes. All of the compiler-rt/llvm tests pass when I remove the loop.

Actually, doesn't it seem like the outer loop would actively do the wrong thing with LTO enabled (insert duplicate instrumentation)?


Repository:
  rL LLVM

https://reviews.llvm.org/D51619