[PATCH] D37157: Fix Bug 30978 by emitting cv file checksums.

[Eric Beckmann via llvm-commits]

Thanks for the comments, however this patch is nowhere near close to ready
for review. I was going to make more changes before adding reviewers.

On Sep 6, 2017 9:27 PM, "Zachary Turner via Phabricator" <
reviews at reviews.llvm.org> wrote:

> zturner added a comment.
>
> Try to be more careful so as not to submit testing code / comments etc in
> uploaded patches.  Reasons like this are why I prefer using a debugger
> instead of printf debugging :)
>
>
>
> ================
> Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:476-481
> +  enum class ChecksumKind : uint8_t {
>      CSK_None,
>      CSK_MD5,
>      CSK_SHA1,
>      CSK_Last = CSK_SHA1 // Should be last enumeration.
>    };
> ----------------
> If you're going to make it an enum class, then it doesn't need the `CSK`
> prefix in the name, since you have to write `ChecksumKind` anyway.  I
> wouldn't suggest churning the code to change the names everywhere it's
> used, so instead maybe just leave this as a regular enum.  It's already
> scoped to `DIFile` anyway, so there's no risk of namespace pollution.
>
>
> ================
> Comment at: llvm/include/llvm/MC/MCCodeView.h:297-300
>    SmallVector<StringRef, 4> Filenames;
>
> +  // Map from filename to checksum.
> +  StringMap<std::pair<StringRef, uint8_t>> Checksums;
> ----------------
> Any reason this isn't just:
>
> ```
> struct FileInfo {
>   StringRef Name;
>   StringRef Checksum;
>   ChecksumKind Kind;
> };
>
> SmallVector<FileInfo> Files;
> ```
>
> having a `StringMap` seems unnecessary if we're already iterating a linear
> container anyway
>
>
> ================
> Comment at: llvm/lib/DebugInfo/CodeView/DebugChecksumsSubsection.cpp:53
>  Error DebugChecksumsSubsectionRef::initialize(BinaryStreamReader Reader)
> {
> +  //errs() << "checksum bytes remaining " << Reader.bytesRemaining() <<
> "\n";
>    if (auto EC = Reader.readArray(Checksums, Reader.bytesRemaining()))
> ----------------
> Delete
>
>
> ================
> Comment at: llvm/lib/DebugInfo/CodeView/DebugStringTableSubsection.cpp:32
>  Error DebugStringTableSubsectionRef::initialize(BinaryStreamReader
> &Reader) {
> +  errs() << "String table bytes remaining " << Reader.bytesRemaining() <<
> "\n";
>    return Reader.readStreamRef(Stream);
> ----------------
> Delete
>
>
> ================
> Comment at: llvm/lib/DebugInfo/CodeView/DebugStringTableSubsection.cpp:38
>  DebugStringTableSubsectionRef::getString(uint32_t Offset) const {
> +  //errs() << "DebugStringTableSubsectionRef getString\n";
>    BinaryStreamReader Reader(Stream);
> ----------------
> Delete
>
>
> ================
> Comment at: llvm/lib/DebugInfo/CodeView/DebugStringTableSubsection.cpp:44
>      return std::move(EC);
> +  //errs() << "leaving getString\n";
>    return Result;
> ----------------
> Delete
>
>
> ================
> Comment at: llvm/lib/MC/MCCodeView.cpp:188-200
> +    // std::pair<StringRef, uint8_t> Checksum = Checksums[Filename];
> +    // if (!Checksum.second) {
> +    //   // There is no checksum.
> +    //   OS.EmitIntValue(0, 4);
> +    //   continue;
> +    // }
> +    //OS.EmitIntValue(static_cast<uint8_t>(Checksum.first.size()), 1);
> ----------------
> What is this?  A bunch of code commented out, and then emitting a bunch of
> magic numbers?
>
>
> ================
> Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:927
>      case DebugSubsectionKind::StringTable:
> +      errs() << "initializing string table\n";
>        error(CVStringTable.initialize(ST));
> ----------------
> Delete
>
>
> ================
> Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:929
>        error(CVStringTable.initialize(ST));
> +      errs() << "initialized string table\n";
>        break;
> ----------------
> Delete
>
>
> ================
> Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1088
>        ListScope S(W, "FilenameSegment");
> +      //errs() << "about to pring filename with offset " <<
> Entry.NameIndex << "\n";
>        printFileNameForOffset("Filename", Entry.NameIndex);
> ----------------
> Delete
>
>
> ================
> Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:1090
>        printFileNameForOffset("Filename", Entry.NameIndex);
> +      //errs() << "printed filename\n";
>        uint32_t ColumnIndex = 0;
> ----------------
> Delete
>
>
> https://reviews.llvm.org/D37157
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170906/e7317299/attachment.html>