<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-17 10:17 GMT-07:00 Alex L <span dir="ltr"><<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Thanks!<br><div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-16 22:38 GMT-07:00 Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span>:<div>
<div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The documentation is going to need more work. Let’s split that out into a separate patch. At least it helps me understand how the reader and writer work.<br>
<div><br>
> On Jul 9, 2014, at 4:39 PM, Alex Lorenz <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:<br>
><br>
> The updated version of the patch that fixes various typos, adds format version to the coverage mapping format, changes the storage of filenames to once per tu, and adds document that describes the actual coverage mapping format.<br>
><br>
> <a href="http://reviews.llvm.org/D4301" target="_blank">http://reviews.llvm.org/D4301</a><br>
<br>
</div>> Index: include/llvm/ProfileData/CoverageMapping.h<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ include/llvm/ProfileData/CoverageMapping.h<br>
> @@ -0,0 +1,196 @@<br>
…<br>
> +/// \brief A Counter expression builder is used to construct the<br>
> +/// counter expressions. It avoids unecessary duplication<br>
> +/// and simplifies algebraic expression.<br>
> +class CounterExpressionBuilder {<br>
<br>
I had commented previously that “expression” should be plural in the comment here. I had not noticed that there were two of them. You changed the first but not the second.<br>
<br>
…<br>
<br>
> +/// \brief A Counter mapping region associates a source range with<br>
> +/// a specific counter.<br>
> +struct CounterMappingRegion {<br>
> + enum RegionKind {<br>
> + /// \brief A CodeRegion associates some code with a counter<br>
> + CodeRegion,<br>
> +<br>
> + /// \brief An ExpansionRegion represents<br>
> + /// a file expansion region that associates<br>
> + /// a source range with the expansion of a<br>
> + /// virtual source file, such as for a macro instantiation<br>
> + /// or #include file.<br>
> + ExpansionRegion,<br>
<br>
My previous review suggested that you reflow comments to 80 columns. For many patches that is about fixing 80 column violations, but in this patch there are a number of comments like this one with very short lines. I would prefer that you take better advantage of the space and fit those on fewer lines.<br>
<br>
…<br>
<br>
> Index: include/llvm/ProfileData/CoverageMappingReader.h<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ include/llvm/ProfileData/CoverageMappingReader.h<br>
<br>
…<br>
<br>
> +/// \brief Base class for the raw coverage mapping and filenames data readers.<br>
> +class RawCoverageReader {<br>
> +protected:<br>
> + StringRef Data;<br>
> +<br>
> + /// \brief Return the error code.<br>
> + std::error_code error(std::error_code EC) { return EC; }<br>
> +<br>
> + /// \brief Clear the current error code and return a successful one.<br>
> + std::error_code success() { return error(instrprof_error::success); }<br>
<br>
Unlike the error() and success() functions in InstrProfReader and ObjectFileCoverageMappingReader, these don’t keep track of the most recent error. They don’t really add much, do they? I don’t have a strong opinion about this, if you want to keep them.<br>
</blockquote><div><br></div></div></div><div>I'd prefer to keep them threre.<br></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Index: lib/ProfileData/CoverageMappingReader.cpp<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ lib/ProfileData/CoverageMappingReader.cpp<br>
…<br>
> +void CoverageMappingIterator::Increment() {<br>
> + if (Reader->readNextRecord(Record))<br>
> + *this = CoverageMappingIterator();<br>
> +}<br>
<br>
Please add a comment to explain what this conditional is checking, presumably the end of the records.<br>
<br>
…<br>
<br>
> +std::error_code RawCoverageReader::readIntMax(uint64_t &Result,<br>
> + uint64_t MaxPlus1) {<br>
> + auto Err = readULEB128(Result);<br>
> + if (Err)<br>
> + return Err;<br>
<br>
Better to limit the scope of the error code to the conditional by doing: “if (auto Err = …) return Err;”<br>
<br>
> + if (Result >= MaxPlus1)<br>
> + return error(instrprof_error::malformed);<br>
> + return success();<br>
> +}<br>
> +<br>
> +std::error_code RawCoverageReader::readSize(uint64_t &Result) {<br>
> + auto Err = readULEB128(Result);<br>
> + if (Err)<br>
> + return Err;<br>
<br>
Ditto.<br>
<br>
> + // Sanity check the number<br>
> + if (Result > Data.size())<br>
> + return error(instrprof_error::malformed);<br>
> + return success();<br>
> +}<br>
> +<br>
> +std::error_code RawCoverageReader::readString(StringRef &Result) {<br>
> + uint64_t Length;<br>
> + auto Err = readSize(Length);<br>
> + if (Err)<br>
> + return Err;<br>
<br>
…and again here.<br>
<br>
> + Result = Data.substr(0, Length);<br>
> + Data = Data.substr(Length);<br>
> + return success();<br>
> +}<br>
> +<br>
><br>
<br>
> +std::error_code RawCoverageFilenamesReader::read() {<br>
> + uint64_t IntValue;<br>
<br>
“IntValue” is a terrible name in this context. How about “NumFilenames”?<br>
<br>
> + auto Err = readSize(IntValue);<br>
> + if (Err)<br>
> + return Err;<br>
> + for (size_t I = 0, S = IntValue; I < S; ++I) {<br>
<br>
“S”?? Why not just use “I < NumFilenames” (or whatever you call it)?<br>
<br>
> + StringRef Filename;<br>
> + auto Err = readString(Filename);<br>
> + if (Err)<br>
> + return Err;<br>
> + Filenames.push_back(Filename);<br>
> + }<br>
> + return success();<br>
> +}<br>
> +<br>
> +std::error_code RawCoverageMappingReader::readCounter(Counter &C) {<br>
> + uint64_t IntValue;<br>
> + auto Err = readIntMax(IntValue, std::numeric_limits<unsigned>::max());<br>
> + if (Err)<br>
> + return Err;<br>
> + Err = decodeCounter(IntValue, C);<br>
> + if (Err)<br>
> + return Err;<br>
<br>
I would define two separate instances of “Err” inside the conditionals here (as above). There are a bunch more cases of this, so I’ll leave it to you to go through and find the rest of them.<br>
<br>
…<br>
<br>
> + return success();<br>
> +}<br>
> +<br>
> +static unsigned const EncodingCounterTagAndExpansionRegionTagBits =<br>
> + Counter::EncodingTagBits + 1;<br>
> +<br>
<br>
> +std::error_code RawCoverageMappingReader::readMappingRegionsSubArray(<br>
> + std::vector<CounterMappingRegion> &MappingRegions, unsigned InferredFileID,<br>
> + size_t VFMsize) {<br>
<br>
Please add a comment before this function to explain its arguments. It isn’t at all obvious what “VFMsize” is (for example).<br>
<br>
> + uint64_t IntValue;<br>
<br>
See my comment above about the variable name. In this case, it should be something like “NumRegions”.<br>
<br>
> + auto Err = readSize(IntValue);<br>
> + if (Err)<br>
> + return Err;<br>
> + unsigned PrevLineStart = 0;<br>
> + for (size_t I = 0, S = IntValue; I < S; ++I) {<br>
<br>
As above: I don’t see any point in introducing a new “S” here.<br>
<br>
> + Counter C;<br>
> + CounterMappingRegion::RegionKind Kind = CounterMappingRegion::CodeRegion;<br>
> +<br>
> + // Read the combined counter + region kind<br>
> + uint64_t IntValue;<br>
<br>
This shadows the “IntValue” in the outer scope, which is bad. You should definitely rename the outer one, and then maybe it is OK to keep this one with the generic name. If you can come up with a more descriptive name for this instance, that would be even better.<br>
<br>
> + auto Err = readIntMax(IntValue, std::numeric_limits<unsigned>::max());<br>
> + if (Err)<br>
> + return Err;<br>
<br>
> + unsigned Tag = IntValue & Counter::EncodingTagMask;<br>
> + uint64_t ExpandedFileID = 0;<br>
> + if (Tag != Counter::Zero) {<br>
> + if (auto Err = decodeCounter(IntValue, C))<br>
> + return Err;<br>
> + } else {<br>
> + // Is it an expansion region?<br>
> + if ((IntValue >> Counter::EncodingTagBits) & 1) {<br>
<br>
Would it make sense to define a constant instead of using that literal “1”? If so, please do.<br>
<br>
> + Kind = CounterMappingRegion::ExpansionRegion;<br>
> + ExpandedFileID =<br>
> + IntValue >> EncodingCounterTagAndExpansionRegionTagBits;<br>
> + if (ExpandedFileID >= VFMsize)<br>
> + return error(instrprof_error::malformed);<br>
> + } else {<br>
> + switch (IntValue >> EncodingCounterTagAndExpansionRegionTagBits) {<br>
> + case CounterMappingRegion::CodeRegion:<br>
> + // Don't do anything when we have a code region with a zero counter.<br>
> + break;<br>
> + case CounterMappingRegion::EmptyRegion:<br>
> + Kind = CounterMappingRegion::EmptyRegion;<br>
> + break;<br>
> + case CounterMappingRegion::SkippedRegion:<br>
> + Kind = CounterMappingRegion::SkippedRegion;<br>
> + break;<br>
> + default:<br>
> + return error(instrprof_error::malformed);<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + // Read the source range<br>
> + uint64_t LocationInfo[4];<br>
<br>
I think it would be better to define each of these as a scalar variable with a meaningful name, e.g., “DeltaLineStart”. Using an array makes it easier to read the values, but with only 4 of them, it’s really not so bad to just call readIntMax 4 separate times.<br>
<br>
> + for (unsigned J = 0; J < 4; ++J) {<br>
> + Err = readIntMax(LocationInfo[J], std::numeric_limits<unsigned>::max());<br>
> + if (Err)<br>
> + return Err;<br>
<br>
> + }<br>
> + PrevLineStart += LocationInfo[0];<br>
> + // Adjust the column locations for the empty regions that<br>
> + // are supposed to cover whole lines.<br>
<br>
The comment is a good start, but I still don’t quite get it. Part of the problem is that there’s nothing here to indicate what the values in LocationInfo[1] and LocationInfo[3] represent. You shouldn’t have to go to the format document to see that those are “columnStart” and “columnEnd”. Using separate variables with meaningful names will help, but that’s still not good enough. What are these empty regions from? (Maybe an example would help.) Why do you set the column start/end to 1 and uint_max?<br>
</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + if (LocationInfo[1] == 0 && LocationInfo[3] == 0) {<br>
> + LocationInfo[1] = 1;<br>
<br>
Why 1 instead of 0 if this region is supposed to cover a whole line?<br></blockquote><div><br></div></div></div><div>That's the usual convention - the column value for the first character in a line is 1 (Similar to how the first line is 1, not 0).<br>
<br></div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + LocationInfo[3] = std::numeric_limits<unsigned>::max();<br>
> + }<br>
> + MappingRegions.push_back(CounterMappingRegion(<br>
> + C, InferredFileID, PrevLineStart, LocationInfo[1],<br>
> + PrevLineStart + LocationInfo[2], LocationInfo[3], Kind));<br>
> + MappingRegions.back().ExpandedFileID = ExpandedFileID;<br>
> + }<br>
> + return success();<br>
> +}<br>
> +<br>
> +std::error_code RawCoverageMappingReader::read(CoverageMappingRecord &Record) {<br>
> + uint64_t IntValue;<br>
> +<br>
> + // Read the virtual file mapping<br>
> + llvm::SmallVector<unsigned, 8> VirtualFileMapping;<br>
> + auto Err = readSize(IntValue);<br>
> + if (Err)<br>
> + return Err;<br>
> + for (size_t I = 0, S = IntValue; I < S; ++I) {<br>
<br>
As above, please use a better name than “IntValue” and there’s no need for a separate “S” variable.<br>
<br>
> + auto Err = readIntMax(IntValue, TranslationUnitFilenames.size());<br>
<br>
…and this instance of “IntValue” should be a separate “FileIndex” (or similar) variable.<br>
<br>
> + if (Err)<br>
> + return Err;<br>
> + VirtualFileMapping.push_back(IntValue);<br>
> + }<br>
> +<br>
> + // Construct the files using unique filenames<br>
> + // and virtual file mapping<br>
> + for (auto I : VirtualFileMapping) {<br>
> + Filenames.push_back(TranslationUnitFilenames[I]);<br>
> + }<br>
> +<br>
> + // Read the expressions<br>
> + Err = readSize(IntValue);<br>
<br>
separate “NumExpressions” variable, please.<br>
<br>
> + if (Err)<br>
> + return Err;<br>
> + // Create an array of dummy expressions that get the proper counters<br>
> + // when the expressions are read, and the proper kinds when the counters<br>
> + // are decoded.<br>
> + Expressions.resize(IntValue, CounterExpression(CounterExpression::Subtract,<br>
> + Counter(), Counter()));<br>
> + for (size_t I = 0, S = IntValue; I < S; ++I) {<br>
<br>
No need for “S” here.<br>
<br>
> + Err = readCounter(Expressions[I].LHS);<br>
> + if (Err)<br>
> + return Err;<br>
> + Err = readCounter(Expressions[I].RHS);<br>
> + if (Err)<br>
> + return Err;<br>
> + }<br>
> +<br>
> + // Read the mapping regions sub-arrays<br>
> + for (unsigned InferredFileID = 0, S = VirtualFileMapping.size();<br>
> + InferredFileID < S; ++InferredFileID) {<br>
> + Err = readMappingRegionsSubArray(MappingRegions, InferredFileID,<br>
> + VirtualFileMapping.size());<br>
> + if (Err)<br>
> + return Err;<br>
> + }<br>
> +<br>
> + // Set the counters for the expansion regions<br>
<br>
nitpick: There should be a period at the end of that sentence.<br>
<br>
> + // i.e. Counter of expansion region = counter of the first region<br>
> + // from the expanded file.<br>
> + // Perform multiple passes to correctly propagate the counters through<br>
> + // all the nested expansion regions.<br>
> + for (unsigned Pass = 1, S = VirtualFileMapping.size(); Pass < S; ++Pass) {<br>
> + for (auto &I : MappingRegions) {<br>
> + if (I.Kind == CounterMappingRegion::ExpansionRegion) {<br>
> + for (const auto &J : MappingRegions) {<br>
> + if (J.FileID == I.ExpandedFileID) {<br>
> + I.Count = J.Count;<br>
> + break;<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + Record.FunctionName = FunctionName;<br>
> + Record.Filenames = Filenames;<br>
> + Record.Expressions = Expressions;<br>
> + Record.MappingRegions = MappingRegions;<br>
> + return success();<br>
> +}<br>
<br>
…<br>
<br>
> +template <typename T><br>
> +std::error_code readCoverageMappingData(<br>
> + SectionRef &ProfileNames, SectionRef &CoverageMapping,<br>
> + SectionRef &CoverageData, SectionRef &CoverageFilenames,<br>
> + SectionRef &CoverageTranslationUnits,<br>
> + std::vector<ObjectFileCoverageMappingReader::ProfileMappingRecord> &Records,<br>
> + std::vector<StringRef> &Filenames) {<br>
> + llvm::DenseSet<T> UniqueFunctionMappingData;<br>
> +<br>
> + // Get the contents of the given sections<br>
> + StringRef Data;<br>
> + auto Err = CoverageTranslationUnits.getContents(Data);<br>
> + if (Err)<br>
> + return Err;<br>
> + SectionData CoverageMappingData, ProfileNamesData, CoverageFunctionsData,<br>
> + CoverageFilenamesData;<br>
> + Err = CoverageMappingData.load(CoverageMapping);<br>
> + if (Err)<br>
> + return Err;<br>
> + Err = CoverageFunctionsData.load(CoverageData);<br>
> + if (Err)<br>
> + return Err;<br>
<br>
The “CoverageData” name in this code confused me. (It’s all coverage data, right?) It seems like it would be more consistent and understandable to call this “CoverageFunctions”.<br>
<br>
I’m going to stop there for now. My main question at this point is whether we can make this work with fewer sections. It would be nice if we could merge the function records from the __llvm_cvm_funcs section into the per-function data in the __llvm_prf_cvmap section.<br>
<br>
</blockquote></div></div></div><br></div><div class="gmail_extra">Yes, I think that it's possible to merge the __llvm_prf_cvmap and the __llvm_cvm_funcs sections, if I create a structure for each function that contains both the pointer to the function's name and the actual mapping data. However, this would probably mean that every function will need a unique LLVM structure type - will IR handle that efficiently? The same principle could also be applied to the filenames and the translation unit sections.<br>
</div></div></div>
</blockquote></div><br></div><div class="gmail_extra">It would be even possible to merge all the 4 coverage sections into one, using the following format:<br><br></div><div class="gmail_extra"> { { Translation Unit Record }, [ { Function record } ... ], [ filenames + coverage mapping for all functions ] }<br>
<br></div><div class="gmail_extra">where:<br> Translation Unit Record {<br></div><div class="gmail_extra"> uint32 : LengthOfFilenamesAndCoverageMappings<br></div><div class="gmail_extra"> uint32 : Version<br> }<br>
<br></div><div class="gmail_extra"> FunctionRecord {<br></div><div class="gmail_extra"> Pointer : Function name<br></div><div class="gmail_extra"> uint32 : Name length<br></div><div class="gmail_extra"> uint32 : Coverage mapping length<br>
}<br></div><div class="gmail_extra"><br></div></div>