[PATCH] Initial code coverage mapping data structures, and reader and writers + C interface for ProfileData library
Alex L
arphaman at gmail.com
Fri Jun 27 14:34:22 PDT 2014
Thanks, I will fix those issues for the next patch.
2014-06-27 13:20 GMT-07:00 Bob Wilson <bob.wilson at apple.com>:
>
> > 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
>
It should fit into 80 columns, I ran clang-format on it.
> 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.
>
The expression kinds are distinguished in the counter because it was
simpler to serialize the counters this way. Originally I didn't have this
distinction, but I added it later to deal with the following problem:
The reader reads the expressions before mapping regions (that's where the
counters are), but the expression kinds aren't written out together with
expressions - they are written out with counters inside the tag of the
counter. Thus, when the reader creates the expressions, it doesn't know
their kind (This is also the reason why an expression has the unknown kind).
I think I will be able to remove this distinction and have a single
Expression CounterKind. I will have to change the reader so that it will
set the kinds of expressions after reading both the expressions and the
mapping regions.
I will then be able to remove the unknown kind from Expression.
> > + 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?
>
I have to rewrite this commend. When an expression is not found, a new one
is created.
>
> > + 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?
>
Yeah, llvm-cov seems more appropriate for it.
> +
> > +/// 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.
>
Okay. However, it's used in the implementation of the C api in this library.
>
> > +
> > +} // end namespace llvm
> > +
> > +#endif // LLVM_PROFILEDATA_COVERAGEMAPPING_H_
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140627/6f618829/attachment.html>
More information about the llvm-commits
mailing list