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

[Rui Ueyama via llvm-commits]

ruiu 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;
 
----------------
zturner wrote:
> Now that we know what this actually is, can you change it to a better name?  Like `getPublicSymbolStreamIndex()`?
Will do.

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

================
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 {
----------------
zturner wrote:
> 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?
I'm not sure yet if the format is the same, so I want to keep as it is for now. It shouldn't be too late to move it to a common file when we find that this is a common thing.

================
Comment at: lib/DebugInfo/PDB/Raw/PublicsStream.cpp:64
@@ +63,3 @@
+    HdrSignature = -1,
+    HdrVersion = 0xeffe0000 + 19990810,
+  };
----------------
zturner wrote:
> Interesting, this is very similar to the other hash table, whose version is `0xEFFEEFFE`.  I wonder what the difference is.
I have no idea. In Microsoft code, this magic number is declared as GSIHashSCImpvV70.

================
Comment at: lib/DebugInfo/PDB/Raw/PublicsStream.cpp:72-75
@@ +71,6 @@
+
+struct PublicsStream::HRFile {
+  ulittle32_t Off;
+  ulittle32_t CRef;
+};
+
----------------
zturner wrote:
> 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.
This should be related to a hash table, but I don"t understand the exact meaning yet. I just took the name from the reference.

================
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.");
+
----------------
zturner wrote:
> 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`.
Sure, then I'll remove this piece of code.


http://reviews.llvm.org/D20256