[PATCH] D24370: Add support for writing TPI hash values

[Zachary Turner via llvm-commits]

zturner added inline comments.

================
Comment at: include/llvm/DebugInfo/PDB/Raw/TpiRecordHashVisitor.h:56-59
@@ +55,6 @@
+public:
+  TpiRecordHashVerificationVisitor(
+      msf::FixedStreamArray<support::ulittle32_t> &HashValues,
+      uint32_t NumHashBuckets)
+      : HashValues(HashValues), NumHashBuckets(NumHashBuckets) {}
+
----------------
majnemer wrote:
> Is this clang-formatted?
It's clang-formatted.  I agree this looks odd, I don't know why it did it formatted it that way.  I'll run it through clang-format again just to make sure.

================
Comment at: lib/DebugInfo/PDB/Raw/TpiStream.cpp:105-107
@@ -195,2 +104,5 @@
       return EC;
+    std::vector<ulittle32_t> HashValueList;
+    for (auto I : HashValues)
+      HashValueList.push_back(I);
 
----------------
majnemer wrote:
> Couldn't you just do `std::vector<ulittle32_t> HashValueList(HashValueList.begin(), HashValueList.end());`
I tried but it didn't work because `HashValueList.begin()` isn't a normal iterator, it's a `FixedStreamArrayIterator<>`.  There might be some magic to get it to work, but I don't know what the specific requirements are for an iterator to be usable in that context.  

================
Comment at: test/DebugInfo/PDB/pdbdump-readwrite.test:13-18
@@ -12,8 +12,8 @@
 CHECK-NEXT:   NumBlocks: 25
-CHECK-NEXT:   NumDirectoryBytes: 136
+CHECK-NEXT:   NumDirectoryBytes:
 CHECK-NEXT:   Unknown1: 0
-CHECK-NEXT:   BlockMapAddr: 24
+CHECK-NEXT:   BlockMapAddr:
 CHECK-NEXT:   NumDirectoryBlocks: 1
-CHECK-NEXT:   DirectoryBlocks: [23]
-CHECK-NEXT:   NumStreams: 17
+CHECK-NEXT:   DirectoryBlocks:
+CHECK-NEXT:   NumStreams:
 CHECK-NEXT: }
----------------
majnemer wrote:
> Is this diff correct?
Yes, I removed these intentionally.  Previously we were re-creating the stream directory and stream size list exactly, even though we weren't filling all the streams with any data.  I removed the code to do that because it makes it difficult / impossible when we need to allocate a non-special stream.  Specifically, in this patch we go and allocate space for the hash value stream.  Which stream was that from the stream table?  If we're trying to map the MSF layout exactly to the same stream indices etc, then we would need to look at the yaml, figure out what stream was the hash value stream, and assign the same index to it.  The logic starts getting complicated for no tangible benefit.

So instead, I decided that I would only reserve special streams, and reserve other dynamic streams as needed.  See all the code in `llvm-pdbdump.cpp` that was deleted.  Because of all this, the directory, number of streams, etc is no longer going to match up.  Someday in the future when we can round-trip a PDB byte for byte, we can add these back.


https://reviews.llvm.org/D24370