[PATCH] D22308: [pdb] Introduce an MsfBuilder class for laying out PDB files

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 13 15:17:32 PDT 2016


ruiu added inline comments.

================
Comment at: include/llvm/DebugInfo/PDB/Raw/MsfBuilder.h:33
@@ +32,3 @@
+                  bool CanGrow = true);
+  void reset();
+
----------------
What's the use case of `reset()`? You can create as many instances of this class as you want, so wouldn't it be easier to make it a non-reusable class?

================
Comment at: include/llvm/DebugInfo/PDB/Raw/MsfBuilder.h:67
@@ +66,3 @@
+  BitVector FreePages;
+  std::vector<std::pair<uint32_t, BlockList>> StreamData;
+};
----------------
Which member contains streams' actual data?

Ahh, so this class is to constructs only the MSF header, and stream data is managed separately outside of this class. Is this correct? So it seems that you'd use this class to create a MSF directory, obtain a Layout object, and write each stream as the returned layout. Not sure how you would handle actual file writing, but this is probably just out of scope of this patch.

================
Comment at: lib/DebugInfo/PDB/Raw/MsfCommon.cpp:22-37
@@ +21,18 @@
+
+  // We don't support blocksizes which aren't a multiple of four bytes.
+  if (SB.BlockSize % sizeof(support::ulittle32_t) != 0)
+    return make_error<RawError>(raw_error_code::corrupt_file,
+                                "Block size is not multiple of 4.");
+
+  switch (SB.BlockSize) {
+  case 512:
+  case 1024:
+  case 2048:
+  case 4096:
+    break;
+  default:
+    // An invalid block size suggests a corrupt PDB file.
+    return make_error<RawError>(raw_error_code::corrupt_file,
+                                "Unsupported block size.");
+  }
+
----------------
These two tests seem to do basically the same thing. You can remove the former check.

================
Comment at: unittests/DebugInfo/PDB/MsfBuilderTest.cpp:149
@@ +148,3 @@
+  // Test growing an existing stream by a value that does not affect the number
+  // of of pages it occupies.
+  MsfBuilder Msf(Allocator);
----------------
of of


http://reviews.llvm.org/D22308





More information about the llvm-commits mailing list