[PATCH] D20256: pdbdump: Print "Publics" stream.

[Zachary Turner via llvm-commits]

zturner added inline comments.

================
Comment at: include/llvm/DebugInfo/PDB/Raw/DbiStream.h:36
@@ -35,2 +35,3 @@
   uint32_t getAge() const;
+  uint16_t getPSSyms() const;
 
----------------
Now that we know what this actually is, can you change it to a better name?  Like `getPublicSymbolStreamIndex()`?

================
Comment at: lib/DebugInfo/PDB/Raw/DbiStream.cpp:187
@@ -186,1 +186,3 @@
 
+uint16_t DbiStream::getPSSyms() const { return Header->PSSyms; }
+
----------------
Same here, can you change the field name in `Header` to be `PublicSymbolStreamIndex`?

================
Comment at: lib/DebugInfo/PDB/Raw/PublicsStream.cpp:47
@@ +46,3 @@
+// This is PSGSIHDR struct defined in
+// https://github.com/Microsoft/microsoft-pdb/blob/master/PDB/dbi/gsi.h
+struct PublicsStream::HeaderInfo {
----------------
The name `gsi` suggests that this structure is used for more than just public symbols.  I bet it stands for `Global Symbol Info`.  There is another stream representing global symbols, and my guess is that they have the exact same format.  Based on that, maybe we can call this entire class something more generic, like just `SymbolStream`.  What do you think?

================
Comment at: lib/DebugInfo/PDB/Raw/PublicsStream.cpp:64
@@ +63,3 @@
+    HdrSignature = -1,
+    HdrVersion = 0xeffe0000 + 19990810,
+  };
----------------
Interesting, this is very similar to the other hash table, whose version is `0xEFFEEFFE`.  I wonder what the difference is.

================
Comment at: lib/DebugInfo/PDB/Raw/PublicsStream.cpp:72-75
@@ +71,6 @@
+
+struct PublicsStream::HRFile {
+  ulittle32_t Off;
+  ulittle32_t CRef;
+};
+
----------------
Do you know what the purpose of `CRef` is here?  In `TpiStream.cpp` we have a very similar structure called `EmbeddedBuff`.  In that case, the Offset points to a location in the stream, and the second field indicates the length of the buffer.  If this is the same thing, perhaps it's worth raising that structure to a higher level so both can reuse it.

================
Comment at: lib/DebugInfo/PDB/Raw/PublicsStream.cpp:93-96
@@ +92,6 @@
+
+  // Check stream size.
+  if (Reader.bytesRemaining() < sizeof(HeaderInfo) + sizeof(GSIHashHeader))
+    return make_error<RawError>(raw_error_code::corrupt_file,
+                                "Publics Stream does not contain a header.");
+
----------------
I think this line isn't necessary, because the condition will be checked automatically when you try to read the `HeaderInfo`, and again when you try to read the `GSIHashHeader`.


http://reviews.llvm.org/D20256