[PATCH] D31636: [PDB] Emit index/offset pairs for TPI and IPI streams

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lld/test/COFF/pdb.test:194
 # RAW-NEXT:       Name: * Linker *
-# RAW-NEXT:       Debug Stream Index: 65535
+# RAW-NEXT:       Debug Stream Index: 9
 # RAW-NEXT:       Object File Name:
----------------
I would remove all these values, otherwise it's going to be high maintenance as we add more and more stuff to the PDB and stuff shifts around.


================
Comment at: lld/test/COFF/pdb.test:355-356
 # RAW-NEXT:       Has EC Info: No
-# RAW-NEXT:       0 Contributing Source Files [
-# RAW-NEXT:       ]
 # RAW-NEXT:     }
----------------
Was this intentional?  Seems like we should be able to be reasonably certain that 0 source files contribute to the Linker-generated module.


================
Comment at: lld/test/COFF/pdb.test:396
 # RAW-NEXT:     Name: .rdata
-# RAW-NEXT:     Virtual Size: 101
+# RAW-NEXT:     Virtual Size: 122
 # RAW-NEXT:     Virtual Address: 16384
----------------
Same here and with other numbers that you changed.  


================
Comment at: lld/test/COFF/pdb.test:500-501
 # RAW-NEXT: ]
-# RAW-NEXT: Globals Stream not present
-# RAW-NEXT: Publics Stream not present
 # RAW-NEXT: Section Headers [
----------------
Intentional?


================
Comment at: lld/test/COFF/pdb.test:565
 # RAW-NEXT: ]
-# RAW-NEXT: New FPO [
-# RAW-NEXT: ]
----------------
Was this intentionally deleted?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/TpiStream.cpp:100
         Header->HashValueBuffer.Length / sizeof(ulittle32_t);
-    if (NumHashValues != NumTypeRecords())
+    // FIXME: Allow zero hash tables for now.
+    if (NumHashValues != NumTypeRecords() && NumHashValues != 0)
----------------
s/hash tables/hash values/


================
Comment at: llvm/lib/DebugInfo/PDB/Native/TpiStreamBuilder.cpp:52-55
+    TypeIndexOffsets.push_back(
+        {codeview::TypeIndex(codeview::TypeIndex::FirstNonSimpleIndex +
+                             TypeRecords.size()),
+         ulittle32_t(TypeRecordBytes)});
----------------
`emplace_back`?  This way you don't have to use the brace initializer syntax.  I think `TypeIndex` constructor is implicit too, so you might even be able to just write `TypeIndexOffsets.emplace_back(codeview::TypeIndex::FirstNonSimpleIndex + TypeRecords.size(), ulittle32_t(TypeRecordBytes));`


================
Comment at: llvm/lib/DebugInfo/PDB/Native/TpiStreamBuilder.cpp:106-107
 uint32_t TpiStreamBuilder::calculateHashBufferSize() const {
   assert(TypeHashes.size() == TypeHashes.size() &&
          "either all or no type records should have hashes");
   return TypeHashes.size() * sizeof(ulittle32_t);
----------------
This looks bogus.


================
Comment at: llvm/test/DebugInfo/PDB/pdb-yaml-types.test:29-31
+This test is mostly checking to make sure we include the type index offset
+table, and eventually hash codes. The type index offsets should be similar to
+what are already present in big-read.pdb.
----------------
Maybe I just never knew you could do this, but can you put random text in the middle of a test file with no comment marker?


https://reviews.llvm.org/D31636