This is an archive of the discontinued LLVM Phabricator instance.

[Coverage] Prevent creating a redundant counter if a nested body ends with a macro.
ClosedPublic

Authored by ikudrin on Aug 4 2016, 5:22 AM.

Details

Summary

If there were several nested statements arranged in a way that all of them end up with the same macro,
then the expansion of this macro was assigned with all the corresponding counters of these statements.
As a result, the wrong counter value was shown for the macro in llvm-cov.

This patch fixes the issue by preventing adding a counter for an expanded source range
if it already has an assigned counter, which is expected to come from the most specific statement.

Diff Detail

Event Timeline

ikudrin updated this revision to Diff 66790.Aug 4 2016, 5:22 AM
ikudrin retitled this revision from to [Coverage] Prevent creating a redundant counter if a nested body ends with a macro..
ikudrin updated this object.
ikudrin added reviewers: vsk, davidxl, bogner.
ikudrin added a subscriber: cfe-commits.

The motivation sample:

$ cat > test.cpp << EOF
void dummy() {}

#define MACRO dummy()

int main()
{
  int i = 0;
  while (i++ < 10)
    if (i < 5)
      MACRO;
  return 0;
}
EOF
$ clang++ -fprofile-instr-generate -fcoverage-mapping dummy.cpp test.cpp
$ ./a.out
$ llvm-profdata merge -o default.profdata default.profraw
$ llvm-cov show a.out -instr-profile --show-expansions default.profdata
test.cpp:
      4|    1|void dummy() {}
       |    2|
     14|    3|#define MACRO dummy()
       |    4|
       |    5|int main()
      1|    6|{
      1|    7|  int i = 0;
     11|    8|  while (i++ < 10)
     10|    9|    if (i < 5)
      4|   10|      MACRO;
  ------------------
  |  |     14|    3|#define MACRO dummy()
  ------------------
      1|   11|  return 0;
      1|   12|}

After applying this patch the counter for MACRO shows a reasonable value:

$ llvm-cov show a.out -instr-profile --show-expansions default.profdata
test.cpp:
      4|    1|void dummy() {}
       |    2|
      4|    3|#define MACRO dummy()
       |    4|
       |    5|int main()
      1|    6|{
      1|    7|  int i = 0;
     11|    8|  while (i++ < 10)
     10|    9|    if (i < 5)
      4|   10|      MACRO;
  ------------------
  |  |      4|    3|#define MACRO dummy()
  ------------------
      1|   11|  return 0;
      1|   12|}
vsk accepted this revision.Aug 4 2016, 11:44 AM
vsk edited edge metadata.

Thanks for catching this! I couldn't really reduce your test case any further. This LGTM.

I guess it never makes sense to have two regions with the exact same start/end loc, and different counters. Do you think we should add assertions in llvm (either in llvm-cov, or in the coverage reader) which guard against this?

This revision is now accepted and ready to land.Aug 4 2016, 11:44 AM
vsk added a subscriber: hans.Aug 4 2016, 3:24 PM

(@hans If there are no objections, and if it's still possible, it would be great to see this merged into 3.9 once it has landed.)

In D23160#506061, @vsk wrote:

I guess it never makes sense to have two regions with the exact same start/end loc, and different counters. Do you think we should add assertions in llvm (either in llvm-cov, or in the coverage reader) which guard against this?

Right now we can have several ranges with the same start and end locations in case of macro fully expanded into another macro. We consider this situation possible and handle it in class SegmentBuilder in llvm/lib/ProfileData/Coverage/CoverageMapping.cpp. Maybe something has to be fixed here too.

By the way, thinking about your words, I found another example which results in spawning not only redundant but also wrong counters. I'm postponing landing this patch until I finish the investigation.

    4|    1|void dummy() {}
     |    2|
    4|    3|#define M_INT dummy()
     |    4|
   11|    5|#define MACRO M_INT
     |    6|
     |    7|int main()
    1|    8|{
    1|    9|  int i = 0;
   11|   10|  while (i++ < 10)
   10|   11|    if (i < 5)
   10|   12|      MACRO;
------------------
|  |     11|    5|#define MACRO M_INT
|  |  ------------------
|  |  |  |      4|    3|#define M_INT dummy()
|  |  ------------------
------------------
    1|   13|  return 0;
    1|   14|}

I've reduced the last sample to the following:

    0|    1|void dummy() {}
     |    2|
    0|    3|#define MACRO_1 dummy()
     |    4|
    1|    5|#define MACRO_2 MACRO_1
     |    6|
    1|    7|int main() {
    1|    8|  int i = 0;
    1|    9|  if (i > 5)
    1|   10|    MACRO_2;
------------------
|  |      1|    5|#define MACRO_2 MACRO_1
|  |  ------------------
|  |  |  |      0|    3|#define MACRO_1 dummy()
|  |  ------------------
------------------
    1|   11|  return 0;
    1|   12|}

Right now, I don't have a good idea how to fix this.

I figured out that the last issue is an independent one, so I prepared a separate fix for it: D23987.

This revision was automatically updated to reflect the committed changes.