[PATCH] llvm-cov: Split GCOVFile's read into GCNO and GCDA.
Justin Bogner
mail at justinbogner.com
Tue Dec 3 16:06:43 PST 2013
Comments inline. Otherwise LGTM.
Yuchen Wu <yuchenericwu at hotmail.com> writes:
> From 8d1b952cb6fa62c27aa1f7be654fbc5cd5cb5213 Mon Sep 17 00:00:00 2001
> From: Yuchen Wu <yuchen_wu at apple.com>
> Date: Tue, 3 Dec 2013 14:18:52 -0800
> Subject: [PATCH 1/9] llvm-cov: Split GCOVFile's read into GCNO and GCDA.
>
> This splits the file-scope read() function into readGCNO() and
> readGCDA(). Also broke file format read into functions that first read
> the file type, then check the version.
> ---
> include/llvm/Support/GCOV.h | 69 +++++++++++++++++++----------
> lib/IR/GCOV.cpp | 104 ++++++++++++++++++++++----------------------
> tools/llvm-cov/llvm-cov.cpp | 4 +-
> 3 files changed, 99 insertions(+), 78 deletions(-)
>
> diff --git a/include/llvm/Support/GCOV.h b/include/llvm/Support/GCOV.h
> index 9aa4413..4f76c49 100644
> --- a/include/llvm/Support/GCOV.h
> +++ b/include/llvm/Support/GCOV.h
> @@ -28,12 +28,9 @@ class GCOVBlock;
> class FileInfo;
>
> namespace GCOV {
> - enum GCOVFormat {
> - InvalidGCOV,
> - GCNO_402,
> - GCNO_404,
> - GCDA_402,
> - GCDA_404
> + enum GCOVVersion {
> + V402,
> + V404
> };
> } // end GCOV namespace
>
> @@ -43,21 +40,43 @@ class GCOVBuffer {
> public:
> GCOVBuffer(MemoryBuffer *B) : Buffer(B), Cursor(0) {}
>
> - /// readGCOVFormat - Read GCOV signature at the beginning of buffer.
> - GCOV::GCOVFormat readGCOVFormat() {
> - StringRef Magic = Buffer->getBuffer().slice(0, 8);
> - Cursor = 8;
> - if (Magic == "oncg*404")
> - return GCOV::GCNO_404;
> - else if (Magic == "oncg*204")
> - return GCOV::GCNO_402;
> - else if (Magic == "adcg*404")
> - return GCOV::GCDA_404;
> - else if (Magic == "adcg*204")
> - return GCOV::GCDA_402;
> -
> - Cursor = 0;
> - return GCOV::InvalidGCOV;
> + /// readGCNOFormat - Read GCNO signature at the beginning of buffer.
> + bool readGCNOFormat() {
> + StringRef File = Buffer->getBuffer().slice(0, 4);
> + if (File != "oncg") {
> + errs() << "Unexpected file type: " << File << ".\n";
> + return false;
> + }
> + Cursor = 4;
> + return true;
> + }
> +
> + /// readGCDAFormat - Read GCDA signature at the beginning of buffer.
> + bool readGCDAFormat() {
> + StringRef File = Buffer->getBuffer().slice(0, 4);
> + if (File != "adcg") {
> + errs() << "Unexpected file type: " << File << ".\n";
> + return false;
> + }
> + Cursor = 4;
> + return true;
> + }
> +
> + /// readGCOVVersion - Read GCOV version.
> + bool readGCOVVersion(GCOV::GCOVVersion &Version) {
> + StringRef VersionStr = Buffer->getBuffer().slice(Cursor, Cursor+4);
> + Cursor += 4;
> + if (VersionStr == "*204") {
> + Cursor += 4;
> + Version = GCOV::V402;
> + return true;
> + } else if (VersionStr == "*404") {
> + Cursor += 4;
> + Version = GCOV::V404;
> + return true;
> + }
> + errs() << "Unexpected version: " << VersionStr << ".\n";
> + return false;
> }
>
> /// readFunctionTag - If cursor points to a function tag then increment the
> @@ -195,10 +214,12 @@ class GCOVFile {
> public:
> GCOVFile() : Checksum(0), Functions(), RunCount(0), ProgramCount(0) {}
> ~GCOVFile();
> - bool read(GCOVBuffer &Buffer);
> + bool readGCNO(GCOVBuffer &Buffer);
> + bool readGCDA(GCOVBuffer &Buffer);
> void dump() const;
> void collectLineCounts(FileInfo &FI);
> private:
> + GCOV::GCOVVersion Version;
> uint32_t Checksum;
> SmallVector<GCOVFunction *, 16> Functions;
> uint32_t RunCount;
> @@ -218,8 +239,8 @@ class GCOVFunction {
> public:
> GCOVFunction() : Ident(0), LineNumber(0) {}
> ~GCOVFunction();
> - bool readGCNO(GCOVBuffer &Buffer, GCOV::GCOVFormat Format);
> - bool readGCDA(GCOVBuffer &Buffer, GCOV::GCOVFormat Format);
> + bool readGCNO(GCOVBuffer &Buffer, GCOV::GCOVVersion Version);
> + bool readGCDA(GCOVBuffer &Buffer, GCOV::GCOVVersion Version);
> StringRef getFilename() const { return Filename; }
> void dump() const;
> void collectLineCounts(FileInfo &FI);
> diff --git a/lib/IR/GCOV.cpp b/lib/IR/GCOV.cpp
> index 8ce9367..1d5584b 100644
> --- a/lib/IR/GCOV.cpp
> +++ b/lib/IR/GCOV.cpp
> @@ -29,61 +29,61 @@ GCOVFile::~GCOVFile() {
> DeleteContainerPointers(Functions);
> }
>
> -/// isGCDAFile - Return true if Format identifies a .gcda file.
> -static bool isGCDAFile(GCOV::GCOVFormat Format) {
> - return Format == GCOV::GCDA_402 || Format == GCOV::GCDA_404;
> -}
> +/// readGCNO - Read GCNO buffer.
> +bool GCOVFile::readGCNO(GCOVBuffer &Buffer) {
> + if (!Buffer.readGCNOFormat()) return false;
> + if (!Buffer.readGCOVVersion(Version)) return false;
> +
> + if (!Buffer.readInt(Checksum)) return false;
> + while (true) {
> + if (!Buffer.readFunctionTag()) break;
> + GCOVFunction *GFun = new GCOVFunction();
> + if (!GFun->readGCNO(Buffer, Version))
> + return false;
> + Functions.push_back(GFun);
> + }
>
> -/// isGCNOFile - Return true if Format identifies a .gcno file.
> -static bool isGCNOFile(GCOV::GCOVFormat Format) {
> - return Format == GCOV::GCNO_402 || Format == GCOV::GCNO_404;
> + return true;
> }
>
> -/// read - Read GCOV buffer.
> -bool GCOVFile::read(GCOVBuffer &Buffer) {
> - GCOV::GCOVFormat Format = Buffer.readGCOVFormat();
> - if (Format == GCOV::InvalidGCOV)
> +/// readGCDA - Read GCDA buffer.
You should probably note in the documentation comment here that this
assumes readGCNO was called first, and will read uninitialized memory if
that isn't true. Some sort of assert to catch the programmer error and
avoid the undefined behaviour wouldn't be bad either.
> +bool GCOVFile::readGCDA(GCOVBuffer &Buffer) {
> + if (!Buffer.readGCDAFormat()) return false;
> + GCOV::GCOVVersion Version2;
> + if (!Buffer.readGCOVVersion(Version2)) return false;
> + if (Version != Version2) {
I realize that the Checksum2/Version2 naming is from the old code, but I
find them confusing (especially this check). GCDAVersion and
GCDAChecksum are a bit clearer.
> + errs() << "GCOV versions do not match.\n";
> return false;
> + }
>
> - if (isGCNOFile(Format)) {
> - if (!Buffer.readInt(Checksum)) return false;
> - while (true) {
> - if (!Buffer.readFunctionTag()) break;
> - GCOVFunction *GFun = new GCOVFunction();
> - if (!GFun->readGCNO(Buffer, Format))
> - return false;
> - Functions.push_back(GFun);
> - }
> - } else if (isGCDAFile(Format)) {
> - uint32_t Checksum2;
> - if (!Buffer.readInt(Checksum2)) return false;
> - if (Checksum != Checksum2) {
> - errs() << "File checksum does not match.\n";
> + uint32_t Checksum2;
> + if (!Buffer.readInt(Checksum2)) return false;
> + if (Checksum != Checksum2) {
> + errs() << "File checksum does not match.\n";
> + return false;
> + }
> + for (size_t i = 0, e = Functions.size(); i < e; ++i) {
> + if (!Buffer.readFunctionTag()) {
> + errs() << "Unexpected number of functions.\n";
> return false;
> }
> - for (size_t i = 0, e = Functions.size(); i < e; ++i) {
> - if (!Buffer.readFunctionTag()) {
> - errs() << "Unexpected number of functions.\n";
> - return false;
> - }
> - if (!Functions[i]->readGCDA(Buffer, Format))
> - return false;
> - }
> - if (Buffer.readObjectTag()) {
> - uint32_t Length;
> - uint32_t Dummy;
> - if (!Buffer.readInt(Length)) return false;
> - if (!Buffer.readInt(Dummy)) return false; // checksum
> - if (!Buffer.readInt(Dummy)) return false; // num
> - if (!Buffer.readInt(RunCount)) return false;;
> - Buffer.advanceCursor(Length-3);
> - }
> - while (Buffer.readProgramTag()) {
> - uint32_t Length;
> - if (!Buffer.readInt(Length)) return false;
> - Buffer.advanceCursor(Length);
> - ++ProgramCount;
> - }
> + if (!Functions[i]->readGCDA(Buffer, Version))
> + return false;
> + }
> + if (Buffer.readObjectTag()) {
> + uint32_t Length;
> + uint32_t Dummy;
> + if (!Buffer.readInt(Length)) return false;
> + if (!Buffer.readInt(Dummy)) return false; // checksum
> + if (!Buffer.readInt(Dummy)) return false; // num
> + if (!Buffer.readInt(RunCount)) return false;;
> + Buffer.advanceCursor(Length-3);
> + }
> + while (Buffer.readProgramTag()) {
> + uint32_t Length;
> + if (!Buffer.readInt(Length)) return false;
> + Buffer.advanceCursor(Length);
> + ++ProgramCount;
> }
>
> return true;
> @@ -117,12 +117,12 @@ GCOVFunction::~GCOVFunction() {
>
> /// readGCNO - Read a function from the GCNO buffer. Return false if an error
> /// occurs.
> -bool GCOVFunction::readGCNO(GCOVBuffer &Buff, GCOV::GCOVFormat Format) {
> +bool GCOVFunction::readGCNO(GCOVBuffer &Buff, GCOV::GCOVVersion Version) {
> uint32_t Dummy;
> if (!Buff.readInt(Dummy)) return false; // Function header length
> if (!Buff.readInt(Ident)) return false;
> if (!Buff.readInt(Dummy)) return false; // Checksum #1
> - if (Format != GCOV::GCNO_402)
> + if (Version != GCOV::V402)
> if (!Buff.readInt(Dummy)) return false; // Checksum #2
>
> if (!Buff.readString(Name)) return false;
> @@ -198,12 +198,12 @@ bool GCOVFunction::readGCNO(GCOVBuffer &Buff, GCOV::GCOVFormat Format) {
>
> /// readGCDA - Read a function from the GCDA buffer. Return false if an error
> /// occurs.
> -bool GCOVFunction::readGCDA(GCOVBuffer &Buff, GCOV::GCOVFormat Format) {
> +bool GCOVFunction::readGCDA(GCOVBuffer &Buff, GCOV::GCOVVersion Version) {
> uint32_t Dummy;
> if (!Buff.readInt(Dummy)) return false; // Function header length
> if (!Buff.readInt(Ident)) return false;
> if (!Buff.readInt(Dummy)) return false; // Checksum #1
> - if (Format != GCOV::GCDA_402)
> + if (Version != GCOV::V402)
> if (!Buff.readInt(Dummy)) return false; // Checksum #2
>
> if (!Buff.readString(Name)) return false;
> diff --git a/tools/llvm-cov/llvm-cov.cpp b/tools/llvm-cov/llvm-cov.cpp
> index 7f4d53e..b1ea33e 100644
> --- a/tools/llvm-cov/llvm-cov.cpp
> +++ b/tools/llvm-cov/llvm-cov.cpp
> @@ -49,7 +49,7 @@ int main(int argc, char **argv) {
> return 1;
> }
> GCOVBuffer GCNO_GB(GCNO_Buff.get());
> - if (!GF.read(GCNO_GB)) {
> + if (!GF.readGCNO(GCNO_GB)) {
> errs() << "Invalid .gcno File!\n";
> return 1;
> }
> @@ -61,7 +61,7 @@ int main(int argc, char **argv) {
> return 1;
> }
> GCOVBuffer GCDA_GB(GCDA_Buff.get());
> - if (!GF.read(GCDA_GB)) {
> + if (!GF.readGCDA(GCDA_GB)) {
> errs() << "Invalid .gcda File!\n";
> return 1;
> }
More information about the llvm-commits
mailing list