[PATCH] D36813: [Coverage] Build sorted and unique segments

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 3 inline comments as done.
vsk added a comment.

Thanks for your comments, I'll update the patch shortly.



================
Comment at: lib/ProfileData/Coverage/CoverageMapping.cpp:373
+      auto CompletedSegmentLoc = ActiveRegions[FirstCompletedRegion]->endLoc();
+      assert((!Loc || CompletedSegmentLoc <= Loc) &&
+             "Completed region ends after the designated location");
----------------
ikudrin wrote:
> We can't get here with `Loc == None` because the only call where it is `None` sets `FirstCompletedRegion` to 0. So, the check for `Loc` in `assert` is a bit misleading. I'd add a separate `asset` to check that `Loc` is set.
Right, I'll tighten the assertion and the logic here.


================
Comment at: lib/ProfileData/Coverage/CoverageMapping.cpp:382
+    // the end location of the previous completed region.
+    for (unsigned I = FirstCompletedRegion + 1, E = ActiveRegions.size(); I < E;
+         ++I) {
----------------
ikudrin wrote:
> The last of completed regions might not be processed completely, here is the test:
> ```
>   ProfileWriter.addRecord({"func1", 0x1234, {1, 2, 3, 4}}, Err);
>   startFunction("func1", 0x1234);
> 
>   addCMR(Counter::getCounter(0), "file1", 1, 1, 8, 8);
>   addCMR(Counter::getCounter(1), "file1", 2, 2, 5, 5);
>   addCMR(Counter::getCounter(2), "file1", 3, 3, 4, 4);
>   addCMR(Counter::getCounter(3), "file1", 6, 6, 7, 7);
> 
>   EXPECT_THAT_ERROR(loadCoverageMapping(), Succeeded());
>   const auto FunctionRecords = LoadedCoverage->getCoveredFunctions();
>   const auto &FunctionRecord = *FunctionRecords.begin();
>   CoverageData Data = LoadedCoverage->getCoverageForFunction(FunctionRecord);
>   std::vector<CoverageSegment> Segments(Data.begin(), Data.end());
> 
>   ASSERT_EQ(8U, Segments.size());
>   EXPECT_EQ(CoverageSegment(1, 1, 1, true), Segments[0]);
>   EXPECT_EQ(CoverageSegment(2, 2, 2, true), Segments[1]);
>   EXPECT_EQ(CoverageSegment(3, 3, 3, true), Segments[2]);
>   EXPECT_EQ(CoverageSegment(4, 4, 2, false), Segments[3]);
>   EXPECT_EQ(CoverageSegment(5, 5, 1, false), Segments[4]);
>   EXPECT_EQ(CoverageSegment(6, 6, 4, true), Segments[5]);
>   EXPECT_EQ(CoverageSegment(7, 7, 1, false), Segments[6]);
>   EXPECT_EQ(CoverageSegment(8, 8, false), Segments[7]);
> ```
Thank you very much for the catch and the test case! The problem is that there may be a "gap" between the end of the last completed region (5:5), and the start of the new region (6:6). In this case, the count from the last active region (1:1 -> 8:8, count=1) should apply to 5:5 -> 6:6.

This scenario was handled correctly in the case where there's only one completed region.


================
Comment at: lib/ProfileData/Coverage/CoverageMapping.cpp:432
+        // as this one.
+      } else if (CurStartLoc == CR.value().endLoc()) {
+        // Avoid making this zero-length region active. If it's the last region,
----------------
ikudrin wrote:
> There can't be another region after a zero-length region with the same StartLoc, right?
> So, if you check this condition first, it'll simplify the code because there will be no need for 'else' branches and an empty 'then' block.
I think there can. If I'm understanding you correctly, this should be handled by the "handle_consecutive_regions_with_zero_length" unit test.


https://reviews.llvm.org/D36813