[PATCH] D16212: Fix reading gcov data that does not have function names

[Justin Bogner via llvm-commits]

Arseny Kapoulkine <arseny.kapoulkine at gmail.com> writes:
> arseny.kapoulkine created this revision.
> arseny.kapoulkine added a reviewer: bogner.
> arseny.kapoulkine added a subscriber: llvm-commits.
>
> In order for recent gcov versions to read the coverage data, you have
> to use UseCfgChecksum=true and FunctionNamesInData=false options for
> coverage profiling pass. This is because gcov is expecting the
> function section in .gcda to be exactly 3 words in size, containing
> ident and two checksums.
>
> While llvm-cov is compatible with UseCfgChecksum=true, it always
> expects a function name in .gcda function sections (it's not
> compatible with FunctionNamesInData=false). Thus it's currently
> impossible to generate one set of coverage files that works with both
> gcov and llvm-cov.
>
> This change fixes the reading of coverage information to only read the
> function name if it's present.

Thanks for looking at this. This change needs tests - take a look at the
gcov tests in tests/tools/llvm-cov to see how these are done.

I've made a couple of comments on the patch itself below.

> http://reviews.llvm.org/D16212
>
> Files:
>   lib/IR/GCOV.cpp
>
>
> Index: lib/IR/GCOV.cpp
> ===================================================================
> --- lib/IR/GCOV.cpp
> +++ lib/IR/GCOV.cpp
> @@ -247,10 +247,22 @@
>  /// readGCDA - Read a function from the GCDA buffer. Return false if an error
>  /// occurs.
>  bool GCOVFunction::readGCDA(GCOVBuffer &Buff, GCOV::GCOVVersion Version) {
> -  uint32_t Dummy;
> -  if (!Buff.readInt(Dummy))
> +  uint32_t HeaderLength;
> +  if (!Buff.readInt(HeaderLength))
>      return false; // Function header length
>  
> +  uint32_t MinHeaderLength = 2;
> +
> +  if (Version != GCOV::V402) {
> +    MinHeaderLength++; // CfgChecksum
> +  }

I'm not a big fan of duplicating this condition here - it'd be better to
handle this near where we already read the CfgChecksum so that it's
harder to introduce a bug by causing the condition checks to diverge.

Maybe this would read better if instead of "MinHeaderLength" we had a
"HeaderWordsRead" that tracked how many words we've read so far?

> +
> +  if (HeaderLength < MinHeaderLength) {
> +    errs() << "Function header is invalid: expected " << MinHeaderLength
> +           << " words, got " << HeaderLength << " (in " << Name << ").\n";
> +    return false;
> +  }
> +
>    uint32_t GCDAIdent;
>    if (!Buff.readInt(GCDAIdent))
>      return false;
> @@ -280,13 +292,15 @@
>      }
>    }
>  
> -  StringRef GCDAName;
> -  if (!Buff.readString(GCDAName))
> -    return false;
> -  if (Name != GCDAName) {
> -    errs() << "Function names do not match: " << Name << " != " << GCDAName
> -           << ".\n";
> -    return false;
> +  if (MinHeaderLength < HeaderLength) {

If we do that, this condition is much easier to understand - it's "have
we read all of the words in the header?" rather than "Is the header
longer than the minimum?", which is a more pertinent question.

> +    StringRef GCDAName;
> +    if (!Buff.readString(GCDAName))
> +      return false;
> +    if (Name != GCDAName) {
> +      errs() << "Function names do not match: " << Name << " != " << GCDAName
> +             << ".\n";
> +      return false;
> +    }
>    }
>  
>    if (!Buff.readArcTag()) {
>
>