[PATCH] [PECOFF][Writer] Emit .reloc section.

[Richard]

  This is my first review comments for llvm, hopefully I'm not getting in the way.


================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:594-596
@@ +593,5 @@
+class BaseRelocChunk : public SectionChunk {
+  typedef std::vector<std::unique_ptr<Chunk>> ChunkVectorT;
+  typedef std::map<uint64_t, std::vector<uint16_t>> PageOffsetT;
+
+public:
----------------
Is this the convention in this code base to put private typedefs at the opening of a class?  I always found it odd to read private implementation details of a class as the first thing in it's declaration.

================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:607-608
@@ +606,4 @@
+  /// By dividing 32 bit RVAs into blocks, COFF saves disk and memory space for
+  /// the base relocation. A block consists with a 32 bit page RVA and 16 bit
+  /// relocation entries which represent offsets in the page. That is a compact
+  /// represetation than a simple vector of 32 bit RVAs.
----------------
"A block consists of a 32 bit page RVA and ..."

"That is a more compact representation than..."

================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:615
@@ +614,3 @@
+      uint64_t pageAddr = i.first;
+      std::vector<uint16_t> offsetsInPage = i.second;
+      appendAtom(createBaseRelocBlock(_file, pageAddr, offsetsInPage));
----------------
Does this need to be a copy or can it be a const reference?

================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:647
@@ +646,3 @@
+  DefinedAtom *createBaseRelocBlock(const File &file, uint64_t pageAddr,
+                                    std::vector<uint16_t> &offsets) {
+    uint32_t size = 8 + offsets.size() * 2;
----------------
offsets is not modified; pass by const reference

================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:649-665
@@ +648,19 @@
+    uint32_t size = 8 + offsets.size() * 2;
+    std::vector<uint8_t> contents(size);
+
+    // The first four bytes is the page RVA.
+    *reinterpret_cast<llvm::support::ulittle32_t *>(&contents[0]) = pageAddr;
+
+    // The second four bytes is the size of the block, including the the page
+    // RVA and this size field.
+    *reinterpret_cast<llvm::support::ulittle32_t *>(&contents[4]) = size;
+
+    // The rest of the block consists of offsets in the page.
+    size_t i = 8;
+    for (uint16_t offset : offsets) {
+      assert(offset < PAGE_SIZE);
+      uint16_t val = (llvm::COFF::IMAGE_REL_BASED_HIGHLOW << 12) | offset;
+      *reinterpret_cast<llvm::support::ulittle16_t *>(&contents[i]) = val;
+      i += 2;
+    }
+    return new (_alloc) BaseRelocAtom(file, std::move(contents));
----------------
yuck, seems like we should have a better facility for building up these COFF chunks.  This seems like something that is going to appear in all the code that builds COFF chunks and all these 4, 8, += 2, type magic numbers are easy to get wrong.  Does something exist that encapsulates these details?

================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:319
@@ +318,3 @@
+    for (const auto *layout : _atomLayouts) {
+      const DefinedAtom *atom = dyn_cast<const DefinedAtom>(layout->_atom);
+      for (const Reference *ref : *atom)
----------------
Can dyn_cast<T> return nullptr?

================
Comment at: lld/trunk/lib/ReaderWriter/PECOFF/WriterPECOFF.cpp:675-676
@@ -542,4 +674,4 @@
 
 class ExecutableWriter : public Writer {
 private:
   /// Apply relocations to the output file buffer. This two pass. In the first
----------------
Can we move all the private members after the public members?


http://llvm-reviews.chandlerc.com/D1126

COMMIT
  http://llvm-reviews.chandlerc.com/rL186336