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

[Eric Beckmann via Phabricator via llvm-commits]

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.
   };
----------------
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.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:357
 
-static const char *ChecksumKindName[DIFile::CSK_Last + 1] = {
-  "CSK_None",
-  "CSK_MD5",
-  "CSK_SHA1"
-};
+static const char
+    *ChecksumKindName[static_cast<uint8_t>(DIFile::ChecksumKind::Last) + 1] = {
----------------
probinson wrote:
> If you require exact correspondence between an enum and some array of strings, we usually do that with a .def file and macros.  See for example Dwarf.def. As it is, the association between this array and the ChecksumKind values is implied rather than explicit.  Safer to be explicit.
Okay this should probably be addressed in another patch as this patch did not introduce the array.


================
Comment at: llvm/lib/MC/MCCodeView.cpp:169
+  for (auto File : Files) {
+    OS.EmitCVFileChecksumOffsetDirective(File.ChecksumTableOffset,
+                                         CurrentOffset);
----------------
rnk wrote:
> I don't think you want to do this, this leaks the internal MCSymbol (file1, etc) to the assembler. Nothing in the assembly defines file1, so the assembly isn't very readable.
Instead of emitting the symbol, we will emit the .cv_filechecksumoffset <fileno> directive.


https://reviews.llvm.org/D37157