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

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

A few minor commentary points; nothing huge.  I've been looking at this mainly with the thought of having to do something similar for DWARF 5 eventually, but it seems likely there won't be a lot to leverage as CodeView and DWARF appear to associate the hashes with files pretty differently.  Oh well.



================
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.
   };
----------------
ecbeckmann wrote:
> probinson wrote:
> > ecbeckmann wrote:
> > > 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.
> > If you need the enums to have specific values, e.g. because those values end up in the object file (hard to tell here but I think that's what you are saying), you should set the enum values explicitly.  This documents the exact values you need (even if they happen to be sequential at the moment).  Making it an enum class doesn't get you that.
> Remove enum class to explicitly define values.
And maybe a comment that these end up in the object file, so some well-meaning person doesn't come a long later and remove the "unnecessary" explicit values.


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:732
 
-  /// \brief Associate a filename with a specified logical file number.  This
-  /// implements the '.cv_file 4 "foo.c"' assembler directive. Returns true on
-  /// success.
-  virtual bool EmitCVFileDirective(unsigned FileNo, StringRef Filename);
+  /// \brief Associate a filename with a specified logical file number, and also
+  /// specify that file's checksum information.  This implements the '.cv_file 4
----------------
As long as you're changing the comment anyway, please remove the \brief.


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:778
 
+  /// \brief This implements the CodeView '.cv_filechecksumoffset' assembler
+  /// directive.
----------------
Remove \brief.


https://reviews.llvm.org/D37157