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

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann marked 2 inline comments as done.
ecbeckmann added inline comments.


================
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.
   };
----------------
zturner wrote:
> 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.
Unfortunately I need the actual enum values to correspond exactly to the checksum types flag integer values actually present in the codeview section, hence the strongly typed enum class.  This is important because I will now be emitting the ChecksumKind as assembly directives as well as directly into the binary.


================
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;
----------------
zturner wrote:
> 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 
Yes, combined all this info into one vector.  It's still useful to have a StringMap because we want to check if a filename is already used before making a new entry.


https://reviews.llvm.org/D37157