[PATCH] Introduce the InstrProfReader interface and a text reader

[Justin Bogner]

"Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
>> This patch introduces the ProfileData library without going into details
>> about the data format that instrumentation based profiling should use,
>> instead just sticking to the naive text format we have now.
>> 
>> This is designed to make readers for multiple formats simple, so that a
>> "raw" format for compiler-rt to commit and an efficient format for clang
>> to consume is easy.
>> 
>> Eric, would you mind checking the build system changes? It builds with
>> both clang and autoconf for me, but I'd appreciate a look over by
>> someone who has a clue how our build works.
>> 
>> <TextInstrProfReader.patch>
>
> LGTM, with a couple of fixups.
>
> But I don’t understand our build system either, so you should wait for
> someone to check that over.

Thanks for looking. New patch attached.

>> +class InstrProfReader {
>> +  error_code LastError;
>> +public:
>> +  InstrProfReader() : LastError(instrprof_error::success) {}
>> +  virtual ~InstrProfReader() {}
>> +
>> +  /// Read a single record.
>> +  virtual error_code readNextRecord(InstrProfRecord &Record) = 0;
>> +  /// Iterator over profile data.
>> +  virtual InstrProfIterator begin() { return InstrProfIterator(this); }
>> +  virtual InstrProfIterator end() { return InstrProfIterator(); }
>
> Do these need to be virtual?

Nope!

>> +
>> +    for (size_t II = 0, EE = I1->Counts.size(); II < EE; ++II) {
>
> Ranged-based for here?
>
>> +      uint64_t Sum = I1->Counts[II] + I2->Counts[II];
>> +      if (Sum < I1->Counts[II])
>> +        exitWithError("Counter overflow for " + I1->Name);
>> +      Output << Sum << "\n";
>>      }
>> +    Output << "\n";

This iterates over two vectors, so not unless we have a zip() adapter.

>> +  if (Reader1->hasError())
>> +    exitWithError(Reader1->getError().message(), Filename1);
>> +  if (Reader2->hasError())
>> +    exitWithError(Reader2->getError().message(), Filename1);
>
> Should this be Filename2?

Yes.

>> +  if (!Reader1->isEOF() || !Reader2->isEOF())
>> +    exitWithError("Number of instrumented functions differ.”);
>
> If we’re assuming for now that the first file is canonical, I think
> you should give Filename2 as the source of the error.

I prefer it this way. The errors in the loop have no filename, and this
is the same type of problem.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: TextInstrProfReader-2.patch
Type: text/x-patch
Size: 23246 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140319/b54197ef/attachment.bin>