[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/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);
----------------
zturner wrote:
> What is this?  A bunch of code commented out, and then emitting a bunch of magic numbers?
It was all debugging code that was unintended for review.  Please disregard.  Sorry again for not making this patch private to begin with.


================
Comment at: llvm/lib/MC/MCCodeView.cpp:418-420
       // File ids are 1 based, and each file checksum table entry is 8 bytes
       // long. See emitFileChecksums above.
+      unsigned FileOffset = 12 * (CurSourceLoc.File - 1);
----------------
rnk wrote:
> We shouldn't assume all DIFiles have MD5 checksums, we'll need some way to know the filechecksum table offset.
> 
> We should probably implement this FIXME:
>   // FIXME: We should store the string table offset of the filename, rather than
>   // the filename itself for efficiency.
>   Filename = addToStringTable(Filename);
>   Filenames[Idx] = Filename;
> 
> We should do that by making FIlenames a vector of two integers: the offset of the file in the file checksum table, and the offset of the filename in the string table.
> 
> All the places where we do `8 * (FileId - 1)` we would rewrite to use `Filenames[FileId - 1].ChecksumTableOffset`.
Filenames renamed to Files, which is now a vector of a massive structure containing all offset and checksum information.


https://reviews.llvm.org/D37157