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

[Zachary Turner via llvm-commits]

Hmm, I was getting emails about it, so I just figured it was ready for
review.  I guess it must have triggered a Phabricator rule that caused it
to send me emails.  Probably best not to upload until it's ready, unless
it's very explicit that it's not ready and you're just soliciting initial
feedback.  But if you're just uploading and not soliciting any feedback,
then I don't see much value in uploading to begin with?  (I could be
missing something, however)

On Wed, Sep 6, 2017 at 9:38 PM Eric Beckmann <ecbeckmann at google.com> wrote:

> 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/20170907/dbbad7b9/attachment.html>