[llvm] c193bb7 - [Coverage] getMaxBitmapSize: Scan `max(BitmapIdx)` instead of the last `Decision` (#78963)

[via llvm-commits]

Author: NAKAMURA Takumi
Date: 2024-01-23T17:59:44+09:00
New Revision: c193bb7e9eee02a435944506c732db9b480f0c84

URL: https://github.com/llvm/llvm-project/commit/c193bb7e9eee02a435944506c732db9b480f0c84
DIFF: https://github.com/llvm/llvm-project/commit/c193bb7e9eee02a435944506c732db9b480f0c84.diff

LOG: [Coverage] getMaxBitmapSize: Scan `max(BitmapIdx)` instead of the last `Decision` (#78963)

In `CoverageMapping.cpp:getMaxBitmapSize()`, 
this assumed that the last `Decision` has the maxmum `BitmapIdx`.

Let it scan `max(BitmapIdx)`.

Note that `<=` is used insted of `<`, because `BitmapIdx == 0` is valid
and `MaxBitmapID` is `unsigned`. `BitmapIdx` is unique in the record.

Fixes #78922

Added: 
    llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.c
    llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.o
    llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.proftext
    llvm/test/tools/llvm-cov/mcdc-maxbs.test

Modified: 
    llvm/lib/ProfileData/Coverage/CoverageMapping.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
index 9c95cd5d76d215..da8e1d87319dde 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -229,6 +229,7 @@ Expected<BitVector> CounterMappingContext::evaluateBitmap(
   unsigned SizeInBits = llvm::alignTo(uint64_t(1) << NC, CHAR_BIT);
   unsigned SizeInBytes = SizeInBits / CHAR_BIT;
 
+  assert(ID + SizeInBytes <= BitmapBytes.size() && "BitmapBytes overrun");
   ArrayRef<uint8_t> Bytes(&BitmapBytes[ID], SizeInBytes);
 
   // Mask each bitmap byte into the BitVector. Go in reverse so that the
@@ -568,14 +569,14 @@ static unsigned getMaxBitmapSize(const CounterMappingContext &Ctx,
                                  const CoverageMappingRecord &Record) {
   unsigned MaxBitmapID = 0;
   unsigned NumConditions = 0;
-  // The last DecisionRegion has the highest bitmap byte index used in the
-  // function, which when combined with its number of conditions, yields the
-  // full bitmap size.
+  // Scan max(BitmapIdx).
+  // Note that `<=` is used insted of `<`, because `BitmapIdx == 0` is valid
+  // and `MaxBitmapID is `unsigned`. `BitmapIdx` is unique in the record.
   for (const auto &Region : reverse(Record.MappingRegions)) {
-    if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion) {
+    if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion &&
+        MaxBitmapID <= Region.MCDCParams.BitmapIdx) {
       MaxBitmapID = Region.MCDCParams.BitmapIdx;
       NumConditions = Region.MCDCParams.NumConditions;
-      break;
     }
   }
   unsigned SizeInBits = llvm::alignTo(uint64_t(1) << NumConditions, CHAR_BIT);

diff  --git a/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.c b/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.c
new file mode 100644
index 00000000000000..83644b29549455
--- /dev/null
+++ b/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.c
@@ -0,0 +1,13 @@
+#define RANGE(a,b,c) ((a) <= (b) && (b) <= (c))
+
+int sub(int c) {
+  if (RANGE('0', c, '9')) return 1;
+  return (('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z'));
+}
+
+extern void __llvm_profile_write_file(void);
+
+int main(int c, char **v)
+{
+  return (c > 1 ? sub(c) : 0);
+}

diff  --git a/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.o b/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.o
new file mode 100644
index 00000000000000..269902224ccae9
Binary files /dev/null and b/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.o 
diff er

diff  --git a/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.proftext b/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.proftext
new file mode 100644
index 00000000000000..f24e984f657f2b
--- /dev/null
+++ b/llvm/test/tools/llvm-cov/Inputs/mcdc-maxbs.proftext
@@ -0,0 +1,9 @@
+main
+# Func Hash:
+99164
+# Num Counters:
+2
+# Counter Values:
+1
+0
+

diff  --git a/llvm/test/tools/llvm-cov/mcdc-maxbs.test b/llvm/test/tools/llvm-cov/mcdc-maxbs.test
new file mode 100644
index 00000000000000..ad5bc828529cee
--- /dev/null
+++ b/llvm/test/tools/llvm-cov/mcdc-maxbs.test
@@ -0,0 +1,46 @@
+# Test not to trigger the assertion failure in llvm-cov with empty bitmap.
+# REQUIRES: asserts
+
+# mcdc-maxbs.o contains the record:
+#
+# sub:
+#   Decision,File 0, 5:11 -> 5:59 = M:1, C:4
+#   Branch,File 0, 5:12 -> 5:20 = #5, ((#0 - #1) - #5) [1,3,2]
+#   Branch,File 0, 5:24 -> 5:32 = #6, (#5 - #6) [3,0,2]
+#   Branch,File 0, 5:38 -> 5:46 = #7, (#4 - #7) [2,4,0]
+#   Branch,File 0, 5:50 -> 5:58 = #8, (#7 - #8) [4,0,0]
+#   Decision,File 1, 1:23 -> 1:47 = M:0, C:2
+#   Branch,File 1, 1:23 -> 1:33 = #2, (#0 - #2) [1,2,0]
+#   Branch,File 1, 1:37 -> 1:47 = #3, (#2 - #3) [2,0,0]
+#
+# With the previous implementation, `Decision M:0 C:2` was picked up as
+# the last `Decision`.
+# The size of the bitmap was reported as `1` in this case.
+# Actually, the largest `Decision` is `M:1 C:4` and the size should be `3`
+# (Idx=1, len=16 bits).
+#
+# When a corresponding bitmap was not emitted, the dummy zero-ed bitmap is
+# allocated as `BitmapBytes` with `getMaxBitmapSize(Record) + 1`
+# (in this case, `1 + 1`, less than `3`).
+# It may overrun and would trigger `unexpected test vector`
+# by trailing uninitialized garbage.
+
+# RUN: llvm-profdata merge %S/Inputs/mcdc-maxbs.proftext -o %t.profdata
+# RUN: llvm-cov report --show-mcdc-summary %S/Inputs/mcdc-maxbs.o -instr-profile %t.profdata
+
+# Instructions for regenerating the test object:
+
+cd %S/Inputs # or copy %S/Inputs/mcdc-maxbs.c into the working directory
+clang -O3 -fcoverage-mcdc -fprofile-instr-generate \
+    -fcoverage-mapping -fcoverage-compilation-dir=. \
+    mcdc-maxbs.c -c -o mcdc-maxbs.o
+
+# Instructions for regenerating the test vector:
+
+clang -fprofile-instr-generate mcdc-maxbs.o
+
+# Doesn't crash if argc > 1
+./a.out
+
+llvm-profdata merge --sparse -o default.profdata default.profraw
+llvm-profdata merge --text -o mcdc-maxbs.proftext default.profdata