[PATCH] Coverage: add HasCodeBefore flag to a mapping region

[Bob Wilson]

This is good to commit, but see my suggested improvement below. I assume it will be tested once we get the llvm-cov support enabled?

> On Jul 31, 2014, at 2:30 PM, Alex Lorenz <arphaman at gmail.com> wrote:
> 
> Hi bogner, bob.wilson,
> 
> This patch adds a flag HasCodeBefore to a coverage mapping region.
> 
> This flag will be used by the coverage tool to compute the execution counts for each line in a source file
> so that the lines' execution counts will show the execution count of a first piece of code on that line
> instead of the execution count of the last mapping region on that line.
> 
> http://reviews.llvm.org/D4746
> 
> Files:
>  include/llvm/ProfileData/CoverageMapping.h
>  lib/ProfileData/CoverageMappingReader.cpp
>  lib/ProfileData/CoverageMappingWriter.cpp
> 
> Index: include/llvm/ProfileData/CoverageMapping.h
> ===================================================================
> --- include/llvm/ProfileData/CoverageMapping.h
> +++ include/llvm/ProfileData/CoverageMapping.h
> @@ -148,13 +148,18 @@
>   unsigned FileID, ExpandedFileID;
>   unsigned LineStart, ColumnStart, LineEnd, ColumnEnd;
>   RegionKind Kind;
> +  /// \brief A flag that is set to true when there is already code before
> +  /// this region on the same line.
> +  /// This is useful to accurately compute the execution counts for a line.
> +  bool HasCodeBefore;
> 
>   CounterMappingRegion(Counter Count, unsigned FileID, unsigned LineStart,
>                        unsigned ColumnStart, unsigned LineEnd,
> -                       unsigned ColumnEnd, RegionKind Kind = CodeRegion)
> +                       unsigned ColumnEnd, bool HasCodeBefore = false,
> +                       RegionKind Kind = CodeRegion)
>       : Count(Count), FileID(FileID), ExpandedFileID(0), LineStart(LineStart),
>         ColumnStart(ColumnStart), LineEnd(LineEnd), ColumnEnd(ColumnEnd),
> -        Kind(Kind) {}
> +        Kind(Kind), HasCodeBefore(HasCodeBefore) {}
> 
>   bool operator<(const CounterMappingRegion &Other) const {
>     if (FileID != Other.FileID)
> Index: lib/ProfileData/CoverageMappingReader.cpp
> ===================================================================
> --- lib/ProfileData/CoverageMappingReader.cpp
> +++ lib/ProfileData/CoverageMappingReader.cpp
> @@ -170,13 +170,16 @@
>     }
> 
>     // Read the source range.
> -    uint64_t LineStartDelta, ColumnStart, NumLines, ColumnEnd;
> +    uint64_t LineStartDelta, CodeBeforeColumnStart, NumLines, ColumnEnd;
>     if (auto Err =
>             readIntMax(LineStartDelta, std::numeric_limits<unsigned>::max()))
>       return Err;
> -    if (auto Err =
> -            readIntMax(ColumnStart, std::numeric_limits<unsigned>::max()))
> +    if (auto Err = readULEB128(CodeBeforeColumnStart))
>       return Err;
> +    bool HasCodeBefore = CodeBeforeColumnStart & 1;
> +    uint64_t ColumnStart = CodeBeforeColumnStart >> 1;

It would be nice if you can define a symbolic constant for this shift amount (and use it to compute the mask for HasCodeBefore). It’s also used in the writer code. It’s not a big deal, but we generally try to avoid magic constants like this.

> +    if (ColumnStart > std::numeric_limits<unsigned>::max())
> +      return error(instrprof_error::malformed);
>     if (auto Err = readIntMax(NumLines, std::numeric_limits<unsigned>::max()))
>       return Err;
>     if (auto Err = readIntMax(ColumnEnd, std::numeric_limits<unsigned>::max()))
> @@ -194,9 +197,9 @@
>       ColumnStart = 1;
>       ColumnEnd = std::numeric_limits<unsigned>::max();
>     }
> -    MappingRegions.push_back(
> -        CounterMappingRegion(C, InferredFileID, LineStart, ColumnStart,
> -                             LineStart + NumLines, ColumnEnd, Kind));
> +    MappingRegions.push_back(CounterMappingRegion(
> +        C, InferredFileID, LineStart, ColumnStart, LineStart + NumLines,
> +        ColumnEnd, HasCodeBefore, Kind));
>     MappingRegions.back().ExpandedFileID = ExpandedFileID;
>   }
>   return success();
> Index: lib/ProfileData/CoverageMappingWriter.cpp
> ===================================================================
> --- lib/ProfileData/CoverageMappingWriter.cpp
> +++ lib/ProfileData/CoverageMappingWriter.cpp
> @@ -181,7 +181,9 @@
>     }
>     assert(I.LineStart >= PrevLineStart);
>     encodeULEB128(I.LineStart - PrevLineStart, OS);
> -    encodeULEB128(I.ColumnStart, OS);
> +    uint64_t CodeBeforeColumnStart =
> +        uint64_t(I.HasCodeBefore) | (uint64_t(I.ColumnStart) << 1);
> +    encodeULEB128(CodeBeforeColumnStart, OS);
>     assert(I.LineEnd >= I.LineStart);
>     encodeULEB128(I.LineEnd - I.LineStart, OS);
>     encodeULEB128(I.ColumnEnd, OS);
> <D4746.12085.patch>