[PATCH] Initial code coverage mapping data structures, and reader and writers + C interface for ProfileData library

[Bob Wilson]

> On Jun 25, 2014, at 4:32 PM, Alex L <arphaman at gmail.com> wrote:
> 
> Hi everyone,
> This is a first patch that implements the data structures, readers and writers used by the new code coverage system. I added the new code to the ProfileData library. I also added a very minimal C api for the ProfileData library.

Let’s skip the C API for now. We need to be especially careful about that because it’s hard to change later, and I don’t think it’s necessary to have it nailed down now.

Here are some comments on the first header file. I’ll send feedback on the other parts soon.

> diff --git a/include/llvm/ProfileData/CoverageMapping.h b/include/llvm/ProfileData/CoverageMapping.h
> new file mode 100644
> index 0000000..414b04a
> --- /dev/null
> +++ b/include/llvm/ProfileData/CoverageMapping.h
> @@ -0,0 +1,225 @@
> +//=-- CoverageMapping.h - Code coverage mapping support ---------*- C++ -*-=//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// Code coverage mapping data is genererated by clang and read by

Typo: “genererated”

> +// llvm-cov to show code coverage statistics for a file.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#ifndef LLVM_PROFILEDATA_COVERAGEMAPPING_H_
> +#define LLVM_PROFILEDATA_COVERAGEMAPPING_H_
> +
> +#include "llvm/ADT/ArrayRef.h"
> +#include "llvm/Support/raw_ostream.h"
> +#include <system_error>
> +
> +namespace llvm {
> +
> +struct CounterExpressions;
> +
> +/// A Counter is an abstract value that will allow
> +/// the user to compute the number of times that a particular region
> +/// of code has been visited when the user has access to
> +/// the collected profile counters.

This should be a “\brief” doxygen comment, shouldn’t it? Please reflow it for 80 columns.

Your description seems unnecessarily complicated, too. How about just: “A Counter is an abstract value that describes how to compute the execution count for a region of code using the collected profile count data."

> +struct Counter {
> +  enum CounterKind {
> +    Zero,
> +    CounterValueReference,
> +    AdditionExpression,
> +    SubtractionExpression
> +  };

Can you explain why this enum distinguishes addition and subtraction expressions? In either case, the expression ID will refer to a CounterExpression, and that CounterExpression also has an enum to distinguish adds and subtracts. This seems redundant.

> +  static const unsigned EncodingTagBits = 2;
> +  static const unsigned EncodingTagMask = 0x3;
> +
> +private:
> +  CounterKind Kind;
> +  unsigned ID;
> +
> +  Counter(CounterKind Kind, unsigned ID) : Kind(Kind), ID(ID) {}
> +
> +public:
> +  Counter() : Kind(Zero), ID(0) {}
> +
> +  CounterKind getKind() const { return Kind; }
> +
> +  bool isZero() const { return Kind == Zero; }
> +
> +  bool isExpression() const {
> +    return Kind == AdditionExpression || Kind == SubtractionExpression;
> +  }
> +
> +  unsigned getCounterID() const { return ID; }
> +
> +  unsigned getExpressionID() const { return ID; }
> +
> +  /// Encodes the counter into a raw integer value
> +  unsigned Encode() const;
> +
> +  /// Decodes the counter from a raw integer value
> +  bool Decode(unsigned Value);

What’s the return value here? Success or failure? Please explain in the comment.

> +
> +  bool operator==(const Counter &Other) const {
> +    return Kind == Other.Kind && ID == Other.ID;
> +  }
> +
> +  /// Return the counter that represents the
> +  /// number zero.

This comment can fit on one line. Also, the coding standard document says this should have a doxygen “\brief” tag. A bunch of the other comments in this patch have the same issues, so I’m not going to call them all out.

> +  static Counter getZero() { return Counter(); }
> +
> +  /// Return the counter that corresponds to a
> +  /// specific profile counter.
> +  static Counter getCounter(unsigned CounterId) {
> +    return Counter(CounterValueReference, CounterId);
> +  }
> +
> +  /// Return the counter that corresponds to a
> +  /// specific addition counter expression.
> +  static Counter getExpression(CounterKind Kind, unsigned ExpressionId) {
> +    return Counter(Kind, ExpressionId);
> +  }
> +};
> +
> +/// A Counter expression is a value that
> +/// represent an arithmetic operation with two counters.

“represents”

> +struct CounterExpression {
> +  enum ExprKind { Unknown, Subtract, Add };

Why do you need an “Unknown” kind here?

> +  ExprKind Kind;
> +  Counter LHS, RHS;
> +
> +  CounterExpression(ExprKind Kind, Counter LHS, Counter RHS)
> +      : Kind(Kind), LHS(LHS), RHS(RHS) {}
> +
> +  bool operator==(const CounterExpression &Other) const {
> +    return Kind == Other.Kind && LHS == Other.LHS && RHS == Other.RHS;
> +  }
> +};
> +
> +/// A Counter expression builder is used to construct the
> +/// counter expression. It avoids unecessary duplication
> +/// and simplifies algebraic expression to minimize the

“…expressions” (plural).  I would just end the comment there, dropping the “….to minimize” phrase. Simplification is obviously good and you don’t need to justify it further.

> +/// number of necessary expressions and mapping regions.
> +class CounterExpressionBuilder {
> +  /// A list of all the counter expressions
> +  llvm::SmallVector<CounterExpression, 16> Expressions;
> +  /// An array of terms used in expression simplification.
> +  llvm::SmallVector<int, 16> Terms;
> +
> +  /// Return the counter which
> +  /// corresponds to the given expression. Use a linear
> +  /// search to try to find the given expression.

What if it is not found? Or is it guaranteed that you’ll always find one?

> +  Counter get(Counter::CounterKind Kind, const CounterExpression &E);
> +
> +  /// Convert the expression tree into a polynomial
> +  /// in the form of K1Counter1 + .. + KNCounterN
> +  /// where K is an integer constant that is stored
> +  /// in the Terms array.

Can you clarify this to “Convert the expression tree represented by a Counter….”? The “where” part should be something like: “…where K1..KN are integer constants…”

> +  void ExtractTerms(Counter C, int Sign = 1);
> +
> +  /// Simplies the given expression tree

“Simplifies”

> +  /// by getting rid of algebraicly redundant

“algebraically”

> +  /// operations.
> +  Counter Simplify(Counter ExpressionTree);
> +
> +public:
> +  CounterExpressionBuilder(unsigned NumCounters);
> +
> +  ArrayRef<CounterExpression> getExpressions() const { return Expressions; }
> +
> +  /// Return a counter that represents the
> +  /// the exression that adds lhs and rhs.

“expression”
You’ve also got two “the”s in a row, and I suggest capitalizing LHS and RHS to match the argument names.

> +  Counter Add(Counter LHS, Counter RHS);
> +
> +  /// Return a counter that represents the
> +  /// expression that subracts rhs from lhs.

“subtracts”, and please capitalize RHS and LHS.

> +  Counter Subtract(Counter LHS, Counter RHS);
> +};
> +
> +/// A Counter mapping region associates a source range with
> +/// a specific counter.
> +struct CounterMappingRegion {
> +  enum RegionKind {
> +    /// A CounterRegion associates some code with a counter

“CodeRegion”?

> +    CodeRegion,
> +
> +    /// An ExpansionRegion represents
> +    /// a file expansion region that associates
> +    /// a source range with the expansion of a
> +    /// virtual source file.
> +    /// Can be used to mark the usages of macroes
> +    /// and #included files in coverage for the C sources.

That last sentence seems awkward. How about combining it with the previous one, e.g.: “…a virtual source file, such as for a macro instantiation or #include file."

> +    ExpansionRegion,
> +
> +    /// An EmptyRegion represents a source range without
> +    /// code, but with a distinct counter.
> +    EmptyRegion,
> +
> +    /// A SkippedRegion represents a source range with
> +    /// code that was skipped by a preprocessor
> +    /// or similar means.
> +    SkippedRegion
> +  };
> +
> +  Counter Count;
> +  unsigned FileID, ExpandedFileID;
> +  unsigned LineStart, ColumnStart, LineEnd, ColumnEnd;
> +  RegionKind Kind;
> +
> +  CounterMappingRegion(Counter Count, unsigned FileID, unsigned LineStart,
> +                       unsigned ColumnStart, unsigned LineEnd,
> +                       unsigned ColumnEnd, RegionKind Kind = CodeRegion)
> +      : Count(Count), FileID(FileID), ExpandedFileID(0), LineStart(LineStart),
> +        ColumnStart(ColumnStart), LineEnd(LineEnd), ColumnEnd(ColumnEnd),
> +        Kind(Kind) {}
> +
> +  bool operator<(const CounterMappingRegion &Other) const {
> +    if (FileID != Other.FileID)
> +      return FileID < Other.FileID;
> +    if (LineStart == Other.LineStart)
> +      return ColumnStart < Other.ColumnStart;
> +    return LineStart < Other.LineStart;
> +  }
> +};
> +
> +/// A MappingRegion associates a source range
> +/// with an execution count.
> +struct MappingRegion : public CounterMappingRegion {
> +  uint64_t ExecutionCount;
> +
> +  MappingRegion(const CounterMappingRegion &R, uint64_t ExecutionCount)
> +      : CounterMappingRegion(R), ExecutionCount(ExecutionCount) {}
> +};

This MappingRegion struct doesn’t seem to be used for anything here, and it seems to me like it probably ought to go in llvm-cov, if anywhere. Can you take it out for now and we’ll revisit it later?

> +
> +/// A Counter mapping context is used
> +/// to connect the counters, expressions and the
> +/// obtained counter values.
> +class CounterMappingContext {
> +  ArrayRef<CounterExpression> Expressions;
> +  ArrayRef<uint64_t> CounterValues;
> +
> +public:
> +  CounterMappingContext(ArrayRef<CounterExpression> Expressions,
> +                        ArrayRef<uint64_t> CounterValues = ArrayRef<uint64_t>())
> +      : Expressions(Expressions), CounterValues(CounterValues) {}
> +
> +  void dump(const Counter &C, llvm::raw_ostream &OS) const;
> +  void dump(const Counter &C) const { dump(C, llvm::outs()); }
> +
> +  /// Return the number of times that a
> +  /// region of code associated with this
> +  /// counter was executed.
> +  int64_t evaluate(const Counter &C, std::error_code *Error) const;
> +  int64_t evaluate(const Counter &C, std::error_code &Error) const {
> +    Error.clear();
> +    return evaluate(C, &Error);
> +  }
> +};

Ditto for the CounterMappingContext. Let’s come back to that later.

> +
> +} // end namespace llvm
> +
> +#endif // LLVM_PROFILEDATA_COVERAGEMAPPING_H_