[CodeView] Fix alignment / padding when writing symbol records

[zturner]

Summary

The rabbit hole runs deep on this one. Apparently when writing object files, symbol records are 1-byte aligned, but when writing PDB files, symbols are 4-byte aligned. I confirmed this through the following sequence of steps:

1. Starting with test/tools/llvm-readobj/codeview-types.test, make it possible to build the code by deleting the #error and adding definitions of the various functions and adding a main function so that it can link into a valid executable.
2. Use the instructions in the aforementioned file to regenerate the object file on a modern version of MSVC.
3. Use llvm-readobj -codeview -codeview-subsection-bytes codeview-types.obj to dump the records. Save this output somewhere
4. Link the object file using link /debug /out:codeview-types.exe codeview-types.obj, which should produce codeview-types.pdb
5. Dump the pdb using llvm-pdbdump raw -dbi-module-syms -sym-record-bytes codeview-types.pdb and save the output somewhere.

When you look at the first file, you can find these two records (among plenty of others):

ObjectName {
  Signature: 0x0
  ObjectName: D:\src\llvmbuild\ninja\codeview-types-cl.obj
}
SymData (
  0000: 00000000 443A5C73 72635C6C 6C766D62  |....D:\src\llvmb|
  0010: 75696C64 5C6E696E 6A615C63 6F646576  |uild\ninja\codev|
  0020: 6965772D 74797065 732D636C 2E6F626A  |iew-types-cl.obj|
  0030: 00                                   |.|
)
FrameProc {
  TotalFrameBytes: 0x28
  PaddingFrameBytes: 0x0
  OffsetToPadding: 0x0
  BytesOfCalleeSavedRegisters: 0x0
  OffsetOfExceptionHandler: 0x0
  SectionIdOfExceptionHandler: 0x0
  Flags [ (0x128200)
    AsynchronousExceptionHandling (0x200)
    OptimizedForSpeed (0x100000)
  ]
}
SymData (
  0000: 28000000 00000000 00000000 00000000  |(...............|
  0010: 00000000 00000082 1200               |..........|
)

Note that the first record consists of 49 bytes and the second consists of 26 bytes, neither of which is a multiple of 4. If you track down the same records in the dumped PDB file, they look like this:

{
  ObjectName {
    Signature: 0x0
    ObjectName: D:\src\llvmbuild\ninja\codeview-types-cl.obj
  }
  Bytes (
    0000: 00000000 443A5C73 72635C6C 6C766D62  |....D:\src\llvmb|
    0010: 75696C64 5C6E696E 6A615C63 6F646576  |uild\ninja\codev|
    0020: 6965772D 74797065 732D636C 2E6F626A  |iew-types-cl.obj|
    0030: 00000000                             |....|
  )
}
{
  FrameProc {
    TotalFrameBytes: 0x28
    PaddingFrameBytes: 0x0
    OffsetToPadding: 0x0
    BytesOfCalleeSavedRegisters: 0x0
    OffsetOfExceptionHandler: 0x0
    SectionIdOfExceptionHandler: 0x0
    Flags [ (0x128200)
      AsynchronousExceptionHandling (0x200)
      OptimizedForSpeed (0x100000)
    ]
  }
  Bytes (
    0000: 28000000 00000000 00000000 00000000  |(...............|
    0010: 00000000 00000082 12000000           |............|
  )

and here we can see that both records have been zero-padded to 4 bytes but are otherwise identical.

Since we use the same code for reading / writing symbols to / from object files / PDB files, this is kind of annoying. To address this, I added an alignment field to the low level SymbolSerializer and SymbolDeserializer classes and propagate this value all the way up to the top-level client, in this case llvm-pdbdump, llvm-readobj, lld, etc and let them specify the alignment based on whether they're handling PDB files or object files.

After this change, we can round-trip symbol records from PDB -> YAML -> PDB, and I modified the existing round-tripping test to verify that this does in fact work now.. There are still about 5 bytes that differ between the original stream's symbol records and the new stream's symbol records, but at least it doesn't crash and the records look the same when re dumped. We can investigate the remaining byte differences in a followup patch.

Another interesting (and annoying) thing I learned is that either Microsoft compiles with #pragma pack(2) or #pragma pack(1) or the symbol record definitions in cvrec.h aren't authoritative. I went off on a wild goose chase for a couple of hours after noticing that FRAMEPROCSYM contains a uint16 followed by a uint32, and so there are 2 bytes of padding in there that we aren't accounting for. I confirmed this with cl /d1reportSingleClassRecordLayoutFRAMEPROCSYM against their structure definition. But this broke many of the existing tests in llvm-readobj. It turns out this 32-bit enum is *actually* written unaligned in the official implementation, so the code was originally correct. Also confirmed by running cvdump -s on an object file, and realizing that the padding to 4 bytes comes *after* the enum, not before.

Ugh. I want my night back.

[rnk]

BTW, cvinfo.h does in fact have #pragma pack ( push, 1 ) in it.

Looks good. :)

llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp

30 ↗

(On Diff #101040)

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?

[inglorion]

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.

llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp

30 ↗

(On Diff #101040)

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.

llvm/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp

71 ↗

(On Diff #101040)

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.

llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp

540 ↗

(On Diff #101040)

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.

[zturner]

Updated with suggestions from inglorion. Note that I actually fixed a bug with this update of the patch. We weren't respecting the PDB / Object File alignment difference when writing the debug subsections, only when writing symbol records. We still had the padding hardcoded to 4 in the debug subsections. This definitely isn't true though, as I currently have this from a dump of an MSVC generated object file.

CodeViewDebugInfo [
  Section: .debug$S (2)
  Magic: 0x4
  Subsection [
    SubSectionType: Symbols (0xF1)
    SubSectionSize: 0x71
    SubSectionContents (
      0000: 33000111 00000000 443A5C73 72635C6C  |3.......D:\src\l|
      0010: 6C766D62 75696C64 5C6E696E 6A615C63  |lvmbuild\ninja\c|
      0020: 6F646576 6965772D 74797065 732D636C  |odeview-types-cl|
      0030: 2E6F626A 003A003C 11012000 00070013  |.obj.:.<.. .....|
      0040: 000A00BB 61000013 000A00BB 6100004D  |....a.......a..M|
      0050: 6963726F 736F6674 20285229 204F7074  |icrosoft (R) Opt|
      0060: 696D697A 696E6720 436F6D70 696C6572  |imizing Compiler|
      0070: 00                                   |.|
    )
...

So in this patch I also update the `DebugSubsectionRecordBuilder` to use the appropriate
alignment when computing its size and when writing itself.

[rnk]

In D33785#770558, @zturner wrote:

Updated with suggestions from inglorion. Note that I actually fixed a bug with this update of the patch. We weren't respecting the PDB / Object File alignment difference when writing the debug subsections, only when writing symbol records. We still had the padding hardcoded to 4 in the debug subsections. This definitely isn't true though, as I currently have this from a dump of an MSVC generated object file.

Do you think there is any harm to writing aligned symbol records into object files? If we were writing aligned records into object files before, I'd kind of like to maintain that property.

[zturner]

I don't think it should matter, because each record describes its own length, and that is the authority on how many bytes we need to skip to get to the next record.

On the other hand, I also don't think this patch really affects that. We do all our writing of object files through CodeViewDebug.cpp and that doesn't use these classes (at least for the time being, it would be nice to merge these in the future). So whatever object files we produced before, we will continue to produce.

So with that said, why bother even supporting both alignment styles? It's nice to have this documented in code somehow, and it gives us some flexibility in the future if we ever decide that we must do it the way Microsoft does it that we can support whatever method we need with the flip of a switch.

[rnk]

I guess I'm saying it seems nice to always write aligned records until we come up with some reason to not write aligned records. Having the flexibility to do either seems good. We definitely have to read the unaligned records.

[amccarth]

In D33785#770614, @rnk wrote:

I guess I'm saying it seems nice to always write aligned records until we come up with some reason to not write aligned records. Having the flexibility to do either seems good. We definitely have to read the unaligned records.

That's an interesting topic for another patch. As Zach says, this doesn't affect writing the objects.

And even if it did, I'd be mildly concerned about aligning output more strictly than the tools we're trying to be compatible with. If all our linker tests (for example) were based on neatly aligned object files, it would be easy for a bug to slip into the linker that assumes objects will always be neatly aligned.

[inglorion]

I guess I'm saying it seems nice to always write aligned records until we come up with some reason to not write aligned records.

@zturner and I had talked about that a little bit before. I mentioned that unaligned records may save some space. They may be slightly less efficient to process, but that could be a good trade-off in object files. I'm not sure how large these records usually are or what the effect on size or speed would be if we always aligned or always packed or how much leeway we have in simplifying the code while being interoperable with other tools. For now, I prefer to focus on getting usable PDBs out of lld; we can perhaps revisit the alignment issue after that if it seems warranted.

[inglorion]

lgtm modulo inline comment about messsage in the assert

llvm/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp

71 ↗

(On Diff #101040)

Please still add a message to the assert here.

lld/trunk/COFF/PDB.cpp

static void dumpDebugS(ScopedPrinter &W, ObjectFile *File) {
 
   BinaryByteStream Stream(Data, llvm::support::little);
   CVSymbolArray Symbols;
   BinaryStreamReader Reader(Stream);
   if (auto EC = Reader.readArray(Symbols, Reader.getLength()))
     fatal(EC, "StreamReader.readArray<CVSymbolArray> failed");
 
   TypeDatabase TDB(0);
-  CVSymbolDumper SymbolDumper(W, TDB, nullptr, false);
+  CVSymbolDumper SymbolDumper(W, TDB, CodeViewContainer::ObjectFile, nullptr,
+                              false);
   if (auto EC = SymbolDumper.dump(Symbols))
     fatal(EC, "CVSymbolDumper::dump failed");
 }
 
 // Dump CodeView debug info. This is for debugging.
 static void dumpCodeView(SymbolTable *Symtab) {
   ScopedPrinter W(outs());
 

llvm/trunk/include/llvm/DebugInfo/CodeView/CodeView.h

struct FrameData {
   support::ulittle16_t SavedRegsSize;
   support::ulittle32_t Flags;
   enum : uint32_t {
     HasSEH = 1 << 0,
     HasEH = 1 << 1,
     IsFunctionStart = 1 << 2,
   };
 };
+
+enum class CodeViewContainer { ObjectFile, Pdb };
+
+inline uint32_t alignOf(CodeViewContainer Container) {
+  if (Container == CodeViewContainer::ObjectFile)
+    return 1;
+  return 4;
+}
 }
 }
 
 #endif

llvm/trunk/include/llvm/DebugInfo/CodeView/CodeViewRecordIO.h

if (isWriting()) {
       }
     }
     return Error::success();
   }
 
   Error mapByteVectorTail(ArrayRef<uint8_t> &Bytes);
   Error mapByteVectorTail(std::vector<uint8_t> &Bytes);
 
+  Error padToAlignment(uint32_t Align);
   Error skipPadding();
 
 private:
   Error writeEncodedSignedInteger(const int64_t &Value);
   Error writeEncodedUnsignedInteger(const uint64_t &Value);
 
   struct RecordLimit {
     uint32_t BeginOffset;

llvm/trunk/include/llvm/DebugInfo/CodeView/DebugSubsectionRecord.h

 struct DebugSubsectionHeader {
   support::ulittle32_t Kind;   // codeview::DebugSubsectionKind enum
   support::ulittle32_t Length; // number of bytes occupied by this record.
 };
 
 class DebugSubsectionRecord {
 public:
   DebugSubsectionRecord();
-  DebugSubsectionRecord(DebugSubsectionKind Kind, BinaryStreamRef Data);
+  DebugSubsectionRecord(DebugSubsectionKind Kind, BinaryStreamRef Data,
+                        CodeViewContainer Container);
 
-  static Error initialize(BinaryStreamRef Stream, DebugSubsectionRecord &Info);
+  static Error initialize(BinaryStreamRef Stream, DebugSubsectionRecord &Info,
+                          CodeViewContainer Container);
 
   uint32_t getRecordLength() const;
   DebugSubsectionKind kind() const;
   BinaryStreamRef getRecordData() const;
 
 private:
+  CodeViewContainer Container;
   DebugSubsectionKind Kind;
   BinaryStreamRef Data;
 };
 
 class DebugSubsectionRecordBuilder {
 public:
-  DebugSubsectionRecordBuilder(DebugSubsectionKind Kind, DebugSubsection &Frag);
+  DebugSubsectionRecordBuilder(DebugSubsectionKind Kind, DebugSubsection &Frag,
+                               CodeViewContainer Container);
   uint32_t calculateSerializedLength();
   Error commit(BinaryStreamWriter &Writer);
 
 private:
+  CodeViewContainer Container;
   DebugSubsectionKind Kind;
   DebugSubsection &Frag;
 };
 
 } // namespace codeview
 
 template <> struct VarStreamArrayExtractor<codeview::DebugSubsectionRecord> {
   typedef void ContextType;
 
   static Error extract(BinaryStreamRef Stream, uint32_t &Length,
                        codeview::DebugSubsectionRecord &Info) {
-    if (auto EC = codeview::DebugSubsectionRecord::initialize(Stream, Info))
+    // FIXME: We need to pass the container type through to this function, but
+    // VarStreamArray doesn't easily support stateful contexts.  In practice
+    // this isn't super important since the subsection header describes its
+    // length and we can just skip it.  It's more important when writing.
+    if (auto EC = codeview::DebugSubsectionRecord::initialize(
+            Stream, Info, codeview::CodeViewContainer::Pdb))
       return EC;
     Length = Info.getRecordLength();
     return Error::success();
   }
 };
 
 namespace codeview {
 typedef VarStreamArray<DebugSubsectionRecord> DebugSubsectionArray;
 }
 } // namespace llvm
 
 #endif // LLVM_DEBUGINFO_CODEVIEW_MODULEDEBUGFRAGMENTRECORD_H

llvm/trunk/include/llvm/DebugInfo/CodeView/SymbolDeserializer.h

 #include "llvm/Support/BinaryStreamReader.h"
 #include "llvm/Support/Error.h"
 
 namespace llvm {
 namespace codeview {
 class SymbolVisitorDelegate;
 class SymbolDeserializer : public SymbolVisitorCallbacks {
   struct MappingInfo {
-    explicit MappingInfo(ArrayRef<uint8_t> RecordData)
+    MappingInfo(ArrayRef<uint8_t> RecordData, CodeViewContainer Container)
         : Stream(RecordData, llvm::support::little), Reader(Stream),
-          Mapping(Reader) {}
+          Mapping(Reader, Container) {}
 
     BinaryByteStream Stream;
     BinaryStreamReader Reader;
     SymbolRecordMapping Mapping;
   };
 
 public:
   template <typename T> static Error deserializeAs(CVSymbol Symbol, T &Record) {
-    SymbolDeserializer S(nullptr);
+    // If we're just deserializing one record, then don't worry about alignment
+    // as there's nothing that comes after.
+    SymbolDeserializer S(nullptr, CodeViewContainer::ObjectFile);
     if (auto EC = S.visitSymbolBegin(Symbol))
       return EC;
     if (auto EC = S.visitKnownRecord(Symbol, Record))
       return EC;
     if (auto EC = S.visitSymbolEnd(Symbol))
       return EC;
     return Error::success();
   }
 
-  explicit SymbolDeserializer(SymbolVisitorDelegate *Delegate)
-      : Delegate(Delegate) {}
+  explicit SymbolDeserializer(SymbolVisitorDelegate *Delegate,
+                              CodeViewContainer Container)
+      : Delegate(Delegate), Container(Container) {}
 
   Error visitSymbolBegin(CVSymbol &Record) override {
     assert(!Mapping && "Already in a symbol mapping!");
-    Mapping = llvm::make_unique<MappingInfo>(Record.content());
+    Mapping = llvm::make_unique<MappingInfo>(Record.content(), Container);
     return Mapping->Mapping.visitSymbolBegin(Record);
   }
   Error visitSymbolEnd(CVSymbol &Record) override {
     assert(Mapping && "Not in a symbol mapping!");
     auto EC = Mapping->Mapping.visitSymbolEnd(Record);
     Mapping.reset();
     return EC;
   }
template <typename T> Error visitKnownRecordImpl(CVSymbol &CVR, T &Record) {
 
     Record.RecordOffset =
         Delegate ? Delegate->getRecordOffset(Mapping->Reader) : 0;
     if (auto EC = Mapping->Mapping.visitKnownRecord(CVR, Record))
       return EC;
     return Error::success();
   }
 
+  CodeViewContainer Container;
   SymbolVisitorDelegate *Delegate;
   std::unique_ptr<MappingInfo> Mapping;
 };
 }
 }
 
 #endif

llvm/trunk/include/llvm/DebugInfo/CodeView/SymbolDumper.h

 
 namespace codeview {
 class TypeCollection;
 
 /// Dumper for CodeView symbol streams found in COFF object files and PDB files.
 class CVSymbolDumper {
 public:
   CVSymbolDumper(ScopedPrinter &W, TypeCollection &Types,
+                 CodeViewContainer Container,
                  std::unique_ptr<SymbolDumpDelegate> ObjDelegate,
                  bool PrintRecordBytes)
-      : W(W), Types(Types), ObjDelegate(std::move(ObjDelegate)),
+      : W(W), Types(Types), Container(Container),
+        ObjDelegate(std::move(ObjDelegate)),
         PrintRecordBytes(PrintRecordBytes) {}
 
   /// Dumps one type record.  Returns false if there was a type parsing error,
   /// and true otherwise.  This should be called in order, since the dumper
   /// maintains state about previous records which are necessary for cross
   /// type references.
   Error dump(CVRecord<SymbolKind> &Record);
 
   /// Dumps the type records in Data. Returns false if there was a type stream
   /// parse error, and true otherwise.
   Error dump(const CVSymbolArray &Symbols);
 
 private:
   ScopedPrinter &W;
   TypeCollection &Types;
+  CodeViewContainer Container;
   std::unique_ptr<SymbolDumpDelegate> ObjDelegate;
 
   bool PrintRecordBytes;
 };
 } // end namespace codeview
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_CODEVIEW_SYMBOLDUMPER_H

llvm/trunk/include/llvm/DebugInfo/CodeView/SymbolRecordMapping.h

 
 namespace llvm {
 class BinaryStreamReader;
 class BinaryStreamWriter;
 
 namespace codeview {
 class SymbolRecordMapping : public SymbolVisitorCallbacks {
 public:
-  explicit SymbolRecordMapping(BinaryStreamReader &Reader) : IO(Reader) {}
-  explicit SymbolRecordMapping(BinaryStreamWriter &Writer) : IO(Writer) {}
+  explicit SymbolRecordMapping(BinaryStreamReader &Reader,
+                               CodeViewContainer Container)
+      : IO(Reader), Container(Container) {}
+  explicit SymbolRecordMapping(BinaryStreamWriter &Writer,
+                               CodeViewContainer Container)
+      : IO(Writer), Container(Container) {}
 
   Error visitSymbolBegin(CVSymbol &Record) override;
   Error visitSymbolEnd(CVSymbol &Record) override;
 
 #define SYMBOL_RECORD(EnumName, EnumVal, Name)                                 \
   Error visitKnownRecord(CVSymbol &CVR, Name &Record) override;
 #define SYMBOL_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
 #include "llvm/DebugInfo/CodeView/CodeViewSymbols.def"
 
 private:
   Optional<SymbolKind> Kind;
 
+  CodeViewContainer Container;
   CodeViewRecordIO IO;
 };
 }
 }
 
 #endif

llvm/trunk/include/llvm/DebugInfo/CodeView/SymbolSerializer.h

Error writeRecordPrefix(SymbolKind Kind) {
     Prefix.RecordLen = 0;
     if (auto EC = Writer.writeObject(Prefix))
       return EC;
     return Error::success();
   }
 
 public:
   template <typename SymType>
-  static CVSymbol writeOneSymbol(SymType &Sym, BumpPtrAllocator &Storage) {
+  static CVSymbol writeOneSymbol(SymType &Sym, BumpPtrAllocator &Storage,
+                                 CodeViewContainer Container) {
     CVSymbol Result;
     Result.Type = static_cast<SymbolKind>(Sym.Kind);
-    SymbolSerializer Serializer(Storage);
+    SymbolSerializer Serializer(Storage, Container);
     consumeError(Serializer.visitSymbolBegin(Result));
     consumeError(Serializer.visitKnownRecord(Result, Sym));
     consumeError(Serializer.visitSymbolEnd(Result));
     return Result;
   }
 
-  explicit SymbolSerializer(BumpPtrAllocator &Storage);
+  SymbolSerializer(BumpPtrAllocator &Storage, CodeViewContainer Container);
 
   virtual Error visitSymbolBegin(CVSymbol &Record) override;
   virtual Error visitSymbolEnd(CVSymbol &Record) override;
 
 #define SYMBOL_RECORD(EnumName, EnumVal, Name)                                 \
   virtual Error visitKnownRecord(CVSymbol &CVR, Name &Record) override {       \
     return visitKnownRecordImpl(CVR, Record);                                  \
   }

llvm/trunk/include/llvm/ObjectYAML/CodeViewYAMLSymbols.h

 namespace CodeViewYAML {
 namespace detail {
 struct SymbolRecordBase;
 }
 
 struct SymbolRecord {
   std::shared_ptr<detail::SymbolRecordBase> Symbol;
 
-  codeview::CVSymbol toCodeViewSymbol(BumpPtrAllocator &Allocator) const;
+  codeview::CVSymbol
+  toCodeViewSymbol(BumpPtrAllocator &Allocator,
+                   codeview::CodeViewContainer Container) const;
   static Expected<SymbolRecord> fromCodeViewSymbol(codeview::CVSymbol Symbol);
 };
 
 } // namespace CodeViewYAML
 } // namespace llvm
 
 LLVM_YAML_DECLARE_MAPPING_TRAITS(CodeViewYAML::SymbolRecord)
 LLVM_YAML_IS_SEQUENCE_VECTOR(CodeViewYAML::SymbolRecord)
 
 #endif

llvm/trunk/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp

Error CodeViewRecordIO::beginRecord(Optional<uint32_t> MaxLength) {
   Limit.BeginOffset = getCurrentOffset();
   Limits.push_back(Limit);
   return Error::success();
 }
 
 Error CodeViewRecordIO::endRecord() {
   assert(!Limits.empty() && "Not in a record!");
   Limits.pop_back();
+  // We would like to assert that we actually read / wrote all the bytes that we
+  // expected to for this record, but unfortunately we can't do this.  Some
+  // producers such as MASM over-allocate for certain types of records and
+  // commit the extraneous data, so when reading we can't be sure every byte
+  // will have been read.  And when writing we over-allocate temporarily since
+  // we don't know how big the record is until we're finished writing it, so
+  // even though we don't commit the extraneous data, we still can't guarantee
+  // we're at the end of the allocated data.
   return Error::success();
 }
 
 uint32_t CodeViewRecordIO::maxFieldLength() const {
   assert(!Limits.empty() && "Not in a record!");
 
   // The max length of the next field is the minimum of all lengths that would
   // be allowed by any of the sub-records we're in.  In practice, we can only
   // ever be at most 1 sub-record deep (in a FieldList), but this works for
   // the general case.
   uint32_t Offset = getCurrentOffset();
   Optional<uint32_t> Min = Limits.front().bytesRemaining(Offset);
   for (auto X : makeArrayRef(Limits).drop_front()) {
     Optional<uint32_t> ThisMin = X.bytesRemaining(Offset);
     if (ThisMin.hasValue())
       Min = (Min.hasValue()) ? std::min(*Min, *ThisMin) : *ThisMin;
   }
   assert(Min.hasValue() && "Every field must have a maximum length!");
 
   return *Min;
 }
 
+Error CodeViewRecordIO::padToAlignment(uint32_t Align) {
+  if (isReading())
+    return Reader->padToAlignment(Align);
+  return Writer->padToAlignment(Align);
+}
+
 Error CodeViewRecordIO::skipPadding() {
   assert(!isWriting() && "Cannot skip padding while writing!");
 
   if (Reader->bytesRemaining() == 0)
     return Error::success();
 
   uint8_t Leaf = Reader->peek();
   if (Leaf < LF_PAD0)

llvm/trunk/lib/DebugInfo/CodeView/DebugSubsectionRecord.cpp

 #include "llvm/DebugInfo/CodeView/DebugSubsection.h"
 
 #include "llvm/Support/BinaryStreamReader.h"
 
 using namespace llvm;
 using namespace llvm::codeview;
 
 DebugSubsectionRecord::DebugSubsectionRecord()
-    : Kind(DebugSubsectionKind::None) {}
+    : Kind(DebugSubsectionKind::None),
+      Container(CodeViewContainer::ObjectFile) {}
 
 DebugSubsectionRecord::DebugSubsectionRecord(DebugSubsectionKind Kind,
-                                             BinaryStreamRef Data)
-    : Kind(Kind), Data(Data) {}
+                                             BinaryStreamRef Data,
+                                             CodeViewContainer Container)
+    : Kind(Kind), Data(Data), Container(Container) {}
 
 Error DebugSubsectionRecord::initialize(BinaryStreamRef Stream,
-                                        DebugSubsectionRecord &Info) {
+                                        DebugSubsectionRecord &Info,
+                                        CodeViewContainer Container) {
   const DebugSubsectionHeader *Header;
   BinaryStreamReader Reader(Stream);
   if (auto EC = Reader.readObject(Header))
     return EC;
 
   DebugSubsectionKind Kind =
       static_cast<DebugSubsectionKind>(uint32_t(Header->Kind));
   switch (Kind) {
   case DebugSubsectionKind::FileChecksums:
   case DebugSubsectionKind::Lines:
   case DebugSubsectionKind::InlineeLines:
     break;
   default:
     llvm_unreachable("Unexpected debug fragment kind!");
   }
   if (auto EC = Reader.readStreamRef(Info.Data, Header->Length))
     return EC;
+  Info.Container = Container;
   Info.Kind = Kind;
   return Error::success();
 }
 
 uint32_t DebugSubsectionRecord::getRecordLength() const {
   uint32_t Result = sizeof(DebugSubsectionHeader) + Data.getLength();
-  assert(Result % 4 == 0);
+  assert(Result % alignOf(Container) == 0);
   return Result;
 }
 
 DebugSubsectionKind DebugSubsectionRecord::kind() const { return Kind; }
 
 BinaryStreamRef DebugSubsectionRecord::getRecordData() const { return Data; }
 
 DebugSubsectionRecordBuilder::DebugSubsectionRecordBuilder(
-    DebugSubsectionKind Kind, DebugSubsection &Frag)
-    : Kind(Kind), Frag(Frag) {}
+    DebugSubsectionKind Kind, DebugSubsection &Frag,
+    CodeViewContainer Container)
+    : Kind(Kind), Frag(Frag), Container(Container) {}
 
 uint32_t DebugSubsectionRecordBuilder::calculateSerializedLength() {
   uint32_t Size = sizeof(DebugSubsectionHeader) +
-                  alignTo(Frag.calculateSerializedSize(), 4);
+                  alignTo(Frag.calculateSerializedSize(), alignOf(Container));
   return Size;
 }
 
 Error DebugSubsectionRecordBuilder::commit(BinaryStreamWriter &Writer) {
+  assert(Writer.getOffset() % alignOf(Container) == 0 &&
+         "Debug Subsection not properly aligned");
+
   DebugSubsectionHeader Header;
   Header.Kind = uint32_t(Kind);
   Header.Length = calculateSerializedLength() - sizeof(DebugSubsectionHeader);
 
   if (auto EC = Writer.writeObject(Header))
     return EC;
   if (auto EC = Frag.commit(Writer))
     return EC;
-  if (auto EC = Writer.padToAlignment(4))
+  if (auto EC = Writer.padToAlignment(alignOf(Container)))
     return EC;
 
   return Error::success();
 }

llvm/trunk/lib/DebugInfo/CodeView/SymbolDumper.cpp

Error CVSymbolDumperImpl::visitUnknownSymbol(CVSymbol &CVR) {
   DictScope S(W, "UnknownSym");
   W.printEnum("Kind", uint16_t(CVR.kind()), getSymbolTypeNames());
   W.printNumber("Length", CVR.length());
   return Error::success();
 }
 
 Error CVSymbolDumper::dump(CVRecord<SymbolKind> &Record) {
   SymbolVisitorCallbackPipeline Pipeline;
-  SymbolDeserializer Deserializer(ObjDelegate.get());
+  SymbolDeserializer Deserializer(ObjDelegate.get(), Container);
   CVSymbolDumperImpl Dumper(Types, ObjDelegate.get(), W, PrintRecordBytes);
 
   Pipeline.addCallbackToPipeline(Deserializer);
   Pipeline.addCallbackToPipeline(Dumper);
   CVSymbolVisitor Visitor(Pipeline);
   return Visitor.visitSymbolRecord(Record);
 }
 
 Error CVSymbolDumper::dump(const CVSymbolArray &Symbols) {
   SymbolVisitorCallbackPipeline Pipeline;
-  SymbolDeserializer Deserializer(ObjDelegate.get());
+  SymbolDeserializer Deserializer(ObjDelegate.get(), Container);
   CVSymbolDumperImpl Dumper(Types, ObjDelegate.get(), W, PrintRecordBytes);
 
   Pipeline.addCallbackToPipeline(Deserializer);
   Pipeline.addCallbackToPipeline(Dumper);
   CVSymbolVisitor Visitor(Pipeline);
   return Visitor.visitSymbolStream(Symbols);
 }

llvm/trunk/lib/DebugInfo/CodeView/SymbolRecordMapping.cpp

 }
 
 Error SymbolRecordMapping::visitSymbolBegin(CVSymbol &Record) {
   error(IO.beginRecord(MaxRecordLength - sizeof(RecordPrefix)));
   return Error::success();
 }
 
 Error SymbolRecordMapping::visitSymbolEnd(CVSymbol &Record) {
+  error(IO.padToAlignment(alignOf(Container)));
   error(IO.endRecord());
   return Error::success();
 }
 
 Error SymbolRecordMapping::visitKnownRecord(CVSymbol &CVR, BlockSym &Block) {
 
   error(IO.mapInteger(Block.Parent));
   error(IO.mapInteger(Block.End));

llvm/trunk/lib/DebugInfo/CodeView/SymbolSerializer.cpp

 //===- SymbolSerializer.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/CodeView/SymbolSerializer.h"
 
 using namespace llvm;
 using namespace llvm::codeview;
 
-SymbolSerializer::SymbolSerializer(BumpPtrAllocator &Allocator)
-  : Storage(Allocator), RecordBuffer(MaxRecordLength), Stream(RecordBuffer, llvm::support::little),
-  Writer(Stream), Mapping(Writer) { }
+SymbolSerializer::SymbolSerializer(BumpPtrAllocator &Allocator,
+                                   CodeViewContainer Container)
+    : Storage(Allocator), RecordBuffer(MaxRecordLength),
+      Stream(RecordBuffer, llvm::support::little), Writer(Stream),
+      Mapping(Writer, Container) {}
 
 Error SymbolSerializer::visitSymbolBegin(CVSymbol &Record) {
   assert(!CurrentSymbol.hasValue() && "Already in a symbol mapping!");
 
   Writer.setOffset(0);
 
   if (auto EC = writeRecordPrefix(Record.kind()))
     return EC;

llvm/trunk/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp

 }
 
 void DbiModuleDescriptorBuilder::setObjFileName(StringRef Name) {
   ObjFileName = Name;
 }
 
 void DbiModuleDescriptorBuilder::addSymbol(CVSymbol Symbol) {
   Symbols.push_back(Symbol);
-  SymbolByteSize += Symbol.data().size();
+  // Symbols written to a PDB file are required to be 4 byte aligned.  The same
+  // is not true of object files.
+  assert(Symbol.length() % alignOf(CodeViewContainer::Pdb) == 0 &&
+         "Invalid Symbol alignment!");
+  SymbolByteSize += Symbol.length();
 }
 
 void DbiModuleDescriptorBuilder::addSourceFile(StringRef Path) {
   SourceFiles.push_back(Path);
 }
 
 uint32_t DbiModuleDescriptorBuilder::calculateC13DebugInfoSize() const {
   uint32_t Result = 0;
if (auto EC =
             SymbolWriter.writeInteger<uint32_t>(COFF::DEBUG_SECTION_MAGIC))
       return EC;
     BinaryItemStream<CVSymbol> Records(llvm::support::endianness::little);
     Records.setItems(Symbols);
     BinaryStreamRef RecordsRef(Records);
     if (auto EC = SymbolWriter.writeStreamRef(RecordsRef))
       return EC;
     // TODO: Write C11 Line data
-
+    assert(SymbolWriter.getOffset() % alignOf(CodeViewContainer::Pdb) == 0 &&
+           "Invalid debug section alignment!");
     for (const auto &Builder : C13Builders) {
       assert(Builder && "Empty C13 Fragment Builder!");
       if (auto EC = Builder->commit(SymbolWriter))
         return EC;
     }
 
     // TODO: Figure out what GlobalRefs substream actually is and populate it.
     if (auto EC = SymbolWriter.writeInteger<uint32_t>(0))
void DbiModuleDescriptorBuilder::addC13Fragment(
   DebugLinesSubsection &Frag = *Lines;
 
   // File Checksums have to come first, so push an empty entry on if this
   // is the first.
   if (C13Builders.empty())
     C13Builders.push_back(nullptr);
 
   this->LineInfo.push_back(std::move(Lines));
-  C13Builders.push_back(
-      llvm::make_unique<DebugSubsectionRecordBuilder>(Frag.kind(), Frag));
+  C13Builders.push_back(llvm::make_unique<DebugSubsectionRecordBuilder>(
+      Frag.kind(), Frag, CodeViewContainer::Pdb));
 }
 
 void DbiModuleDescriptorBuilder::addC13Fragment(
     std::unique_ptr<codeview::DebugInlineeLinesSubsection> Inlinees) {
   DebugInlineeLinesSubsection &Frag = *Inlinees;
 
   // File Checksums have to come first, so push an empty entry on if this
   // is the first.
   if (C13Builders.empty())
     C13Builders.push_back(nullptr);
 
   this->Inlinees.push_back(std::move(Inlinees));
-  C13Builders.push_back(
-      llvm::make_unique<DebugSubsectionRecordBuilder>(Frag.kind(), Frag));
+  C13Builders.push_back(llvm::make_unique<DebugSubsectionRecordBuilder>(
+      Frag.kind(), Frag, CodeViewContainer::Pdb));
 }
 
 void DbiModuleDescriptorBuilder::setC13FileChecksums(
     std::unique_ptr<DebugChecksumsSubsection> Checksums) {
   assert(!ChecksumInfo && "Can't have more than one checksum info!");
 
   if (C13Builders.empty())
     C13Builders.push_back(nullptr);
 
   ChecksumInfo = std::move(Checksums);
   C13Builders[0] = llvm::make_unique<DebugSubsectionRecordBuilder>(
-      ChecksumInfo->kind(), *ChecksumInfo);
+      ChecksumInfo->kind(), *ChecksumInfo, CodeViewContainer::Pdb);
 }

llvm/trunk/lib/ObjectYAML/CodeViewYAMLSymbols.cpp

 
 struct SymbolRecordBase {
   codeview::SymbolKind Kind;
   explicit SymbolRecordBase(codeview::SymbolKind K) : Kind(K) {}
 
   virtual ~SymbolRecordBase() {}
   virtual void map(yaml::IO &io) = 0;
   virtual codeview::CVSymbol
-  toCodeViewSymbol(BumpPtrAllocator &Allocator) const = 0;
+  toCodeViewSymbol(BumpPtrAllocator &Allocator,
+                   CodeViewContainer Container) const = 0;
   virtual Error fromCodeViewSymbol(codeview::CVSymbol Type) = 0;
 };
 
 template <typename T> struct SymbolRecordImpl : public SymbolRecordBase {
   explicit SymbolRecordImpl(codeview::SymbolKind K)
       : SymbolRecordBase(K), Symbol(static_cast<SymbolRecordKind>(K)) {}
 
   void map(yaml::IO &io) override;
 
   codeview::CVSymbol
-  toCodeViewSymbol(BumpPtrAllocator &Allocator) const override {
-    return SymbolSerializer::writeOneSymbol(Symbol, Allocator);
+  toCodeViewSymbol(BumpPtrAllocator &Allocator,
+                   CodeViewContainer Container) const override {
+    return SymbolSerializer::writeOneSymbol(Symbol, Allocator, Container);
   }
   Error fromCodeViewSymbol(codeview::CVSymbol CVS) override {
     return SymbolDeserializer::deserializeAs<T>(CVS, Symbol);
   }
 
   mutable T Symbol;
 };
 
template <> void SymbolRecordImpl<ThreadLocalDataSym>::map(IO &IO) {
   IO.mapRequired("Type", Symbol.Type);
   IO.mapRequired("DisplayName", Symbol.Name);
 }
 }
 }
 }
 
 CVSymbol CodeViewYAML::SymbolRecord::toCodeViewSymbol(
-    BumpPtrAllocator &Allocator) const {
-  return Symbol->toCodeViewSymbol(Allocator);
+    BumpPtrAllocator &Allocator, CodeViewContainer Container) const {
+  return Symbol->toCodeViewSymbol(Allocator, Container);
 }
 
 namespace llvm {
 namespace yaml {
 template <> struct MappingTraits<SymbolRecordBase> {
   static void mapping(IO &io, SymbolRecordBase &Record) { Record.map(io); }
 };
 }

llvm/trunk/test/DebugInfo/PDB/pdbdump-write.test

 ; This testcase checks to make sure that we can write PDB files.  It
 ; works by first reading a known good PDB file and dumping the contents
 ; to YAML.  Then it tries to reconstruct as much of the original PDB as
 ; possible, although depending on what flags are specified when generating
 ; the YAML, the PDB might be missing data required for any standard tool
 ; to recognize it.  Finally, it dumps the same set of fields from the newly
 ; constructed PDB to YAML, and verifies that the YAML is the same as the
 ; original YAML generated from the good PDB.  Note that when doing the
 ; final comparison it must dump the original and the new pdb without any
 ; stream metadata, since the layout of the MSF file might be different
 ; (for example if we don't write the entire stream)
 ;
 ; RUN: llvm-pdbdump pdb2yaml -stream-metadata -stream-directory \
-; RUN:   -pdb-stream -tpi-stream %p/Inputs/empty.pdb > %t.1
+; RUN:   -pdb-stream -tpi-stream -dbi-module-syms %p/Inputs/empty.pdb > %t.1
 ; RUN: llvm-pdbdump yaml2pdb -pdb=%t.2 %t.1
 ; RUN: llvm-pdbdump pdb2yaml -pdb-stream -tpi-stream \
-; RUN:   -no-file-headers %p/Inputs/empty.pdb > %t.3
+; RUN:   -dbi-module-syms -no-file-headers %p/Inputs/empty.pdb > %t.3
 ; RUN: llvm-pdbdump pdb2yaml -pdb-stream -tpi-stream \
-; RUN:   -no-file-headers %t.2 > %t.4
+; RUN:   -dbi-module-syms -no-file-headers %t.2 > %t.4
 ; RUN: diff %t.3 %t.4

llvm/trunk/tools/llvm-pdbdump/LLVMOutputStyle.cpp

for (uint32_t I = 0; I < Modules.getModuleCount(); ++I) {
 
         if (ShouldDumpSymbols) {
           auto ExpectedTypes = initializeTypeDatabase(StreamTPI);
           if (!ExpectedTypes)
             return ExpectedTypes.takeError();
           auto &Types = *ExpectedTypes;
 
           ListScope SS(P, "Symbols");
-          codeview::CVSymbolDumper SD(P, Types, nullptr, false);
+          codeview::CVSymbolDumper SD(P, Types, CodeViewContainer::Pdb, nullptr,
+                                      false);
           bool HadError = false;
           for (auto S : ModS.symbols(&HadError)) {
             DictScope LL(P, "");
             if (opts::raw::DumpModuleSyms) {
               if (auto EC = SD.dump(S)) {
                 llvm::consumeError(std::move(EC));
                 HadError = true;
                 break;
Error LLVMOutputStyle::dumpPublicsStream() {
   P.printList("Section Offsets", Publics->getSectionOffsets(),
               printSectionOffset);
   ListScope L(P, "Symbols");
   auto ExpectedTypes = initializeTypeDatabase(StreamTPI);
   if (!ExpectedTypes)
     return ExpectedTypes.takeError();
   auto &Tpi = *ExpectedTypes;
 
-  codeview::CVSymbolDumper SD(P, Tpi, nullptr, false);
+  codeview::CVSymbolDumper SD(P, Tpi, CodeViewContainer::Pdb, nullptr, false);
   bool HadError = false;
   for (auto S : Publics->getSymbols(&HadError)) {
     DictScope DD(P, "");
 
     if (auto EC = SD.dump(S)) {
       HadError = true;
       break;
     }

llvm/trunk/tools/llvm-pdbdump/llvm-pdbdump.cpp

static void yamlToPdb(StringRef Path) {
   for (const auto &MI : Dbi.ModInfos) {
     auto &ModiBuilder = ExitOnErr(DbiBuilder.addModuleInfo(MI.Mod));
     ModiBuilder.setObjFileName(MI.Obj);
 
     for (auto S : MI.SourceFiles)
       ExitOnErr(DbiBuilder.addModuleSourceFile(MI.Mod, S));
     if (MI.Modi.hasValue()) {
       const auto &ModiStream = *MI.Modi;
-      for (auto Symbol : ModiStream.Symbols)
-        ModiBuilder.addSymbol(Symbol.toCodeViewSymbol(Allocator));
+      for (auto Symbol : ModiStream.Symbols) {
+        ModiBuilder.addSymbol(
+            Symbol.toCodeViewSymbol(Allocator, CodeViewContainer::Pdb));
+      }
     }
     if (MI.FileLineInfo.hasValue()) {
       const auto &FLI = *MI.FileLineInfo;
 
       // File Checksums must be emitted before line information, because line
       // info records use offsets into the checksum buffer to reference a file's
       // source file name.
       auto Checksums = llvm::make_unique<DebugChecksumsSubsection>(Strings);

llvm/trunk/tools/llvm-readobj/COFFDumper.cpp

 
 void COFFDumper::printCodeViewSymbolsSubsection(StringRef Subsection,
                                                 const SectionRef &Section,
                                                 StringRef SectionContents) {
   ArrayRef<uint8_t> BinaryData(Subsection.bytes_begin(),
                                Subsection.bytes_end());
   auto CODD = llvm::make_unique<COFFObjectDumpDelegate>(*this, Section, Obj,
                                                         SectionContents);
-  CVSymbolDumper CVSD(W, Types, std::move(CODD), opts::CodeViewSubsectionBytes);
+  CVSymbolDumper CVSD(W, Types, CodeViewContainer::ObjectFile, std::move(CODD),
+                      opts::CodeViewSubsectionBytes);
   CVSymbolArray Symbols;
   BinaryStreamReader Reader(BinaryData, llvm::support::little);
   if (auto EC = Reader.readArray(Symbols, Reader.getLength())) {
     consumeError(std::move(EC));
     W.flush();
     error(object_error::parse_failed);
   }