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

[Eric Beckmann via llvm-commits]

I upload it just so I can use the Phab UI to diff. It's probably not the
best way but it's what I've been using. I think there's a way to make
patches private only to me, I should probably do that next time.

On Sep 6, 2017 9:45 PM, "Zachary Turner" <zturner at google.com> wrote:

> 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/666a51df/attachment.html>