[PATCH] D33785: [CodeView] Fix alignment / padding when writing symbol records

[Bob Haarman via Phabricator via llvm-commits]

inglorion added a comment.

Thanks for fixing this, @zturner! I bumped into this a few days ago, but never really came up with a fix I was confident was the right thing.

Besides a few inline comments, there are also a few things I'd like you to add while you're at it. Specifically, in ModuleDebugFileChecksumFragment::commit() and ModuleDebugFragmentRecordBuilder::commit(), we use BinaryStreamWriter::padToAlignment() to pad the records we're writing. However, this actually pads the stream to a multiple of the alignment instead of the record, and so will only do the right thing if the stream was aligned before we wrote the record. I think we expect this to always be true. Can you add asserts to the beginning of the commit() methods to verify that the stream is 4-byte aligned at the beginning? Having those asserts there would have saved me some time while I was debugging the problem you're fixing here.



================
Comment at: llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp:30
   Limits.pop_back();
+  if (isReading()) {
+    // We would like to assert that we actually read all the bytes that we
----------------
rnk wrote:
> It sounds like we can't have either of these assertions, and I'd rather not have commented out code and this dead if. Can we simplify this to say that some writers (such as MASM) over-allocate for pointer records, and we over-allocate when writing?
I agree with rnk that we should probably not have a dead if here. I do like the explanation. I'm ok with a simpler explanation of why we can't verify that the size matches exactly, but I'm also ok with the explanation as it stands. But if we're unable to actually assert anything, please do remove the dead code.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp:71
+  // is not true of object files.
+  assert(Symbol.length() % 4 == 0);
+  SymbolByteSize += Symbol.length();
----------------
Can you change this to

  assert(Symbol.length % 4 == 0 && "Symbol records must be a multiple of 4 bytes when writing PDBs")

or some such so that we get the explanation right in the error message? Then you can remove the comment.


================
Comment at: llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp:540
+        // Symbol records in PDBs are padded to 4 bytes.
+        ModiBuilder.addSymbol(Symbol.toCodeViewSymbol(Allocator, 4));
+      }
----------------
I'm ok with this as-is, but since I'm asking for some changes I'd like to take the opportunity to suggest some alternatives to consider:

One would be to replace the 1 and 4 with named constants (say ObjectFileSymbolAlignment and PDBSymbolAlignment). That way, the code expresses the intent, you won't need to add the comment, and readers won't have to wonder what 1 or 4 means.

The other alternative is to do something like toCOFFCodeViewSymbol() and toPDBCodeViewSymbol(). I think that would also require replacing a few constructor calls (such as CVSymbolDumper in COFFDumper.cpp) with static method invocations, so may be more effort than it is worth.


https://reviews.llvm.org/D33785