[CodeView] Properly align symbol records on read/write.

Object files have symbol records not aligned to any particular
boundary (e.g. 1-byte aligned), while PDB files have symbol
records padded to 4-byte aligned boundaries.  Since they share
the same reading / writing code, we have to provide an option to
specify the alignment and propagate it up to the producer or
consumer who knows what the alignment is supposed to be for the
given container type.

Added a test for this by modifying the existing PDB -> YAML -> PDB
round-tripping code to round trip symbol records as well as types.

Differential Revision: https://reviews.llvm.org/D33785

llvm-svn: 304484

lld/COFF/PDB.cpp

@@ -157,7 +157,8 @@ static void dumpDebugS(ScopedPrinter &W, ObjectFile *File) {
     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");
 }

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

@@ -574,6 +574,14 @@ struct FrameData {
     IsFunctionStart = 1 << 2,
   };
 };
+
+enum class CodeViewContainer { ObjectFile, Pdb };
+
+inline uint32_t alignOf(CodeViewContainer Container) {
+  if (Container == CodeViewContainer::ObjectFile)
+    return 1;
+  return 4;
+}
 }
 }
 

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

@@ -136,6 +136,7 @@ class CodeViewRecordIO {
   Error mapByteVectorTail(ArrayRef<uint8_t> &Bytes);
   Error mapByteVectorTail(std::vector<uint8_t> &Bytes);
 
+  Error padToAlignment(uint32_t Align);
   Error skipPadding();
 
 private:

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

@@ -31,26 +31,31 @@ struct DebugSubsectionHeader {
 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;
 };
@@ -62,7 +67,12 @@ template <> struct VarStreamArrayExtractor<codeview::DebugSubsectionRecord> {
 
   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();

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

@@ -24,9 +24,9 @@ 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;
@@ -35,7 +35,9 @@ class SymbolDeserializer : public SymbolVisitorCallbacks {
 
 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))
@@ -45,12 +47,13 @@ class SymbolDeserializer : public SymbolVisitorCallbacks {
     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 {
@@ -77,6 +80,7 @@ class SymbolDeserializer : public SymbolVisitorCallbacks {
     return Error::success();
   }
 
+  CodeViewContainer Container;
   SymbolVisitorDelegate *Delegate;
   std::unique_ptr<MappingInfo> Mapping;
 };

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

@@ -26,9 +26,11 @@ class TypeCollection;
 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,
@@ -44,6 +46,7 @@ class CVSymbolDumper {
 private:
   ScopedPrinter &W;
   TypeCollection &Types;
+  CodeViewContainer Container;
   std::unique_ptr<SymbolDumpDelegate> ObjDelegate;
 
   bool PrintRecordBytes;

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

@@ -20,8 +20,12 @@ 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;
@@ -34,6 +38,7 @@ class SymbolRecordMapping : public SymbolVisitorCallbacks {
 private:
   Optional<SymbolKind> Kind;
 
+  CodeViewContainer Container;
   CodeViewRecordIO IO;
 };
 }

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

@@ -46,17 +46,18 @@ class SymbolSerializer : public SymbolVisitorCallbacks {
 
 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;

llvm/include/llvm/ObjectYAML/CodeViewYAMLSymbols.h

@@ -28,7 +28,9 @@ 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);
 };
 

llvm/lib/DebugInfo/CodeView/CodeViewRecordIO.cpp

@@ -27,6 +27,14 @@ Error CodeViewRecordIO::beginRecord(Optional<uint32_t> MaxLength) {
 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();
 }
 
@@ -49,6 +57,12 @@ uint32_t CodeViewRecordIO::maxFieldLength() const {
   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!");
 

llvm/lib/DebugInfo/CodeView/DebugSubsectionRecord.cpp

@@ -16,14 +16,17 @@ 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))
@@ -41,13 +44,14 @@ Error DebugSubsectionRecord::initialize(BinaryStreamRef Stream,
   }
   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;
 }
 
@@ -56,16 +60,20 @@ 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);
@@ -74,7 +82,7 @@ Error DebugSubsectionRecordBuilder::commit(BinaryStreamWriter &Writer) {
     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/lib/DebugInfo/CodeView/SymbolDumper.cpp

@@ -668,7 +668,7 @@ Error CVSymbolDumperImpl::visitUnknownSymbol(CVSymbol &CVR) {
 
 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);
@@ -679,7 +679,7 @@ Error CVSymbolDumper::dump(CVRecord<SymbolKind> &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);

llvm/lib/DebugInfo/CodeView/SymbolRecordMapping.cpp

@@ -40,6 +40,7 @@ Error SymbolRecordMapping::visitSymbolBegin(CVSymbol &Record) {
 }
 
 Error SymbolRecordMapping::visitSymbolEnd(CVSymbol &Record) {
+  error(IO.padToAlignment(alignOf(Container)));
   error(IO.endRecord());
   return Error::success();
 }

llvm/lib/DebugInfo/CodeView/SymbolSerializer.cpp

@@ -12,9 +12,11 @@
 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!");

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

@@ -66,7 +66,11 @@ void DbiModuleDescriptorBuilder::setObjFileName(StringRef 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) {
@@ -153,7 +157,8 @@ Error DbiModuleDescriptorBuilder::commit(BinaryStreamWriter &ModiWriter,
     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))
@@ -179,8 +184,8 @@ void DbiModuleDescriptorBuilder::addC13Fragment(
     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(
@@ -193,8 +198,8 @@ void DbiModuleDescriptorBuilder::addC13Fragment(
     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(
@@ -206,5 +211,5 @@ void DbiModuleDescriptorBuilder::setC13FileChecksums(
 
   ChecksumInfo = std::move(Checksums);
   C13Builders[0] = llvm::make_unique<DebugSubsectionRecordBuilder>(
-      ChecksumInfo->kind(), *ChecksumInfo);
+      ChecksumInfo->kind(), *ChecksumInfo, CodeViewContainer::Pdb);
 }